GROOVY-10099 Behave when varargs method called with null single argument.

[StrangeNoises]

Fix ambiguous behaviour when varargs methods are called with a single varargs argument, which may evaluate to null. Do it while compiling, when we can (often) tell if a given argument expression will be an array or not, and if clearly not, wrap it in an array. Do nothing for @CompileStatic, which already exhibits consistent behaviour; this makes @CompileDynamic behaviour consistent with @CompileStatic in conditions where the argument type can be known.

And if all else fails (the argument expression is irretrievably dynamic so we couldn't help), the Groovy coder can now force the behaviour they expected with a cast-or-coerce. (It wouldn't have helped before.)

The only existing tests that have been changed are those that call a varargs method with a single null constant, as that behaviour is now changed to accept that as a null object, in line with @CompileStatic behaviour. Many new test lines have been added to VarargsMethodTest to pick through the various situations that can arise.

[eric-milles]

There is a lot of discussion under GROOVY-10099 to unpack. And then this commit presents quite a bit more in the comments and hundreds of lines of code to sift through. Is it possible to break this down into smaller parts? Like is there one test case that is the biggest source of confusion and is there a small change that can make a non-breaking improvement?

I just don't see how we could accept a change of this size when the issue ticket itself was in dispute. Best to pick up a smaller, non-controversial ticket to start with.

[StrangeNoises]

small point of order as I'm new here (I'll get to the substantive points later): am i supposed to be doing something about that failed Distribute action in the checks? The same command succeeds here on my usual dev system (Mac OS Big Sur (intel)) and another Ubuntu 20.04 system (both using java 11.0.11+9). And besides it looks really nothing to do with any code changes, but rather maybe a transient issue fetching a couple of dependencies on the build box? (Also of course, noted the failure to find a hard-coded path but that doesn't seem to be triggering the failure)

[paulk-asert]

@StrangeNoises Don't worry about that intermittent build failure

[paulk-asert]

Excellent work Rachel. You certainly dived deep for your first PR! As Eric said, it will take us some time to digest. I was at first expecting the following program to exhibit the same behavior under Java and Groovy (with your changes):

import static org.codehaus.groovy.runtime.DefaultGroovyMethods.dump;

public class Main {
    public static void method(Object... args) {
        System.out.println(dump(args));
        if (args != null) {
            System.out.println(args.length);
            if (args.length > 0) {
                System.out.println(args[0]);
            }
        }
    }

    public static void main(String[] args) {
        method(null);
        method((Object) null);
        method((Object[]) null);
    }
}

But I see you aligned with statically compiled Groovy rather than Java for the no cast case and for that case we have different behavior. I will need to work through our precedence conventions and convince myself that we have that edge case right for statically compiled Groovy. There are a few spots where documentation also needs some finessing but we can cover that once we have sorted the behavior.

[StrangeNoises]

Yes, I'm afraid I wasn't trawling the issues list for something I thought I could handle. 😉 This was a reaction to a real problem I hit integrating Groovy scripting into our software, which we want to do so we can hire people who either have, or want, Groovy on their resumé rather than our obscure and older-than-groovy scripting language! So there's an acceptance issue when we start hitting NPEs caused by this, where the equivalent java version just works.

Yes, Java won't let you compile that with just the plain null constant; you have to choose: null object or null array. I noticed Groovy under @CompileStatic was behaving as if null constant was a non-array, and thought it sensible to make it do the same thing when dynamic.

It's also the thing I care the least about. 😀 I don't really care about the null constants, it's just nice to be consistent. And I'm not trying to affect what happens when the expression type is explicitly dynamic or otherwise can't be discovered at compile-time. I just want to fix it so when the expression type is known at compile-time - even if it's just because the coder put an explicit cast-or-coerce on an otherwise dynamic expression - it does the right thing. Because of course the absolute first thing I tried when I encountered the problem was to cast it, to push Groovy into doing the right thing, and it ignored me. Now it would work.

I've been wondering how to make it smaller. The only bit that actually changes the generated code is:

                        arguments = ale.transformExpression(expr -> {
                            if (expr == lastArg) {
                                return new ArrayExpression(componentType, Collections.singletonList(expr));
                            }
                            return expr;
                        });

All the rest is just matching the fairly singular condition where we need to do it. I'm sure there's scope to make it more concise but actually reducing the checks it does would just give us false positives. It would probably actually be easier to make it do more: to do the array wrapping here for all cases where the argument isn't dynamic or already definitely an array. I was trying to make the footprint - in terms of how much behaviour it changes - smaller. 😀

The risky bits, from my point of view, are where it tries to discover the matching real method for a MethodCallExpression that doesn't have a target MethodNode already set (for both identifying a method is varargs, and a method called as an argument to a varargs, to find its return type).

(If the target is set it gets dealt with by writeDirectMethodCall, which works fine already.)

I'm sure that process could be improved, because it feels a bit messy. Or perhaps there's some other way from earlier in the process to definitively set the appropriate MethodNode on a MethodCallExpression - the same one you'd set as the target when static compiling, but here not as the target, but just as information for compiling dynamic code, like this.

I'm also aware that the methods we discover here might be intercepted or otherwise occluded by runtime metaprogramming. My guess is that, in order for such interceptions to even intercept, they'd have to be basically compatible with the methods they're intercepting, so using the originals to decide what to do about the varargs arguments seems a reasonably safe bet. But that's why it works by fiddling the argument list, rather than just converting it to a direct method call. (I did try that. It burnt the test suite to the ground!)

[StrangeNoises]

Ah. I was actually trying to move the work for this PR into a new branch of my fork, rather than leaving it in master - just an attempted housekeeping/fixing-what-i-did-wrong change, to allow me to work on other issues, not trying to actually close the PR, but that seems to have been triggered anyway. Should I recreate it as a new PR or is there a way to fix/update this one?

[daniellansun]

You could create a new PR 😉