Raise warning for -identifiernamestring rules matching inlinable fields
Bug: 214263216
Change-Id: Ibc135459f8f88285a3871db47db2c1d07203b1cf
diff --git a/src/main/java/com/android/tools/r8/graph/DexEncodedField.java b/src/main/java/com/android/tools/r8/graph/DexEncodedField.java
index 3ae4b1f..1aa41dc 100644
--- a/src/main/java/com/android/tools/r8/graph/DexEncodedField.java
+++ b/src/main/java/com/android/tools/r8/graph/DexEncodedField.java
@@ -367,10 +367,33 @@
isInlinableByJavaC = true;
}
- public boolean isInlinableByJavaC() {
+ public boolean getIsInlinableByJavaC() {
return isInlinableByJavaC;
}
+ public boolean getOrComputeIsInlinableByJavaC(DexItemFactory dexItemFactory) {
+ if (getIsInlinableByJavaC()) {
+ return true;
+ }
+ if (!isStatic() || !isFinal()) {
+ return false;
+ }
+ if (!hasExplicitStaticValue()) {
+ return false;
+ }
+ if (getType().isPrimitiveType()) {
+ return true;
+ }
+ if (getType() != dexItemFactory.stringType) {
+ return false;
+ }
+ if (!getStaticValue().isDexValueString()) {
+ return false;
+ }
+ markAsInlinableByJavaC();
+ return true;
+ }
+
public static class Builder {
private DexField field;
diff --git a/src/main/java/com/android/tools/r8/shaking/IfRuleEvaluator.java b/src/main/java/com/android/tools/r8/shaking/IfRuleEvaluator.java
index 98ffa2a..8596642 100644
--- a/src/main/java/com/android/tools/r8/shaking/IfRuleEvaluator.java
+++ b/src/main/java/com/android/tools/r8/shaking/IfRuleEvaluator.java
@@ -237,7 +237,6 @@
// Fields that are javac inlined are unsound as predicates for conditional rules.
// Ignore any such field members and record it for possible reporting later.
if (isFieldInlinedByJavaC(f)) {
- f.markAsInlinableByJavaC();
fieldsInlinedByJavaC.add(DexClassAndField.create(targetClass, f));
return false;
}
@@ -317,21 +316,9 @@
if (enqueuer.getMode().isFinalTreeShaking()) {
// Ignore any field value in the final tree shaking pass so it remains consistent with the
// initial pass.
- return field.isInlinableByJavaC();
+ return field.getIsInlinableByJavaC();
}
- if (!field.isStatic() || !field.isFinal()) {
- return false;
- }
- if (!field.hasExplicitStaticValue()) {
- return false;
- }
- if (field.getType().isPrimitiveType()) {
- return true;
- }
- if (field.getType() != appView.dexItemFactory().stringType) {
- return false;
- }
- return field.getStaticValue().isDexValueString();
+ return field.getOrComputeIsInlinableByJavaC(appView.dexItemFactory());
}
private void materializeIfRule(ProguardIfRule rule, Set<DexReference> preconditions) {
diff --git a/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java b/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java
index 9629fb2..2f7d3f4 100644
--- a/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java
+++ b/src/main/java/com/android/tools/r8/shaking/RootSetUtils.java
@@ -62,6 +62,7 @@
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.Reporter;
import com.android.tools.r8.utils.StringDiagnostic;
import com.android.tools.r8.utils.ThreadUtils;
import com.android.tools.r8.utils.TraversalContinuation;
@@ -1299,13 +1300,7 @@
throw new Unreachable();
}
} else if (context instanceof ProguardIdentifierNameStringRule) {
- if (item.isField()) {
- identifierNameStrings.add(item.asField().getReference());
- context.markAsUsed();
- } else if (item.isMethod()) {
- identifierNameStrings.add(item.asMethod().getReference());
- context.markAsUsed();
- }
+ evaluateIdentifierNameStringRule(item, context, ifRule);
} else if (context instanceof ConstantArgumentRule) {
if (item.isMethod()) {
keepParametersWithConstantValue.add(item.asMethod().getReference());
@@ -1559,6 +1554,36 @@
}
}
+ private void evaluateIdentifierNameStringRule(
+ Definition item, ProguardConfigurationRule context, ProguardIfRule ifRule) {
+ // Main dex rules should not contain -identifiernamestring rules.
+ assert !isMainDexRootSetBuilder();
+
+ // We don't expect -identifiernamestring rules to be used as the subsequent of -if rules.
+ assert ifRule == null;
+
+ if (item.isClass()) {
+ return;
+ }
+
+ if (item.isField()) {
+ DexClassAndField field = item.asField();
+ if (field.getDefinition().getOrComputeIsInlinableByJavaC(appView.dexItemFactory())) {
+ Reporter reporter = appView.reporter();
+ reporter.warning(
+ new StringDiagnostic(
+ "Rule matches the static final field `"
+ + field.toSourceString()
+ + "`, which may have been inlined: "
+ + context.toString(),
+ context.getOrigin()));
+ }
+ }
+
+ identifierNameStrings.add(item.asMember().getReference());
+ context.markAsUsed();
+ }
+
private boolean isInterfaceMethodNeedingDesugaring(ProgramDefinition item) {
return options.isInterfaceMethodDesugaringEnabled()
&& item.isMethod()
diff --git a/src/test/java/com/android/tools/r8/naming/identifiernamestring/IdentifierNameStringStaticFinalFieldTest.java b/src/test/java/com/android/tools/r8/naming/identifiernamestring/IdentifierNameStringStaticFinalFieldTest.java
new file mode 100644
index 0000000..6d0c21d
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/identifiernamestring/IdentifierNameStringStaticFinalFieldTest.java
@@ -0,0 +1,61 @@
+// Copyright (c) 2022, 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.naming.identifiernamestring;
+
+import static com.android.tools.r8.DiagnosticsMatcher.diagnosticMessage;
+import static org.hamcrest.CoreMatchers.equalTo;
+
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.AndroidApiLevel;
+import com.android.tools.r8.utils.StringUtils;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class IdentifierNameStringStaticFinalFieldTest extends TestBase {
+
+ @Parameter(0)
+ public TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withNoneRuntime().build();
+ }
+
+ @Test
+ public void test() throws Exception {
+ testForR8(Backend.DEX)
+ .addInnerClasses(getClass())
+ .addKeepClassAndMembersRules(Main.class)
+ .addKeepRules("-identifiernamestring class * { java.lang.String CLASS_NAME; }")
+ .allowDiagnosticWarningMessages()
+ .setMinApi(AndroidApiLevel.B)
+ .compileWithExpectedDiagnostics(
+ diagnostics ->
+ diagnostics.assertWarningsMatch(
+ diagnosticMessage(
+ equalTo(
+ StringUtils.joinLines(
+ "Rule matches the static final field `java.lang.String "
+ + Main.class.getTypeName()
+ + ".CLASS_NAME`, which may have been inlined:"
+ + " -identifiernamestring class * {",
+ " java.lang.String CLASS_NAME;",
+ "}")))));
+ }
+
+ static class Main {
+
+ // @IdentifierNameString
+ public static final String CLASS_NAME = "Foo";
+
+ public static void main(String[] args) {}
+ }
+}
