StrangeNoises commented on pull request #1591:
URL: https://github.com/apache/groovy/pull/1591#issuecomment-856956800
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!)
--
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]
