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 18:37:14 UTC
[GitHub] [groovy] nescohen opened a new pull request #1391: Fix possible null dereference in EnumCompletionVistor.java
nescohen opened a new pull request #1391:
URL: https://github.com/apache/groovy/pull/1391
----------------------------------------------------------------
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 #1391: Fix possible null dereference in EnumCompletionVistor.java
Posted by GitBox <gi...@apache.org>.
nescohen commented on a change in pull request #1391:
URL: https://github.com/apache/groovy/pull/1391#discussion_r498487633
##########
File path: src/main/java/org/codehaus/groovy/classgen/EnumCompletionVisitor.java
##########
@@ -83,7 +83,13 @@ private void completeEnum(ClassNode enumClass) {
private static void addImplicitConstructors(ClassNode enumClass, boolean aic) {
if (aic) {
ClassNode sn = enumClass.getSuperClass();
- List<ConstructorNode> sctors = new ArrayList<ConstructorNode>(sn.getDeclaredConstructors());
Review comment:
I was looking specifically at this function:
https://github.com/nescohen/groovy/blob/master/src/main/java/org/codehaus/groovy/ast/ClassNode.java#L1014
I made the assumption that it was possible for the return value to be null specifically because of the null check on line 1014. It is possible this was a mistake however. I did not look at/consider the actual call to `redirect().getUnresolvedSuperClass();`. What do you think?
----------------------------------------------------------------
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 #1391: Fix possible null dereference in EnumCompletionVistor.java
Posted by GitBox <gi...@apache.org>.
eric-milles commented on a change in pull request #1391:
URL: https://github.com/apache/groovy/pull/1391#discussion_r498489873
##########
File path: src/main/java/org/codehaus/groovy/classgen/EnumCompletionVisitor.java
##########
@@ -83,7 +83,13 @@ private void completeEnum(ClassNode enumClass) {
private static void addImplicitConstructors(ClassNode enumClass, boolean aic) {
if (aic) {
ClassNode sn = enumClass.getSuperClass();
- List<ConstructorNode> sctors = new ArrayList<ConstructorNode>(sn.getDeclaredConstructors());
Review comment:
I had a bit of confusion. `getDeclaredConstructors()` is what I was referring to. `getSuperClass()` may return null for an interface but should return `java.lang.Enum` for an enum class. If that isn't the case, there is a problem upstream where `enumClass` is created.
----------------------------------------------------------------
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 closed pull request #1391: Fix possible null dereference in EnumCompletionVistor.java
Posted by GitBox <gi...@apache.org>.
paulk-asert closed pull request #1391:
URL: https://github.com/apache/groovy/pull/1391
--
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
[GitHub] [groovy] paulk-asert commented on pull request #1391: Fix possible null dereference in EnumCompletionVistor.java
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1391:
URL: https://github.com/apache/groovy/pull/1391#issuecomment-1050720763
No further feedback, I'll close this.
--
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
[GitHub] [groovy] eric-milles commented on a change in pull request #1391: Fix possible null dereference in EnumCompletionVistor.java
Posted by GitBox <gi...@apache.org>.
eric-milles commented on a change in pull request #1391:
URL: https://github.com/apache/groovy/pull/1391#discussion_r498475812
##########
File path: src/main/java/org/codehaus/groovy/classgen/EnumCompletionVisitor.java
##########
@@ -83,7 +83,13 @@ private void completeEnum(ClassNode enumClass) {
private static void addImplicitConstructors(ClassNode enumClass, boolean aic) {
if (aic) {
ClassNode sn = enumClass.getSuperClass();
- List<ConstructorNode> sctors = new ArrayList<ConstructorNode>(sn.getDeclaredConstructors());
Review comment:
How is it that this could return a null? I don't see any of the 3 implementation ever returning null.
----------------------------------------------------------------
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 #1391: Fix possible null dereference in EnumCompletionVistor.java
Posted by GitBox <gi...@apache.org>.
nescohen commented on a change in pull request #1391:
URL: https://github.com/apache/groovy/pull/1391#discussion_r498608776
##########
File path: src/main/java/org/codehaus/groovy/classgen/EnumCompletionVisitor.java
##########
@@ -83,7 +83,13 @@ private void completeEnum(ClassNode enumClass) {
private static void addImplicitConstructors(ClassNode enumClass, boolean aic) {
if (aic) {
ClassNode sn = enumClass.getSuperClass();
- List<ConstructorNode> sctors = new ArrayList<ConstructorNode>(sn.getDeclaredConstructors());
Review comment:
So I guess the question would be, would you prefer to have a NPE thrown vs en empty constructor list OR I could even refactor to throw an exception of the semantically correct type. Would you know of one to recommend? I guess I just like to err on the side of never throwing NPEs, but I know that sometimes it is preferable to fail fast.
Thanks!
----------------------------------------------------------------
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 #1391: Fix possible null dereference in EnumCompletionVistor.java
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on a change in pull request #1391:
URL: https://github.com/apache/groovy/pull/1391#discussion_r500227550
##########
File path: src/main/java/org/codehaus/groovy/classgen/EnumCompletionVisitor.java
##########
@@ -83,7 +83,13 @@ private void completeEnum(ClassNode enumClass) {
private static void addImplicitConstructors(ClassNode enumClass, boolean aic) {
if (aic) {
ClassNode sn = enumClass.getSuperClass();
- List<ConstructorNode> sctors = new ArrayList<ConstructorNode>(sn.getDeclaredConstructors());
Review comment:
For an enum class, I see no way `null` can arise. If we were trying to make the code ultra robust I'd add:
```
if (sc == null || sc.getDeclaredConstructors() == null) {
throw new GroovyBugError("enum had null superclass or constructors weren't initialized correctly");
}
```
But I think that is overkill here. I'd prefer over time to start using @NonNull, e.g. @NonNull on `getDeclaredConstructors` would allow the second term in the above to be dropped and so on. Currently the "NonNullness" is deduced by hand but I believe is currently the case. Also, there have no doubt been millions of enums compiled by Groovy and no-one has spotted an NPE in this area of the code to date.
I'd recommend closing. But it's a good exercise to check our assumptions in such places and see if we can't make implicit assumptions more explicit somehow.
----------------------------------------------------------------
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