Support for field annotations in local records and tuples #38488

chiranSachintha · 2022-11-01T03:40:26Z

Purpose

$title.

Check List

codecov · 2022-11-03T09:45:15Z

Codecov Report

Base: 76.83% // Head: 76.85% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (01f2595) compared to base (74e19c8).
Patch coverage: 96.03% of modified lines in pull request are covered.

Additional details and impacted files

@@                    Coverage Diff                    @@
##             annot-tuple-members   #38488      +/-   ##
=========================================================
+ Coverage                  76.83%   76.85%   +0.01%     
- Complexity                 53429    53469      +40     
=========================================================
  Files                       3350     3350              
  Lines                     199739   199889     +150     
  Branches                   25780    25800      +20     
=========================================================
+ Hits                      153475   153615     +140     
- Misses                     37676    37688      +12     
+ Partials                    8588     8586       -2

Impacted Files	Coverage Δ
.../ballerinalang/compiler/BIRPackageSymbolEnter.java	`89.05% <ø> (ø)`
...llerinalang/compiler/bir/codegen/JvmConstants.java	`100.00% <ø> (ø)`
.../compiler/semantics/model/symbols/BTypeSymbol.java	`87.50% <ø> (ø)`
...ina/runtime/internal/values/TypedescValueImpl.java	`81.25% <83.33%> (-0.24%)`	⬇️
...va/org/wso2/ballerinalang/compiler/bir/BIRGen.java	`92.39% <90.90%> (+<0.01%)`	⬆️
...llerinalang/compiler/desugar/ClosureGenerator.java	`78.96% <94.11%> (ø)`
...java/io/ballerina/runtime/internal/ValueUtils.java	`94.28% <100.00%> (+0.43%)`	⬆️
...lerina/runtime/internal/values/TupleValueImpl.java	`75.52% <100.00%> (+0.25%)`	⬆️
...nalang/compiler/bir/codegen/JvmInstructionGen.java	`89.77% <100.00%> (+0.02%)`	⬆️
...allerinalang/compiler/bir/codegen/JvmValueGen.java	`93.10% <100.00%> (+0.37%)`	⬆️
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

MaryamZi

Let's schedule a code review for this.

bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/api/values/BTypedesc.java

MaryamZi · 2022-11-03T06:54:56Z

bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/TypeChecker.java

+            if (annotation != null) {
+                return annotation;
+            }


If the annotation is null here, why do we need to go on to check in the type?

still, annotations related to record types are in the type. I didn't move those to the map created in typedesc.

Why didn't we move it?

No specific reason for that, Will add them to the annotation map created for each type, instead of the global map

@MaryamZi Currently, we create the typedesc for type definitions in a separate method. However, this approach leads to not being able to identify the necessary annotations for typedesc during creation. To resolve this issue, we need to make changes to move the generation of typedesc for type definitions to the init function, as previously discussed

bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TypedescValueImpl.java

compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/BIRGen.java

...ballerina-unit-test/src/test/resources/test-src/annotations/local_record_annotation_test.bal

…ina-lang into field-annotations-for-local-records

github-actions · 2022-11-29T19:54:06Z

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

github-actions · 2022-12-15T21:11:11Z

This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the stale label is removed or commented.

...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmConstants.java

...er/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmValueGen.java

MaryamZi

Have we added BIR tests yet? We need them for compile-time annotation access also. E.g., org.ballerinalang.test.bala.annotation.AnnotationTests#testParamAnnotAttachmentsViaBir

bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TypedescValue.java

compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/BIRGen.java

...iler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/BIRPackageSymbolEnter.java

...-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/model/symbols/BTypeSymbol.java

...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java

...-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/DataflowAnalyzer.java

MaryamZi · 2023-01-23T15:03:37Z

@chiranSachintha, org.ballerinalang.test.runtime.api.RuntimeAPITest is failing.

ballerina-new-parser-test-suite > jballerina-test > org.ballerinalang.test.runtime.api.RuntimeAPITest > testRuntimeAPIs[4](types) FAILED
    io.ballerina.runtime.internal.util.exceptions.BLangRuntimeException: error: Annotation is not available.
    	at testorg.types.typeref.1:checkInlineRecordAnnotations(typeref.bal:501)
    	   testorg.types.typeref.1:validateInlineRecordField(typeref.bal:494)
    	   testorg.types.typeref.1:validateTypeRef(typeref.bal:92)
    	   testorg.types.1:main(main.bal:24)
        at org.ballerinalang.test.BRunUtil.invoke(BRunUtil.java:206)
        at org.ballerinalang.test.BRunUtil.invokeOnJBallerina(BRunUtil.java:148)
        at org.ballerinalang.test.BRunUtil.invoke(BRunUtil.java:87)
        at org.ballerinalang.test.BRunUtil.invoke(BRunUtil.java:269)
        at org.ballerinalang.test.runtime.api.RuntimeAPITest.testRuntimeAPIs(RuntimeAPITest.java:36)

        Caused by:
        error("error: Annotation is not available.
        	at testorg.types.typeref.1:checkInlineRecordAnnotations(typeref.bal:501)
        	   testorg.types.typeref.1:validateInlineRecordField(typeref.bal:494)
        	   testorg.types.typeref.1:validateTypeRef(typeref.bal:92)
        	   testorg.types.1:main(main.bal:24)")

…atform/ballerina-lang into field-annotations-for-local-records

MaryamZi · 2023-01-24T04:49:50Z

bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TypedescValueImpl.java

@@ -84,13 +91,17 @@ public Object instantiate(Strand s) {
        return instantiate(s, new BInitialValueEntry[0]);
    }

+    public MapValue getAnnotations() {


Do we need this?

Removed in chiranSachintha#8

MaryamZi · 2023-01-24T04:51:56Z

compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/BIRGen.java

+    private BIROperand getAnnotations(BTypeSymbol typeSymbol, BIRGenEnv env) {
+        if (typeSymbol == null || typeSymbol.annotations == null) {
+            return null;
+        }
+        return new BIROperand(getAnnotations(typeSymbol.annotations, env));
+    }
+
+    private BIRVariableDcl getAnnotations(BVarSymbol annotations, BIRGenEnv env) {
+        if (env.symbolVarMap.containsKey(annotations)) {
+            return env.symbolVarMap.get(annotations);
+        }
+        return globalVarMap.get(annotations);
+    }


Move private function to the bottom.

MaryamZi · 2023-01-24T04:53:16Z

compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/BIRGen.java

+        visitTypedesc(pos, type, varDcls, null);
+    }
+
+    private void visitTypedesc(Location pos, BType type, List<BIROperand> varDcls, BIROperand annotations) {


Why do we need a separate method?

...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java

MaryamZi · 2023-01-24T06:08:26Z

...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java

+        BSymbol symbol = localVarRef.symbol;
+        if (encInvokable == null || (symbol.tag & SymTag.VARIABLE) != SymTag.VARIABLE) {
+            result = localVarRef;
+            return;


Codecov warning.

Updated(#39349)

...-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/model/symbols/BTypeSymbol.java

...na-unit-test/src/test/java/org/ballerinalang/test/annotations/LocalRecordAnnotationTest.java

MaryamZi · 2023-01-24T06:10:17Z

...ina-unit-test/src/test/java/org/ballerinalang/test/annotations/LocalTupleAnnotationTest.java

+        Assert.assertTrue(annot1 instanceof BMap);
+        Assert.assertTrue(annot2 instanceof BMap);


We should assert the values also.

I believe you can also use io.ballerina.runtime.api.utils.TypeUtils#getType and check by tag instead of the instanceof check.

Better yet, why not do these assertions also in Ballerina, similar to others?

Updated(#39349)

MaryamZi · 2023-01-24T06:12:45Z

...jballerina-unit-test/src/test/resources/test-src/annotations/local_tuple_annotation_test.bal

+function testGlobalAnnotationsOnFunctionPointerReturnType() {
+    map<any> m1 = getLocalTupleAnnotations(typeof x(), "$field$.0");


Test function name says global but this is local.

Btw we use module-level, not global.

Change name to testTupleAnnotationsOnFunctionPointerReturnType

MaryamZi

@chiranSachintha, please address suggestions in separate PRs.

warunalakshitha · 2023-01-24T11:01:01Z

...er/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmValueGen.java

@@ -429,6 +436,32 @@ private void createTypeDescConstructor(ClassWriter cw) {
        mv.visitEnd();
    }

+    private void createTypeDescConstructorWithAnnotations(ClassWriter cw, String name) {
+


Lets remove these new lines in the method. Better to remove with separate PR.

Updated(#39349)

warunalakshitha · 2023-01-24T11:01:46Z

Runtime changes LGTM

chiranSachintha requested review from warunalakshitha, sameerajayasoma and hasithaa as code owners November 1, 2022 03:40

chiranSachintha added 6 commits November 1, 2022 09:21

Generate closures for annotations of local record types

9593c36

Introduce details of annotations to typedesc instruction

9dd1884

Instantiate annotations using typedesc

d1e1e7c

Add support to access annotations using annotation map in typedesc

1f2b606

Add tests for local record annotations

8069fe1

Update version

5696819

chiranSachintha force-pushed the field-annotations-for-local-records branch from acee3c7 to 5696819 Compare November 1, 2022 03:52

chiranSachintha added the Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. label Nov 1, 2022

chiranSachintha requested review from suleka96, ushirask, MaryamZi and gimantha November 3, 2022 06:13

MaryamZi reviewed Nov 3, 2022

View reviewed changes

chiranSachintha added 5 commits November 14, 2022 11:54

Merge branch 'master' of https://github.com/ballerina-platform/baller…

347ed0f

…ina-lang into field-annotations-for-local-records

Move local annotation desugar logic to parameter desugar phase

ebc4c18

fix getting unused variable warning

d66a17c

fix review suggestions

e084377

Rename ParameterDesugar class as ClosureGenerator

b88ddd0

chiranSachintha force-pushed the field-annotations-for-local-records branch from 2d42177 to b88ddd0 Compare November 14, 2022 06:33

chiranSachintha added 2 commits November 14, 2022 20:22

Add annotation details to BIR

5429015

Update bir spec with annotation details

097c3eb

github-actions bot added the Stale label Nov 29, 2022

chiranSachintha removed the Stale label Nov 30, 2022

github-actions bot added the Stale label Dec 15, 2022

chiranSachintha added 3 commits January 23, 2023 15:07

Add unit test

65b7432

Add annotation for anonymous records

85a4717

Retrieved annotation from type

04d0403

warunalakshitha reviewed Jan 23, 2023

View reviewed changes

...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmConstants.java Outdated Show resolved Hide resolved

warunalakshitha reviewed Jan 23, 2023

View reviewed changes

...er/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmValueGen.java Outdated Show resolved Hide resolved

Add unit tests

ffa50c3

chiranSachintha force-pushed the field-annotations-for-local-records branch from 77e4305 to ffa50c3 Compare January 23, 2023 11:47

MaryamZi reviewed Jan 23, 2023

View reviewed changes

bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TypedescValue.java Outdated Show resolved Hide resolved

compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/BIRGen.java Outdated Show resolved Hide resolved

MaryamZi reviewed Jan 23, 2023

View reviewed changes

...iler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/BIRPackageSymbolEnter.java Outdated Show resolved Hide resolved

MaryamZi reviewed Jan 23, 2023

View reviewed changes

chiranSachintha added 7 commits January 23, 2023 21:43

Fix review suggestions

b4798a6

Update ksy file

294bcc7

fix getting unused variable warning with tuple annotation

bf4915b

Add tests for unused variable warnings

e14d9f1

Merge branch 'annot-tuple-members' of https://github.com/ballerina-pl…

ca112e1

…atform/ballerina-lang into field-annotations-for-local-records

Add bala test

e1da96f

Add annotation for anonymous records to global map

b9b862e

chiranSachintha requested review from MaryamZi, dulajdilshan and warunalakshitha January 24, 2023 04:31

Update bala tests

01f2595

MaryamZi reviewed Jan 24, 2023

View reviewed changes

MaryamZi approved these changes Jan 24, 2023

View reviewed changes

warunalakshitha reviewed Jan 24, 2023

View reviewed changes

warunalakshitha approved these changes Jan 24, 2023

View reviewed changes

MaryamZi merged commit c219653 into ballerina-platform:annot-tuple-members Jan 24, 2023

This was referenced Jan 27, 2023

Support field annotations for local records and tuples #37241

Closed

[Bug]: Getting unused variable warning although the variable used in the annotation of local record field #38487

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for field annotations in local records and tuples #38488

Support for field annotations in local records and tuples #38488

chiranSachintha commented Nov 1, 2022 •

edited

Loading

codecov bot commented Nov 3, 2022 •

edited

Loading

MaryamZi left a comment

MaryamZi Nov 3, 2022

chiranSachintha Nov 10, 2022

MaryamZi Dec 21, 2022

chiranSachintha Jan 18, 2023

chiranSachintha Jan 22, 2023 •

edited

Loading

github-actions bot commented Nov 29, 2022

github-actions bot commented Dec 15, 2022

MaryamZi left a comment

MaryamZi commented Jan 23, 2023

MaryamZi Jan 24, 2023

chiranSachintha Jan 24, 2023

MaryamZi Jan 24, 2023

MaryamZi Jan 24, 2023

MaryamZi Jan 24, 2023

chiranSachintha Jan 24, 2023

MaryamZi Jan 24, 2023

MaryamZi Jan 24, 2023

MaryamZi Jan 24, 2023

chiranSachintha Jan 24, 2023

MaryamZi Jan 24, 2023

chiranSachintha Jan 24, 2023

MaryamZi left a comment

warunalakshitha Jan 24, 2023

chiranSachintha Jan 24, 2023

warunalakshitha commented Jan 24, 2023

		Assert.assertTrue(annot1 instanceof BMap);
		Assert.assertTrue(annot2 instanceof BMap);

		function testGlobalAnnotationsOnFunctionPointerReturnType() {
		map<any> m1 = getLocalTupleAnnotations(typeof x(), "$field$.0");

Support for field annotations in local records and tuples #38488

Support for field annotations in local records and tuples #38488

Conversation

chiranSachintha commented Nov 1, 2022 • edited Loading

Purpose

Check List

codecov bot commented Nov 3, 2022 • edited Loading

Codecov Report

MaryamZi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiranSachintha Jan 22, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Nov 29, 2022

github-actions bot commented Dec 15, 2022

MaryamZi left a comment

Choose a reason for hiding this comment

MaryamZi commented Jan 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaryamZi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warunalakshitha commented Jan 24, 2023

chiranSachintha commented Nov 1, 2022 •

edited

Loading

codecov bot commented Nov 3, 2022 •

edited

Loading

chiranSachintha Jan 22, 2023 •

edited

Loading