Version 2.1.35
Cherry-pick: Add warnings for -assumenosideeffects
CL: https://r8-review.googlesource.com/c/r8/+/51863
Bug: 152492625
Bug: 158018192
Change-Id: I3c0a838b5384cc742a57987a4496f715711d8e1f
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index 84acd31..bc82245 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.1.34";
+ public static final String LABEL = "2.1.35";
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 1a318c0..60101db 100644
--- a/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
+++ b/src/main/java/com/android/tools/r8/graph/DexItemFactory.java
@@ -147,6 +147,10 @@
public final DexString boxedNumberDescriptor = createString("Ljava/lang/Number;");
public final DexString boxedVoidDescriptor = createString("Ljava/lang/Void;");
+ 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 ffb940a..73171b4 100644
--- a/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
+++ b/src/main/java/com/android/tools/r8/shaking/RootSetBuilder.java
@@ -40,6 +40,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.PredicateSet;
import com.android.tools.r8.utils.StringDiagnostic;
import com.android.tools.r8.utils.ThreadUtils;
@@ -113,6 +114,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 AppInfoWithClassHierarchy> appView,
SubtypingInfo subtypingInfo,
@@ -304,6 +308,7 @@
} finally {
application.timing.end();
}
+ generateAssumeNoSideEffectsWarnings();
if (!noSideEffects.isEmpty() || !assumedValues.isEmpty()) {
BottomUpClassHierarchyTraversal.forAllClasses(appView, subtypingInfo)
.visit(appView.appInfo().classes(), this::propagateAssumeRules);
@@ -1117,6 +1122,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) {
@@ -1376,6 +1382,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.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/DiagnosticsMatcher.java b/src/test/java/com/android/tools/r8/DiagnosticsMatcher.java
index 5f6b792..70518d7 100644
--- a/src/test/java/com/android/tools/r8/DiagnosticsMatcher.java
+++ b/src/test/java/com/android/tools/r8/DiagnosticsMatcher.java
@@ -4,6 +4,7 @@
package com.android.tools.r8;
import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.position.Position;
import com.android.tools.r8.utils.ExceptionDiagnostic;
import org.hamcrest.Description;
import org.hamcrest.Matcher;
@@ -64,7 +65,21 @@
@Override
protected void explain(Description description) {
- description.appendText("orgin ").appendText(origin.toString());
+ description.appendText("origin ").appendText(origin.toString());
+ }
+ };
+ }
+
+ public static Matcher<Diagnostic> diagnosticPosition(Position position) {
+ return new DiagnosticsMatcher() {
+ @Override
+ protected boolean eval(Diagnostic diagnostic) {
+ return diagnostic.getPosition().equals(position);
+ }
+
+ @Override
+ protected void explain(Description description) {
+ description.appendText("position ").appendText(position.getDescription());
}
};
}
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..71614d2 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().withAllApiLevelsAlsoForCf().build(),
+ TestConfig.values());
}
private final TestParameters parameters;
@@ -145,7 +152,7 @@
public void testR8() throws Exception {
testForR8(parameters.getBackend())
.addLibraryFiles(libJarPath)
- .addLibraryFiles(ToolHelper.getDefaultAndroidJar())
+ .addLibraryFiles(ToolHelper.getFirstSupportedAndroidJar(parameters.getApiLevel()))
.addProgramClasses(ProgramBase.class, ProgramSub.class, MAIN)
.addKeepMainRule(MAIN)
.addKeepRules(config.getKeepRule())
@@ -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
index fe72a94..560f6ea 100644
--- a/src/test/java/com/android/tools/r8/shaking/assumenosideeffects/B152492625.java
+++ b/src/test/java/com/android/tools/r8/shaking/assumenosideeffects/B152492625.java
@@ -4,15 +4,28 @@
package com.android.tools.r8.shaking.assumenosideeffects;
+import static com.android.tools.r8.DiagnosticsMatcher.diagnosticMessage;
+import static com.android.tools.r8.DiagnosticsMatcher.diagnosticOrigin;
+import static com.android.tools.r8.DiagnosticsMatcher.diagnosticPosition;
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.Diagnostic;
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;
@@ -47,13 +60,118 @@
}));
}
+ private Matcher<Diagnostic> matchAssumeNoSideEffectsWarningMessage() {
+ return diagnosticMessage(
+ containsString(
+ "The -assumenosideeffects rule matches methods on `java.lang.Object` with"
+ + " wildcards"));
+ }
+
+ private Matcher<Diagnostic> matchWarningMessageForAllProblematicMethods() {
+ return diagnosticMessage(
+ allOf(
+ containsString("void notify()"),
+ containsString("void notifyAll()"),
+ containsString("void wait()"),
+ containsString("void wait(long)"),
+ containsString("void wait(long, int)")));
+ }
+
+ private Matcher<Diagnostic> matchWarningMessageForWaitMethods() {
+ return diagnosticMessage(
+ 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 testR8() throws Exception {
+ public void testR8AllMatch() throws Exception {
testForR8(parameters.getBackend())
- .addInnerClasses(B152492625.class)
+ .addProgramClasses(TestClass.class, B.class)
.addKeepMainRule(TestClass.class)
.addKeepRules("-assumenosideeffects class " + B.class.getTypeName() + " { *; }")
.setMinApi(parameters.getApiLevel())
+ .allowDiagnosticWarningMessages()
+ .compileWithExpectedDiagnostics(
+ diagnostics -> {
+ diagnostics.assertOnlyWarnings();
+ diagnostics.assertWarningsMatch(matchAssumeNoSideEffectsWarningMessage());
+ diagnostics.assertWarningsMatch(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())
+ .allowDiagnosticWarningMessages()
+ .compileWithExpectedDiagnostics(
+ diagnostics -> {
+ diagnostics.assertOnlyWarnings();
+ diagnostics.assertWarningsMatch(
+ ImmutableList.of(
+ allOf(
+ matchAssumeNoSideEffectsWarningMessage(),
+ matchWarningMessageForAllProblematicMethods(),
+ diagnosticOrigin(starRuleOrigin),
+ diagnosticPosition(textRangeForString(starRule))),
+ allOf(
+ matchAssumeNoSideEffectsWarningMessage(),
+ matchWarningMessageForAllProblematicMethods(),
+ diagnosticOrigin(methodsRuleOrigin),
+ diagnosticPosition(textRangeForString(methodsRule)))));
+ })
+ .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)
@@ -61,11 +179,85 @@
}
@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())
+ .allowDiagnosticWarningMessages()
+ .compileWithExpectedDiagnostics(
+ diagnostics -> {
+ diagnostics.assertOnlyWarnings();
+ diagnostics.assertWarningsMatch(matchAssumeNoSideEffectsWarningMessage());
+ diagnostics.assertWarningsMatch(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())
+ .allowDiagnosticWarningMessages()
+ .compileWithExpectedDiagnostics(
+ diagnostics -> {
+ diagnostics.assertOnlyWarnings();
+ diagnostics.assertWarningsMatch(matchAssumeNoSideEffectsWarningMessage());
+ diagnostics.assertWarningsMatch(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.assertErrorsMatch(
+ diagnosticMessage(
+ 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()
- .addInnerClasses(B152492625.class)
+ .addProgramClasses(TestClass.class, B.class)
.addKeepMainRule(TestClass.class)
.addKeepRules("-assumenosideeffects class " + B.class.getTypeName() + " { *; }")
.addKeepRules("-dontwarn " + B152492625.class.getTypeName())
@@ -80,7 +272,7 @@
Assume.assumeTrue(parameters.isCfRuntime());
testForProguard()
- .addInnerClasses(B152492625.class)
+ .addProgramClasses(TestClass.class, B.class)
.addKeepMainRule(TestClass.class)
.addKeepRules("-assumenosideeffects class java.lang.Object { void wait(); }")
.addKeepRules("-dontwarn " + B152492625.class.getTypeName())
