Cleanup of OverridesJavaxInjectableMethod

look at all super methods, not just when a method has @OverRide as an
annotation.

MOE_MIGRATED_REVID=130455624

core/src/main/java/com/google/errorprone/bugpatterns/inject/guice/OverridesJavaxInjectableMethod.java

@@ -31,65 +31,54 @@
 import com.google.errorprone.matchers.Matchers;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.MethodTree;
-import com.sun.tools.javac.code.Symbol;
 import com.sun.tools.javac.code.Symbol.MethodSymbol;
-import com.sun.tools.javac.code.Symbol.TypeSymbol;
 
 /**
- * This checker matches methods that
- *   1) are not themselves annotated with @Inject
- *   2) descend from a method that is annotated with @javax.inject.Inject
- *   3) do not descent from a method that is annotated with @com.google.inject.Inject
+ * This checker matches methods that 1) are not themselves annotated with @Inject 2) descend from a
+ * method that is annotated with @javax.inject.Inject 3) do not descent from a method that is
+ * annotated with @com.google.inject.Inject
  *
  * @author sgoldfeder@google.com (Steven Goldfeder)
  */
 @BugPattern(
   name = "OverridesJavaxInjectableMethod",
   summary =
-      "This method is not annotated with @Inject, but it overrides a  method that is "
-          + " annotated with @javax.inject.Inject.",
-  explanation =
-      "According to the JSR-330 spec, a method that overrides a method annotated "
-          + "with javax.inject.Inject will not be injected unless it iself is annotated with "
-          + " @Inject",
+      "This method is not annotated with @Inject, but it overrides a method that is "
+          + " annotated with @javax.inject.Inject. The method will not be Injected.",
   category = GUICE,
   severity = ERROR,
   maturity = EXPERIMENTAL
 )
 public class OverridesJavaxInjectableMethod extends BugChecker implements MethodTreeMatcher {
 
-  private static final String OVERRIDE_ANNOTATION = "java.lang.Override";
   private static final String GUICE_INJECT_ANNOTATION = "com.google.inject.Inject";
   private static final String JAVAX_INJECT_ANNOTATION = "javax.inject.Inject";
 
   private static final Matcher<MethodTree> INJECTABLE_METHOD_MATCHER =
       Matchers.<MethodTree>anyOf(
           hasAnnotation(GUICE_INJECT_ANNOTATION), hasAnnotation(JAVAX_INJECT_ANNOTATION));
 
-  private static final Matcher<MethodTree> OVERRIDE_METHOD_MATCHER =
-      Matchers.<MethodTree>hasAnnotation(OVERRIDE_ANNOTATION);
-
   @Override
   public Description matchMethod(MethodTree methodTree, VisitorState state) {
     // if method is itself annotated with @Inject or it has no ancestor methods, return NO_MATCH;
-    if (INJECTABLE_METHOD_MATCHER.matches(methodTree, state)
-        || !OVERRIDE_METHOD_MATCHER.matches(methodTree, state)) {
+    if (INJECTABLE_METHOD_MATCHER.matches(methodTree, state)) {
       return Description.NO_MATCH;
     }
+
     boolean foundJavaxInject = false;
-    MethodSymbol method = ASTHelpers.getSymbol(methodTree);
-    MethodSymbol superMethod = null;
-    for (boolean checkSuperClass = true; checkSuperClass; method = superMethod) {
-      superMethod = findSuperMethod(method, state);
+    for (MethodSymbol superMethod :
+        ASTHelpers.findSuperMethods(ASTHelpers.getSymbol(methodTree), state.getTypes())) {
+
+      // With a Guice annotation, Guice will still inject the subclass-overridden method.
       if (ASTHelpers.hasAnnotation(superMethod, GUICE_INJECT_ANNOTATION, state)) {
         return Description.NO_MATCH;
       }
+
       // is not necessarily a match even if we find javax Inject on an ancestor
       // since a higher up ancestor may have @com.google.inject.Inject
-      foundJavaxInject = ASTHelpers.hasAnnotation(superMethod, JAVAX_INJECT_ANNOTATION, state);
-      // check if there are ancestor methods
-      checkSuperClass = ASTHelpers.hasAnnotation(superMethod, OVERRIDE_ANNOTATION, state);
+      foundJavaxInject |= ASTHelpers.hasAnnotation(superMethod, JAVAX_INJECT_ANNOTATION, state);
     }
+
     if (foundJavaxInject) {
       return describeMatch(
           methodTree,
@@ -101,14 +90,4 @@ public Description matchMethod(MethodTree methodTree, VisitorState state) {
     return Description.NO_MATCH;
   }
 
-  private MethodSymbol findSuperMethod(MethodSymbol method, VisitorState state) {
-    TypeSymbol superClass = method.enclClass().getSuperclass().tsym;
-    for (Symbol s : superClass.members().getSymbols()) {
-      if (s.name.contentEquals(method.name)
-          && method.overrides(s, superClass, state.getTypes(), true)) {
-        return (MethodSymbol) s;
-      }
-    }
-    return null;
-  }
 }

core/src/test/java/com/google/errorprone/bugpatterns/inject/guice/testdata/OverridesJavaxInjectableMethodNegativeCases.java

@@ -16,6 +16,7 @@
 
 package com.google.errorprone.bugpatterns.inject.guice.testdata;
 
+
 /**
  * @author sgoldfeder@gooogle.com (Steven Goldfeder)
  */
@@ -43,55 +44,35 @@ public class TestClass3 {
     public void foo() {}
   }
 
-  /**
-   * Class with a method foo() that is not annotated with @Inject, but overrides a method that is
-   * annotated with @com.google.inject.Inject.
-   */
+  /** OK, as it overrides a Guice-Inject method */
   public class TestClass4 extends TestClass2 {
     @Override
     public void foo() {}
   }
 
-  /**
-   * Class with a method foo() annotated with @com.google.inject.Inject that overrides a method
-   * annotated with @javax.inject.Inject.
-   */
+  /** gInject <- jInject */
   public class TestClass5 extends TestClass3 {
     @com.google.inject.Inject
     public void foo() {}
   }
 
-  /**
-   * Class with a method foo() annotated with @javax.inject.Inject that overrides a method 
-   * annotated with @com.google.inject.Inject.
-   */
+  /** jInject <- gInject */
   public class TestClass6 extends TestClass2 {
     @javax.inject.Inject
     public void foo() {}
   }
 
-  /**
-   * Class with a method foo() that is not annotated, but overrides a method that is annotated with
-   * @javax.inject.Inject, and that method in turn overrides a method annotated with
-   * @com.google.inject.Inject
-   */
+  /** OK, as 7 <- jInject <- gInject */
   public class TestClass7 extends TestClass6 {
     public void foo() {}
   }
 
-  /**
-   * Class with a method foo() that is not annotated, but overrides a method that is annotated with
-   * @com.google.inject.Inject, and that method in turn overrides a method annotated with
-   * @javax.inject.Inject
-   */
+  /** OK, as 8 <- gInject */
   public class TestClass8 extends TestClass5 {
     public void foo() {}
   }
 
-  /**
-   * Class with a method foo() that is not annotated, but overrides a method annotated with
-   * @javax.inject.Inject. Error is suppressed.
-   */
+  /** Explicitly suppressed warning */
   public class TestClass9 extends TestClass3 {
     @Override 
     @SuppressWarnings("OverridesJavaxInjectableMethod")

core/src/test/java/com/google/errorprone/bugpatterns/inject/guice/testdata/OverridesJavaxInjectableMethodPositiveCases.java

@@ -21,11 +21,16 @@
  */
 public class OverridesJavaxInjectableMethodPositiveCases {
 
+  /** Class with foo() */
+  public class TestClass0 {
+    public void foo() {}
+  }
+
   /**
-   * Class with a method foo() that is annotated with @javax.inject.Inject. Other test classes will
-   * extend this class.
+   * Class with a method foo() that is annotated with {@code javax.inject.Inject}. Other test
+   * classes will extend this class.
    */
-  public class TestClass1 {
+  public class TestClass1 extends TestClass0 {
     @javax.inject.Inject
     public void foo() {}
   }
@@ -35,7 +40,6 @@ public void foo() {}
    * @javax.inject.Inject.
    */
   public class TestClass2 extends TestClass1 {
-    @Override 
     // BUG: Diagnostic contains: @Inject
     public void foo() {}
   }
@@ -45,7 +49,6 @@ public void foo() {}
    * a method that is annotated with @javax.inject.Inject.
    */
   public class TestClass3 extends TestClass2 {
-    @Override 
     // BUG: Diagnostic contains: @Inject
     public void foo() {}
   }

docs/bugpattern/OverridesJavaxInjectableMethod.md

@@ -0,0 +1,26 @@
+When classes declare that they have an `@javax.inject.Inject`ed method,
+dependency injection tools must call those methods after first calling any
+`@javax.inject.Inject` constructor, and performing any field injection. These
+methods are part of the initialization contract for the object.
+
+When subclasses override methods annotated with `@javax.inject.Inject` and
+*don't* also annotate themselves with `@javax.inject.Inject`, the injector will
+not call those methods as part of the subclass's initialization. This may
+unexpectedly cause assumptions taken in the superclass (e.g.: this
+post-initialization routine is finished, meaning that I can safely use this
+field) to no longer hold.
+
+This compile error is intended to prevent this unintentional breaking of
+assumptions. Possible resolutions to this error include:
+
+*   `@Inject` the overridden method, calling the `super` method to maintain the
+    initialization contract.
+*   Make the superclass' method `final` to avoid subclasses unintentionally
+    masking the injected method.
+*   Move the initialization work performed by the superclass method into the
+    constructor.
+*   Suppress this error, and very carefully inspect the initialization routine
+    performed by the superclass, making sure that any work that needs to be done
+    there is done in an @Inject method in the subclass. You may want to refactor
+    portions of the body of the superclass method into a `protected` method for
+    this subclass to use.