-
Notifications
You must be signed in to change notification settings - Fork 754
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 a typedesc instruction once for record and tuple types #39101
Generate a typedesc instruction once for record and tuple types #39101
Conversation
compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/BIRGen.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Override | ||
public void visit(BLangFunctionTypeNode functionTypeNode) { |
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.
Don't we have to visit parameter and return types?
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.
It is not possible to assign a value to the parameters of a function node. And also no usage in creating a type descriptor for the return value
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.
What about
public function main() {
function (record {| float f; |}) returns record {| string b = "hello"; |} y;
}
Where are the new typedesc instructions generated for these records?
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.
Didn't generate typedesc
for this because there is no usage of defining typedescs for these records.
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.
What do you mean no usage?
import ballerina/io;
annotation record {| float f = foo(); |} annot on record field;
public function main() {
function (record {| @annot float f; |}) r;
}
isolated function foo() returns float {
io:println("foo");
return 1.0;
}
This should print "foo"
, but it doesn't atm.
Either way, we shouldn't make assumptions about usage. The spec defines when the typedesc is resolved, and we have to resolve it at that point.
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.
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 assume we need changes in #38488 for the sample I shared above to work as expected? We need to make sure we have tests for every possible type node scenario.
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.
Did we add this as a test in the other PR?
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, any update on this? Doesn't seem to work for me with the other PR also, will recheck.
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.
Can you send a draft PR with changes from both PRs, resolving conflicts?
if (field.typeNode != null) { | ||
field.typeNode.accept(this); | ||
} | ||
} |
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.
How about the param/return types in methods?
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.
When defining an object, the methods of the object are added to a function list and when visiting the function list, type descriptors for the parameters and return type will be created(no need to generate type descriptors here).
compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/BIRGen.java
Show resolved
Hide resolved
...ler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/ClosureDesugar.java
Outdated
Show resolved
Hide resolved
...ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/model/BIRNonTerminator.java
Show resolved
Hide resolved
...lerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmInstructionGen.java
Outdated
Show resolved
Hide resolved
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #39101 +/- ##
============================================
+ Coverage 76.35% 76.83% +0.47%
- Complexity 52520 53442 +922
============================================
Files 2881 3347 +466
Lines 198492 199736 +1244
Branches 25807 25813 +6
============================================
+ Hits 151565 153464 +1899
+ Misses 38563 37680 -883
- Partials 8364 8592 +228
☔ View full report in Codecov by Sentry. |
This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the |
Closed PR due to inactivity for more than 18 days. |
…ina-lang into new-typedesc # Conflicts: # compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/split/types/JvmRecordTypeGen.java
This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the |
This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the |
Closed PR due to inactivity for more than 18 days. |
This PR has been open for more than 15 days with no activity. This will be closed in 3 days unless the |
I have opened a new PR for this change as we have modified the logic(#41945) |
Purpose
Fixes #38844
Check List