fix possible null dereference in staticTypeCheckingVisitor class

[nescohen]

No description provided.

[eric-milles]

Is it possible for receivers to be empty? If not, mn is always initialized. Setting to null stops compiler from saying we use an uninitialized variable.

[nescohen]

Hey Eric, sorry for the late response here. Lots to do at ApacheCon :P

I would say for this one that I can't say for 100% sure that receivers CAN be empty, but there's a couple of reasons that I would argue setting mn to an empty list is going to be better here.

1. receivers starts as an empty list, so even if there is no actual codepath where it is empty now, will that be true in the future? An empty list is still a valid value for that type so someone could easily refactor it to be empty under some conditions.

2. By setting mn to an empty list we are ensuring that the mn.isEmpty() call will not throw a NPE, rather it will trigger the appropriate exception of addNoMatchingMethodError.

I'm not sure if I can make this call for you as I am relatively new to the project, but I think this change will result in more resilient code, while possibly fixing erroneous codepaths in the now.

[paulk-asert]

The other option would be just have an additional null guard for mn before calling the first isEmpty? Saves creating an empty list that we believe we will always throw away.

--- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
+++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
@@ -2603,7 +2603,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
                     break;
                 }
             }
-            if (mn.isEmpty()) {
+            if (mn == null || mn.isEmpty()) {
                 mn = extension.handleMissingMethod(receiver, name, argumentList, args, call);
             }
             boolean callArgsVisited = false;

[nescohen]

Good point @paulk-asert. I think a null check would also be a reasonable solution.

The one nice thing about using Collections.emtpyList() though is that no new list is allocated. It's simply a statically defined, immutable empty list. See https://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#emptyList()

[paulk-asert]

That's true of course @nescohen about allocation for Collections.emtpyList() though it might still be confusing. We are assigning a list that we currently don't think the code would ever use. If we had @nonnull or similar or final it would be the way to go.

[paulk-asert]

I merged with the tweak I mentioned. I hope that's okay.

[paulk-asert]

Actually, thinking more about it, I think fail fast is the way to go here. I'll adjust appropriately.

[nescohen]

Thanks for helping out! Definitely I am a bit new to Groovy