Remove the AllowUnvalidatedInlinings flag.

PiperOrigin-RevId: 375695911
google · May 25, 2021 · faac827 · faac827
1 parent e602e46
commit faac827
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 39 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Inliner.java b/core/src/main/java/com/google/errorprone/bugpatterns/inlineme/Inliner.java
@@ -36,14 +36,12 @@
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.ErrorProneFlags;
 import com.google.errorprone.VisitorState;
-import com.google.errorprone.annotations.InlineMeValidationDisabled;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
 import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher;
 import com.google.errorprone.fixes.SuggestedFix;
 import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
-import com.google.errorprone.util.ASTHelpers;
 import com.google.errorprone.util.MoreAnnotations;
 import com.sun.source.tree.ExpressionStatementTree;
 import com.sun.source.tree.ExpressionTree;
@@ -78,20 +76,19 @@ public final class Inliner extends BugChecker
   static final String PREFIX_FLAG = "InlineMe:Prefix";
 
   static final String ALLOW_BREAKING_CHANGES_FLAG = "InlineMe:AllowBreakingChanges";
-  static final String ALLOW_UNVALIDATED_INLININGS_FLAG = "InlineMe:AllowUnvalidatedInlinings";
 
   private static final String INLINE_ME = "com.google.errorprone.annotations.InlineMe";
 
+  private static final String VALIDATION_DISABLED =
+      "com.google.errorprone.annotations.InlineMeValidationDisabled";
+
   private final ImmutableSet<String> apiPrefixes;
   private final boolean allowBreakingChanges;
-  private final boolean refactorUnvalidatedMethods;
 
   public Inliner(ErrorProneFlags flags) {
     this.apiPrefixes =
         ImmutableSet.copyOf(flags.getSet(PREFIX_FLAG).orElse(ImmutableSet.<String>of()));
     this.allowBreakingChanges = flags.getBoolean(ALLOW_BREAKING_CHANGES_FLAG).orElse(false);
-    this.refactorUnvalidatedMethods =
-        flags.getBoolean(ALLOW_UNVALIDATED_INLININGS_FLAG).orElse(false);
   }
 
   // TODO(b/163596864): Add support for inlining fields
@@ -144,13 +141,10 @@ private Description match(
       return Description.NO_MATCH;
     }
 
-    Api api = Api.create(symbol);
+    Api api = Api.create(symbol, state);
     if (!matchesApiPrefixes(api)) {
       return Description.NO_MATCH;
     }
-    if (shouldSkipUnvalidatedMethod(symbol, state)) {
-      return Description.NO_MATCH;
-    }
 
     Attribute.Compound inlineMe =
         symbol.getRawAttributes().stream()
@@ -269,11 +263,21 @@ private Description describe(Tree tree, SuggestedFix fix, Api api) {
   abstract static class Api {
     private static final Splitter CLASS_NAME_SPLITTER = Splitter.on('.');
 
-    static Api create(MethodSymbol method) {
+    static Api create(MethodSymbol method, VisitorState state) {
+      String extraMessage = "";
+      if (hasAnnotation(method, VALIDATION_DISABLED, state)) {
+        Attribute.Compound inlineMeValidationDisabled =
+            method.getRawAttributes().stream()
+                .filter(a -> a.type.tsym.getQualifiedName().contentEquals(VALIDATION_DISABLED))
+                .collect(onlyElement());
+        String reason = Iterables.getOnlyElement(getStrings(inlineMeValidationDisabled, "value"));
+        extraMessage = " NOTE: this is an unvalidated inlining! Reasoning: " + reason;
+      }
       return new AutoValue_Inliner_Api(
           method.owner.getQualifiedName().toString(),
           method.getSimpleName().toString(),
-          method.isConstructor());
+          method.isConstructor(),
+          extraMessage);
     }
 
     abstract String className();
@@ -282,10 +286,12 @@ static Api create(MethodSymbol method) {
 
     abstract boolean isConstructor();
 
+    abstract String extraMessage();
+
     final String deprecationMessage() {
       return shortName()
           + " is deprecated and should be inlined"
-      ;
+          + extraMessage();
     }
 
     /** Returns {@code FullyQualifiedClassName#methodName}. */
@@ -307,11 +313,6 @@ final String simpleClassName() {
     }
   }
 
-  private boolean shouldSkipUnvalidatedMethod(MethodSymbol symbol, VisitorState state) {
-    return !refactorUnvalidatedMethods
-        && ASTHelpers.hasAnnotation(symbol, InlineMeValidationDisabled.class, state);
-  }
-
   private boolean matchesApiPrefixes(Api api) {
     if (apiPrefixes.isEmpty()) {
       return true;

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/inlineme/InlinerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/inlineme/InlinerTest.java
@@ -18,9 +18,8 @@
 
 import static com.google.errorprone.bugpatterns.inlineme.Inliner.PREFIX_FLAG;
 
-import com.google.common.collect.ImmutableMap;
 import com.google.errorprone.BugCheckerRefactoringTestHelper;
-import com.google.errorprone.ErrorProneFlags;
+import com.google.errorprone.CompilationTestHelper;
 import com.google.errorprone.scanner.ScannerSupplier;
 import org.junit.Ignore;
 import org.junit.Test;
@@ -724,7 +723,7 @@ public void testReturnThis_alone() {
   }
 
   @Test
-  public void dontInlineUnvalidatedInlining() {
+  public void inlineUnvalidatedInline() {
     refactoringTestHelper
         .addInputLines(
             "Client.java",
@@ -737,6 +736,8 @@ public void dontInlineUnvalidatedInlining() {
             "  @InlineMe(replacement = \"Client.create()\", imports = \"foo.Client\")",
             "  public Client() {}",
             "  ",
+            // The Inliner wants to inline the body of this factory method to the factory method :)
+            "  @SuppressWarnings(\"InlineMeInliner\")",
             "  public static Client create() { return new Client(); }",
             "}")
         .expectUnchanged()
@@ -748,18 +749,21 @@ public void dontInlineUnvalidatedInlining() {
             "    Client client = new Client();",
             "  }",
             "}")
-        .expectUnchanged()
+        .addOutputLines(
+            "out/Caller.java",
+            "import foo.Client;",
+            "public final class Caller {",
+            "  public void doTest() {",
+            "    Client client = Client.create();",
+            "  }",
+            "}")
         .doTest();
   }
 
   @Test
-  public void inlineUnvalidatedInlineWithExtraFlagOn() {
-    BugCheckerRefactoringTestHelper.newInstance(
-            new Inliner(
-                ErrorProneFlags.fromMap(
-                    ImmutableMap.of(Inliner.ALLOW_UNVALIDATED_INLININGS_FLAG, "true"))),
-            getClass())
-        .addInputLines(
+  public void inlineUnvalidatedInlineMessage() {
+    CompilationTestHelper.newInstance(Inliner.class, getClass())
+        .addSourceLines(
             "Client.java",
             "package foo;",
             "import com.google.errorprone.annotations.InlineMe;",
@@ -774,23 +778,16 @@ public void inlineUnvalidatedInlineWithExtraFlagOn() {
             "  @SuppressWarnings(\"InlineMeInliner\")",
             "  public static Client create() { return new Client(); }",
             "}")
-        .expectUnchanged()
-        .addInputLines(
+        .addSourceLines(
             "Caller.java",
             "import foo.Client;",
             "public final class Caller {",
             "  public void doTest() {",
+            "    // BUG: Diagnostic contains: NOTE: this is an unvalidated inlining!"
+                + " Reasoning: Migrating to factory method",
             "    Client client = new Client();",
             "  }",
             "}")
-        .addOutputLines(
-            "out/Caller.java",
-            "import foo.Client;",
-            "public final class Caller {",
-            "  public void doTest() {",
-            "    Client client = Client.create();",
-            "  }",
-            "}")
         .doTest();
   }