-
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
Remove casts to imported anonymous types and add a validation to disallow the same #28462
Remove casts to imported anonymous types and add a validation to disallow the same #28462
Conversation
Also changes a loop that assumes langlib methods are always called on a value of the same type as the langlib.
BLangExpression expr = valueAccessExpr.expr; | ||
// Since `$result$` is always a record (with a single `value` field), add a cast to `map<any|error>` | ||
// to avoid casting to an anonymous type - https://github.com/ballerina-platform/ballerina-lang/issues/28262. | ||
valueAccessExpr.expr = addConversionExprIfRequired(expr, symTable.mapAllType); |
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.
Out of curiosity, what happens if we don't have this conversion here? Issues in jvm codegen or does it get re-added somehow via desugaring the desugared constructs?
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.
So the type of expr
($result$
) here is the union of a record and nil - record {|T value;|}?
. Although semantically it is invalid to do $result$.value
in such a scenario, desugaring proceeds as a field access on a record union due to the check at https://github.com/ballerina-platform/ballerina-lang/blob/master/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/desugar/Desugar.java#L5171. And then at org.wso2.ballerinalang.compiler.bir.codegen.JvmInstructionGen#generateMapLoadIns we don't seem to be adding a cast so probably works as is there?
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.
Better if we can add a positive test case which was failing previously with anonemous type cast.
Any suggestions on how we can add such a test? There are several conditions that needs to be met. For example if we consider using
|
Will go ahead and merge this for now, will add a test in a separate PR if we can figure out a way to test this. |
Purpose
$title, as a temporary fix for #28262.
Not sure if we can write a test for either change.
Check List