From e552ba76ac18929b6cacb5f1a722df3ec8a04e19 Mon Sep 17 00:00:00 2001 From: Michael Hixson Date: Fri, 2 Apr 2021 10:18:32 -0700 Subject: [PATCH] Add baseline support for records (#377) Fixes #374 Avoid throwing exceptions when the source code contains records. Since records are a new kind of class, refactor the switch statements that make assertions about which elements and trees represent classes. Use general purpose `isClassElement` and `isClassTree` methods that are forward-compatible with records instead of enumerating `ElementKind` or `Tree.Kind` constants. Refactor `checkFieldInitialization` to ignore final fields. This check was wrongly flagging records because it couldn't analyze record constructors properly. Without the change to ignore final fields, this check runs into two kinds of trouble with records: 1. It doesn't inspect record constructors at all because they are generated. `NullAway.isConstructor(MethodTree)` returns false and so those trees are skipped. 2. The trees for the generated constructors don't contain explicit field assignments. That's just not how those were implemented. Something else is responsible for assigning the fields at runtime, apparently. Rather than trying to teach NullAway about record constructors specifically, count on javac to verify that all final fields are initialized. Presumably, if NullAway were to complain that a constructor did not initialize a final field, that complaint would be redundant (with the error that javac would produce) at best and incorrect otherwise. Co-authored-by: Manu Sridharan --- .../main/java/com/uber/nullaway/NullAway.java | 39 +++++++------------ 1 file changed, 14 insertions(+), 25 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 7f613feb89..744d6cbffa 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -109,6 +109,8 @@ import javax.lang.model.element.NestingKind; import javax.lang.model.type.TypeKind; import org.checkerframework.dataflow.cfg.node.MethodInvocationNode; +import org.checkerframework.javacutil.ElementUtils; +import org.checkerframework.javacutil.TreeUtils; /** * Checker for nullability errors. It assumes that any field, method parameter, or return type that @@ -1422,6 +1424,12 @@ private void checkFieldInitialization(ClassTree tree, VisitorState state) { constructorInitInfo = checkConstructorInitialization(entities, state); notInitializedInConstructors = ImmutableSet.copyOf(constructorInitInfo.values()); } + // Filter out final fields, since javac will already check initialization + notInitializedInConstructors = + ImmutableSet.copyOf( + Sets.filter( + notInitializedInConstructors, + symbol -> !symbol.getModifiers().contains(Modifier.FINAL))); class2ConstructorUninit.putAll(classSymbol, notInitializedInConstructors); Set notInitializedAtAll = notAssignedInAnyInitializer(entities, notInitializedInConstructors, state); @@ -1734,6 +1742,10 @@ private FieldInitEntities collectEntities(ClassTree tree, VisitorState state) { // we assume getMembers() returns members in the same order as the declarations for (Tree memberTree : tree.getMembers()) { + if (TreeUtils.isClassTree(memberTree)) { + // do nothing + continue; + } switch (memberTree.getKind()) { case METHOD: // check if it is a constructor or an @Initializer method @@ -1776,12 +1788,6 @@ private FieldInitEntities collectEntities(ClassTree tree, VisitorState state) { instanceInitializerBlocks.add(blockTree); } break; - case ENUM: - case CLASS: - case INTERFACE: - case ANNOTATION_TYPE: - // do nothing - break; default: throw new RuntimeException( memberTree.getKind().toString() + " " + state.getSourceForNode(memberTree)); @@ -1981,30 +1987,13 @@ private boolean mayBeNullFieldAccess(VisitorState state, ExpressionTree expr, Sy return exprMayBeNull ? nullnessFromDataflow(state, expr) : false; } - /** - * @param kind an element kind. - * @return true if a deference of the kind might dereference null, false - * otherwise - */ - private boolean kindMayDeferenceNull(ElementKind kind) { - switch (kind) { - case CLASS: - case PACKAGE: - case ENUM: - case INTERFACE: - case ANNOTATION_TYPE: - return false; - default: - return true; - } - } - private Description matchDereference( ExpressionTree baseExpression, ExpressionTree derefExpression, VisitorState state) { Symbol dereferenced = ASTHelpers.getSymbol(baseExpression); if (dereferenced == null || dereferenced.type.isPrimitive() - || !kindMayDeferenceNull(dereferenced.getKind())) { + || dereferenced.getKind() == ElementKind.PACKAGE + || ElementUtils.isClassElement(dereferenced)) { // we know we don't have a null dereference here return Description.NO_MATCH; }