-
Notifications
You must be signed in to change notification settings - Fork 738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generate closures for default values in type inclusions #42428
Generate closures for default values in type inclusions #42428
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #42428 +/- ##
============================================
+ Coverage 76.64% 76.82% +0.17%
- Complexity 53437 53971 +534
============================================
Files 2902 2924 +22
Lines 201848 203904 +2056
Branches 26271 26592 +321
============================================
+ Hits 154706 156646 +1940
- Misses 38651 38718 +67
- Partials 8491 8540 +49 ☔ View full report in Codecov by Sentry. |
.../resources/test-src/bala/test_projects/test_project_records/closed_record_type_reference.bal
Outdated
Show resolved
Hide resolved
...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java
Outdated
Show resolved
Hide resolved
@@ -39,3 +39,8 @@ public type ClosedVehicleWithNever record {| | |||
int j; | |||
never p?; | |||
|}; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add more tests
- with multiple levels of inclusion: e.g.,
i. baz:C includes bar:B which includes foo:A, where the default value is specified in foo
ii. baz:C includes bar:B which includes foo:A, where the default value is specified in bar - to make sure the default value is used only when a value is not specified
- a record includes two records with the same field (where one or both have a default), but the including record has to specify the field explicitly then. In such cases, add tests for both when the including record's field has a default value and doesn't have a default value. In either case default values from the included records shouldn't be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added with 4d48e7a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't testing 2 (in conjunction with 3 also) are we? I guess that's OK since we have tests for the normal case.
Please add a test like 1, where foo:A has a default for a field which is overridden by bar:B, also with a default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added with 6094bb8
375981d
to
4d48e7a
Compare
...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java
Outdated
Show resolved
Hide resolved
...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java
Outdated
Show resolved
Hide resolved
...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java
Outdated
Show resolved
Hide resolved
...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java
Outdated
Show resolved
Hide resolved
BTypeSymbol typeSymbol) { | ||
Map<String, BInvokableSymbol> defaultValues = ((BRecordTypeSymbol) typeSymbol).defaultValues; | ||
String typeName = recordTypeNode.symbol.name.value; | ||
for (BLangType type : recordTypeNode.typeRefs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to be creating quite a few closures unnecessarily.
- Isn't the problem only with inclusions from a different module? Why are we creating closures for defaults from all inclusions?
For the following points, inclusions can be from different modules once 1. is addressed.
- For
type Foo record {|
int a = 1;
int b = 2;
int c = 3;
|};
type Bar record {|
*Foo;
int b = 22;
int c;
|};
We are creating a closure wrapping c's default value closure unnecessarily. That default value closure (Foo's c field's) is not required here.
- For
type Foo record {|
int a = 1;
|};
type Bar record {|
*Foo;
|};
type Baz record {|
*Foo;
|};
closures wrapping a's closure are created twice. Can't we avoid (or at least minimize) this using some combination of the included record's fully-qualified identifier and field name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, Issue when type inclusion in a different module. Earlier, I created closures for when it's in the same module to keep the logic simple. I've updated that.
Do we need to skip the second and third scenarios? We need to do extra checks for those. But these scenarios are very rare, so do we need to skip them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would fix at least 2. Easy fix, right? Just need to check by explicitly-specified fields rather than those with defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Will update that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Udapted 52836fd
result = recordTypeNode; | ||
} | ||
|
||
private void generateClosuresForDefaultValuesInTypeRefs(BLangRecordTypeNode recordTypeNode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment explaining why we are doing this, mention that it's temporary, and point to relevant issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
Fixed with #41949
499274c
to
09915f1
Compare
//In the BIR, we generate instruction for closures when visiting the lambda function. | ||
//Due to that, if the inclusions are in different modules, closures are not created for default values. | ||
//Fixed with #41949 issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the issue more about not being able to find that closure? Can we rephrase. Mention the workaround we are adopting here too.
Add a space after //.
I would also say "Will be fixed with ..." or "Tracked with ..." instead of "Fixed with ...". since if it is fixed, we don't need the workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. please check
...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java
Outdated
Show resolved
Hide resolved
...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java
Outdated
Show resolved
Hide resolved
@@ -39,3 +39,8 @@ public type ClosedVehicleWithNever record {| | |||
int j; | |||
never p?; | |||
|}; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't testing 2 (in conjunction with 3 also) are we? I guess that's OK since we have tests for the normal case.
Please add a test like 1, where foo:A has a default for a field which is overridden by bar:B, also with a default.
...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java
Outdated
Show resolved
Hide resolved
...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java
Outdated
Show resolved
Hide resolved
...r/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureGenerator.java
Outdated
Show resolved
Hide resolved
@@ -1050,7 +1097,8 @@ public void visit(BLangCompoundAssignment compoundAssignment) { | |||
public void visit(BLangInvocation invocation) { | |||
rewriteInvocationExpr(invocation); | |||
BLangInvokableNode encInvokable = env.enclInvokable; | |||
if (encInvokable == null || !invocation.functionPointerInvocation) { | |||
if (encInvokable == null || !invocation.functionPointerInvocation || | |||
!env.enclPkg.packageID.equals(invocation.symbol.pkgID)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have had cases like this previously too, right? When the default value is a function call of a function from a different module? Can we add such a test also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added test with c681a7f
186a393
to
c681a7f
Compare
.../src/test/resources/test-src/bala/test_bala/functions/test_different_function_signatures.bal
Show resolved
Hide resolved
.../src/test/resources/test-src/bala/test_bala/functions/test_different_function_signatures.bal
Outdated
Show resolved
Hide resolved
38b08a8
to
cbddffd
Compare
@@ -35,6 +35,7 @@ public class MapConstantEqualityInDifferentBalaTest { | |||
|
|||
@BeforeClass | |||
public void setup() { | |||
BCompileUtil.compileAndCacheBala("test-src/bala/test_projects/test_project_utils"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that what we are introducing is a simple test, I would extract that test and packages out separately, and avoid these unnecessary compilations for other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chiranSachintha, please send a separate PR fixing this, will merge this for now since it is required for U9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the test case
cbddffd
to
dfde2e3
Compare
7782a44
into
ballerina-platform:master
Purpose
Fixes #42356
Approach
Samples
Remarks
Check List