[KeepAnno] Rename classTypeName to className and refactor parsing.
The parser refactoring is to make each kind of info more self contained
and to help ensure correct "single declarations", e.g., that an item
cannot both specify `classConstant` and `className`.
Bug: b/248408342
Change-Id: I28c8aa069e0f2f9a86cdef70bfb56df9fcfdafa1
diff --git a/src/keepanno/java/com/android/tools/r8/keepanno/annotations/KeepCondition.java b/src/keepanno/java/com/android/tools/r8/keepanno/annotations/KeepCondition.java
index 8b76c13..2d5eb60 100644
--- a/src/keepanno/java/com/android/tools/r8/keepanno/annotations/KeepCondition.java
+++ b/src/keepanno/java/com/android/tools/r8/keepanno/annotations/KeepCondition.java
@@ -25,11 +25,11 @@
@Target(ElementType.ANNOTATION_TYPE)
@Retention(RetentionPolicy.CLASS)
public @interface KeepCondition {
+ String className() default "";
+
Class<?> classConstant() default Object.class;
- String classTypeName() default "";
-
- String extendsClassTypeName() default "";
+ String extendsClassName() default "";
Class<?> extendsClassConstant() default Object.class;
diff --git a/src/keepanno/java/com/android/tools/r8/keepanno/annotations/KeepConstants.java b/src/keepanno/java/com/android/tools/r8/keepanno/annotations/KeepConstants.java
index 2e23bc9..02eb5df 100644
--- a/src/keepanno/java/com/android/tools/r8/keepanno/annotations/KeepConstants.java
+++ b/src/keepanno/java/com/android/tools/r8/keepanno/annotations/KeepConstants.java
@@ -48,10 +48,11 @@
// Implicit hidden item which is "super type" of Condition and Target.
public static final class Item {
+ public static final String className = "className";
public static final String classConstant = "classConstant";
+ public static final String extendsClassName = "extendsClassName";
public static final String extendsClassConstant = "extendsClassConstant";
- public static final String extendsTypeName = "extendsTypeName";
public static final String methodName = "methodName";
public static final String methodReturnType = "methodReturnType";
@@ -63,6 +64,12 @@
// Default values for the optional entries. The defaults should be chosen such that they do
// not coincide with any actual valid value. E.g., the empty string in place of a name or type.
// These must be 1:1 with the value defined on the actual annotation definition.
+ public static final String classNameDefault = "";
+ public static final Class<?> classConstantDefault = Object.class;
+
+ public static final String extendsClassNameDefault = "";
+ public static final Class<?> extendsClassConstantDefault = Object.class;
+
public static final String methodNameDefaultValue = "";
public static final String methodReturnTypeDefaultValue = "";
public static final String[] methodParametersDefaultValue = new String[] {""};
diff --git a/src/keepanno/java/com/android/tools/r8/keepanno/annotations/KeepTarget.java b/src/keepanno/java/com/android/tools/r8/keepanno/annotations/KeepTarget.java
index 8a01df9..e4cb263 100644
--- a/src/keepanno/java/com/android/tools/r8/keepanno/annotations/KeepTarget.java
+++ b/src/keepanno/java/com/android/tools/r8/keepanno/annotations/KeepTarget.java
@@ -34,11 +34,11 @@
// Shared KeepItem content ========================
+ String className() default "";
+
Class<?> classConstant() default Object.class;
- String classTypeName() default "";
-
- String extendsClassTypeName() default "";
+ String extendsClassName() default "";
Class<?> extendsClassConstant() default Object.class;
diff --git a/src/keepanno/java/com/android/tools/r8/keepanno/asm/KeepEdgeReader.java b/src/keepanno/java/com/android/tools/r8/keepanno/asm/KeepEdgeReader.java
index 73f9b00..c7621a6 100644
--- a/src/keepanno/java/com/android/tools/r8/keepanno/asm/KeepEdgeReader.java
+++ b/src/keepanno/java/com/android/tools/r8/keepanno/asm/KeepEdgeReader.java
@@ -19,7 +19,7 @@
import com.android.tools.r8.keepanno.ast.KeepFieldPattern;
import com.android.tools.r8.keepanno.ast.KeepFieldTypePattern;
import com.android.tools.r8.keepanno.ast.KeepItemPattern;
-import com.android.tools.r8.keepanno.ast.KeepItemPattern.Builder;
+import com.android.tools.r8.keepanno.ast.KeepMemberPattern;
import com.android.tools.r8.keepanno.ast.KeepMethodNamePattern;
import com.android.tools.r8.keepanno.ast.KeepMethodParametersPattern;
import com.android.tools.r8.keepanno.ast.KeepMethodPattern;
@@ -385,118 +385,149 @@
}
}
- private abstract static class KeepItemVisitorBase extends AnnotationVisitorBase {
- private final Parent<KeepItemPattern> parent;
+ private abstract static class Declaration<T> {
+ abstract String kind();
- private KeepQualifiedClassNamePattern classNamePattern = null;
- private KeepMethodPattern.Builder lazyMethodBuilder = null;
- private KeepFieldPattern.Builder lazyFieldBuilder = null;
+ abstract T getValue();
- private KeepExtendsPattern extendsPattern = null;
- private String extendPatternDeclaration = null;
-
- public KeepItemVisitorBase(Parent<KeepItemPattern> parent) {
- this.parent = parent;
+ boolean tryParse(String name, Object value) {
+ return false;
}
- private KeepMethodPattern.Builder methodBuilder() {
- if (lazyFieldBuilder != null) {
- throw new KeepEdgeException("Cannot define both a field and a method pattern");
- }
- if (lazyMethodBuilder == null) {
- lazyMethodBuilder = KeepMethodPattern.builder();
- }
- return lazyMethodBuilder;
+ AnnotationVisitor tryParseArray(String name) {
+ return null;
}
+ }
- private KeepFieldPattern.Builder fieldBuilder() {
- if (lazyMethodBuilder != null) {
- throw new KeepEdgeException("Cannot define both a field and a method pattern");
- }
- if (lazyFieldBuilder == null) {
- lazyFieldBuilder = KeepFieldPattern.builder();
- }
- return lazyFieldBuilder;
+ private abstract static class SingleDeclaration<T> extends Declaration<T> {
+ private String declarationName = null;
+ private T declarationValue = null;
+
+ abstract T getDefaultValue();
+
+ abstract T parse(String name, Object value);
+
+ @Override
+ public final T getValue() {
+ return declarationValue == null ? getDefaultValue() : declarationValue;
}
@Override
- public void visit(String name, Object value) {
+ final boolean tryParse(String name, Object value) {
+ T result = parse(name, value);
+ if (result != null) {
+ if (declarationName != null) {
+ throw new KeepEdgeException(
+ "Multiple declarations defining "
+ + kind()
+ + ": '"
+ + declarationName
+ + "' and '"
+ + name
+ + "'");
+ }
+ declarationName = name;
+ declarationValue = result;
+ return true;
+ }
+ return false;
+ }
+ }
+
+ private static class ClassDeclaration extends SingleDeclaration<KeepQualifiedClassNamePattern> {
+ @Override
+ String kind() {
+ return "class";
+ }
+
+ @Override
+ KeepQualifiedClassNamePattern getDefaultValue() {
+ return KeepQualifiedClassNamePattern.any();
+ }
+
+ @Override
+ KeepQualifiedClassNamePattern parse(String name, Object value) {
if (name.equals(Item.classConstant) && value instanceof Type) {
- classNamePattern = KeepQualifiedClassNamePattern.exact(((Type) value).getClassName());
- return;
+ return KeepQualifiedClassNamePattern.exact(((Type) value).getClassName());
}
- if (tryParseExtendsPattern(name, value)) {
- return;
+ if (name.equals(Item.className) && value instanceof String) {
+ return KeepQualifiedClassNamePattern.exact(((String) value));
}
+ return null;
+ }
+ }
+
+ private static class ExtendsDeclaration extends SingleDeclaration<KeepExtendsPattern> {
+
+ @Override
+ String kind() {
+ return "extends";
+ }
+
+ @Override
+ KeepExtendsPattern getDefaultValue() {
+ return KeepExtendsPattern.any();
+ }
+
+ @Override
+ KeepExtendsPattern parse(String name, Object value) {
+ if (name.equals(Item.extendsClassConstant) && value instanceof Type) {
+ return KeepExtendsPattern.builder()
+ .classPattern(KeepQualifiedClassNamePattern.exact(((Type) value).getClassName()))
+ .build();
+ }
+ if (name.equals(Item.extendsClassName) && value instanceof String) {
+ return KeepExtendsPattern.builder()
+ .classPattern(KeepQualifiedClassNamePattern.exact(((String) value)))
+ .build();
+ }
+ return null;
+ }
+ }
+
+ private static class MethodDeclaration extends Declaration<KeepMethodPattern> {
+
+ private KeepMethodPattern.Builder builder = null;
+
+ private KeepMethodPattern.Builder getBuilder() {
+ if (builder == null) {
+ builder = KeepMethodPattern.builder();
+ }
+ return builder;
+ }
+
+ @Override
+ String kind() {
+ return "method";
+ }
+
+ @Override
+ KeepMethodPattern getValue() {
+ return builder != null ? builder.build() : null;
+ }
+
+ @Override
+ boolean tryParse(String name, Object value) {
if (name.equals(Item.methodName) && value instanceof String) {
String methodName = (String) value;
if (!Item.methodNameDefaultValue.equals(methodName)) {
- methodBuilder().setNamePattern(KeepMethodNamePattern.exact(methodName));
+ getBuilder().setNamePattern(KeepMethodNamePattern.exact(methodName));
}
- return;
+ return true;
}
if (name.equals(Item.methodReturnType) && value instanceof String) {
String returnType = (String) value;
if (!Item.methodReturnTypeDefaultValue.equals(returnType)) {
- methodBuilder()
+ getBuilder()
.setReturnTypePattern(KeepEdgeReaderUtils.methodReturnTypeFromString(returnType));
}
- return;
- }
- if (name.equals(Item.fieldName) && value instanceof String) {
- String fieldName = (String) value;
- if (!Item.fieldNameDefaultValue.equals(fieldName)) {
- fieldBuilder().setNamePattern(KeepFieldNamePattern.exact(fieldName));
- }
- return;
- }
- if (name.equals(Item.fieldType) && value instanceof String) {
- String fieldType = (String) value;
- if (!Item.fieldTypeDefaultValue.equals(fieldType)) {
- fieldBuilder()
- .setTypePattern(
- KeepFieldTypePattern.fromType(
- KeepEdgeReaderUtils.typePatternFromString(fieldType)));
- }
- return;
- }
- super.visit(name, value);
- }
-
- private boolean tryParseExtendsPattern(String name, Object value) {
- if (name.equals(Item.extendsClassConstant) && value instanceof Type) {
- checkExtendsDeclaration(name);
- extendsPattern =
- KeepExtendsPattern.builder()
- .classPattern(KeepQualifiedClassNamePattern.exact(((Type) value).getClassName()))
- .build();
- return true;
- }
- if (name.equals(Item.extendsTypeName) && value instanceof String) {
- checkExtendsDeclaration(name);
- extendsPattern =
- KeepExtendsPattern.builder()
- .classPattern(KeepQualifiedClassNamePattern.exact(((String) value)))
- .build();
return true;
}
return false;
}
- private void checkExtendsDeclaration(String declaration) {
- if (extendPatternDeclaration != null) {
- throw new KeepEdgeException(
- "Multiple declarations defining an extends pattern: '"
- + extendPatternDeclaration
- + "' and '"
- + declaration
- + "'");
- }
- extendPatternDeclaration = declaration;
- }
-
@Override
- public AnnotationVisitor visitArray(String name) {
+ AnnotationVisitor tryParseArray(String name) {
if (name.equals(Item.methodParameters)) {
return new StringArrayVisitor(
params -> {
@@ -507,29 +538,135 @@
for (String param : params) {
builder.addParameterTypePattern(KeepEdgeReaderUtils.typePatternFromString(param));
}
- methodBuilder().setParametersPattern(builder.build());
+ getBuilder().setParametersPattern(builder.build());
});
}
+ return null;
+ }
+ }
+
+ private static class FieldDeclaration extends Declaration<KeepFieldPattern> {
+
+ private KeepFieldPattern.Builder builder = null;
+
+ private KeepFieldPattern.Builder getBuilder() {
+ if (builder == null) {
+ builder = KeepFieldPattern.builder();
+ }
+ return builder;
+ }
+
+ @Override
+ String kind() {
+ return "field";
+ }
+
+ @Override
+ KeepFieldPattern getValue() {
+ return builder != null ? builder.build() : null;
+ }
+
+ @Override
+ boolean tryParse(String name, Object value) {
+ if (name.equals(Item.fieldName) && value instanceof String) {
+ String fieldName = (String) value;
+ if (!Item.fieldNameDefaultValue.equals(fieldName)) {
+ getBuilder().setNamePattern(KeepFieldNamePattern.exact(fieldName));
+ }
+ return true;
+ }
+ if (name.equals(Item.fieldType) && value instanceof String) {
+ String fieldType = (String) value;
+ if (!Item.fieldTypeDefaultValue.equals(fieldType)) {
+ getBuilder()
+ .setTypePattern(
+ KeepFieldTypePattern.fromType(
+ KeepEdgeReaderUtils.typePatternFromString(fieldType)));
+ }
+ return true;
+ }
+ return false;
+ }
+ }
+
+ private static class MemberDeclaration extends Declaration<KeepMemberPattern> {
+
+ private MethodDeclaration methodDeclaration = new MethodDeclaration();
+ private FieldDeclaration fieldDeclaration = new FieldDeclaration();
+
+ @Override
+ String kind() {
+ return "member";
+ }
+
+ @Override
+ public KeepMemberPattern getValue() {
+ KeepMethodPattern method = methodDeclaration.getValue();
+ KeepFieldPattern field = fieldDeclaration.getValue();
+ if (method != null && field != null) {
+ throw new KeepEdgeException("Cannot define both a field and a method pattern");
+ }
+ if (method != null) {
+ return method;
+ }
+ if (field != null) {
+ return field;
+ }
+ return KeepMemberPattern.none();
+ }
+
+ @Override
+ boolean tryParse(String name, Object value) {
+ return methodDeclaration.tryParse(name, value) || fieldDeclaration.tryParse(name, value);
+ }
+
+ @Override
+ AnnotationVisitor tryParseArray(String name) {
+ AnnotationVisitor visitor = methodDeclaration.tryParseArray(name);
+ if (visitor != null) {
+ return visitor;
+ }
+ return fieldDeclaration.tryParseArray(name);
+ }
+ }
+
+ private abstract static class KeepItemVisitorBase extends AnnotationVisitorBase {
+ private final Parent<KeepItemPattern> parent;
+ private final ClassDeclaration classDeclaration = new ClassDeclaration();
+ private final ExtendsDeclaration extendsDeclaration = new ExtendsDeclaration();
+ private final MemberDeclaration memberDeclaration = new MemberDeclaration();
+
+ public KeepItemVisitorBase(Parent<KeepItemPattern> parent) {
+ this.parent = parent;
+ }
+
+ @Override
+ public void visit(String name, Object value) {
+ if (classDeclaration.tryParse(name, value)
+ || extendsDeclaration.tryParse(name, value)
+ || memberDeclaration.tryParse(name, value)) {
+ return;
+ }
+ super.visit(name, value);
+ }
+
+ @Override
+ public AnnotationVisitor visitArray(String name) {
+ AnnotationVisitor visitor = memberDeclaration.tryParseArray(name);
+ if (visitor != null) {
+ return visitor;
+ }
return super.visitArray(name);
}
@Override
public void visitEnd() {
- assert lazyMethodBuilder == null || lazyFieldBuilder == null;
- Builder itemBuilder = KeepItemPattern.builder();
- if (classNamePattern != null) {
- itemBuilder.setClassPattern(classNamePattern);
- }
- if (extendsPattern != null) {
- itemBuilder.setExtendsPattern(extendsPattern);
- }
- if (lazyMethodBuilder != null) {
- itemBuilder.setMemberPattern(lazyMethodBuilder.build());
- }
- if (lazyFieldBuilder != null) {
- itemBuilder.setMemberPattern(lazyFieldBuilder.build());
- }
- parent.accept(itemBuilder.build());
+ parent.accept(
+ KeepItemPattern.builder()
+ .setClassPattern(classDeclaration.getValue())
+ .setExtendsPattern(extendsDeclaration.getValue())
+ .setMemberPattern(memberDeclaration.getValue())
+ .build());
}
}
diff --git a/src/test/java/com/android/tools/r8/keepanno/KeepInvalidTargetTest.java b/src/test/java/com/android/tools/r8/keepanno/KeepInvalidTargetTest.java
new file mode 100644
index 0000000..83b1768
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/keepanno/KeepInvalidTargetTest.java
@@ -0,0 +1,103 @@
+// 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.keepanno;
+
+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.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.keepanno.annotations.KeepTarget;
+import com.android.tools.r8.keepanno.annotations.UsesReflection;
+import com.android.tools.r8.keepanno.ast.KeepEdgeException;
+import org.hamcrest.Matcher;
+import org.junit.Test;
+import org.junit.function.ThrowingRunnable;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class KeepInvalidTargetTest extends TestBase {
+
+ @Parameterized.Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withNoneRuntime().build();
+ }
+
+ public KeepInvalidTargetTest(TestParameters parameters) {
+ parameters.assertNoneRuntime();
+ }
+
+ private void assertThrowsWith(ThrowingRunnable fn, Matcher<String> matcher) {
+ try {
+ fn.run();
+ fail("Expected run to fail");
+ } catch (KeepEdgeException e) {
+ assertThat(e.getMessage(), matcher);
+ } catch (Throwable e) {
+ fail("Expected run to fail with KeepEdgeException");
+ }
+ }
+
+ @Test
+ public void testInvalidClassDecl() throws Exception {
+ assertThrowsWith(
+ () -> KeepEdgeAnnotationsTest.getKeepRulesForClass(MultipleClassDeclarations.class),
+ allOf(
+ containsString("Multiple declarations"),
+ containsString("className"),
+ containsString("classConstant")));
+ }
+
+ static class MultipleClassDeclarations {
+
+ @UsesReflection(@KeepTarget(className = "foo", classConstant = MultipleClassDeclarations.class))
+ public static void main(String[] args) throws Exception {
+ System.out.println("Hello, world");
+ }
+ }
+
+ @Test
+ public void testInvalidExtendsDecl() throws Exception {
+ assertThrowsWith(
+ () -> KeepEdgeAnnotationsTest.getKeepRulesForClass(MultipleExtendsDeclarations.class),
+ allOf(
+ containsString("Multiple declarations"),
+ containsString("extendsClassName"),
+ containsString("extendsClassConstant")));
+ }
+
+ static class MultipleExtendsDeclarations {
+
+ @UsesReflection(
+ @KeepTarget(
+ extendsClassName = "foo",
+ extendsClassConstant = MultipleClassDeclarations.class))
+ public static void main(String[] args) throws Exception {
+ System.out.println("Hello, world");
+ }
+ }
+
+ @Test
+ public void testInvalidMemberDecl() throws Exception {
+ assertThrowsWith(
+ () -> KeepEdgeAnnotationsTest.getKeepRulesForClass(MultipleMemberDeclarations.class),
+ allOf(containsString("field"), containsString("method")));
+ }
+
+ static class MultipleMemberDeclarations {
+
+ @UsesReflection(
+ @KeepTarget(
+ classConstant = MultipleClassDeclarations.class,
+ methodName = "foo",
+ fieldName = "bar"))
+ public static void main(String[] args) throws Exception {
+ System.out.println("Hello, world");
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/keepanno/KeepUsesReflectionOnFieldTest.java b/src/test/java/com/android/tools/r8/keepanno/KeepUsesReflectionOnFieldTest.java
index d3decee..ee605b0 100644
--- a/src/test/java/com/android/tools/r8/keepanno/KeepUsesReflectionOnFieldTest.java
+++ b/src/test/java/com/android/tools/r8/keepanno/KeepUsesReflectionOnFieldTest.java
@@ -95,7 +95,11 @@
@UsesReflection(
description = "Keep the\nstring-valued fields",
- value = {@KeepTarget(classConstant = A.class, fieldType = "java.lang.String")})
+ value = {
+ @KeepTarget(
+ className = "com.android.tools.r8.keepanno.KeepUsesReflectionOnFieldTest$A",
+ fieldType = "java.lang.String")
+ })
public void foo() throws Exception {
for (Field field : getClass().getDeclaredFields()) {
if (field.getType().equals(String.class)) {
