Skip to content

Commit

Permalink
Merge pull request #40545 from dulajdilshan/fix/deprecate-record-fiel…
Browse files Browse the repository at this point in the history 
…d-40363

Implement logging warnings for deprecated param usages in funcion call and fix inconsistent behaviour in warnings in deprecated record fields
  • Loading branch information
@dulajdilshan
dulajdilshan authored Aug 14, 2023
2 parents 4381741 + 3f153e0 commit 567e622
Show file tree
Hide file tree
Showing 3 changed files with 314 additions and 25 deletions.
148 changes: 129 additions & 19 deletions ...a-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/CodeAnalyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2232,32 +2232,59 @@ public void visit(BLangTableConstructorExpr tableConstructorExpr, AnalyzerData d

@Override
public void visit(BLangRecordLiteral recordLiteral, AnalyzerData data) {
List<RecordLiteralNode.RecordField> fields = recordLiteral.fields;
BType referredType = Types.getReferredType(recordLiteral.getBType());
boolean isRecord = referredType.tag == TypeTags.RECORD;
List<RecordLiteralNode.RecordField> recordLiteralFields = recordLiteral.fields;

for (RecordLiteralNode.RecordField field : fields) {
LinkedHashMap<String, BField> recordFields = null;
if (isRecord) {
recordFields = ((BRecordType) referredType).getFields();
}

for (RecordLiteralNode.RecordField field : recordLiteralFields) {
if (field.isKeyValueField()) {
analyzeExpr(((BLangRecordKeyValueField) field).valueExpr, data);
reportIfDeprecatedUsage(((BLangRecordKeyValueField) field).key.fieldSymbol, recordLiteral,
((BLangRecordKeyValueField) field).pos);
} else if (field.getKind() == NodeKind.SIMPLE_VARIABLE_REF) {
analyzeExpr((BLangRecordLiteral.BLangRecordVarNameField) field, data);
BLangRecordLiteral.BLangRecordVarNameField recField
= (BLangRecordLiteral.BLangRecordVarNameField) field;
analyzeExpr(recField, data);

if (isRecord) {
BField matchingField = recordFields.get(recField.symbol.getName().getValue());
if (matchingField != null) {
reportIfDeprecatedUsage(matchingField.symbol, recordLiteral, recField.pos);
}
}
} else {
analyzeExpr(((BLangRecordLiteral.BLangRecordSpreadOperatorField) field).expr, data);
BLangRecordLiteral.BLangRecordSpreadOperatorField spreadField
= (BLangRecordLiteral.BLangRecordSpreadOperatorField) field;
analyzeExpr(spreadField.expr, data);

BType spreadFieldType = Types.getReferredType(spreadField.expr.getBType());
if (isRecord && spreadFieldType != null && spreadFieldType.tag == TypeTags.RECORD) {
for (BField fieldEntry: ((BRecordType) spreadFieldType).getFields().values()) {
BField matchingField = recordFields.get(fieldEntry.getName().getValue());
if (matchingField != null) {
reportIfDeprecatedUsage(matchingField.symbol, recordLiteral, spreadField.expr.pos);
}
}
}
}
}

Set<Object> names = new HashSet<>();
Set<Object> neverTypedKeys = new HashSet<>();
BType literalBType = recordLiteral.getBType();
BType type = Types.getReferredType(literalBType);
boolean isRecord = type.tag == TypeTags.RECORD;
boolean isOpenRecord = isRecord && !((BRecordType) type).sealed;
boolean isOpenRecord = isRecord && !((BRecordType) referredType).sealed;

// A record type is inferred for a record literal even if the contextually expected type is a map, if the
// mapping constructor expression has `readonly` fields.
boolean isInferredRecordForMapCET = isRecord && recordLiteral.expectedType != null &&
recordLiteral.expectedType.tag == TypeTags.MAP;

BLangRecordLiteral.BLangRecordSpreadOperatorField inclusiveTypeSpreadField = null;
for (RecordLiteralNode.RecordField field : fields) {
for (RecordLiteralNode.RecordField field : recordLiteralFields) {

BLangExpression keyExpr;

Expand All @@ -2277,7 +2304,7 @@ public void visit(BLangRecordLiteral recordLiteral, AnalyzerData data) {
}
inclusiveTypeSpreadField = spreadOpField;

if (fields.size() > 1) {
if (recordLiteralFields.size() > 1) {
if (names.size() > 0) {
this.dlog.error(spreadOpExpr.pos,
DiagnosticErrorCode.SPREAD_FIELD_MAY_DULPICATE_ALREADY_SPECIFIED_KEYS,
Expand Down Expand Up @@ -2320,7 +2347,7 @@ public void visit(BLangRecordLiteral recordLiteral, AnalyzerData data) {
if (bField.type.tag != TypeTags.NEVER) {
this.dlog.error(spreadOpExpr.pos,
DiagnosticErrorCode.DUPLICATE_KEY_IN_RECORD_LITERAL_SPREAD_OP,
type.getKind().typeName(), fieldName, spreadOpField);
referredType.getKind().typeName(), fieldName, spreadOpField);
}
continue;
}
Expand Down Expand Up @@ -2365,7 +2392,8 @@ public void visit(BLangRecordLiteral recordLiteral, AnalyzerData data) {
unescapedName, inclusiveTypeSpreadField);
}

if (!isInferredRecordForMapCET && isOpenRecord && !((BRecordType) type).fields.containsKey(name)) {
if (!isInferredRecordForMapCET && isOpenRecord
&& !((BRecordType) referredType).fields.containsKey(name)) {
dlog.error(keyExpr.pos, DiagnosticErrorCode.INVALID_RECORD_LITERAL_IDENTIFIER_KEY,
unescapedName);
}
Expand All @@ -2388,7 +2416,7 @@ public void visit(BLangRecordLiteral recordLiteral, AnalyzerData data) {
}

if (isInferredRecordForMapCET) {
recordLiteral.expectedType = type;
recordLiteral.expectedType = referredType;
}
}

Expand Down Expand Up @@ -2464,12 +2492,7 @@ public void visit(BLangFieldBasedAccess.BLangNSPrefixedFieldBasedAccess nsPrefix
private void analyzeFieldBasedAccessExpr(BLangFieldBasedAccess fieldAccessExpr, AnalyzerData data) {
BLangExpression expr = fieldAccessExpr.expr;
analyzeExpr(expr, data);
BSymbol symbol = fieldAccessExpr.symbol;
if (symbol != null && Symbols.isFlagOn(fieldAccessExpr.symbol.flags, Flags.DEPRECATED)) {
String deprecatedConstruct = generateDeprecatedConstructString(expr, fieldAccessExpr.field.toString(),
symbol);
dlog.warning(fieldAccessExpr.pos, DiagnosticWarningCode.USAGE_OF_DEPRECATED_CONSTRUCT, deprecatedConstruct);
}
reportIfDeprecatedUsage(fieldAccessExpr.symbol, expr, fieldAccessExpr.pos);
}

@Override
Expand All @@ -2496,6 +2519,7 @@ public void visit(BLangInvocation invocationExpr, AnalyzerData data) {
logDeprecatedWarningForInvocation(invocationExpr);
}
}
analyzeInvocationParams(invocationExpr, data);
}

@Override
Expand Down Expand Up @@ -3875,6 +3899,83 @@ private boolean checkReturnValidityInTransaction(AnalyzerData data) {
&& data.withinTransactionScope;
}

private void analyzeInvocationParams(BLangInvocation iExpr, AnalyzerData data) {
if (iExpr.symbol == null) {
return;
}

BType invocableType = Types.getReferredType(iExpr.symbol.type);
BInvokableSymbol invokableSymbol = ((BInvokableSymbol) iExpr.symbol);
List<BVarSymbol> reqParamSymbols = invokableSymbol.params;
int parameterCountForPositionalArgs = ((BInvokableType) invocableType).getParameterTypes().size();

int visitedArgCount = 0;
for (BLangExpression expr : iExpr.argExprs) {
switch (expr.getKind()) {
case NAMED_ARGS_EXPR:
reportIfDeprecatedUsage(((BLangNamedArgsExpression) expr).varSymbol, expr, expr.pos);
visitedArgCount++;
break;
case REST_ARGS_EXPR:
if (visitedArgCount >= parameterCountForPositionalArgs) {
reportIfDeprecatedUsage(invokableSymbol.restParam, expr, expr.pos);
continue;
}

BLangExpression restExpr = ((BLangRestArgsExpression) expr).expr;
if (restExpr.getKind() == NodeKind.LIST_CONSTRUCTOR_EXPR) {
visitedArgCount = analyzeRestArgsAgainstReqParams((BLangListConstructorExpr) restExpr,
visitedArgCount, reqParamSymbols, invokableSymbol.restParam);
continue;
}

for (int i = visitedArgCount; i < parameterCountForPositionalArgs; i++) {
reportIfDeprecatedUsage(reqParamSymbols.get(i), expr, expr.pos);
}
break;
default: // positional args
if (visitedArgCount < parameterCountForPositionalArgs) {
BVarSymbol paramSymbol = reqParamSymbols.get(visitedArgCount);
reportIfDeprecatedUsage(paramSymbol, expr, expr.pos);
if (Symbols.isFlagOn(reqParamSymbols.get(visitedArgCount).flags, Flags.INCLUDED)) {
analyzeExpr(expr, data);
}
} else {
reportIfDeprecatedUsage(invokableSymbol.restParam, expr, expr.pos);
}
visitedArgCount++;
}
}
}

private int analyzeRestArgsAgainstReqParams(BLangListConstructorExpr listConstructorExpr, int visitedArgCount,
List<BVarSymbol> reqParamSymbols, BVarSymbol restParamSymbol) {
for (BLangExpression expr : listConstructorExpr.exprs) {
if (visitedArgCount >= reqParamSymbols.size()) {
// Visiting args matching with the rest-param
reportIfDeprecatedUsage(restParamSymbol, expr, expr.pos);
continue;
}

if (expr.getKind() != NodeKind.LIST_CONSTRUCTOR_SPREAD_OP) {
reportIfDeprecatedUsage(reqParamSymbols.get(visitedArgCount++), expr, expr.pos);
continue;
}

BLangExpression innerExpr = ((BLangListConstructorSpreadOpExpr) expr).expr;
if (innerExpr.getKind() == NodeKind.LIST_CONSTRUCTOR_EXPR) {
visitedArgCount = analyzeRestArgsAgainstReqParams((BLangListConstructorExpr) innerExpr,
visitedArgCount, reqParamSymbols, restParamSymbol);
continue;
}

for (int i = visitedArgCount; i < reqParamSymbols.size(); i++) {
reportIfDeprecatedUsage(reqParamSymbols.get(i), innerExpr, innerExpr.pos);
}
}
return visitedArgCount;
}

private void validateModuleInitFunction(BLangFunction funcNode) {
if (funcNode.attachedFunction || !Names.USER_DEFINED_INIT_SUFFIX.value.equals(funcNode.name.value)) {
return;
Expand Down Expand Up @@ -3925,6 +4026,15 @@ private BType getErrorTypes(BType bType) {
return errorType;
}

private boolean reportIfDeprecatedUsage(BSymbol constructSymbol, BLangExpression expr, Location usagePos) {
if (constructSymbol != null && Symbols.isFlagOn(constructSymbol.flags, Flags.DEPRECATED)) {
dlog.warning(usagePos, DiagnosticWarningCode.USAGE_OF_DEPRECATED_CONSTRUCT,
generateDeprecatedConstructString(expr, constructSymbol.name.getValue(), constructSymbol));
return true;
}
return false;
}

/**
* This class contains the state machines for a set of workers.
*/
Expand Down
49 changes: 48 additions & 1 deletion ...unit-test/src/test/java/org/ballerinalang/test/annotations/DeprecationAnnotationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,13 @@ public void testDeprecationAnnotation() {
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Object3.fieldOne' is deprecated", 194, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Object3.t' is deprecated", 195, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'add1' is deprecated", 200, 13);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'x' is deprecated", 200, 18);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'y' is deprecated", 200, 21);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'z' is deprecated", 200, 24);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'add2' is deprecated", 201, 13);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'x' is deprecated", 201, 18);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'y' is deprecated", 201, 21);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'z' is deprecated", 201, 24);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Object1' is deprecated", 202, 5);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'z' is deprecated", 213, 13);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Foo' is deprecated", 216, 38);
Expand All @@ -86,20 +92,61 @@ public void testDeprecationAnnotation() {
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Person.name' is deprecated", 287, 16);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Person.getName' is deprecated", 288, 16);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'myFunction' is deprecated", 298, 5);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Employee.name' is deprecated", 316, 26);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Employee.job' is deprecated", 316, 49);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Job.experience' is deprecated", 316, 68);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Employee.name' is deprecated", 317, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Employee.job' is deprecated", 319, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Employee.job' is deprecated", 320, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Employee.job' is deprecated", 321, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Job.experiance' is deprecated", 321, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Job.experience' is deprecated", 321, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Employee2.name' is deprecated", 343, 28);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Employee2.job' is deprecated", 343, 51);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Job.experience' is deprecated", 343, 70);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Employee2.name' is deprecated", 344, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Employee2.job' is deprecated", 346, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'name' is deprecated", 354, 17);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'name' is deprecated", 357, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'name' is deprecated", 370, 17);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'name' is deprecated", 373, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Employee5.address' is deprecated",
393, 28);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Employee5.address' is deprecated",
394, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Employee5.address' is deprecated",
395, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'line02' is deprecated", 395, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Job.experience' is deprecated", 414, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Job.experience' is deprecated", 420, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Job.experience' is deprecated", 426, 12);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Company.name' is deprecated", 433, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Company.city' is deprecated", 434, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Company.country' is deprecated", 435, 12);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'x2' is deprecated", 458, 15);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'x2' is deprecated", 459, 15);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'x2' is deprecated", 460, 20);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Company.city' is deprecated", 463, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Company.country' is deprecated", 464, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'Company.name' is deprecated", 465, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'city' is deprecated", 470, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'country' is deprecated", 471, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'name' is deprecated", 472, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'b' is deprecated", 476, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'c' is deprecated", 477, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'c' is deprecated", 478, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'c' is deprecated", 479, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'y1' is deprecated", 482, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'y2' is deprecated", 483, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'y1' is deprecated", 486, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'y2' is deprecated", 487, 9);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'age' is deprecated", 493, 13);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'x2' is deprecated", 499, 11);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'x2' is deprecated", 502, 13);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'x2' is deprecated", 509, 13);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'x2' is deprecated", 516, 16);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'b' is deprecated", 523, 13);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'c' is deprecated", 524, 13);
BAssertUtil.validateWarning(compileResult, i++, "usage of construct 'c' is deprecated", 525, 13);
Assert.assertEquals(compileResult.getWarnCount(), i);
}

Expand Down
Loading

0 comments on commit 567e622

Please sign in to comment.