[GitHub] [groovy] paulk-asert commented on a change in pull request #1391: Fix possible null dereference in EnumCompletionVistor.java

[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