Merge pull request #28938 from lasinicl/fix-annots

Allow `mapping-constructor-expr` to be optional in annotations
ballerina-platform · Mar 8, 2021 · c9852c8 · c9852c8
2 parents da74fa6 + b75e5a2
commit c9852c8
Show file tree

Hide file tree

Showing 10 changed files with 324 additions and 41 deletions.
diff --git a/...llerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/AnnotationDesugar.java b/...llerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/AnnotationDesugar.java
@@ -300,6 +300,9 @@ private void attachSchedulerPolicy(BLangFunction function) {
             if (!annotation.annotationName.value.equals("strand")) {
                 continue;
             }
+            if (annotation.expr == null) {
+                continue;
+            }
             List<RecordLiteralNode.RecordField> fields = ((BLangRecordLiteral) annotation.expr).fields;
             for (RecordLiteralNode.RecordField field : fields) {
                 if (field.getKind() != NodeKind.RECORD_LITERAL_KEY_VALUE) {

diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java
@@ -1292,8 +1292,15 @@ public void visit(BLangAnnotation annotationNode) {
     }
 
     public void visit(BLangAnnotationAttachment annAttachmentNode) {
-        annAttachmentNode.expr = rewrite(annAttachmentNode.expr, env);
+        if (annAttachmentNode.expr == null && annAttachmentNode.annotationSymbol.attachedType != null) {
+            BType attachedType = annAttachmentNode.annotationSymbol.attachedType.type;
+            if (attachedType.tag != TypeTags.FINITE) {
+                annAttachmentNode.expr = ASTBuilderUtil.createEmptyRecordLiteral(annAttachmentNode.pos,
+                        attachedType.tag == TypeTags.ARRAY ? ((BArrayType) attachedType).eType : attachedType);
+            }
+        }
         if (annAttachmentNode.expr != null) {
+            annAttachmentNode.expr = rewrite(annAttachmentNode.expr, env);
             for (AttachPoint point : annAttachmentNode.annotationSymbol.points) {
                 if (!point.source) {
                     annAttachmentNode.expr = visitCloneReadonly(annAttachmentNode.expr, annAttachmentNode.expr.type);

diff --git a/...ng/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SemanticAnalyzer.java b/...ng/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SemanticAnalyzer.java
@@ -3500,28 +3500,47 @@ private void validateAnnotationAttachmentExpr(BLangAnnotationAttachment annAttac
             return;
         }
 
-
-        // At this point the type is a subtype of  map<anydata>|record{ anydata...; } or
-        // map<anydata>[]|record{ anydata...; }[], thus an expression is required.
+        BType annotType = annotationSymbol.attachedType.type;
         if (annAttachmentNode.expr == null) {
-            this.dlog.error(annAttachmentNode.pos, DiagnosticErrorCode.ANNOTATION_ATTACHMENT_REQUIRES_A_VALUE,
-                            annotationSymbol);
-            return;
+            BRecordType recordType = annotType.tag == TypeTags.RECORD ? (BRecordType) annotType :
+                    annotType.tag == TypeTags.ARRAY && ((BArrayType) annotType).eType.tag == TypeTags.RECORD ?
+                            (BRecordType) ((BArrayType) annotType).eType : null;
+            if (recordType != null && hasRequiredFields(recordType)) {
+                this.dlog.error(annAttachmentNode.pos, DiagnosticErrorCode.ANNOTATION_ATTACHMENT_REQUIRES_A_VALUE,
+                        recordType);
+                return;
+            }
         }
 
-        BType annotType = annotationSymbol.attachedType.type;
-        this.typeChecker.checkExpr(annAttachmentNode.expr, env,
-                                   annotType.tag == TypeTags.ARRAY ? ((BArrayType) annotType).eType : annotType);
+        if (annAttachmentNode.expr != null) {
+            this.typeChecker.checkExpr(annAttachmentNode.expr, env,
+                    annotType.tag == TypeTags.ARRAY ? ((BArrayType) annotType).eType : annotType);
 
-        if (Symbols.isFlagOn(annotationSymbol.flags, Flags.CONSTANT)) {
-            if (annotationSymbol.points.stream().anyMatch(attachPoint -> !attachPoint.source)) {
-                constantAnalyzer.analyzeExpr(annAttachmentNode.expr);
-                return;
+            if (Symbols.isFlagOn(annotationSymbol.flags, Flags.CONSTANT)) {
+                if (annotationSymbol.points.stream().anyMatch(attachPoint -> !attachPoint.source)) {
+                    constantAnalyzer.analyzeExpr(annAttachmentNode.expr);
+                    return;
+                }
+                checkAnnotConstantExpression(annAttachmentNode.expr);
             }
-            checkAnnotConstantExpression(annAttachmentNode.expr);
         }
     }
 
+    /**
+     * Check whether a record type has required fields.
+     *
+     * @param recordType Record type.
+     * @return true if the record type has required fields; false otherwise.
+     */
+    public boolean hasRequiredFields(BRecordType recordType) {
+        for (BField field : recordType.fields.values()) {
+            if (Symbols.isFlagOn(field.symbol.flags, Flags.REQUIRED)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     private void validateAnnotationAttachmentCount(List<BLangAnnotationAttachment> attachments) {
         Map<BAnnotationSymbol, Integer> attachmentCounts = new HashMap<>();
         for (BLangAnnotationAttachment attachment : attachments) {

diff --git a/compiler/ballerina-lang/src/main/resources/compiler.properties b/compiler/ballerina-lang/src/main/resources/compiler.properties
@@ -625,7 +625,7 @@ error.annotation.attachment.cannot.have.a.value=\
   no annotation value expected for annotation ''{0}''
 
 error.annotation.attachment.requires.a.value=\
-  annotation value expected for annotation ''{0}''
+  annotation value expected for annotation of record type ''{0}'' with required fields
 
 error.annotation.attachment.cannot.specify.multiple.values=\
   cannot specify more than one annotation value for annotation ''{0}''

diff --git a/...st/src/test/java/org/ballerinalang/test/annotations/AnnotationAttachmentNegativeTest.java b/...st/src/test/java/org/ballerinalang/test/annotations/AnnotationAttachmentNegativeTest.java
@@ -37,7 +37,7 @@ public class AnnotationAttachmentNegativeTest {
     @BeforeClass
     public void setup() {
         compileResult = BCompileUtil.compile("test-src/annotations/annot_attachments_negative.bal");
-        Assert.assertEquals(compileResult.getErrorCount(), 250);
+        Assert.assertEquals(compileResult.getErrorCount(), 253);
     }
 
     @Test
@@ -455,18 +455,23 @@ public void testInvalidAttachmentWithValue() {
 
     @Test
     public void testInvalidAttachmentWithoutValue() {
-        validateError(compileResult, 246,
-                      "annotation value expected for annotation 'ballerina/lang.annotations:1.0.0:strand'",
-                      872, 5);
-        validateError(compileResult, 247, "annotation value expected for annotation 'v1'",
-                      878, 1);
+        validateError(compileResult, 246, "annotation value expected for annotation of " +
+                        "record type 'Annot' with required fields", 871, 1);
+        validateError(compileResult, 247, "annotation value expected for annotation of " +
+                        "record type 'Annot' with required fields", 874, 1);
+        validateError(compileResult, 248, "annotation value expected for annotation of " +
+                        "record type 'Annot' with required fields", 875, 22);
+        validateError(compileResult, 249, "annotation value expected for annotation of " +
+                "record type 'Annot' with required fields", 881, 1);
+        validateError(compileResult, 250, "annotation value expected for annotation of " +
+                "record type 'Annot' with required fields", 882, 1);
     }
 
     @Test
     public void testInvalidAttachmentCount() {
-        validateError(compileResult, 248, "cannot specify more than one annotation value for " +
-                              "annotation 'ballerina/lang.annotations:1.0.0:tainted'", 881, 1);
-        validateError(compileResult, 249,
-                      "cannot specify more than one annotation value for annotation 'v1'", 883, 1);
+        validateError(compileResult, 251, "cannot specify more than one annotation value for " +
+                              "annotation 'ballerina/lang.annotations:1.0.0:tainted'", 886, 1);
+        validateError(compileResult, 252,
+                      "cannot specify more than one annotation value for annotation 'v1'", 888, 1);
     }
 }
diff --git a/...-unit-test/src/test/java/org/ballerinalang/test/annotations/AnnotationAttachmentTest.java b/...-unit-test/src/test/java/org/ballerinalang/test/annotations/AnnotationAttachmentTest.java
@@ -16,12 +16,14 @@
  */
 package org.ballerinalang.test.annotations;
 
+import org.ballerinalang.core.model.types.TypeTags;
 import org.ballerinalang.model.tree.NodeKind;
 import org.ballerinalang.test.BCompileUtil;
 import org.ballerinalang.test.CompileResult;
 import org.testng.Assert;
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
+import org.wso2.ballerinalang.compiler.semantics.model.types.BMapType;
 import org.wso2.ballerinalang.compiler.tree.BLangAnnotationAttachment;
 import org.wso2.ballerinalang.compiler.tree.BLangClassDefinition;
 import org.wso2.ballerinalang.compiler.tree.BLangExternalFunctionBody;
@@ -367,4 +369,89 @@ public void testAnnotsWithConstLists() {
         Assert.assertEquals(element.getKind(), NodeKind.LITERAL);
         Assert.assertEquals(((BLangLiteral) element).value, "test");
     }
+
+    @Test
+    public void testAnnotWithEmptyMapConstructorOnType() {
+        List<BLangAnnotationAttachment> attachments = (List<BLangAnnotationAttachment>)
+                compileResult.getAST().getTypeDefinitions().get(9).getAnnotationAttachments();
+        validateEmptyMapConstructorExprInAnnot(attachments, "v16", "A");
+    }
+
+    @Test
+    public void testAnnotWithEmptyMapConstructorOnFunction() {
+        BLangFunction function = getFunction("myFunction1");
+        validateEmptyMapConstructorExprInAnnot(function.annAttachments, "v17", "A");
+        validateEmptyMapConstructorExprInAnnot(function.requiredParams.get(0).annAttachments, "v19", "A");
+    }
+
+    @Test
+    public void testAnnotWithEmptyMapConstructorOnService() {
+        List<BLangAnnotationAttachment> attachments = (List<BLangAnnotationAttachment>)
+                compileResult.getAST().getClassDefinitions().stream()
+                        .filter(classNode -> classNode.getName().getValue().equals("$anonType$_7"))
+                        .findFirst()
+                        .get().getAnnotationAttachments();
+        validateEmptyMapConstructorExprInAnnot(attachments, "v20", "A");
+    }
+
+    @Test
+    public void testAnnotWithEmptyMapConstructorOnResource() {
+        BLangFunction function = getFunction("$anonType$_7.$get$res");
+        validateEmptyMapConstructorExprInAnnot(function.annAttachments, "v18", "A");
+        validateEmptyMapConstructorExprInAnnot(function.requiredParams.get(0).annAttachments, "v19", "A");
+    }
+
+    @Test
+    public void testAnnotWithEmptyMapConstructorOnFunction2() {
+        BLangFunction function = getFunction("myFunction2");
+        validateEmptyMapConstructorExprInAnnot(function.annAttachments, "v21", "map");
+        validateEmptyMapConstructorExprInAnnot(function.requiredParams.get(0).annAttachments, "v22", "map");
+    }
+
+    @Test
+    public void testAnnotWithEmptyMapConstructorOnFunction3() {
+        BLangFunction function = getFunction("myFunction3");
+        validateEmptyMapConstructorExprInAnnot(function.annAttachments, "v23", "A");
+    }
+
+    @Test
+    public void testAnnotWithEmptyMapConstructorOnFunction4() {
+        BLangFunction function = getFunction("myFunction4");
+        validateEmptyMapConstructorExprInAnnot(function.annAttachments, "v17", "A");
+        validateEmptyMapConstructorExprInAnnot(function.requiredParams.get(0).annAttachments, "v19", "A");
+    }
+
+    @Test
+    public void testAnnotWithEmptyMapConstructorOnFunction5() {
+        BLangFunction function = getFunction("myFunction5");
+        validateEmptyMapConstructorExprInAnnot(function.annAttachments, "v23", "A");
+    }
+
+    @Test
+    public void testAnnotWithEmptyMapConstructorOnFunction6() {
+        BLangFunction function = getFunction("myFunction6");
+        validateEmptyMapConstructorExprInAnnot(function.annAttachments, "v24", "C");
+    }
+
+    public void validateEmptyMapConstructorExprInAnnot(List<BLangAnnotationAttachment> attachments,
+                                                       String annotationName, String typeName) {
+        int i = 0;
+        for (BLangAnnotationAttachment attachment : attachments) {
+            Assert.assertEquals(attachment.annotationName.getValue(), annotationName);
+            BLangExpression expression = ((BLangInvocation) attachment.expr).expr;
+            Assert.assertEquals(expression.getKind(), NodeKind.RECORD_LITERAL_EXPR);
+            BLangRecordLiteral recordLiteral = (BLangRecordLiteral) expression;
+            Assert.assertEquals(recordLiteral.getFields().size(), 0);
+            Assert.assertTrue(recordLiteral.type.tag == TypeTags.RECORD_TYPE_TAG
+                    || recordLiteral.type.tag == TypeTags.MAP_TAG);
+            if (recordLiteral.type.tag == TypeTags.RECORD_TYPE_TAG) {
+                Assert.assertEquals(recordLiteral.type.tsymbol.name.value, typeName);
+            } else {
+                Assert.assertEquals(recordLiteral.type.tag, TypeTags.MAP_TAG);
+                Assert.assertEquals(((BMapType) recordLiteral.type).constraint.tag, TypeTags.INT_TAG);
+            }
+            i++;
+        }
+        Assert.assertEquals(attachments.size(), i);
+    }
 }
diff --git a/...ina-unit-test/src/test/java/org/ballerinalang/test/annotations/AnnotationRuntimeTest.java b/...ina-unit-test/src/test/java/org/ballerinalang/test/annotations/AnnotationRuntimeTest.java
@@ -53,6 +53,16 @@ public void setup() {
         resultAccessNegative = BCompileUtil.compile("test-src/annotations/annotation_access_negative.bal");
     }
 
+    @Test(dataProvider = "annotAccessTests")
+    public void testAnnotAccess(String testFunction) {
+        CompileResult resultOne = BCompileUtil.compile("test-src/annotations/annot_access.bal");
+        Assert.assertEquals(resultOne.getErrorCount(), 0);
+        BValue[] returns = BRunUtil.invoke(resultOne, testFunction);
+        Assert.assertEquals(returns.length, 1);
+        Assert.assertSame(returns[0].getClass(), BBoolean.class);
+        Assert.assertTrue(((BBoolean) returns[0]).booleanValue());
+    }
+
     @Test(description = "test accessing source only annots at runtime, the annots should not be available",
             dataProvider = "annotAccessWithSourceOnlyPointsTests")
     public void testSourceOnlyAnnotAccess(String testFunction) {
@@ -91,13 +101,17 @@ public Object[][] annotAccessTests() {
                 { "testObjectTypeAnnotAccess1" },
                 { "testObjectTypeAnnotAccess2" },
                 { "testObjectTypeAnnotAccess3" },
-                { "testServiceAnnotAccess1" },
+                //{ "testServiceAnnotAccess1" },
                 { "testServiceAnnotAccess2" },
                 { "testServiceAnnotAccess3" },
                 { "testServiceAnnotAccess4" },
                 { "testFunctionAnnotAccess1" },
                 { "testFunctionAnnotAccess2" },
-                { "testInlineAnnotAccess" }
+                { "testInlineAnnotAccess" },
+                { "testAnnotWithEmptyMappingConstructor1" },
+                { "testAnnotWithEmptyMappingConstructor2" },
+                { "testAnnotWithEmptyMappingConstructor3" },
+                { "testAnnotWithEmptyMappingConstructor4" }
         };
     }
 

diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/annotations/annot_access.bal b/tests/jballerina-unit-test/src/test/resources/test-src/annotations/annot_access.bal
@@ -240,8 +240,87 @@ class Listener {
     }
 }
 
-// TODO: #17936
-//public function testInlineAnnotAccess() returns boolean {
-//    Annot? f = (typeof a).@v1;
-//    return f is Annot;
-//}
+public function testInlineAnnotAccess() returns boolean {
+    Annot? f = (typeof a).@v1;
+    return f is Annot;
+}
+
+type A record {|
+    string val = "ABC";
+|};
+
+public annotation A v9 on function;
+
+@v9
+function myFunction1() {
+
+}
+
+function testAnnotWithEmptyMappingConstructor1() returns boolean {
+    typedesc<any> t = typeof myFunction1;
+    A? annot = t.@v9;
+    if (annot is A) {
+        return annot.val == "ABC";
+    }
+    return false;
+}
+
+public annotation map<string> v10 on function;
+
+@v10
+function myFunction2() {
+
+}
+
+function testAnnotWithEmptyMappingConstructor2() returns boolean {
+    typedesc<any> t = typeof myFunction2;
+    map<string>? annot = t.@v10;
+    if (annot is map<string>) {
+        return annot == {};
+    }
+    return false;
+}
+
+public annotation map<string>[] v11 on function;
+
+@v11
+@v11
+function myFunction3() {
+
+}
+
+function testAnnotWithEmptyMappingConstructor3() returns boolean {
+    typedesc<any> t = typeof myFunction3;
+    map<string>[]? annot = t.@v11;
+    if (annot is map<string>[]) {
+        if (annot.length() != 2) {
+            return false;
+        }
+        map<string> annot1 = annot[0];
+        map<string> annot2 = annot[1];
+        return annot1 == {} && annot2 == {};
+    }
+    return false;
+}
+
+public annotation A[] v12 on function;
+
+@v12
+@v12
+function myFunction4() {
+
+}
+
+function testAnnotWithEmptyMappingConstructor4() returns boolean {
+    typedesc<any> t = typeof myFunction4;
+    A[]? annot = t.@v12;
+    if (annot is A[]) {
+        if (annot.length() != 2) {
+            return false;
+        }
+        A annot1 = annot[0];
+        A annot2 = annot[1];
+        return annot1.val == "ABC" && annot2.val == "ABC";
+    }
+    return false;
+}