Skip to content

Commit 5b768ff

Browse files
cushonError Prone Team
cushon
authored and
Error Prone Team
committed
Require -XDaddTypeAnnotationsToSymbol=true, and remove workaround for JDK-8225377
This removes workarounds for https://bugs.openjdk.org/browse/JDK-8225377, which is fixed in JDK 21.0.8 and newer releases if `-XDaddTypeAnnotationsToSymbol=true` is enabled. #5426 Startblock: * unknown commit is submitted RELNOTES=The javac flag `-XDaddTypeAnnotationsToSymbol=true` is now required for Error Prone invocations on JDK 21, to enable the javac fix for JDK-8225377 (type annotations are not visible to javac plugins across compilation boundaries). See #5426 for details. PiperOrigin-RevId: 853255760
1 parent 0e004e5 commit 5b768ff

12 files changed

Lines changed: 94 additions & 514 deletions

File tree

‎check_api/src/main/java/com/google/errorprone/BaseErrorProneJavaCompiler.java‎

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ public CompilationTask getTask(
7272
javacOpts = defaultToLatestSupportedLanguageLevel(javacOpts);
7373
javacOpts = setCompilePolicyToByFile(javacOpts);
7474
javacOpts = setShouldStopIfErrorPolicyToFlow(javacOpts);
75+
javacOpts = setAddTypeAnnotationsToSymbol(javacOpts);
7576
JavacTask task =
7677
(JavacTask)
7778
javacTool.getTask(
@@ -83,9 +84,11 @@ public CompilationTask getTask(
8384
static void addTaskListener(
8485
JavacTask javacTask, ScannerSupplier scannerSupplier, ErrorProneOptions errorProneOptions) {
8586
Context context = ((BasicJavacTask) javacTask).getContext();
86-
checkCompilePolicy(Options.instance(context).get("compilePolicy"));
87-
checkShouldStopIfErrorPolicy(Options.instance(context).get("should-stop.ifError"));
8887
setupMessageBundle(context);
88+
Options options = Options.instance(context);
89+
checkCompilePolicy(options.get("compilePolicy"));
90+
checkShouldStopIfErrorPolicy(options.get("should-stop.ifError"));
91+
checkAddTypeAnnotationsToSymbol(options.get("addTypeAnnotationsToSymbol"));
8992
RefactoringCollection[] refactoringCollection = {null};
9093
javacTask.addTaskListener(
9194
ErrorProneAnalyzer.createAnalyzer(
@@ -220,6 +223,34 @@ private static ImmutableList<String> setShouldStopIfErrorPolicyToFlow(
220223
return ImmutableList.<String>builder().addAll(args).add("--should-stop=ifError=FLOW").build();
221224
}
222225

226+
private static ImmutableList<String> setAddTypeAnnotationsToSymbol(ImmutableList<String> args) {
227+
for (String arg : args) {
228+
if (arg.startsWith("-XDaddTypeAnnotationsToSymbol")) {
229+
String value = arg.substring(arg.indexOf('=') + 1);
230+
checkAddTypeAnnotationsToSymbol(value);
231+
return args; // don't do anything if a valid policy is already set
232+
}
233+
}
234+
return ImmutableList.<String>builder()
235+
.addAll(args)
236+
.add("-XDaddTypeAnnotationsToSymbol=true")
237+
.build();
238+
}
239+
240+
private static void checkAddTypeAnnotationsToSymbol(String value) {
241+
if (value == null) {
242+
if (Runtime.version().feature() <= 21) {
243+
throw new InvalidCommandLineOptionException(
244+
"-XDaddTypeAnnotationsToSymbol=true is required by Error Prone on JDK 21");
245+
}
246+
return;
247+
}
248+
if (!Boolean.parseBoolean(value)) {
249+
throw new InvalidCommandLineOptionException(
250+
"-XDaddTypeAnnotationsToSymbol must be set to true, was: " + value);
251+
}
252+
}
253+
223254
/** Registers our message bundle. */
224255
public static void setupMessageBundle(Context context) {
225256
ResourceBundle bundle = ResourceBundle.getBundle("com.google.errorprone.errors");

‎check_api/src/main/java/com/google/errorprone/dataflow/AccessPath.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
package com.google.errorprone.dataflow;
1717

1818
import com.google.common.collect.ImmutableList;
19-
import com.google.errorprone.util.MoreAnnotations;
2019
import com.sun.source.tree.IdentifierTree;
2120
import com.sun.source.tree.MemberSelectTree;
2221
import com.sun.source.tree.MethodInvocationTree;
@@ -29,6 +28,7 @@
2928
import javax.lang.model.element.Element;
3029
import javax.lang.model.element.ElementKind;
3130
import javax.lang.model.element.Modifier;
31+
import javax.lang.model.element.TypeElement;
3232
import org.checkerframework.errorprone.dataflow.cfg.node.AssignmentNode;
3333
import org.checkerframework.errorprone.dataflow.cfg.node.FieldAccessNode;
3434
import org.checkerframework.errorprone.dataflow.cfg.node.LocalVariableNode;
@@ -88,9 +88,12 @@ public static boolean isAutoValueAccessor(Tree tree) {
8888
&& // class, not interface
8989
rcvrType.tsym.getKind() == ElementKind.CLASS
9090
&& // annotated @AutoValue
91-
MoreAnnotations.getDeclarationAndTypeAttributes(rcvrType.tsym)
92-
.map(Object::toString)
93-
.anyMatch("@com.google.auto.value.AutoValue"::equals);
91+
rcvrType.tsym.getAnnotationMirrors().stream()
92+
.anyMatch(
93+
a ->
94+
((TypeElement) a.getAnnotationType().asElement())
95+
.getQualifiedName()
96+
.contentEquals("com.google.auto.value.AutoValue"));
9497
}
9598

9699
/**

‎check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessAnnotations.java‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
import static javax.lang.model.element.ElementKind.TYPE_PARAMETER;
2121

2222
import com.google.common.collect.ImmutableList;
23-
import com.google.errorprone.util.MoreAnnotations;
2423
import com.sun.source.tree.AnnotationTree;
2524
import com.sun.source.tree.IdentifierTree;
2625
import com.sun.source.tree.MemberSelectTree;
@@ -118,27 +117,21 @@ public static Optional<Nullness> fromAnnotationsOn(@Nullable Symbol sym) {
118117
/*
119118
* We try to read annotations in two ways:
120119
*
121-
* 1. from the TypeMirror: This is how we "should" always read *type-use* annotations, but
122-
* JDK-8225377 prevents it from working across compilation boundaries.
120+
* 1. from the TypeMirror
123121
*
124-
* 2. from getRawAttributes(): This works around the problem across compilation boundaries, and
125-
* it handles declaration annotations (though there are other ways we could handle declaration
126-
* annotations). But it has a bug of its own with type-use annotations on inner classes
127-
* (b/203207989). To reduce the chance that we hit the inner-class bug, we apply it only if the
128-
* first approach fails.
122+
* 2. from the Symbols, to handle declaration annotations
129123
*/
130124
TypeMirror elementType =
131125
switch (sym.getKind()) {
132126
case METHOD -> ((ExecutableElement) sym).getReturnType();
133-
case FIELD, PARAMETER -> sym.asType();
134-
default -> null;
127+
default -> sym.asType();
135128
};
136129
Optional<Nullness> fromElement = fromAnnotationsOn(elementType);
137130
if (fromElement.isPresent()) {
138131
return fromElement;
139132
}
140133

141-
return fromAnnotationStream(MoreAnnotations.getDeclarationAndTypeAttributes(sym));
134+
return fromAnnotationStream(sym.getAnnotationMirrors().stream());
142135
}
143136

144137
public static Optional<Nullness> fromAnnotationsOn(@Nullable TypeMirror type) {

‎check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1714,24 +1714,6 @@ public static boolean variableIsStaticFinal(VarSymbol var) {
17141714
return (var.isStatic() || var.owner.isEnum()) && var.getModifiers().contains(Modifier.FINAL);
17151715
}
17161716

1717-
/**
1718-
* Returns declaration annotations of the given symbol, as well as 'top-level' type annotations,
1719-
* including :
1720-
*
1721-
* <ul>
1722-
* <li>Type annotations of the return type of a method.
1723-
* <li>Type annotations on the type of a formal parameter or field.
1724-
* </ul>
1725-
*
1726-
* <p>One might expect this to be equivalent to information returned by {@link
1727-
* Type#getAnnotationMirrors}, but javac doesn't associate type annotation information with types
1728-
* for symbols completed from class files, so that approach doesn't work across compilation
1729-
* boundaries.
1730-
*/
1731-
public static Stream<Attribute.Compound> getDeclarationAndTypeAttributes(Symbol sym) {
1732-
return MoreAnnotations.getDeclarationAndTypeAttributes(sym);
1733-
}
1734-
17351717
/**
17361718
* Return a mirror of this annotation.
17371719
*

‎check_api/src/main/java/com/google/errorprone/util/MoreAnnotations.java‎

Lines changed: 0 additions & 118 deletions
Original file line numberDiff line numberDiff line change
@@ -16,22 +16,9 @@
1616

1717
package com.google.errorprone.util;
1818

19-
import static java.util.stream.Collectors.groupingBy;
20-
import static java.util.stream.Collectors.toList;
21-
2219
import com.google.common.base.MoreObjects;
2320
import com.google.common.collect.Streams;
2421
import com.sun.tools.javac.code.Attribute;
25-
import com.sun.tools.javac.code.Attribute.Compound;
26-
import com.sun.tools.javac.code.Attribute.TypeCompound;
27-
import com.sun.tools.javac.code.Symbol;
28-
import com.sun.tools.javac.code.Symbol.MethodSymbol;
29-
import com.sun.tools.javac.code.TargetType;
30-
import com.sun.tools.javac.code.Type;
31-
import com.sun.tools.javac.code.TypeAnnotationPosition;
32-
import com.sun.tools.javac.code.TypeTag;
33-
import com.sun.tools.javac.tree.JCTree;
34-
import java.util.LinkedHashMap;
3522
import java.util.List;
3623
import java.util.Map;
3724
import java.util.Optional;
@@ -46,111 +33,6 @@
4633
/** Annotation-related utilities. */
4734
public final class MoreAnnotations {
4835

49-
/**
50-
* Returns declaration annotations of the given symbol, as well as 'top-level' type annotations,
51-
* including :
52-
*
53-
* <ul>
54-
* <li>Type annotations of the return type of a method.
55-
* <li>Type annotations on the type of a formal parameter or field.
56-
* </ul>
57-
*
58-
* <p>One might expect this to be equivalent to information returned by {@link
59-
* com.sun.tools.javac.code.Type#getAnnotationMirrors}, but javac doesn't associate type
60-
* annotation information with types for symbols completed from class files, so that approach
61-
* doesn't work across compilation boundaries.
62-
*/
63-
public static Stream<Compound> getDeclarationAndTypeAttributes(Symbol sym) {
64-
return Streams.concat(sym.getRawAttributes().stream(), getTopLevelTypeAttributes(sym))
65-
.collect(
66-
groupingBy(c -> c.type.asElement().getQualifiedName(), LinkedHashMap::new, toList()))
67-
.values()
68-
.stream()
69-
.map(c -> c.getFirst());
70-
}
71-
72-
/**
73-
* Returns "top-level" type annotations of the given symbol, including:
74-
*
75-
* <ul>
76-
* <li>Type annotations of the return type of a method.
77-
* <li>Type annotations on the type of a formal parameter or field.
78-
* </ul>
79-
*
80-
* <p>These annotations are not always included in those returned by {@link
81-
* com.sun.tools.javac.code.Type#getAnnotationMirrors} because javac doesn't associate type
82-
* annotation information with types for symbols completed from class files. These type
83-
* annotations won't be included when the symbol is not in the current compilation.
84-
*/
85-
public static Stream<TypeCompound> getTopLevelTypeAttributes(Symbol sym) {
86-
Symbol typeAnnotationOwner =
87-
switch (sym.getKind()) {
88-
case PARAMETER -> sym.owner;
89-
default -> sym;
90-
};
91-
return typeAnnotationOwner.getRawTypeAttributes().stream()
92-
.filter(anno -> isAnnotationOnType(sym, anno.position));
93-
}
94-
95-
private static boolean isAnnotationOnType(Symbol sym, TypeAnnotationPosition position) {
96-
if (!position.location.stream()
97-
.allMatch(e -> e.tag == TypeAnnotationPosition.TypePathEntryKind.INNER_TYPE)) {
98-
return false;
99-
}
100-
if (!targetTypeMatches(sym, position)) {
101-
return false;
102-
}
103-
Type type =
104-
switch (sym.getKind()) {
105-
case METHOD, CONSTRUCTOR -> ((MethodSymbol) sym).getReturnType();
106-
default -> sym.asType();
107-
};
108-
return isAnnotationOnType(type, position.location);
109-
}
110-
111-
private static boolean isAnnotationOnType(
112-
Type type, com.sun.tools.javac.util.List<TypeAnnotationPosition.TypePathEntry> location) {
113-
com.sun.tools.javac.util.List<TypeAnnotationPosition.TypePathEntry> expected =
114-
com.sun.tools.javac.util.List.nil();
115-
for (Type curr = type.getEnclosingType();
116-
curr != null && !curr.hasTag(TypeTag.NONE);
117-
curr = curr.getEnclosingType()) {
118-
expected = expected.append(TypeAnnotationPosition.TypePathEntry.INNER_TYPE);
119-
}
120-
return expected.equals(location);
121-
}
122-
123-
private static boolean targetTypeMatches(Symbol sym, TypeAnnotationPosition position) {
124-
return switch (sym.getKind()) {
125-
case LOCAL_VARIABLE, BINDING_VARIABLE -> position.type == TargetType.LOCAL_VARIABLE;
126-
// treated like a field
127-
case FIELD, ENUM_CONSTANT -> position.type == TargetType.FIELD;
128-
case CONSTRUCTOR, METHOD -> position.type == TargetType.METHOD_RETURN;
129-
case PARAMETER ->
130-
switch (position.type) {
131-
case METHOD_FORMAL_PARAMETER -> {
132-
int parameterIndex = position.parameter_index;
133-
if (position.onLambda != null) {
134-
com.sun.tools.javac.util.List<JCTree.JCVariableDecl> lambdaParams =
135-
position.onLambda.params;
136-
yield parameterIndex < lambdaParams.size()
137-
&& lambdaParams.get(parameterIndex).sym.equals(sym);
138-
} else {
139-
yield ((Symbol.MethodSymbol) sym.owner).getParameters().indexOf(sym)
140-
== parameterIndex;
141-
}
142-
}
143-
default -> false;
144-
};
145-
// There are no type annotations on the top-level type of the class being declared, only
146-
// on other types in the signature (e.g. `class Foo extends Bar<@A Baz> {}`).
147-
case CLASS -> false;
148-
default ->
149-
throw new AssertionError(
150-
"unsupported element kind in MoreAnnotation#isAnnotationOnType: " + sym.getKind());
151-
};
152-
}
153-
15436
/**
15537
* Returns the value of the annotation element-value pair with the given name if it is explicitly
15638
* set.

‎check_api/src/test/java/com/google/errorprone/util/ASTHelpersTest.java‎

Lines changed: 0 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
2323
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
2424
import static com.google.errorprone.util.ASTHelpers.canonicalConstructor;
25-
import static com.google.errorprone.util.ASTHelpers.getEnclosedElements;
2625
import static com.google.errorprone.util.ASTHelpers.getStartPosition;
2726
import static com.google.errorprone.util.ASTHelpers.getSymbol;
2827
import static com.google.errorprone.util.ASTHelpers.hasAnnotation;
@@ -79,17 +78,12 @@
7978
import com.sun.source.tree.TypeParameterTree;
8079
import com.sun.source.tree.VariableTree;
8180
import com.sun.source.util.TreePathScanner;
82-
import com.sun.tools.javac.api.BasicJavacTask;
83-
import com.sun.tools.javac.api.JavacTool;
8481
import com.sun.tools.javac.code.Symbol;
8582
import com.sun.tools.javac.code.Symbol.ClassSymbol;
86-
import com.sun.tools.javac.code.Symbol.MethodSymbol;
8783
import com.sun.tools.javac.code.Symbol.PackageSymbol;
88-
import com.sun.tools.javac.code.Symbol.VarSymbol;
8984
import com.sun.tools.javac.code.Type;
9085
import com.sun.tools.javac.code.Type.TypeVar;
9186
import com.sun.tools.javac.main.Main.Result;
92-
import com.sun.tools.javac.model.JavacElements;
9387
import com.sun.tools.javac.tree.JCTree.JCLiteral;
9488
import java.lang.annotation.Retention;
9589
import java.lang.annotation.RetentionPolicy;
@@ -1279,47 +1273,6 @@ public static class Lib {
12791273
}
12801274
}
12811275

1282-
@Test
1283-
public void getDeclarationAndTypeAttributesTest() {
1284-
BasicJavacTask tool =
1285-
(BasicJavacTask) JavacTool.create().getTask(null, null, null, null, null, null);
1286-
ClassSymbol element =
1287-
JavacElements.instance(tool.getContext()).getTypeElement(Lib.class.getCanonicalName());
1288-
VarSymbol field =
1289-
(VarSymbol)
1290-
getEnclosedElements(element).stream()
1291-
.filter(e -> e.getSimpleName().contentEquals("field"))
1292-
.findAny()
1293-
.get();
1294-
assertThat(ASTHelpers.getDeclarationAndTypeAttributes(field).map(String::valueOf))
1295-
.containsExactly(
1296-
"@com.google.errorprone.util.ASTHelpersTest.B(\"one\")",
1297-
"@com.google.errorprone.util.ASTHelpersTest.A(1)");
1298-
1299-
MethodSymbol method =
1300-
(MethodSymbol)
1301-
getEnclosedElements(element).stream()
1302-
.filter(e -> e.getSimpleName().contentEquals("method"))
1303-
.findAny()
1304-
.get();
1305-
assertThat(ASTHelpers.getDeclarationAndTypeAttributes(method).map(String::valueOf))
1306-
.containsExactly(
1307-
"@com.google.errorprone.util.ASTHelpersTest.B(\"two\")",
1308-
"@com.google.errorprone.util.ASTHelpersTest.A(4)");
1309-
assertThat(
1310-
ASTHelpers.getDeclarationAndTypeAttributes(method.getParameters().get(0))
1311-
.map(String::valueOf))
1312-
.containsExactly(
1313-
"@com.google.errorprone.util.ASTHelpersTest.B(\"three\")",
1314-
"@com.google.errorprone.util.ASTHelpersTest.A(7)");
1315-
assertThat(
1316-
ASTHelpers.getDeclarationAndTypeAttributes(method.getParameters().get(1))
1317-
.map(String::valueOf))
1318-
.containsExactly(
1319-
"@com.google.errorprone.util.ASTHelpersTest.B(\"four\")",
1320-
"@com.google.errorprone.util.ASTHelpersTest.A(10)");
1321-
}
1322-
13231276
/** A {@link BugChecker} that prints if the method can be overridden. */
13241277
@BugPattern(
13251278
severity = SeverityLevel.ERROR,

0 commit comments

Comments
 (0)