Skip to content

Commit

Permalink
Merge pull request #40179 from KavinduZoysa/more-of-gby-invocation
Browse files Browse the repository at this point in the history 
Log errors for invalid usages of sequence variables
  • Loading branch information
@gimantha
gimantha committed Apr 18, 2023
2 parents 9bfcdf3 + 005d635 commit a04c354
Show file tree
Hide file tree
Showing 7 changed files with 220 additions and 28 deletions.
4 changes: 4 additions & 0 deletions ...r/ballerina-lang/src/main/java/org/ballerinalang/util/diagnostic/DiagnosticErrorCode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,10 @@ public enum DiagnosticErrorCode implements DiagnosticCode {
"BCS4045", "unsupported.empty.character.class"),
USER_DEFINED_FUNCTIONS_ARE_DISALLOWED_WITH_AGGREGATED_VARIABLES(
"BCS4046", "user.defined.functions.not.allowed.with.aggregated.variables"),
SEQUENCE_VARIABLE_CANNOT_BE_USED_IN_REQUIRED_ARG(
"BCS4047", "seq.var.cannot.be.used.in.required.arg"),
SEQUENCE_VARIABLE_CAN_BE_USED_IN_SINGLE_ELEMENT_LIST_CTR_OR_FUNC_INVOCATION(
"BCS4048", "seq.var.used.in.single.element.list.ctr.or.func.invocation"),
;

private String diagnosticId;
Expand Down
143 changes: 134 additions & 9 deletions ...ng/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/QueryTypeChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@
import org.wso2.ballerinalang.compiler.parser.NodeCloner;
import org.wso2.ballerinalang.compiler.semantics.model.SymbolEnv;
import org.wso2.ballerinalang.compiler.semantics.model.SymbolTable;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.BConstantSymbol;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.BInvokableSymbol;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.BSequenceSymbol;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.BSymbol;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.BVarSymbol;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.SymTag;
import org.wso2.ballerinalang.compiler.semantics.model.symbols.Symbols;
import org.wso2.ballerinalang.compiler.semantics.model.types.BArrayType;
import org.wso2.ballerinalang.compiler.semantics.model.types.BField;
Expand All @@ -48,6 +50,7 @@
import org.wso2.ballerinalang.compiler.semantics.model.types.BTupleMember;
import org.wso2.ballerinalang.compiler.semantics.model.types.BTupleType;
import org.wso2.ballerinalang.compiler.semantics.model.types.BType;
import org.wso2.ballerinalang.compiler.semantics.model.types.BTypedescType;
import org.wso2.ballerinalang.compiler.semantics.model.types.BUnionType;
import org.wso2.ballerinalang.compiler.semantics.model.types.BXMLType;
import org.wso2.ballerinalang.compiler.tree.BLangIdentifier;
Expand All @@ -74,8 +77,10 @@
import org.wso2.ballerinalang.compiler.tree.expressions.BLangListConstructorExpr;
import org.wso2.ballerinalang.compiler.tree.expressions.BLangQueryAction;
import org.wso2.ballerinalang.compiler.tree.expressions.BLangQueryExpr;
import org.wso2.ballerinalang.compiler.tree.expressions.BLangSimpleVarRef;
import org.wso2.ballerinalang.compiler.tree.statements.BLangDo;
import org.wso2.ballerinalang.compiler.tree.types.BLangLetVariable;
import org.wso2.ballerinalang.compiler.tree.types.BLangType;
import org.wso2.ballerinalang.compiler.util.BArrayState;
import org.wso2.ballerinalang.compiler.util.CompilerContext;
import org.wso2.ballerinalang.compiler.util.ImmutableTypeCloner;
Expand All @@ -92,6 +97,8 @@
import java.util.Stack;
import java.util.stream.Collectors;

import static org.ballerinalang.model.symbols.SymbolOrigin.VIRTUAL;

/**
* @since 2201.5.0
*/
Expand Down Expand Up @@ -142,7 +149,6 @@ public void visit(BLangQueryExpr queryExpr, TypeChecker.AnalyzerData data) {
public void checkQueryType(BLangQueryExpr queryExpr, TypeChecker.AnalyzerData data) {
AnalyzerData prevData = data.queryData;
data.queryData = new AnalyzerData();
data.queryData.afterGroupBy = prevData.afterGroupBy;

Types.CommonAnalyzerData commonAnalyzerData = data.commonAnalyzerData;

Expand Down Expand Up @@ -798,7 +804,6 @@ public void visit(BLangGroupByClause groupByClause, TypeChecker.AnalyzerData dat
new BSequenceType(originalSymbol.getType()), originalSymbol.owner, originalSymbol.pos);
groupByEnv.scope.define(name, sequenceSymbol);
}
data.queryData.afterGroupBy = true;
}

@Override
Expand Down Expand Up @@ -871,7 +876,10 @@ public void visit(BLangInvocation iExpr, TypeChecker.AnalyzerData data) {
List<BVarSymbol> params = functionSymbol.params;
for (int i = 0; i < params.size(); i++) {
BLangExpression arg = iExpr.argExprs.get(i);
checkTypeParamExpr(arg, params.get(i).type, data);
checkArg(arg, params.get(i).type, data);
if (arg.getBType().tag == TypeTags.SEQUENCE) {
dlog.error(arg.pos, DiagnosticErrorCode.SEQUENCE_VARIABLE_CANNOT_BE_USED_IN_REQUIRED_ARG);
}
iExpr.requiredArgs.add(arg);
argCount = i;
}
Expand All @@ -883,7 +891,7 @@ public void visit(BLangInvocation iExpr, TypeChecker.AnalyzerData data) {
iExpr.restArgs.add(argExpr);
if (argExprKind == NodeKind.SIMPLE_VARIABLE_REF) {
if (silentTypeCheckExpr(argExpr, symTable.noType, data).tag == TypeTags.SEQUENCE) {
checkTypeParamExpr(argExpr, restParamType, data);
checkArg(argExpr, restParamType, data);
continue;
}
}
Expand All @@ -892,9 +900,9 @@ public void visit(BLangInvocation iExpr, TypeChecker.AnalyzerData data) {
continue;
}
if (restParamType.tag == TypeTags.ARRAY) {
checkTypeParamExpr(argExpr, ((BArrayType) restParamType).eType, data);
checkArg(argExpr, ((BArrayType) restParamType).eType, data);
} else {
checkTypeParamExpr(argExpr, restParamType, data);
checkArg(argExpr, restParamType, data);
}
}
}
Expand All @@ -908,17 +916,133 @@ public void visit(BLangInvocation iExpr, TypeChecker.AnalyzerData data) {
}
}

// Check the argument within sequence context.
private void checkArg(BLangExpression arg, BType expectedType, TypeChecker.AnalyzerData data) {
data.queryData.withinSequenceContext = effectiveSimpleVarRef(arg);
checkTypeParamExpr(arg, expectedType, data);
data.queryData.withinSequenceContext = false;
}

private boolean effectiveSimpleVarRef(BLangExpression expr) {
// TODO: Improve this method to handle grouping-expr
NodeKind kind = expr.getKind();
return kind == NodeKind.SIMPLE_VARIABLE_REF;
}

// TODO: Combine this with the method in TypeChecker
public void visit(BLangSimpleVarRef varRefExpr, TypeChecker.AnalyzerData data) {
// Set error type as the actual type.
BType actualType = symTable.semanticError;

BLangIdentifier identifier = varRefExpr.variableName;
Name varName = names.fromIdNode(identifier);
if (varName == Names.IGNORE) {
varRefExpr.setBType(this.symTable.anyType);

// If the variable name is a wildcard('_'), the symbol should be ignorable.
varRefExpr.symbol = new BVarSymbol(0, true, varName,
names.originalNameFromIdNode(identifier),
data.env.enclPkg.symbol.pkgID, varRefExpr.getBType(), data.env.scope.owner,
varRefExpr.pos, VIRTUAL);

data.resultType = varRefExpr.getBType();
return;
}

Name compUnitName = getCurrentCompUnit(varRefExpr);
BSymbol pkgSymbol = symResolver.resolvePrefixSymbol(data.env, names.fromIdNode(varRefExpr.pkgAlias),
compUnitName);
varRefExpr.pkgSymbol = pkgSymbol;
if (pkgSymbol == symTable.notFoundSymbol) {
varRefExpr.symbol = symTable.notFoundSymbol;
dlog.error(varRefExpr.pos, DiagnosticErrorCode.UNDEFINED_MODULE, varRefExpr.pkgAlias);
}

if (pkgSymbol.tag == SymTag.XMLNS) {
actualType = symTable.stringType;
} else if (pkgSymbol != symTable.notFoundSymbol) {
BSymbol symbol = symResolver.lookupMainSpaceSymbolInPackage(varRefExpr.pos, data.env,
names.fromIdNode(varRefExpr.pkgAlias), varName);
// if no symbol, check same for object attached function
BLangType enclType = data.env.enclType;
if (symbol == symTable.notFoundSymbol && enclType != null && enclType.getBType().tsymbol.scope != null) {
Name objFuncName = Names.fromString(Symbols
.getAttachedFuncSymbolName(enclType.getBType().tsymbol.name.value, varName.value));
symbol = symResolver.resolveStructField(varRefExpr.pos, data.env, objFuncName,
enclType.getBType().tsymbol);
}

// TODO: call to isInLocallyDefinedRecord() is a temporary fix done to disallow local var references in
// locally defined record type defs. This check should be removed once local var referencing is supported.
if (((symbol.tag & SymTag.VARIABLE) == SymTag.VARIABLE)) {
BVarSymbol varSym = (BVarSymbol) symbol;
checkSelfReferences(varRefExpr.pos, data.env, varSym);
varRefExpr.symbol = varSym;
actualType = varSym.type;
markAndRegisterClosureVariable(symbol, varRefExpr.pos, data.env, data);
} else if ((symbol.tag & SymTag.SEQUENCE) == SymTag.SEQUENCE) {
varRefExpr.symbol = symbol;
actualType = symbol.type;
data.queryData.foundSeqVarInExpr = true;
if (!data.queryData.withinSequenceContext) {
dlog.error(varRefExpr.pos,
DiagnosticErrorCode.
SEQUENCE_VARIABLE_CAN_BE_USED_IN_SINGLE_ELEMENT_LIST_CTR_OR_FUNC_INVOCATION);
}
} else if ((symbol.tag & SymTag.TYPE_DEF) == SymTag.TYPE_DEF) {
actualType = symbol.type.tag == TypeTags.TYPEDESC ? symbol.type : new BTypedescType(symbol.type, null);
varRefExpr.symbol = symbol;
} else if ((symbol.tag & SymTag.CONSTANT) == SymTag.CONSTANT) {
BConstantSymbol constSymbol = (BConstantSymbol) symbol;
varRefExpr.symbol = constSymbol;
BType symbolType = symbol.type;
BType expectedType = Types.getReferredType(data.expType);
if (symbolType != symTable.noType && expectedType.tag == TypeTags.FINITE ||
(expectedType.tag == TypeTags.UNION && types.getAllTypes(expectedType, true).stream()
.anyMatch(memType -> memType.tag == TypeTags.FINITE &&
types.isAssignable(symbolType, memType)))) {
actualType = symbolType;
} else {
actualType = constSymbol.literalType;
}

// If the constant is on the LHS, modifications are not allowed.
// E.g. m.k = "10"; // where `m` is a constant.
if (varRefExpr.isLValue || varRefExpr.isCompoundAssignmentLValue) {
actualType = symTable.semanticError;
dlog.error(varRefExpr.pos, DiagnosticErrorCode.CANNOT_UPDATE_CONSTANT_VALUE);
}
} else {
varRefExpr.symbol = symbol; // Set notFoundSymbol
logUndefinedSymbolError(varRefExpr.pos, varName.value);
}
}

// Check type compatibility
if (data.expType.tag == TypeTags.ARRAY && isArrayOpenSealedType((BArrayType) data.expType)) {
dlog.error(varRefExpr.pos, DiagnosticErrorCode.CANNOT_INFER_SIZE_ARRAY_SIZE_FROM_THE_CONTEXT);
data.resultType = symTable.semanticError;
return;
}

data.resultType = types.checkType(varRefExpr, actualType, data.expType);
}

private boolean checkInvocationAfterGroupBy(BLangInvocation invocation, TypeChecker.AnalyzerData data) {
data.queryData.foundSeqVarInExpr = false;
invocation.argExprs.forEach(arg -> silentTypeCheckExpr(arg, symTable.noType, data));
return data.queryData.afterGroupBy && data.queryData.foundSeqVarInExpr;
return data.queryData.foundSeqVarInExpr;
}

@Override
public void visit(BLangListConstructorExpr listConstructor, TypeChecker.AnalyzerData data) {
// TODO: Refactor this method
BType expType = data.expType;
if (expType.tag == TypeTags.NONE || expType.tag == TypeTags.READONLY) {
data.queryData.withinSequenceContext =
listConstructor.exprs.size() == 1 && effectiveSimpleVarRef(listConstructor.exprs.get(0));
BType inferredType = getInferredTupleType(listConstructor, expType, data);
data.queryData.withinSequenceContext = false;
checkTupleWithSequence(inferredType);
data.resultType = inferredType == symTable.semanticError ?
symTable.semanticError : types.checkType(listConstructor, inferredType, expType);
Expand All @@ -929,7 +1053,9 @@ public void visit(BLangListConstructorExpr listConstructor, TypeChecker.Analyzer
if (expr.getKind() == NodeKind.SIMPLE_VARIABLE_REF) {
BType type = silentTypeCheckExpr(expr, symTable.noType, data);
if (type.tag == TypeTags.SEQUENCE) {
data.queryData.withinSequenceContext = true;
checkExpr(expr, data.env, expType, data);
data.queryData.withinSequenceContext = false;
data.resultType = new BTupleType(null, new ArrayList<>(0), ((BSequenceType) type).elementType, 0);
listConstructor.setBType(data.resultType);
return;
Expand Down Expand Up @@ -989,8 +1115,7 @@ private BLangNode getLastInputNodeFromEnv(SymbolEnv env) {
public static class AnalyzerData {
boolean queryCompletesEarly = false;
HashSet<BType> completeEarlyErrorList = new HashSet<>();
boolean afterGroupBy = false;
boolean foundSeqVarInExpr = false;
boolean withinArgument = false;
boolean withinSequenceContext = false;
}
}
6 changes: 1 addition & 5 deletions ...na-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2893,10 +2893,6 @@ public void visit(BLangSimpleVarRef varRefExpr, AnalyzerData data) {
varRefExpr.symbol = varSym;
actualType = varSym.type;
markAndRegisterClosureVariable(symbol, varRefExpr.pos, data.env, data);
} else if ((symbol.tag & SymTag.SEQUENCE) == SymTag.SEQUENCE) {
varRefExpr.symbol = symbol;
actualType = symbol.type;
data.queryData.foundSeqVarInExpr = true;
} else if ((symbol.tag & SymTag.TYPE_DEF) == SymTag.TYPE_DEF) {
actualType = symbol.type.tag == TypeTags.TYPEDESC ? symbol.type : new BTypedescType(symbol.type, null);
varRefExpr.symbol = symbol;
Expand Down Expand Up @@ -9247,7 +9243,7 @@ private void markChildrenAsImmutable(BLangXMLElementLiteral bLangXMLElementLiter
}
}

private void logUndefinedSymbolError(Location pos, String name) {
protected void logUndefinedSymbolError(Location pos, String name) {
if (!missingNodesHelper.isMissingNode(name)) {
dlog.error(pos, DiagnosticErrorCode.UNDEFINED_SYMBOL, name);
}
Expand Down
8 changes: 7 additions & 1 deletion compiler/ballerina-lang/src/main/resources/compiler.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1969,4 +1969,10 @@ error.unsupported.empty.character.class=\
empty character class disallowed

error.user.defined.functions.not.allowed.with.aggregated.variables=\
user defined functions are not allowed when arguments contain aggregated variable
user defined functions are not allowed when arguments contain aggregated variable

error.seq.var.cannot.be.used.in.required.arg=\
sequence variable cannot be used in a required argument

error.seq.var.used.in.single.element.list.ctr.or.func.invocation=\
sequence variable can be used in a single element list constructor or function invocation
29 changes: 29 additions & 0 deletions tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/query/GroupByClauseTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@
*/
package org.ballerinalang.test.query;

import org.ballerinalang.test.BAssertUtil;
import org.ballerinalang.test.BCompileUtil;
import org.ballerinalang.test.BRunUtil;
import org.ballerinalang.test.CompileResult;
import org.testng.Assert;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;
Expand All @@ -38,6 +40,7 @@ public class GroupByClauseTest {
public void setup() {
resultWithListCtr = BCompileUtil.compile("test-src/query/group_by_clause_with_list_ctr.bal");
resultWithInvocation = BCompileUtil.compile("test-src/query/group_by_clause_with_invocation.bal");
negativeResult = BCompileUtil.compile("test-src/query/group_by_clause_negative.bal");
}

@Test(dataProvider = "dataToTestGroupByClauseWithListCtr")
Expand Down Expand Up @@ -145,4 +148,30 @@ public Object[] dataToTestGroupByClauseWithInvocation() {
"testGroupByExpressionWithMapOutput"
};
}

@Test
public void testNegativeCases() {
int i = 0;
BAssertUtil.validateError(negativeResult, i++, "sequence variable cannot be used in a required argument",
23, 37);
BAssertUtil.validateError(negativeResult, i++, "sequence variable can be used in a single element list " +
"constructor or function invocation", 29, 24);
BAssertUtil.validateError(negativeResult, i++, "incompatible types: expected 'int', found 'seq int'", 32, 28);
BAssertUtil.validateError(negativeResult, i++, "sequence variable can be used in a single element list " +
"constructor or function invocation", 32, 28);
BAssertUtil.validateError(negativeResult, i++, "sequence variable can be used in a single element list " +
"constructor or function invocation", 35, 25);
BAssertUtil.validateError(negativeResult, i++, "incompatible types: expected 'seq int', found 'seq int'",
36, 20);
BAssertUtil.validateError(negativeResult, i++, "operator '+' not defined for 'seq int' and 'int'", 39, 20);
BAssertUtil.validateError(negativeResult, i++, "sequence variable can be used in a single element list " +
"constructor or function invocation", 39, 20);
BAssertUtil.validateError(negativeResult, i++, "sequence variable can be used in a single element list " +
"constructor or function invocation", 42, 25);
BAssertUtil.validateError(negativeResult, i++, "sequence variable can be used in a single element list " +
"constructor or function invocation", 42, 33);
BAssertUtil.validateError(negativeResult, i++, "sequence variable can be used in a single element list " +
"constructor or function invocation", 45, 26);
Assert.assertEquals(negativeResult.getErrorCount(), i);
}
}
Loading

0 comments on commit a04c354

Please sign in to comment.