Version 2.0.79

Cherry-pick: Add warnings for -assumenosideeffects
CL: https://r8-review.googlesource.com/c/r8/+/51863

The test-part of the above CL has been patched as the branch does not
have the some test infrastructure for checking dianostic messages as
master.

Cherry-pick: Add a test for b/152492625
CL: https://r8-review.googlesource.com/c/r8/+/50047

Bug: 152492625
Bug: 158018192
Change-Id: I8d4b66acb3038d90af93642b546c269a708bbce0
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index 988ad95..c918040 100644
--- a/src/main/java/com/android/tools/r8/Version.java
+++ b/src/main/java/com/android/tools/r8/Version.java
@@ -11,7 +11,7 @@
 
   // This field is accessed from release scripts using simple pattern matching.
   // Therefore, changing this field could break our release scripts.
-  public static final String LABEL = "2.0.78";
+  public static final String LABEL = "2.0.79";
 
   private Version() {
   }
diff --git a/src/main/java/com/android/tools/r8/graph/DexItemFactory.java b/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
index c7934e7..799df94 100644
--- a/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
+++ b/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
@@ -132,6 +132,10 @@
   public final DexString boxedShortDescriptor = createString("Ljava/lang/Short;");
   public final DexString boxedNumberDescriptor = createString("Ljava/lang/Number;");
 
+  public final DexString waitMethodName = createString("wait");
+  public final DexString notifyMethodName = createString("notify");
+  public final DexString notifyAllMethodName = createString("notifyAll");
+
   public final DexString unboxBooleanMethodName = createString("booleanValue");
   public final DexString unboxByteMethodName = createString("byteValue");
   public final DexString unboxCharMethodName = createString("charValue");
diff --git a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
index 3df34bd..85a1c8b 100644
--- a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
+++ b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
@@ -37,6 +37,7 @@
 import com.android.tools.r8.utils.Consumer3;
 import com.android.tools.r8.utils.InternalOptions;
 import com.android.tools.r8.utils.MethodSignatureEquivalence;
+import com.android.tools.r8.utils.OriginWithPosition;
 import com.android.tools.r8.utils.StringDiagnostic;
 import com.android.tools.r8.utils.ThreadUtils;
 import com.google.common.base.Equivalence.Wrapper;
@@ -107,6 +108,9 @@
   private final DexStringCache dexStringCache = new DexStringCache();
   private final Set<ProguardIfRule> ifRules = Sets.newIdentityHashSet();
 
+  private final Map<OriginWithPosition, List<DexMethod>> assumeNoSideEffectsWarnings =
+      new HashMap<>();
+
   public RootSetBuilder(
       AppView<? extends AppInfoWithSubtyping> appView,
       DexApplication application,
@@ -298,6 +302,7 @@
     } finally {
       application.timing.end();
     }
+    generateAssumeNoSideEffectsWarnings();
     if (!noSideEffects.isEmpty() || !assumedValues.isEmpty()) {
       BottomUpClassHierarchyTraversal.forAllClasses(appView)
           .visit(appView.appInfo().classes(), this::propagateAssumeRules);
@@ -1113,6 +1118,7 @@
       mayHaveSideEffects.put(item.toReference(), rule);
       context.markAsUsed();
     } else if (context instanceof ProguardAssumeNoSideEffectRule) {
+      checkAssumeNoSideEffectsWarnings(item, (ProguardAssumeNoSideEffectRule) context, rule);
       noSideEffects.put(item.toReference(), rule);
       context.markAsUsed();
     } else if (context instanceof ProguardWhyAreYouKeepingRule) {
@@ -1336,6 +1342,66 @@
     }
   }
 
+  private void checkAssumeNoSideEffectsWarnings(
+      DexDefinition item, ProguardAssumeNoSideEffectRule context, ProguardMemberRule rule) {
+    if (rule.getRuleType() == ProguardMemberType.METHOD && rule.isSpecific()) {
+      return;
+    }
+    if (item.isDexEncodedMethod()) {
+      DexEncodedMethod method = item.asDexEncodedMethod();
+      if (method.method.holder == options.itemFactory.objectType) {
+        OriginWithPosition key = new OriginWithPosition(context.getOrigin(), context.getPosition());
+        assumeNoSideEffectsWarnings.computeIfAbsent(key, k -> new ArrayList<>()).add(method.method);
+      }
+    }
+  }
+
+  private boolean isWaitOrNotifyMethod(DexMethod method) {
+    return method.name == options.itemFactory.waitMethodName
+        || method.name == options.itemFactory.notifyMethodName
+        || method.name == options.itemFactory.notifyAllMethodName;
+  }
+
+  private void generateAssumeNoSideEffectsWarnings() {
+    ProguardClassFilter dontWarnPatterns =
+        options.getProguardConfiguration() != null
+            ? options.getProguardConfiguration().getDontWarnPatterns()
+            : ProguardClassFilter.empty();
+    if (dontWarnPatterns.matches(options.itemFactory.objectType)) {
+      return;
+    }
+
+    assumeNoSideEffectsWarnings.forEach(
+        (originWithPosition, methods) -> {
+          boolean waitOrNotifyMethods = methods.stream().anyMatch(this::isWaitOrNotifyMethod);
+          StringBuilder message = new StringBuilder();
+          message.append(
+              "The -assumenosideeffects rule matches methods on `java.lang.Object` with wildcards");
+          if (waitOrNotifyMethods) {
+            message.append(" including the method(s) ");
+            for (int i = 0; i < methods.size(); i++) {
+              if (i > 0) {
+                message.append(i < methods.size() - 1 ? ", " : " and ");
+              }
+              message.append("`");
+              message.append(methods.get(i).toSourceStringWithoutHolder());
+              message.append("`");
+            }
+            message.append(". ");
+            message.append("This will most likely cause problems. ");
+          } else {
+            message.append(". ");
+            message.append("This is most likely not intended. ");
+          }
+          message.append("Consider specifying the methods more precisely.");
+          options.reporter.warning(
+              new StringDiagnostic(
+                  message.toString(),
+                  originWithPosition.getOrigin(),
+                  originWithPosition.getPosition()));
+        });
+  }
+
   public static class RootSet extends RootSetBase {
 
     public final ImmutableList<DexReference> reasonAsked;
diff --git a/src/main/java/com/android/tools/r8/utils/OriginWithPosition.java b/src/main/java/com/android/tools/r8/utils/OriginWithPosition.java
new file mode 100644
index 0000000..8459d19
--- /dev/null
+++ b/src/main/java/com/android/tools/r8/utils/OriginWithPosition.java
@@ -0,0 +1,40 @@
+// Copyright (c) 2020, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+package com.android.tools.r8.utils;
+
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.position.Position;
+import java.util.Objects;
+
+public class OriginWithPosition {
+  private final Origin origin;
+  private final Position position;
+
+  public OriginWithPosition(Origin origin, Position position) {
+    this.origin = origin;
+    this.position = position;
+  }
+
+  public Origin getOrigin() {
+    return origin;
+  }
+
+  public Position getPosition() {
+    return position;
+  }
+
+  @Override
+  public int hashCode() {
+    return origin.hashCode() * 13 + position.hashCode();
+  }
+
+  @Override
+  public boolean equals(Object other) {
+    if (other instanceof OriginWithPosition) {
+      return Objects.equals(((OriginWithPosition) other).origin, origin)
+          && Objects.equals(((OriginWithPosition) other).position, position);
+    }
+    return false;
+  }
+}
diff --git a/src/test/java/com/android/tools/r8/shaking/assumenosideeffects/AssumenosideeffectsVisibleMethodsTest.java b/src/test/java/com/android/tools/r8/shaking/assumenosideeffects/AssumenosideeffectsVisibleMethodsTest.java
index ebf633c..ae29f09 100644
--- a/src/test/java/com/android/tools/r8/shaking/assumenosideeffects/AssumenosideeffectsVisibleMethodsTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/assumenosideeffects/AssumenosideeffectsVisibleMethodsTest.java
@@ -41,27 +41,32 @@
         case RULE_THAT_REFERS_LIB_BASE:
           return StringUtils.lines(
               "-assumenosideeffects class " + LibraryBase.class.getTypeName() + " {",
-              "  *;",
+              "  throwing(...);",
+              "  debug(...);",
               "}");
         case RULE_THAT_REFERS_PRG_BASE:
           return StringUtils.lines(
               "-assumenosideeffects class " + ProgramBase.class.getTypeName() + " {",
-              "  *;",
+              "  throwing(...);",
+              "  debug(...);",
               "}");
         case RULE_THAT_REFERS_PRG_SUB:
           return StringUtils.lines(
               "-assumenosideeffects class " + ProgramSub.class.getTypeName() + " {",
-              "  *;",
+              "  throwing(...);",
+              "  debug(...);",
               "}");
         case RULE_WITH_EXTENDS_LIB_BASE:
           return StringUtils.lines(
               "-assumenosideeffects class * extends " + LibraryBase.class.getTypeName() + " {",
-              "  *;",
+              "  throwing(...);",
+              "  debug(...);",
               "}");
         case RULE_WITH_EXTENDS_PRG_BASE:
           return StringUtils.lines(
               "-assumenosideeffects class * extends " + ProgramBase.class.getTypeName() + " {",
-              "  *;",
+              "  throwing(...);",
+              "  debug(...);",
               "}");
       }
       throw new Unreachable();
@@ -112,7 +117,9 @@
 
   @Parameterized.Parameters(name = "{0} {1}")
   public static Collection<Object[]> data() {
-    return buildParameters(getTestParameters().withAllRuntimes().build(), TestConfig.values());
+    return buildParameters(
+        getTestParameters().withAllRuntimesAndApiLevels().withAllApiLevels().build(),
+        TestConfig.values());
   }
 
   private final TestParameters parameters;
@@ -153,7 +160,7 @@
         .enableMergeAnnotations()
         .enableNeverClassInliningAnnotations()
         .enableInliningAnnotations()
-        .setMinApi(parameters.getRuntime())
+        .setMinApi(parameters.getApiLevel())
         .compile()
         .addRunClasspathFiles(parameters.isDexRuntime() ? libDexPath : libJarPath)
         .run(parameters.getRuntime(), MAIN)
diff --git a/src/test/java/com/android/tools/r8/shaking/assumenosideeffects/B152492625.java b/src/test/java/com/android/tools/r8/shaking/assumenosideeffects/B152492625.java
new file mode 100644
index 0000000..65ad0eb
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/assumenosideeffects/B152492625.java
@@ -0,0 +1,278 @@
+// Copyright (c) 2020, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+package com.android.tools.r8.shaking.assumenosideeffects;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.CoreMatchers.allOf;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.fail;
+
+import com.android.tools.r8.CompilationFailedException;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.position.TextPosition;
+import com.android.tools.r8.position.TextRange;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import com.android.tools.r8.utils.codeinspector.InstructionSubject;
+import com.google.common.collect.ImmutableList;
+import org.hamcrest.Matcher;
+import org.junit.Assert;
+import org.junit.Assume;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class B152492625 extends TestBase {
+
+  private final TestParameters parameters;
+
+  @Parameterized.Parameters(name = "{0}")
+  public static TestParametersCollection data() {
+    return getTestParameters().withAllRuntimesAndApiLevels().build();
+  }
+
+  public B152492625(TestParameters parameters) {
+    this.parameters = parameters;
+  }
+
+  private void noCallToWait(CodeInspector inspector) {
+    ClassSubject classSubject = inspector.clazz(TestClass.class);
+    assertThat(classSubject, isPresent());
+    classSubject.forAllMethods(
+        foundMethodSubject ->
+            foundMethodSubject
+                .instructions(InstructionSubject::isInvokeVirtual)
+                .forEach(
+                    instructionSubject -> {
+                      Assert.assertNotEquals(
+                          "wait", instructionSubject.getMethod().name.toString());
+                    }));
+  }
+
+  private Matcher<String> matchAssumeNoSideEffectsWarningMessage() {
+      return containsString(
+          "The -assumenosideeffects rule matches methods on `java.lang.Object` with"
+              + " wildcards");
+  }
+
+  private Matcher<String> matchWarningMessageForAllProblematicMethods() {
+    return allOf(
+        containsString("void notify()"),
+        containsString("void notifyAll()"),
+        containsString("void wait()"),
+        containsString("void wait(long)"),
+        containsString("void wait(long, int)"));
+  }
+
+  private Matcher<String> matchWarningMessageForWaitMethods() {
+    return allOf(
+        containsString("void wait()"),
+        containsString("void wait(long)"),
+        containsString("void wait(long, int)"));
+  }
+
+  private TextRange textRangeForString(String s) {
+    return new TextRange(
+        new TextPosition(0, 1, 1), new TextPosition(s.length(), 1, s.length() + 1));
+  }
+
+  @Test
+  public void testR8AllMatch() throws Exception {
+    testForR8(parameters.getBackend())
+        .addProgramClasses(TestClass.class, B.class)
+        .addKeepMainRule(TestClass.class)
+        .addKeepRules("-assumenosideeffects class " + B.class.getTypeName() + " { *; }")
+        .setMinApi(parameters.getApiLevel())
+        .compileWithExpectedDiagnostics(
+            diagnostics -> {
+              diagnostics.assertOnlyWarnings();
+              diagnostics.assertWarningMessageThatMatches(matchAssumeNoSideEffectsWarningMessage());
+              diagnostics.assertWarningMessageThatMatches(matchWarningMessageForAllProblematicMethods());
+            })
+        .inspect(this::noCallToWait)
+        .run(parameters.getRuntime(), TestClass.class)
+        .assertSuccessWithOutputLines("Hello, world");
+  }
+
+  @Test
+  public void testR8AllMatchMultipleRules() throws Exception {
+    class MyOrigin extends Origin {
+      private final String part;
+
+      public MyOrigin(String part) {
+        super(Origin.root());
+        this.part = part;
+      }
+
+      @Override
+      public String part() {
+        return part;
+      }
+    }
+
+    Origin starRuleOrigin = new MyOrigin("star rule");
+    Origin methodsRuleOrigin = new MyOrigin("methods rule");
+
+    String starRule = "-assumenosideeffects class " + B.class.getTypeName() + " { *; }";
+    String methodsRule = "-assumenosideeffects class " + B.class.getTypeName() + " { <methods>; }";
+
+    testForR8(parameters.getBackend())
+        .addProgramClasses(TestClass.class, B.class)
+        .addKeepMainRule(TestClass.class)
+        .apply(
+            b ->
+                b.getBuilder().addProguardConfiguration(ImmutableList.of(starRule), starRuleOrigin))
+        .apply(
+            b ->
+                b.getBuilder()
+                    .addProguardConfiguration(ImmutableList.of(methodsRule), methodsRuleOrigin))
+        .setMinApi(parameters.getApiLevel())
+        .compileWithExpectedDiagnostics(
+            diagnostics -> {
+              diagnostics.assertOnlyWarnings();
+              diagnostics.assertWarningMessageThatMatches(matchAssumeNoSideEffectsWarningMessage());
+              diagnostics.assertWarningMessageThatMatches(matchWarningMessageForAllProblematicMethods());
+            })
+        .inspect(this::noCallToWait)
+        .run(parameters.getRuntime(), TestClass.class)
+        .assertSuccessWithOutputLines("Hello, world");
+  }
+
+  @Test
+  public void testR8AllMatchDontWarn() throws Exception {
+    testForR8(parameters.getBackend())
+        .addProgramClasses(TestClass.class, B.class)
+        .addKeepMainRule(TestClass.class)
+        .addKeepRules("-assumenosideeffects class " + B.class.getTypeName() + " { *; }")
+        .addKeepRules("-dontwarn java.lang.Object")
+        .setMinApi(parameters.getApiLevel())
+        .compile()
+        .inspect(this::noCallToWait)
+        .run(parameters.getRuntime(), TestClass.class)
+        .assertSuccessWithOutputLines("Hello, world");
+  }
+
+  @Test
+  public void testR8AllMethodsMatch() throws Exception {
+    testForR8(parameters.getBackend())
+        .addProgramClasses(TestClass.class, B.class)
+        .addKeepMainRule(TestClass.class)
+        .addKeepRules("-assumenosideeffects class " + B.class.getTypeName() + " { <methods>; }")
+        .setMinApi(parameters.getApiLevel())
+        .compileWithExpectedDiagnostics(
+            diagnostics -> {
+              diagnostics.assertOnlyWarnings();
+              diagnostics.assertWarningMessageThatMatches(matchAssumeNoSideEffectsWarningMessage());
+              diagnostics.assertWarningMessageThatMatches(matchWarningMessageForAllProblematicMethods());
+            })
+        .inspect(this::noCallToWait)
+        .run(parameters.getRuntime(), TestClass.class)
+        .assertSuccessWithOutputLines("Hello, world");
+  }
+
+  @Test
+  public void testR8WaitMethodMatch() throws Exception {
+    testForR8(parameters.getBackend())
+        .addProgramClasses(TestClass.class, B.class)
+        .addKeepMainRule(TestClass.class)
+        .addKeepRules("-assumenosideeffects class " + B.class.getTypeName() + " { *** w*(...); }")
+        .setMinApi(parameters.getApiLevel())
+        .compileWithExpectedDiagnostics(
+            diagnostics -> {
+              diagnostics.assertOnlyWarnings();
+              diagnostics.assertWarningMessageThatMatches(matchAssumeNoSideEffectsWarningMessage());
+              diagnostics.assertWarningMessageThatMatches(matchWarningMessageForWaitMethods());
+            })
+        .inspect(this::noCallToWait)
+        .run(parameters.getRuntime(), TestClass.class)
+        .assertSuccessWithOutputLines("Hello, world");
+  }
+
+  @Test
+  public void testR8WaitSpecificMethodMatch() throws Exception {
+    testForR8(parameters.getBackend())
+        .addProgramClasses(TestClass.class, B.class)
+        .addKeepMainRule(TestClass.class)
+        .addKeepRules("-assumenosideeffects class java.lang.Object { void wait(); }")
+        .setMinApi(parameters.getApiLevel())
+        .compile()
+        .inspect(this::noCallToWait)
+        .run(parameters.getRuntime(), TestClass.class)
+        .assertSuccessWithOutputLines("Hello, world");
+  }
+
+  @Test
+  public void testR8AssumeNoSideEffectsNotConditional() throws Exception {
+    try {
+      testForR8(parameters.getBackend())
+          .addProgramClasses(TestClass.class, B.class)
+          .addKeepMainRule(TestClass.class)
+          .addKeepRules(
+              "-if class " + TestClass.class.getTypeName(),
+              " -assumenosideeffects class " + B.class.getTypeName() + " { *; }")
+          .setMinApi(parameters.getApiLevel())
+          .compileWithExpectedDiagnostics(
+              diagnostics -> {
+                diagnostics.assertOnlyErrors();
+                diagnostics.assertErrorMessageThatMatches(
+                    containsString("Expecting '-keep' option after '-if' option"));
+              });
+      fail("Expected failed compilation");
+    } catch (CompilationFailedException e) {
+      // Expected.
+    }
+  }
+
+  @Test
+  public void testProguardNotRemovingWait() throws Exception {
+    Assume.assumeTrue(parameters.isCfRuntime());
+
+    testForProguard()
+        .addProgramClasses(TestClass.class, B.class)
+        .addKeepMainRule(TestClass.class)
+        .addKeepRules("-assumenosideeffects class " + B.class.getTypeName() + " { *; }")
+        .addKeepRules("-dontwarn " + B152492625.class.getTypeName())
+        .setMinApi(parameters.getApiLevel())
+        .compile()
+        .run(parameters.getRuntime(), TestClass.class)
+        .assertFailureWithErrorThatThrows(IllegalMonitorStateException.class);
+  }
+
+  @Test
+  public void testProguardRemovingWait() throws Exception {
+    Assume.assumeTrue(parameters.isCfRuntime());
+
+    testForProguard()
+        .addProgramClasses(TestClass.class, B.class)
+        .addKeepMainRule(TestClass.class)
+        .addKeepRules("-assumenosideeffects class java.lang.Object { void wait(); }")
+        .addKeepRules("-dontwarn " + B152492625.class.getTypeName())
+        .setMinApi(parameters.getApiLevel())
+        .compile()
+        .inspect(this::noCallToWait)
+        .run(parameters.getRuntime(), TestClass.class)
+        .assertSuccessWithOutputLines("Hello, world");
+  }
+
+  static class TestClass {
+
+    public void m() throws Exception {
+      System.out.println("Hello, world");
+      // test fails if wait is not removed.
+      wait();
+    }
+
+    public static void main(String[] args) throws Exception {
+      new TestClass().m();
+    }
+  }
+
+  static class B {}
+}