Fix native JsMethods with varargs called from Java varargs methods

[niloc132]

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]

Full build https://github.com/niloc132/gwt/actions/runs/9088542441

[niloc132]

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]

I think the concerns above are addressed - the native property ref sideeffect has been backed out, and the JDT issue is resolved, though more tests need to be written to confirm. Here's the fully build succeeding with the latest JDT change: https://github.com/niloc132/gwt/actions/runs/9586615514

The fix revolves around the fact that there are subtypes of MethodBinding that didn't previously exist - using MethodBinding.original() we can get back the underlying method so that we see that the method is varargs (so may need an array creation synthesized). However, original() also discards generic parameterization, so types no longer match, so this must be done somewhat carefully, and more tests are needed to confirm that the behavior and output is correct. At the very least, tests should include:

• varargs with Object... (as this will accept all objects), tested with null, empty, one argument, several arguments, array
• repeat with a non-vararg param
• varargs with type args, where the typearg is unrelated to the vararg param
• varargs with type args, where the typearg is used to specify the vararg param (like Arrays.asList)

Unfortunately, GWT has effectively no tests for simple Java features, only indirect tests that often expose problems. I assume either that these tests exist but were never ported to open source, or that they were not needed since Google had enough code written using GWT that such problems were discovered on their own. Nevertheless, it makes bugs like this and switch/case hard to hunt down, since simple tests like "are varargs methods called correctly" or "does the correct case get used in a switch" don't seem to exist in their plainest form.

[vegegoku]

Since we dropped support for java 7 and now we only support java 8+ wont it make sense to start using newer syntax like Objects.nonNull(...) and Objects.isNull(...)

[niloc132]

Specifically, we dropped support for running this test on Java 7 - the code may or may not still run, but the test won't pass. We are not yet compiling code to use Java 9+ APIs, but could use Java features like that.

That said, two thoughts:

• isNull() is great to use as a lambda, but it any better than the != null? It appears to be the same number of chars (assuming you add spaces, and assuming you do a static import), but will add the cost of a method call (until it is inlined by the jit, etc). As I understand it, the purpose is mostly for passing as a method reference...?
• If I'm not changing the logic (I only moved the null check up a line), I'd rather not change the code to another way of doing the same thing, all else equals.

[vegegoku]

Same here Objects.isNull

[niloc132]

Objects.equals() probably? I think that would be better than what I have here now, as a replacement for ==. I've never seen this fail, but it certainly could, and then would break here, which is why I changed it.

[vasvir]

Sorry if this is irrelevant:

Is this patch close to the issue #9675 ? Or is it in a different part of the code?

[jnehlmeier]

@vasvir This patch fixes JsMethod varargs methods, e.g. console.log(Object... o), which caused compiler errors. This patch does not change how GWT generates JS code for Java lambdas.

[niloc132]

@vasvir I think the issue you linked has an accurate summary here - there isn't a nice way to do this outside of the ones you've presented.

Getting into the details of "what does the Function object say about how you can call it" seem fraught - either we need an extra wrapper per function (Roberto's solution of a new wrapper per func), a huge runtime cost (your option 2 or 3 solution) or we need to be satisfied with only 0-10 args, and never preserving the name of the arguments (your option 1). While 0-10 (or maybe 0-5? 0-50?) might suffice for your own work, it would break JS projects/tools that inspect the names params would still break (angular does this for example for its DI feature).

Another option not included in your list is to get rid of the runtime arg count check and instead have your 11 (or 51, etc) makeLambdaArgumentN functions in Runtime.java, and tweak GenerateJavaScriptAST.constructJsFunctionObject

gwt/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java

Lines 1010 to 1020 in a53bb51

	private JsExpression constructJsFunctionObject(SourceInfo sourceInfo, JClassType type,
	JsName ctorName, JsNameRef ctorReference, JsExpression ctorArguments) {
	// Foo.prototype.functionMethodName
	JMethod jsFunctionMethod = getJsFunctionMethod(type);
	JsNameRef funcNameRef = JsUtils.createQualifiedNameRef(sourceInfo,
	ctorName, prototype, polymorphicNames.get(jsFunctionMethod));
	// makeLambdaFunction(Foo.prototype.functionMethodName, new Foo(...))
	return constructInvocation(sourceInfo, RuntimeConstants.RUNTIME_MAKE_LAMBDA_FUNCTION,
	funcNameRef, ctorReference, ctorArguments);
	}

to instead dispatch based on arg count, so the work is done at compile time.

In theory this could be an optional feature this way, and the 99% of apps that have never wanted this don't need to be impacted by this case.