StrangeNoises opened a new pull request #1601: URL: https://github.com/apache/groovy/pull/1601
This is a replacement for PR #1591 which was accidentally closed on the occasion of my attempting to move the work into a branch of my fork, rather than master, so I can potentially work on other issues. There is discussion there. Summary: It attempts to resolve the question of what happens when a varargs method is invoked with a single argument to the varargs parameter, which is null. When running dynamic code, Groovy has no way to distinguish whether a single null value is a null object reference (and should thus be wrapped in an array for a varargs) or a null array reference (and thus should not). At present it always does the latter, which leads to surprising behaviour when that argument, for example, comes from a method call with a known non-array return type, or even an explicit cast to a non-array type, and would be expected to go into a varargs as an array containing that single null. At compiletime we have this information... at least some of the time. This PR attempts to make use of that information, in the case of the single argument, to examine the expression to determine if it's an array type or not, or dynamic and thus still impossible to resolve. And, when it's confident it's not dynamic nor an array type, to write the array-wrapping into the generated code then, but still leave it as a dynamic method call, so it could still be intercepted by an equivalent replacement of the method known to exist at compiletime. While the tests pass, it's clearly a worry to be making such a deep change, especially as someone's first Groovy PR! On the other hand this change does make varargs methods in this situation behave consistently between @CompileStatic and @CompileDynamic code. @CompileStatic code is not altered in any way by this PR. That said, there is an argument that @CompileStatic's current behaviour may not be correct anyway - but only really with respect to when the null in question is a null constant. The more useful reason for the change is to try to make it behave better when the null happens to be the value of a typed variable or method return type, or some other kind of expression with a knowable type, including and especially in fact, cast expressions, which may reflect the coder's attempts to assert precisely what should happen here. More problematic, perhaps, is the way this PR breaks the ideal dynamic behaviour of only acting upon what information is available at runtime, and rather opportunistically borrowing information available only at compiletime (but also potentially made incorrect by runtime metaprogramming). There is a question as to whether that *kind* of change is desirable at all. On the other side, the fact remains that the question answered here *cannot* be answered at runtime. A null value retains no clue as to what type of reference it's a null of. But there's a case that such behaviour shouldn't change now, regardless of whether it's right or wrong, for fear it would just break too much. I have every sympathy with that, and personally doubt this will ever get merged now, but the discussion wasn't definitively *over*, so recreating the PR for that sake. The code changes here are identical to the original. In addition the branch has been caught up to the present state of apache/groovy:master. Nothing there appears to clash. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
