[GitHub] [groovy] StrangeNoises commented on pull request #1601: GROOVY-10099: Resolve ambiguous varargs behaviour in dynamic mode by recourse to compiler-stage information

[GitBox <gi...@apache.org>]

StrangeNoises commented on pull request #1601:
URL: https://github.com/apache/groovy/pull/1601#issuecomment-873406970


   I've tried to make it less scary. 😀
   
   The scope of the change is reduced to only apply in `@TypeChecked` code, and brings that into line with `@CompileStatic` behaviour relating to single varargs arguments that might be null. The code has been considerably simplified as a result, as it is no longer trying to identify the relevant methods and property types itself (which was the part *I* was least confident about), but using the metadata values for these left behind by a `@TypeChecked` compile.
   
   In fact this change does not explicitly limit itself to `@TypeChecked` but is simply taking advantage of the metadata if present, however it got there earlier in the compile, but that does seem *primarily* to be from `StaticTypeCheckingVisitor` at present. Should in future dynamic compilation also helpfully leave that metadata lying around, it will take advantage then too.
   
   The `VarargsMethodTest` test has been expanded (though reduced from earlier versions of this PR) to show it working as expected for `@TypeChecked` and also illustrating with "shouldFails" where the `@CompileDynamic` behaviour differs. Essentially it treats any single null as an array, and only the tests that are really giving it an array expression pass, but only by coincidence. The `@CompileStatic` tests have been removed as its behaviour has not changed, but it is the same (now) as `@TypeChecked`. No existing tests have had to be altered to make them pass with this code change.
   
   So this keeps the "purity" of the fully dynamic mode, only depending on runtime information, and defers to the future any more considered discussion of whether that's desirable in this situation. But now the "middle-ground" of `@TypeChecked` can be used to correct the behaviour as well as `@CompileStatic`.
   
   The code here still explicitly makes null constants behave as non-arrays, to be consistent with `@CompileStatic`, but I recall earlier in the discussion @paulk-asert queried whether that was the correct behaviour. If minds should change, it's a simple change from `true` to `false` here to make it follow suit. As I've said before, I'm pretty indifferent to the fate of null constants, and only really care about whether explicit types are being respected, where given.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@groovy.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org