You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by GitBox <gi...@apache.org> on 2020/10/01 19:25:55 UTC
[GitHub] [groovy] nescohen opened a new pull request #1392: fix possible null dereference in staticTypeCheckingVisitor class
nescohen opened a new pull request #1392:
URL: https://github.com/apache/groovy/pull/1392
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [groovy] asfgit closed pull request #1392: fix possible null dereference in staticTypeCheckingVisitor class
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1392:
URL: https://github.com/apache/groovy/pull/1392
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [groovy] nescohen commented on a change in pull request #1392: fix possible null dereference in staticTypeCheckingVisitor class
Posted by GitBox <gi...@apache.org>.
nescohen commented on a change in pull request #1392:
URL: https://github.com/apache/groovy/pull/1392#discussion_r499737579
##########
File path: src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
##########
@@ -2592,7 +2592,7 @@ public void visitStaticMethodCallExpression(final StaticMethodCallExpression cal
// in that order
List<Receiver<String>> receivers = new LinkedList<>();
addReceivers(receivers, makeOwnerList(new ClassExpression(receiver)), false);
- List<MethodNode> mn = null;
+ List<MethodNode> mn = Collections.emptyList();
Receiver<String> chosenReceiver = null;
for (Receiver<String> currentReceiver : receivers) {
mn = findMethod(currentReceiver.getType(), name, args);
Review comment:
Thanks for helping out! Definitely I am a bit new to Groovy
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on a change in pull request #1392: fix possible null dereference in staticTypeCheckingVisitor class
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on a change in pull request #1392:
URL: https://github.com/apache/groovy/pull/1392#discussion_r499122817
##########
File path: src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
##########
@@ -2592,7 +2592,7 @@ public void visitStaticMethodCallExpression(final StaticMethodCallExpression cal
// in that order
List<Receiver<String>> receivers = new LinkedList<>();
addReceivers(receivers, makeOwnerList(new ClassExpression(receiver)), false);
- List<MethodNode> mn = null;
+ List<MethodNode> mn = Collections.emptyList();
Receiver<String> chosenReceiver = null;
for (Receiver<String> currentReceiver : receivers) {
mn = findMethod(currentReceiver.getType(), name, args);
Review comment:
I merged with the tweak I mentioned. I hope that's okay.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [groovy] nescohen commented on a change in pull request #1392: fix possible null dereference in staticTypeCheckingVisitor class
Posted by GitBox <gi...@apache.org>.
nescohen commented on a change in pull request #1392:
URL: https://github.com/apache/groovy/pull/1392#discussion_r499004203
##########
File path: src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
##########
@@ -2592,7 +2592,7 @@ public void visitStaticMethodCallExpression(final StaticMethodCallExpression cal
// in that order
List<Receiver<String>> receivers = new LinkedList<>();
addReceivers(receivers, makeOwnerList(new ClassExpression(receiver)), false);
- List<MethodNode> mn = null;
+ List<MethodNode> mn = Collections.emptyList();
Receiver<String> chosenReceiver = null;
for (Receiver<String> currentReceiver : receivers) {
mn = findMethod(currentReceiver.getType(), name, args);
Review comment:
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()
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on a change in pull request #1392: fix possible null dereference in staticTypeCheckingVisitor class
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on a change in pull request #1392:
URL: https://github.com/apache/groovy/pull/1392#discussion_r498695757
##########
File path: src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
##########
@@ -2592,7 +2592,7 @@ public void visitStaticMethodCallExpression(final StaticMethodCallExpression cal
// in that order
List<Receiver<String>> receivers = new LinkedList<>();
addReceivers(receivers, makeOwnerList(new ClassExpression(receiver)), false);
- List<MethodNode> mn = null;
+ List<MethodNode> mn = Collections.emptyList();
Receiver<String> chosenReceiver = null;
for (Receiver<String> currentReceiver : receivers) {
mn = findMethod(currentReceiver.getType(), name, args);
Review comment:
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on a change in pull request #1392: fix possible null dereference in staticTypeCheckingVisitor class
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on a change in pull request #1392:
URL: https://github.com/apache/groovy/pull/1392#discussion_r498695757
##########
File path: src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
##########
@@ -2592,7 +2592,7 @@ public void visitStaticMethodCallExpression(final StaticMethodCallExpression cal
// in that order
List<Receiver<String>> receivers = new LinkedList<>();
addReceivers(receivers, makeOwnerList(new ClassExpression(receiver)), false);
- List<MethodNode> mn = null;
+ List<MethodNode> mn = Collections.emptyList();
Receiver<String> chosenReceiver = null;
for (Receiver<String> currentReceiver : receivers) {
mn = findMethod(currentReceiver.getType(), name, args);
Review comment:
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;
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [groovy] nescohen commented on a change in pull request #1392: fix possible null dereference in staticTypeCheckingVisitor class
Posted by GitBox <gi...@apache.org>.
nescohen commented on a change in pull request #1392:
URL: https://github.com/apache/groovy/pull/1392#discussion_r498608358
##########
File path: src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
##########
@@ -2592,7 +2592,7 @@ public void visitStaticMethodCallExpression(final StaticMethodCallExpression cal
// in that order
List<Receiver<String>> receivers = new LinkedList<>();
addReceivers(receivers, makeOwnerList(new ClassExpression(receiver)), false);
- List<MethodNode> mn = null;
+ List<MethodNode> mn = Collections.emptyList();
Receiver<String> chosenReceiver = null;
for (Receiver<String> currentReceiver : receivers) {
mn = findMethod(currentReceiver.getType(), name, args);
Review comment:
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on a change in pull request #1392: fix possible null dereference in staticTypeCheckingVisitor class
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on a change in pull request #1392:
URL: https://github.com/apache/groovy/pull/1392#discussion_r499121500
##########
File path: src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
##########
@@ -2592,7 +2592,7 @@ public void visitStaticMethodCallExpression(final StaticMethodCallExpression cal
// in that order
List<Receiver<String>> receivers = new LinkedList<>();
addReceivers(receivers, makeOwnerList(new ClassExpression(receiver)), false);
- List<MethodNode> mn = null;
+ List<MethodNode> mn = Collections.emptyList();
Receiver<String> chosenReceiver = null;
for (Receiver<String> currentReceiver : receivers) {
mn = findMethod(currentReceiver.getType(), name, args);
Review comment:
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [groovy] eric-milles commented on a change in pull request #1392: fix possible null dereference in staticTypeCheckingVisitor class
Posted by GitBox <gi...@apache.org>.
eric-milles commented on a change in pull request #1392:
URL: https://github.com/apache/groovy/pull/1392#discussion_r498491178
##########
File path: src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
##########
@@ -2592,7 +2592,7 @@ public void visitStaticMethodCallExpression(final StaticMethodCallExpression cal
// in that order
List<Receiver<String>> receivers = new LinkedList<>();
addReceivers(receivers, makeOwnerList(new ClassExpression(receiver)), false);
- List<MethodNode> mn = null;
+ List<MethodNode> mn = Collections.emptyList();
Receiver<String> chosenReceiver = null;
for (Receiver<String> currentReceiver : receivers) {
mn = findMethod(currentReceiver.getType(), name, args);
Review comment:
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on a change in pull request #1392: fix possible null dereference in staticTypeCheckingVisitor class
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on a change in pull request #1392:
URL: https://github.com/apache/groovy/pull/1392#discussion_r499134905
##########
File path: src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
##########
@@ -2592,7 +2592,7 @@ public void visitStaticMethodCallExpression(final StaticMethodCallExpression cal
// in that order
List<Receiver<String>> receivers = new LinkedList<>();
addReceivers(receivers, makeOwnerList(new ClassExpression(receiver)), false);
- List<MethodNode> mn = null;
+ List<MethodNode> mn = Collections.emptyList();
Receiver<String> chosenReceiver = null;
for (Receiver<String> currentReceiver : receivers) {
mn = findMethod(currentReceiver.getType(), name, args);
Review comment:
Actually, thinking more about it, I think fail fast is the way to go here. I'll adjust appropriately.
----------------------------------------------------------------
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:
users@infra.apache.org