Skip to content
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

Fix native JsMethods with varargs called from Java varargs methods #9957

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

niloc132
Copy link
Contributor

Native jsinterop varargs calls of the form instance.method(argsArray) sometimes implemented using instance.method.apply(instance, argsArray), where instance represents some expression. JsUtils.createApplyInvocation assumes that ImplementJsVarargs has already created a local variable to ensure that no side effects can take place when cloning the "instance", but ImplementJsVarargs doesn't actually check if a side effect was able to happen.

This fix only adds an additional check, creating a local variable in the case where the instance expression has side effects, and clarifies that a native JsProperty might have sideeffects (as it could call a JS property accessor).

An additional fix is made to JsSafeCloner, to let it successfully clone a qualified reference (e.g. $wnd.console) rather than produce an invalid JS AST. This ended up not being strictly required for the fix, but appears to be more correct and will at least generate code rather than a NullPointerException in a later location.

Fixes #9932

@niloc132
Copy link
Contributor Author

@niloc132 niloc132 added this to the 2.12 milestone May 15, 2024
Comment on lines +95 to +97
if (getField().isJsNative()) {
return false;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just plain backwards - I think I inverted it at some point for testing purposes - but I think I'd prefer to remove it entirely.

The idea was supposed to be as a way to trigger ImplementJsVarargs own "if this has side effects I should only eval it once" in a nicer way than assuming it recognizes all qualified name references (since its failure to do so caused the original bug). However, this ends up having other side effects, like preventing a lot of dead code elimination from working correctly.

The "hasSideEffects" check itself in ImplementJsVarargs isn't wrong, but this particular attempt at a safeguard is. The comment in ImplementJsVarargs should also be corrected.

@niloc132
Copy link
Contributor Author

niloc132 commented Jun 3, 2024

This patch has a few issues with it - the native property ref issue is one, discussed above, but also that the fix is incomplete. While this does appear to address the long-standing issue (at least since 2.9, probably older), it seems that the recent JDT update changes how varargs are handled in some small way. Perhaps that issue could be handled separately, but they seem somewhat linked - when I understand what is happening there I'll see if it makes sense to land them together or apart.

@niloc132 niloc132 marked this pull request as draft June 3, 2024 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in jsinterop varargs causing internal compiler error
1 participant