@nullable suggestions for fields

RELNOTES: experimental @nullable suggestions for fields

MOE_MIGRATED_REVID=134727357

core/src/main/java/com/google/errorprone/bugpatterns/nullness/FieldMissingNullable.java

@@ -0,0 +1,182 @@
+/*
+ * Copyright 2016 Google Inc. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns.nullness;
+
+import static com.google.errorprone.BugPattern.Category.JDK;
+import static com.google.errorprone.BugPattern.MaturityLevel.EXPERIMENTAL;
+import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.AssignmentTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher;
+import com.google.errorprone.dataflow.nullnesspropagation.Nullness;
+import com.google.errorprone.dataflow.nullnesspropagation.TrustingNullnessAnalysis;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.AssignmentTree;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.tree.VariableTree;
+import com.sun.source.util.TreePath;
+import com.sun.source.util.Trees;
+import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.processing.JavacProcessingEnvironment;
+import javax.annotation.Nullable;
+import javax.lang.model.element.ElementKind;
+
+/**
+ * {@link Nullable} suggestions for fields based on values assigned to them.  For simplicity this
+ * check will not suggest annotations for fields that are never assigned in a constructor.  While
+ * fields like that <i>seem</i> like obvious candidates for being nullable, they are really not
+ * because fields may be assigned to in methods called from a constructor or super-constructor, for
+ * instance.  We'd also need an analysis that tells us about uninitialized fields.
+ *
+ * @author kmb@google.com (Kevin Bierhoff)
+ */
+@BugPattern(
+  name = "FieldMissingNullable",
+  summary = "Fields that can be null should be annotated @Nullable",
+  category = JDK,
+  severity = SUGGESTION,
+  maturity = EXPERIMENTAL
+)
+public class FieldMissingNullable extends BugChecker
+    implements AssignmentTreeMatcher, VariableTreeMatcher {
+
+  @Override
+  public Description matchVariable(VariableTree tree, VisitorState state) {
+    Symbol assigned = ASTHelpers.getSymbol(tree);
+    if (assigned == null
+        || assigned.getKind() != ElementKind.FIELD
+        || assigned.type.isPrimitive()) {
+      return Description.NO_MATCH; // not a field of nullable type
+    }
+
+    ExpressionTree expression = tree.getInitializer();
+    if (expression == null || ASTHelpers.constValue(expression) != null) {
+      // This should include literals such as "true" or a string
+      return Description.NO_MATCH;
+    }
+
+    if (TrustingNullnessAnalysis.hasNullableAnnotation(assigned)) {
+      return Description.NO_MATCH; // field already annotated
+    }
+
+
+    // Don't need dataflow to tell us that null is nullable
+    if (expression.getKind() == Tree.Kind.NULL_LITERAL) {
+      return makeFix(tree, tree, "Initializing field with null literal");
+    }
+
+    // OK let's see what dataflow says
+    // TODO(kmb): Merge this method with matchAssignment once we unify nullness analysis entry point
+    Nullness nullness =
+        TrustingNullnessAnalysis.instance(state.context)
+            .getFieldInitializerNullness(state.getPath(), state.context);
+    switch (nullness) {
+      case BOTTOM:
+      case NONNULL:
+        return Description.NO_MATCH;
+      case NULL:
+        return makeFix(tree, tree, "Initializing field with null");
+      case NULLABLE:
+        return makeFix(tree, tree, "May initialize field with null");
+      default:
+        throw new AssertionError("Impossible: " + nullness);
+    }
+  }
+
+  @Override
+  public Description matchAssignment(AssignmentTree tree, VisitorState state) {
+    Symbol assigned = ASTHelpers.getSymbol(tree.getVariable());
+    if (assigned == null
+        || assigned.getKind() != ElementKind.FIELD
+        || assigned.type.isPrimitive()) {
+      return Description.NO_MATCH; // not a field of nullable type
+    }
+
+    // Best-effort try to avoid running the dataflow analysis
+    // TODO(kmb): bail on more non-null expressions, such as "this", arithmethic, logical, and &&/||
+    ExpressionTree expression = tree.getExpression();
+    if (ASTHelpers.constValue(expression) != null) {
+      // This should include literals such as "true" or a string
+      return Description.NO_MATCH;
+    }
+
+    if (TrustingNullnessAnalysis.hasNullableAnnotation(assigned)) {
+      return Description.NO_MATCH; // field already annotated
+    }
+
+    VariableTree fieldDecl = findDeclaration(state, assigned);
+    if (fieldDecl == null) {
+      return Description.NO_MATCH; // skip fields declared elsewhere for simplicity
+    }
+
+    // Don't need dataflow to tell us that null is nullable
+    if (expression.getKind() == Tree.Kind.NULL_LITERAL) {
+      return makeFix(fieldDecl, tree, "Assigning null literal to field");
+    }
+
+    // OK let's see what dataflow says
+    Nullness nullness =
+        TrustingNullnessAnalysis.instance(state.context)
+            .getNullness(new TreePath(state.getPath(), expression), state.context);
+    if (nullness == null) {
+      // This can currently happen if the assignment is inside a lambda expression
+      // TODO(kmb): Make dataflow work for lambda expressions
+      return Description.NO_MATCH;
+    }
+    switch (nullness) {
+      case BOTTOM:
+      case NONNULL:
+        return Description.NO_MATCH;
+      case NULL:
+        return makeFix(fieldDecl, tree, "Assigning null to field");
+      case NULLABLE:
+        return makeFix(fieldDecl, tree, "May assign null to field");
+      default:
+        throw new AssertionError("Impossible: " + nullness);
+    }
+  }
+
+  @Nullable
+  private VariableTree findDeclaration(VisitorState state, Symbol field) {
+    JavacProcessingEnvironment javacEnv = JavacProcessingEnvironment.instance(state.context);
+    TreePath fieldDeclPath = Trees.instance(javacEnv).getPath(field);
+    // Skip fields declared in other compilation units since we can't make a fix for them here.
+    if (fieldDeclPath != null
+        && fieldDeclPath.getCompilationUnit() == state.getPath().getCompilationUnit()
+        && (fieldDeclPath.getLeaf() instanceof VariableTree)) {
+      return (VariableTree) fieldDeclPath.getLeaf();
+    }
+    return null;
+  }
+
+  private Description makeFix(VariableTree declaration, Tree matchedTree, String message) {
+    return buildDescription(matchedTree)
+        .setMessage(message)
+        .addFix(
+            SuggestedFix.builder()
+                .addImport("javax.annotation.Nullable")
+                .prefixWith(declaration, "@Nullable ")
+                .build())
+        .build();
+  }
+}

core/src/main/java/com/google/errorprone/bugpatterns/nullness/MissingNullableReturn.java → core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java

@@ -39,13 +39,13 @@
 
 /** @author kmb@google.com (Kevin Bierhoff) */
 @BugPattern(
-  name = "MissingNullableReturn",
+  name = "ReturnMissingNullable",
   summary = "Methods that can return null should be annotated @Nullable",
   category = JDK,
   severity = SUGGESTION,
   maturity = EXPERIMENTAL
 )
-public class MissingNullableReturn extends BugChecker implements ReturnTreeMatcher {
+public class ReturnMissingNullable extends BugChecker implements ReturnTreeMatcher {
 
   @Override
   public Description matchReturn(ReturnTree tree, VisitorState state) {
@@ -65,21 +65,13 @@ public Description matchReturn(ReturnTree tree, VisitorState state) {
     if (method == null || isIgnoredReturnType(method, state)) {
       return Description.NO_MATCH;
     }
-    switch (TrustingNullnessAnalysis.nullnessFromAnnotations(method.sym)) {
-      case NULLABLE:
-      case NULL:
-        // Method already annotated
-        return Description.NO_MATCH;
-      default:
-        break;
+    if (TrustingNullnessAnalysis.hasNullableAnnotation(method.sym)) {
+      return Description.NO_MATCH;
     }
 
     // Don't need dataflow to tell us that null is nullable
-    switch (returnExpression.getKind()) {
-      case NULL_LITERAL:
-        return makeFix(method, tree, "Returning null literal");
-      default:
-        break;
+    if (returnExpression.getKind() == ExpressionTree.Kind.NULL_LITERAL) {
+      return makeFix(method, tree, "Returning null literal");
     }
 
     // OK let's see what dataflow says

core/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/TrustingNullnessAnalysis.java

@@ -16,12 +16,24 @@
 
 package com.google.errorprone.dataflow.nullnesspropagation;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
 import com.google.errorprone.dataflow.DataFlow;
+import com.google.errorprone.dataflow.LocalStore;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.tree.VariableTree;
 import com.sun.source.util.TreePath;
+import com.sun.tools.javac.processing.JavacProcessingEnvironment;
+import com.sun.tools.javac.tree.JCTree.JCVariableDecl;
 import com.sun.tools.javac.util.Context;
 import java.io.Serializable;
-import javax.lang.model.element.AnnotationMirror;
 import javax.lang.model.element.Element;
+import javax.lang.model.element.ElementKind;
+import org.checkerframework.dataflow.analysis.Analysis;
+import org.checkerframework.dataflow.cfg.CFGBuilder;
+import org.checkerframework.dataflow.cfg.ControlFlowGraph;
+import org.checkerframework.dataflow.cfg.UnderlyingAST;
 
 /**
  * An interface to the "trusting" nullness analysis. This variant "trusts" {@code Nullabe}
@@ -70,15 +82,38 @@ public Nullness getNullness(TreePath exprPath, Context context) {
   }
 
   /**
-   * Returns nullability based on the presence of a {@code Nullable} annotation.
+   * Returns {@link Nullness} of the initializer of the {@link VariableTree} at the leaf of the
+   * given {@code fieldDeclPath}.  Returns {@link Nullness#NULL} should there be no initializer.
    */
-  public static Nullness nullnessFromAnnotations(Element element) {
-    for (AnnotationMirror anno : element.getAnnotationMirrors()) {
-      // Check for Nullable like ReturnValueIsNonNull
-      if (anno.getAnnotationType().toString().endsWith(".Nullable")) {
-        return Nullness.NULLABLE;
-      }
+  // TODO(kmb): Fold this functionality into Dataflow.expressionDataflow
+  public Nullness getFieldInitializerNullness(TreePath fieldDeclPath, Context context) {
+    Tree decl = fieldDeclPath.getLeaf();
+    checkArgument(
+        decl instanceof VariableTree && ((JCVariableDecl) decl).sym.getKind() == ElementKind.FIELD,
+        "Leaf of fieldDeclPath must be a field declaration: %s", decl);
+
+    ExpressionTree initializer = ((VariableTree) decl).getInitializer();
+    if (initializer == null) {
+      // An uninitialized field is null or 0 to start :)
+      return ((JCVariableDecl) decl).type.isPrimitive() ? Nullness.NONNULL : Nullness.NULL;
     }
-    return Nullness.NONNULL;
+    TreePath initializerPath = TreePath.getPath(fieldDeclPath, initializer);
+    JavacProcessingEnvironment javacEnv = JavacProcessingEnvironment.instance(context);
+    UnderlyingAST ast = new UnderlyingAST.CFGStatement(decl);
+    ControlFlowGraph cfg =
+        CFGBuilder.build(
+            initializerPath,
+            javacEnv,
+            ast,
+            /* assumeAssertionsEnabled */ false,
+            /* assumeAssertionsDisabled */ false);
+    Analysis<Nullness, LocalStore<Nullness>, TrustingNullnessPropagation> analysis =
+        new Analysis<>(javacEnv, nullnessPropagation);
+    analysis.performAnalysis(cfg);
+    return analysis.getValue(initializer);
+  }
+
+  public static boolean hasNullableAnnotation(Element element) {
+    return TrustingNullnessPropagation.nullnessFromAnnotations(element) == Nullness.NULLABLE;
   }
 }

core/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/TrustingNullnessPropagation.java

@@ -20,6 +20,7 @@
 import com.google.errorprone.dataflow.LocalStore;
 import java.util.List;
 import javax.annotation.Nullable;
+import javax.lang.model.element.AnnotationMirror;
 import javax.lang.model.element.Element;
 import org.checkerframework.dataflow.cfg.UnderlyingAST;
 import org.checkerframework.dataflow.cfg.node.LocalVariableNode;
@@ -63,7 +64,7 @@ public LocalStore<Nullness> initialStore(
     LocalStore.Builder<Nullness> result = LocalStore.<Nullness>empty().toBuilder();
     for (LocalVariableNode param : parameters) {
       Element element = param.getElement();
-      Nullness assumed = TrustingNullnessAnalysis.nullnessFromAnnotations(element);
+      Nullness assumed = nullnessFromAnnotations(element);
       result.setInformation(element, assumed);
     }
     return result.build();
@@ -76,15 +77,28 @@ Nullness fieldNullness(@Nullable ClassAndField accessed) {
     }
     // In the absence of annotations, this will do the right thing for things like primitives,
     // array length, .class, etc.
-    return TrustingNullnessAnalysis.nullnessFromAnnotations(accessed.symbol);
+    return nullnessFromAnnotations(accessed.symbol);
+  }
+
+  /**
+   * Returns nullability based on the presence of a {@code Nullable} annotation.
+   */
+  static Nullness nullnessFromAnnotations(Element element) {
+    for (AnnotationMirror anno : element.getAnnotationMirrors()) {
+      // Check for Nullable like ReturnValueIsNonNull
+      if (anno.getAnnotationType().toString().endsWith(".Nullable")) {
+        return Nullness.NULLABLE;
+      }
+    }
+    return Nullness.NONNULL;
   }
 
   private enum TrustReturnAnnotation implements Predicate<MethodInfo> {
     INSTANCE;
 
     /**
-     * Returns {@code true} where {@link TrustingNullnessAnalysis#nullnessFromAnnotations} would
-     * return {@link Nullness#NONNULL}.
+     * Returns {@code true} where {@link #nullnessFromAnnotations} would return
+     * {@link Nullness#NONNULL}.
      */
     @Override
     public boolean apply(MethodInfo input) {