-
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
Improve runtime anonymous type resolution #29792
Conversation
… into fix-28262-v2
… into fix-28262-v2
I think we need to add the string equals test too to avoid hash collusions. Similar to type T record {|
int i;
string j;
|}; Generated (decompiled to java) String var2 = ((BString)var1).getValue();
switch(var2.hashCode()) {
case 105:
if (var2.equals("i")) {
return this.i;
}
break;
case 106:
if (var2.equals("j")) {
return this.j;
}
}
return super.get(var1);
} |
...ler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmTypeGen.java
Outdated
Show resolved
Hide resolved
+1 for @rdhananjaya 's suggestion, but since current hash calculator ( |
@manuranga, @rdhananjaya I'll add the hash validation in a separate PR. (issue to track this: #29928) |
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.
This is before the last commit to avoid generating signatures for the same module.
This means for the similar anonymous types we have the same signature. Is this expected?
public type Address object {
public string city;
public string country;
public function value() returns string;
};
public type Address3 object {
public string city;
public string country;
public function value() returns string;
};
public function main() {
var address = object Address {
// In object constructor expressions, the `init` method does not take any parameters.
public function init() {
self.city = "Colombo";
self.country = "Sri Lanka";
}
public function value() returns string {
return self.city + ", " + self.country;
}
};
var address2 = object Address3 {
// In object constructor expressions, the `init` method does not take any parameters.
public function init() {
self.city = "Colombo";
self.country = "Sri Lanka";
}
public function value() returns string {
return self.city + ", " + self.country;
}
};
}
Here for both
final BObjectType duplicate = ((BObjectType)new $_init().getAnonType(-203469200, "Package: $_init, TypeName: $type$$anonType$_0, Shape: object { public string city; public string country; public function value () returns (string); }")).duplicate();
final BObjectType duplicate2 = ((BObjectType)new $_init().getAnonType(-203469200, "Package: $_init, TypeName: $type$$anonType$_1, Shape: object { public string city; public string country; public function value () returns (string); }")).duplicate();
@Override
public Type getAnonType(final int n, final String str) {
switch (n) {
case -203469200: {
return $_init.$type$$anonType$_0;
}
default: {
throw new ErrorValue(StringUtils.fromString("No such type: " + str));
}
}
}
@KRVPerera yes. We have only considered the shape. So, such situations can happen. Do you think this can lead to errors? |
Think it is okay. We are taking type ids also to the hash. Sorry for the noise. |
I really wasn't aware whether there can be any issues by treating similar shapes as one. That's why. :) Anyway, we didn't use the type name initially, because if there's an anon type name and if we capture that as part of the shape, there's a possibility for that anon name to change later on (since we are using incremental anon type name generator, like in the root issue), and cause the hash to change as well. |
...ler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmTypeGen.java
Outdated
Show resolved
Hide resolved
...g/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/internal/TypeHashComparator.java
Outdated
Show resolved
Hide resolved
...a-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeHashVisitor.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
...a-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/TypeHashVisitor.java
Outdated
Show resolved
Hide resolved
…ompiler/bir/codegen/internal/TypeHashComparator.java Co-authored-by: Maryam Ziyad <maryamziyadm@gmail.com>
…ompiler/semantics/analyzer/TypeHashVisitor.java Co-authored-by: Maryam Ziyad <maryamziyadm@gmail.com>
...ler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmTypeGen.java
Show resolved
Hide resolved
… into fix-28262-v2
… into fix-28262-v2
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.
Approving since review suggestions are added.
Purpose
The purpose of this PR is to $title.
Fixes #28262
Approach
In this PR, it'll code-gen
public Type getAnonType(int hash, String shapeInfo)
method into module init, and that will allow type resolution like shown below.generated method:
generated usage:
In case, if the type couldn't be resolved, a runtime error similar to the following will be thrown.
Samples
n/a
Remarks
Check List