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 {}
+}