[
https://issues.apache.org/jira/browse/GROOVY-10099?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17360099#comment-17360099
]
Rachel Greenham commented on GROOVY-10099:
------------------------------------------
Seems to be an argument for ignoring issues because it's easier than fixing
them, which seems to have been the fate of all those issues quoted: No debate,
just... ignored and left to wither. It feels like this one only got attention
(for which thank you, by the way) because it was followed up by an actual PR
which had to be responded to. :)
(Seriously, there's nothing wrong with being a voice for caution, but it feels
like it invites a robust defence, so here goes: :) )
So the advice is: "If you need to call a varargs method, do so via a
statically-compiled intermediate method, so it doesn't blow up in certain
situations." It's a bit... rubbish, isn't it? If you're in @CompileStatic
anyway for other reasons, like "this bit needs to run faster", fine, but having
to do it, and lose the _benefits_ of a dynamic language, just to get
predictable behaviour out of varargs calls, is extremely retrograde.
I'm trying to get Groovy integrated into our platform. We already have a way of
invoking scripts that basically goes through a method on a context object that
looks like this (names have been changed):
{code:java}
public Object perform(String scriptName, Object... args) {...}{code}
Which will now preferentially find and execute a groovy script, and fall back
to <legacy> scripts. And it may be called trivially both from scripts in the
legacy scripting language (obscure, older-than-groovy, barrier-to-hiring), and
from Java code, like:
{code:java}
perform("/misc/sanitize-basket", session.getBasket());{code}
Which could be used verbatim in Groovy, though more idiomatically like:
{code:java}
perform '/misc/sanitize-basket', session.basket{code}
Only, when you do this - either - in the rare-but-valid case of that
Session::getBasket method returning null, the target script blows up, because
its generated code looks like:
{code:java}
public Object run(Object[] args) {
Basket basket = (Basket)args[0];
if (basket != null) { ...
{code}
But only when called from Groovy. Not from anywhere else, because normal
varargs behaviour is, if basket is null you still get the array with a null
basket, because the argument _type_... is Basket. It's known by the compiler so
it wraps it up.
If I _wanted_ to call it with a null constant, or obtain it untyped, I'd
_expect_ to have to cast it, in case it was null:
{code:java}
perform '/misc/sanitize-basket', (Basket)null
perform '/misc/sanitize-basket', session.attrs["basket-${name}"] as Basket
{code}
... but at the moment that wouldn't even work.
It's an acceptance issue. Imagine the conversation:
{quote}"So you changed perform so it catches nulls and makes an array which...
hopefully doesn't break anything else..."
"The <legacy> scripts have to be given an args array the length of the number
of declared parameters, but if they're not declaring any they don't even look
at it. So that should be safe if it's not null. I don't think they ever let you
actually declare an array as a parameter. It's one of the limitations."
"Hmm. But otherwise the workaround is, to call varargs methods via another
intermediate method compiled statically?"
"Yeah. Or you can make it an array here, like: {{}}
{code:java}
[session.basket] as Basket[]{code}
"So now you're making an ArrayList and toArraying it. Every time."
"Uh, yeah. Or you can make the array directly, like we did in Java before
varargs."
_<Withering stare>_
"
{code:java}
session.basket ?: Basket.ARRAY_WITH_NULL
{code}
? :D"
"Did we look at JRuby yet?"{color:#172b4d} {color}
{quote}
:)
I can't stress enough that the PR is attempting to intervene only where the
expression type is known, and known not to be dynamically typed or an array.
Maybe there's bugs in that identification, so we fix those if they arise. But
as for the principle of it: if the type is known, when is it ever right to be
knowingly sending a non-array argument to an array parameter? Such that it
could only escape an immediate ClassCastException if it's null, or if it's
intercepted and turned into an identical array anyway at runtime.
And I don't care about the no-cast null constant case. I thought it made sense
to make it consistent with @CompileStatic behaviour, but that's not a hill I
want to die on. It can go on instead being treated as a dynamically-typed
expression. This is only fixing what happens when we _have_ the type. The
reporter of GROOVY-6146 appeared to think the same way: Marking in bold the
important bit: that even when you cast it explicitly to String it ignores you
and still treats it like it's an array. This PR would fix that bug too.
Does it break the mitigations people might have put in to get around this?
* If they ensure the argument can never be null? Then no: It's just turning it
into an array a moment earlier than it would have anyway.
* If they call via a @CompileStatic method? Then no: Static compile is
expressly not affected by this change. (It's about to do the same thing anyway.)
* If they wrap the argument in an array? Then no: Arrays will be passed
through as they are already.
* If they only wrap it in an array if it's null? Then no: That array would be
passed through, and nothing changes.
* Intermediate method does the same? (My workaround for perform, which I'd
remove if this fix gets in): Then no, because it'll already have been sent an
array, not null, just as it would have from a java/static caller.
* If they have a non-varargs overload of the varargs method that takes just
the single parameter, which should be matched preferentially? Then no, because
the method call doesn't unambiguously match to a single varargs method, so it
does nothing, and everything behaves as before.
* If the first parameter is non-varargs and subsequent ones are varargs (eg:
like Path::of)? Then no, because it's not a varargs parameter, and it won't
match because the argument list isn't the same length as the parameters list,
so it does nothing. Same problem arises if the _second_ argument evaluates to
null though, _and_ there isn't a third...
* If they handle it in a Groovy invokeMethod or methodMissing, or otherwise in
similar fashion just take an Object and work out what you have? Then no: It
can't identify the method call is to a varargs method so it does nothing.
* The varargs method exists at compiletime so the argument is modified, but
the class is a GroovyInterceptable and they catch and fix the nulls during
method invoke. Then, mmmmaybe? depending on how they wrote it, such as, if what
they wrote can't cope with being given an array anyway. Which seems odd
behaviour for something written to intercept a varargs method call... I suppose
if we're really worried about this we can suppress the fix for receivers that
implement GroovyInterceptable...
* Ditto for varargs method exists at compiletime but is "replaced" by an
expando metaclass closure that breaks if it's actually given an array. I admit
I am assuming that if you're using metaprogramming to overlay or occlude an
existing method, it kinda has to be argument-compatible with it?
* If in the target they check for args being null but not args[0] being
null... well maybe but that would be odd because they'll also have written a
varargs method that can't take more than one argument if the first is null.
They have bigger problems... For one, it won't work when called from Java, or
from a @CompileStatic origin...
* They didn't mean it to be a varargs method, but Groovy thinks any method
where the last argument is an array is a varargs method. Well, that wasn't my
choice... but still, how does that make it right to let an array parameter
receive a non-array argument?
I have in fact found a case of the latter, which I thought might cause a
change, but doesn't: AstSpecificationCompiler.groovy, method makeArrayOfNodes
which... sends a single object to a List::toArray (not really a varargs
method). This PR would wrap that in an array of the parameter's component type,
and not to the type of the object given... but as it turns out,
ParameterTypes::fitToVargs does that too, so the behaviour (the component type
of the resulting array) isn't changed.
In fact it's doubly safe, because the argument type is Object, which (except in
the case of an explicit cast) is taken to mean a dynamic type, so it gets left
alone anyway by the PR's change, and the former behaviour still applies. But in
a similar situation where the argument type is something more specific than
Object it would still work for the reason described: It uses the parameter
component type.
> A single null argument to a varargs parameter is received as null
> -----------------------------------------------------------------
>
> Key: GROOVY-10099
> URL: https://issues.apache.org/jira/browse/GROOVY-10099
> Project: Groovy
> Issue Type: Bug
> Environment: Observed on Groovy 3.0.8 on macOS Big Sur (Intel), but I
> don't think that's relevant; it'll be everywhere.
> Reporter: Rachel Greenham
> Priority: Major
> Fix For: 4.x
>
> Attachments: VarArgsTest.groovy, VarArgsTest.jsh
>
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> (NB: I would set the priority to P2 default to be triaged, but I seem not to
> have that option, so I left it at the default I was presented with.)
> When calling a method with a varargs parameter with a single value, that
> value is wrapped into an array of length 1. This is the behaviour in Java,
> and is the expected behaviour, and _most_ of the time is the behaviour in
> Groovy too.
> But when that single value is null, Groovy will instead just pass the null
> into the method. Java will not do this: You'll get an array with a single
> null in it. Because Groovy's behaviour is unexpected, especially when
> interfacing with Java code, NullPointerExceptions can often ensue.
> Adding to the inconsistencies, if the Groovy code calling the method is in a
> {{@CompileStatic}} context, it behaves just like Java, and the method
> (whether or not _it_ is statically compiled or a dynamic Groovy method)
> receives an array with a null in it.
> So the behaviour in Groovy is inconsistent, both with itself in a
> {{@CompileStatic}} situation, and with Java, and is requiring workarounds in
> Java code to handle this normally-impossible eventuality. (Even if no varargs
> parameter is given you get an empty array, as in fact you do in Groovy too.)
> This may be an "early instalment weirdness": There's an ancient ticket, from
> not long after varargs were introduced into Java, which appears to have
> argued - successfully at the time - that the normal behaviour is a bug:
> https://issues.apache.org/jira/browse/GROOVY-1026
> Further adding to the confusion may be that Groovy usually elides the
> difference between an {{Object[]}} parameter and an {{Object...}} parameter:
> They both behave the same.
> The offending code appears to be in
> org.codehaus.groovy.reflection.ParameterTypes.java in method fitToVars, lines
> 200-215 in master at the time of writing, which even includes a comment that
> "if the last argument is null, then we don't have to do anything", with which
> I respectfully disagree. :) That behaviour should be to return an array with
> a single null in it (Handily, MetaClassHelper.ARRAY_WITH_NULL saves having to
> make a new one.)
> In principle it's an easy fix (although I've left tagging to others as this
> is my first issue here), but there'd be an obvious nervousness about changing
> behaviour like this when there might be a lot of old code out there depending
> on it behaving the way it does now. OTOH the way it behaves now is breaking
> the expectations of those of us coming to Groovy from a lifetime of Java...
> Attachments:
> VarArgsTest.groovy - a script saved from, and runnable in, groovyConsole,
> demonstrating the behaviour. The behaviour is the same regardless of whether
> the console is launched with the -indy option. (The issue was initially
> observed in indy.) The dynamic portion of the test, when run, ends in a
> NullPointerException as Arrays.asList is not expecting a null varargs
> parameter. Output seen (-indy or not):
>
> {code:java}
> name: the static name 1
> params is null? false
> params length is 1
> [blah]
> name: the static name 2
> params is null? false
> params length is 2
> [blah, blue]
> name: the static name 3 with blah=null
> params is null? false
> params length is 1
> [null]
> Arrays.asList(blah)? [null]
> name: the dynamic name 1
> params is null? false
> params length is 1
> [blah]
> name: the dynamic name 2
> params is null? false
> params length is 2
> [blah, blue]
> name: the dynamic name 3 with blah=null
> params is null? true
> Exception thrown
> java.lang.NullPointerException
> ...{code}
> (etc. stack trace not shown for formatting reasons.)
> VarArgsTest.jsh - a jshell script demonstrating Java's behaviour, very
> similar to the groovy test, but omitting the dynamic portion of the test for
> obvious reasons. (The statements in the Groovy script ending in semicolons
> are left that way precisely to mark that they're identical to the Java test.)
> Runnable with
>
> {code:java}
> jshell PRINTING VarArgsTest.jsh
> {code}
> Output seen:
> {code:java}
> name: the static name 1
> params is null? false
> params length is 1
> [blah]
> name: the static name 2
> params is null? false
> params length is 2
> [blah, blue]
> name: the static name 3 with blah=null
> params is null? false
> params length is 1
> [null]
> Arrays.asList(blah)? [null]
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
