[Master] Reduce creating unwanted type definitions for const annotations #40964

prakanth97 · 2023-07-10T07:12:06Z

Purpose

Approach

According to the spec, information related to source annotation shouldn't be propagated to runtime. So, we removed all the type definitions created for constant values of source annotation.
Remove unnecessary type definitions created for constant annotations. It reduced by approximately half.

Samples

For the following code, the jar size is reduced from 9.773571 Mb to 7.729598 Mb (20.913 %). Tried the native image size for same code in mac is reduced from 41.151857 Mb to 31.244385 Mb (24.075 %).

public function main() {
    int numbers = addNumbers(1, 2);
}

function addNumbers(int i, int j) returns int {
    return i + j;
}

Remarks

List any other known issues, related PRs, TODO items, or any other notes related to the PR.

Check List

…ina-lang into issue_40914

codecov · 2023-07-12T10:59:41Z

Codecov Report

Patch coverage: 97.67% and no project coverage change.

Comparison is base (b2f1436) 76.63% compared to head (24d343b) 76.63%.

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #40964   +/-   ##
=========================================
  Coverage     76.63%   76.63%           
- Complexity    55494    55509   +15     
=========================================
  Files          3393     3393           
  Lines        208749   208779   +30     
  Branches      27035    27043    +8     
=========================================
+ Hits         159965   159999   +34     
+ Misses        39954    39949    -5     
- Partials       8830     8831    +1

Impacted Files	Coverage Δ
...c/main/java/org/wso2/ballerinalang/util/Flags.java	`95.20% <ø> (ø)`
...iler/semantics/analyzer/ConstantValueResolver.java	`45.24% <93.75%> (+1.10%)`	⬆️
...lerinalang/compiler/bir/codegen/JvmPackageGen.java	`89.66% <100.00%> (+0.07%)`	⬆️
...lerinalang/compiler/bir/writer/BIRWriterUtils.java	`96.63% <100.00%> (+0.14%)`	⬆️
...ng/compiler/semantics/analyzer/SymbolResolver.java	`80.83% <100.00%> (+0.09%)`	⬆️
...lerinalang/diagramutil/SyntaxTreeMapGenerator.java	`89.00% <100.00%> (+0.11%)`	⬆️

... and 7 files with indirect coverage changes

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

.../src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/ConstantValueResolver.java

compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/util/Flags.java

.../src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/ConstantValueResolver.java

hasithaa · 2023-07-13T05:57:52Z

Can you add a test case, where you have a record type definition which used for both annotation purposes and used for normal value creations?

prakanth97 · 2023-07-13T07:54:07Z

Can you add a test case, where you have a record type definition which used for both annotation purposes and used for normal value creations?

Added c6e9721

…ina-lang into issue_40914

MaryamZi · 2023-07-17T09:24:08Z

tests/jballerina-unit-test/src/test/resources/test-src/typedefs/type-definitions.bal

+    string value;
+|};
+
+const annotation Data dataAnon on type;


Anon or Annot?

MaryamZi · 2023-07-17T09:24:50Z

tests/jballerina-unit-test/src/test/resources/test-src/typedefs/type-definitions.bal

+    Person2 foo = {name: "James"};
+    typedesc<any> t = typeof foo;
+    Data? annon = t.@dataAnon;
+    assertEquality("{\"value\":\"T1\"}", annon.toString());


Generally better to assert the value rather than the string representation IMO (since it'll take types also into account).

MaryamZi · 2023-07-17T09:28:09Z

tests/jballerina-unit-test/src/test/resources/test-src/typedefs/type-definitions.bal

+    Data data = {value: "T2"};
+    assertEquality("{\"value\":\"T2\"}" , data.toString());


Is this to test the availability of Data at runtime? If so, aren't tests that actually validate against Data better? E.g., positive and negative is checks. Can also use a record with a default, use cloneWithType with it with a value that doesn't have a field for the field with the default, and test that the default is used.

MaryamZi · 2023-07-17T09:30:38Z

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

@@ -2588,6 +2589,15 @@ public void populateAnnotationAttachmentSymbol(BLangAnnotationAttachment annotat
                                                                                 constantSymbol);
    }

+    private boolean isSourceAnonOnly(Set<AttachPoint> attachPoints) {


Need to change where called also.

MaryamZi · 2023-07-17T09:35:11Z

.../src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/ConstantValueResolver.java

@@ -667,8 +672,8 @@ private void updateConstantType(BConstantSymbol symbol, BLangExpression expr, Sy
            return;
        }

-        if (resolvedType.getKind() == TypeKind.INTERSECTION && isListOrMapping(type.tag)) {
-            expr.setBType(((BIntersectionType) resolvedType).effectiveType);
+        if (resolvedType.getKind() == TypeKind.RECORD && isListOrMapping(type.tag)) {


Was this intersection only for records? Not for lists (tuples)?

Since this fix is only for the constant annotation. Then record would be the only possible case not tuple. Right?

I was thinking about when there's a list as a member (field) of the annotation? I don't think we get here for that though.

const annotation map<map<string>[]> annot on source var; @annot { x: [{a: "hello", b: "world"}] } var val = 1;

MaryamZi · 2023-07-17T09:36:19Z

.../src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/ConstantValueResolver.java

+    private void updateRecordFields(BRecordType recordType, Location pos, SymbolEnv env) {
+        BTypeSymbol structureSymbol = recordType.tsymbol;
+        for (BField field : recordType.fields.values()) {
+            field.type = ImmutableTypeCloner.getImmutableType(pos, types, field.type, env,


Within this we still create type defs, right? E.g., mapping within mapping. Shall we create an issue to avoid this also as a future improvement?

Reduce creating type defns for const annotations

17a928f

prakanth97 force-pushed the issue_40914 branch from b80d2b3 to 589b466 Compare July 10, 2023 17:03

Update failing tests

9158364

prakanth97 force-pushed the issue_40914 branch from 589b466 to 9158364 Compare July 11, 2023 03:11

Fix SpotBugs failure

ca9413d

prakanth97 force-pushed the issue_40914 branch from 8c9b61b to a7cedec Compare July 11, 2023 07:16

Remove typeDefinition for source annotation in jar

87301b8

prakanth97 force-pushed the issue_40914 branch from a7cedec to 87301b8 Compare July 11, 2023 07:21

prakanth97 marked this pull request as ready for review July 11, 2023 07:27

prakanth97 requested review from sameerajayasoma and hasithaa as code owners July 11, 2023 07:27

prakanth97 changed the title ~~Reduce creating type definitions for const annotations~~ Reduce creating unwanted type definitions for const annotations Jul 11, 2023

prakanth97 added the Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. label Jul 11, 2023

prakanth97 marked this pull request as draft July 11, 2023 07:53

prakanth97 added 2 commits July 12, 2023 11:05

Fix BIRSpecTest failures

ae1ad75

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

3ccb8c1

…ina-lang into issue_40914

prakanth97 marked this pull request as ready for review July 12, 2023 06:31

Remove unwanted changes to fix list const tests

e626030

prakanth97 force-pushed the issue_40914 branch from 4f26767 to e626030 Compare July 12, 2023 09:20

ushirask reviewed Jul 13, 2023

View reviewed changes

.../src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/ConstantValueResolver.java Outdated Show resolved Hide resolved

ushirask reviewed Jul 13, 2023

View reviewed changes

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

prakanth97 changed the title ~~Reduce creating unwanted type definitions for const annotations~~ [Master] Reduce creating unwanted type definitions for const annotations Jul 13, 2023

lochana-chathura reviewed Jul 13, 2023

View reviewed changes

.../src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/ConstantValueResolver.java Outdated Show resolved Hide resolved

hasithaa reviewed Jul 13, 2023

View reviewed changes

.../src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/ConstantValueResolver.java Outdated Show resolved Hide resolved

Address review suggestions

c6e9721

prakanth97 force-pushed the issue_40914 branch from 8cb4975 to c6e9721 Compare July 13, 2023 07:53

prakanth97 requested a review from hasithaa July 13, 2023 07:54

prakanth97 added 2 commits July 13, 2023 13:51

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

ade619b

…ina-lang into issue_40914

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

5c09a46

…ina-lang into issue_40914

prakanth97 added this to the 2201.8.0 milestone Jul 14, 2023

Resolve merge conflicts

24d343b

hasithaa approved these changes Jul 17, 2023

View reviewed changes

hasithaa merged commit d7ad710 into ballerina-platform:master Jul 17, 2023
17 checks passed

MaryamZi reviewed Jul 17, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Master] Reduce creating unwanted type definitions for const annotations #40964

[Master] Reduce creating unwanted type definitions for const annotations #40964

prakanth97 commented Jul 10, 2023 •

edited

Loading

codecov bot commented Jul 12, 2023 •

edited

Loading

hasithaa commented Jul 13, 2023

prakanth97 commented Jul 13, 2023

MaryamZi Jul 17, 2023 •

edited

Loading

MaryamZi Jul 17, 2023

MaryamZi Jul 17, 2023

MaryamZi Jul 17, 2023

MaryamZi Jul 17, 2023

MaryamZi Jul 17, 2023

prakanth97 Jul 17, 2023

MaryamZi Jul 17, 2023

MaryamZi Jul 17, 2023

		Data data = {value: "T2"};
		assertEquality("{\"value\":\"T2\"}" , data.toString());

[Master] Reduce creating unwanted type definitions for const annotations #40964

[Master] Reduce creating unwanted type definitions for const annotations #40964

Conversation

prakanth97 commented Jul 10, 2023 • edited Loading

Purpose

Approach

Samples

Remarks

Check List

codecov bot commented Jul 12, 2023 • edited Loading

Codecov Report

hasithaa commented Jul 13, 2023

prakanth97 commented Jul 13, 2023

MaryamZi Jul 17, 2023 • edited Loading

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

prakanth97 commented Jul 10, 2023 •

edited

Loading

codecov bot commented Jul 12, 2023 •

edited

Loading

MaryamZi Jul 17, 2023 •

edited

Loading