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:
[email protected]
