Skip to content

Commit

Permalink
Fix modelling of array types for annotation processing
Browse files Browse the repository at this point in the history 
A `@C int @A [] @b []` is an '`@A` array of `@B` arrays of `@C int`s'. Turbine was parsing that example as `(ArrayType @b (ArrayType @A (PrimitiveType @C int)))`, and then reversing the list of element types to compute the type path for the annotations in the output, and producing correct class files. The difference was still visible to the annotation processing API, and the wrong type annotations were showing up on `ArrayType#getComponentType` for multi-dimensional arrays.

This change fixes the bug by parsing the above example as `(ArrayType @A (ArrayType @b (PrimitiveType @C int)))`. This requires reversing the list when pretty-printing the AST node, but simplifies computation of type_paths, and makes the right annotations show up on `ArrayType#getComponentType`.

This also simplifies parsing logic, the handling of trailing 'c-style' dimension
specifiers can now be handled by the same logic that parses regular dimension
specifiers.

Some combinations of multi-variable declarations and c-style arrays still
disagree with javac, which has [known bugs](https://bugs.openjdk.org/browse/JDK-8178065) in that area.

Finally, the integration test being added here for type annotations and
annotation processing will be generalized to also exercise type annotations
read from bytecode, to test the fix for b/307310010.

PiperOrigin-RevId: 578682527
  • Loading branch information
@cushon
cushon authored and Javac Team committed Nov 1, 2023
1 parent cf5258c commit a1c7f7e
Show file tree
Hide file tree
Showing 11 changed files with 694 additions and 96 deletions.
20 changes: 4 additions & 16 deletions java/com/google/turbine/lower/Lower.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,7 @@
import com.google.turbine.type.Type.WildTy;
import com.google.turbine.types.Erasure;
import java.lang.annotation.RetentionPolicy;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Deque;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -865,7 +863,7 @@ private void lowerTypeAnnotations(Type type, TypePath path) {
lowerClassTypeTypeAnnotations((ClassTy) type, path);
break;
case ARRAY_TY:
lowerArrayTypeAnnotations(type, path);
lowerArrayTypeAnnotations((ArrayTy) type, path);
break;
case WILD_TY:
lowerWildTyTypeAnnotations((WildTy) type, path);
Expand Down Expand Up @@ -904,19 +902,9 @@ private void lowerWildTyTypeAnnotations(WildTy type, TypePath path) {
}
}

private void lowerArrayTypeAnnotations(Type type, TypePath path) {
Type base = type;
Deque<ArrayTy> flat = new ArrayDeque<>();
while (base instanceof ArrayTy) {
ArrayTy arrayTy = (ArrayTy) base;
flat.addFirst(arrayTy);
base = arrayTy.elementType();
}
for (ArrayTy arrayTy : flat) {
lowerTypeAnnotations(arrayTy.annos(), path);
path = path.array();
}
lowerTypeAnnotations(base, path);
private void lowerArrayTypeAnnotations(ArrayTy type, TypePath path) {
lowerTypeAnnotations(type.annos(), path);
lowerTypeAnnotations(type.elementType(), path.array());
}

private void lowerClassTypeTypeAnnotations(ClassTy type, TypePath path) {
Expand Down
91 changes: 30 additions & 61 deletions java/com/google/turbine/parse/Parser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,6 @@
import com.google.turbine.tree.Tree.VarDecl;
import com.google.turbine.tree.Tree.WildTy;
import com.google.turbine.tree.TurbineModifier;
import java.util.ArrayDeque;
import java.util.Deque;
import java.util.EnumSet;
import java.util.List;
import java.util.Optional;
Expand Down Expand Up @@ -894,14 +892,12 @@ private ImmutableList<Tree> member(
ident,
ImmutableList.<Type>of(),
ImmutableList.of());
result = maybeDims(maybeAnnos(), result);
break;
}
case LT:
{
result =
new ClassTy(position, Optional.<ClassTy>empty(), ident, tyargs(), ImmutableList.of());
result = maybeDims(maybeAnnos(), result);
break;
}
case DOT:
Expand All @@ -927,7 +923,7 @@ private ImmutableList<Tree> member(
}
result = classty((ClassTy) result);
}
result = maybeDims(maybeAnnos(), result);
result = maybeDims(result);
pos = position;
name = eatIdent();
switch (token) {
Expand Down Expand Up @@ -1101,34 +1097,7 @@ private Tree methodRest(
* int [] @A [] x}, not {@code int @A [] [] x}.
*/
private Type extraDims(Type ty) {
ImmutableList<Anno> annos = maybeAnnos();
if (!annos.isEmpty() && token != Token.LBRACK) {
// orphaned type annotations
throw error(token);
}
Deque<ImmutableList<Anno>> extra = new ArrayDeque<>();
while (maybe(Token.LBRACK)) {
eat(Token.RBRACK);
extra.push(annos);
annos = maybeAnnos();
}
ty = extraDims(ty, extra);
return ty;
}

private Type extraDims(Type type, Deque<ImmutableList<Anno>> extra) {
if (extra.isEmpty()) {
return type;
}
if (type == null) {
// trailing dims without a type, e.g. for a constructor declaration
throw error(token);
}
if (type.kind() == Kind.ARR_TY) {
ArrTy arrTy = (ArrTy) type;
return new ArrTy(arrTy.position(), arrTy.annos(), extraDims(arrTy.elem(), extra));
}
return new ArrTy(type.position(), extra.pop(), extraDims(type, extra));
return maybeDims(ty);
}

private ImmutableList<ClassTy> exceptions() {
Expand Down Expand Up @@ -1159,29 +1128,7 @@ private VarDecl formalParam() {
ImmutableList.Builder<Anno> annos = ImmutableList.builder();
EnumSet<TurbineModifier> access = modifiersAndAnnotations(annos);
Type ty = referenceTypeWithoutDims(ImmutableList.of());
ImmutableList<Anno> typeAnnos = maybeAnnos();
OUTER:
while (true) {
switch (token) {
case LBRACK:
next();
eat(Token.RBRACK);
ty = new ArrTy(position, typeAnnos, ty);
typeAnnos = maybeAnnos();
break;
case ELLIPSIS:
next();
access.add(VARARGS);
ty = new ArrTy(position, typeAnnos, ty);
typeAnnos = ImmutableList.of();
break OUTER;
default:
break OUTER;
}
}
if (!typeAnnos.isEmpty()) {
throw error(token);
}
ty = paramDims(access, ty);
// the parameter name is `this` for receiver parameters, and a qualified this expression
// for inner classes
Ident name = identOrThis();
Expand All @@ -1195,6 +1142,25 @@ private VarDecl formalParam() {
position, access, annos.build(), ty, name, Optional.<Expression>empty(), null);
}

private Type paramDims(EnumSet<TurbineModifier> access, Type ty) {
ImmutableList<Anno> typeAnnos = maybeAnnos();
switch (token) {
case LBRACK:
next();
eat(Token.RBRACK);
return new ArrTy(position, typeAnnos, paramDims(access, ty));
case ELLIPSIS:
next();
access.add(VARARGS);
return new ArrTy(position, typeAnnos, ty);
default:
if (!typeAnnos.isEmpty()) {
throw error(token);
}
return ty;
}
}

private Ident identOrThis() {
switch (token) {
case IDENT:
Expand Down Expand Up @@ -1415,14 +1381,17 @@ private Type referenceTypeWithoutDims(ImmutableList<Anno> typeAnnos) {

private Type referenceType(ImmutableList<Anno> typeAnnos) {
Type ty = referenceTypeWithoutDims(typeAnnos);
return maybeDims(maybeAnnos(), ty);
return maybeDims(ty);
}

private Type maybeDims(ImmutableList<Anno> typeAnnos, Type ty) {
while (maybe(Token.LBRACK)) {
private Type maybeDims(Type ty) {
ImmutableList<Anno> typeAnnos = maybeAnnos();
if (maybe(Token.LBRACK)) {
eat(Token.RBRACK);
ty = new ArrTy(position, typeAnnos, ty);
typeAnnos = maybeAnnos();
return new ArrTy(position, typeAnnos, maybeDims(ty));
}
if (!typeAnnos.isEmpty()) {
throw error(token);
}
return ty;
}
Expand Down
2 changes: 1 addition & 1 deletion java/com/google/turbine/processing/TurbineElement.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,7 @@ public List<? extends TypeMirror> getBounds() {

@Override
public TypeMirror asType() {
return factory.asTypeMirror(Type.TyVar.create(sym, info().annotations()));
return factory.asTypeMirror(Type.TyVar.create(sym, ImmutableList.of()));
}

@Override
Expand Down
21 changes: 16 additions & 5 deletions java/com/google/turbine/tree/Pretty.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,17 +108,28 @@ Pretty append(String s) {

@Override
public @Nullable Void visitArrTy(Tree.ArrTy arrTy, @Nullable Void input) {
arrTy.elem().accept(this, null);
if (!arrTy.annos().isEmpty()) {
append(' ');
printAnnos(arrTy.annos());
ImmutableList.Builder<Tree.ArrTy> flat = ImmutableList.builder();
Tree next = arrTy;
do {
Tree.ArrTy curr = (Tree.ArrTy) next;
flat.add(curr);
next = curr.elem();
} while (next.kind().equals(Tree.Kind.ARR_TY));

next.accept(this, null);
for (Tree.ArrTy dim : flat.build()) {
if (!dim.annos().isEmpty()) {
append(' ');
printAnnos(dim.annos());
}
append("[]");
}
append("[]");
return null;
}

@Override
public @Nullable Void visitPrimTy(Tree.PrimTy primTy, @Nullable Void input) {
printAnnos(primTy.annos());
append(primTy.tykind().toString());
return null;
}
Expand Down
38 changes: 28 additions & 10 deletions javatests/com/google/turbine/lower/IntegrationTestSupport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import javax.annotation.processing.Processor;
import javax.tools.DiagnosticCollector;
import javax.tools.JavaFileObject;
import javax.tools.StandardLocation;
Expand Down Expand Up @@ -551,7 +552,19 @@ public static JavacTask runJavacAnalysis(
throws Exception {
FileSystem fs = Jimfs.newFileSystem(Configuration.unix());
Path out = fs.getPath("out");
return setupJavac(sources, classpath, options, collector, fs, out);
return setupJavac(sources, classpath, options, collector, fs, out, ImmutableList.of());
}

public static JavacTask runJavacAnalysis(
Map<String, String> sources,
Collection<Path> classpath,
ImmutableList<String> options,
DiagnosticCollector<JavaFileObject> collector,
ImmutableList<Processor> processors)
throws Exception {
FileSystem fs = Jimfs.newFileSystem(Configuration.unix());
Path out = fs.getPath("out");
return setupJavac(sources, classpath, options, collector, fs, out, processors);
}

public static Map<String, byte[]> runJavac(
Expand All @@ -568,7 +581,8 @@ public static Map<String, byte[]> runJavac(
FileSystem fs = Jimfs.newFileSystem(Configuration.unix());
Path out = fs.getPath("out");

JavacTask task = setupJavac(sources, classpath, options, collector, fs, out);
JavacTask task =
setupJavac(sources, classpath, options, collector, fs, out, ImmutableList.of());

if (!task.call()) {
fail(collector.getDiagnostics().stream().map(d -> d.toString()).collect(joining("\n")));
Expand Down Expand Up @@ -601,7 +615,8 @@ private static JavacTask setupJavac(
ImmutableList<String> options,
DiagnosticCollector<JavaFileObject> collector,
FileSystem fs,
Path out)
Path out,
Iterable<? extends Processor> processors)
throws IOException {
Path srcs = fs.getPath("srcs");

Expand Down Expand Up @@ -631,13 +646,16 @@ private static JavacTask setupJavac(
StandardLocation.locationFor("MODULE_SOURCE_PATH"), ImmutableList.of(srcs));
}

return compiler.getTask(
new PrintWriter(new BufferedWriter(new OutputStreamWriter(System.err, UTF_8)), true),
fileManager,
collector,
options,
ImmutableList.of(),
fileManager.getJavaFileObjectsFromPaths(inputs));
JavacTask task =
compiler.getTask(
new PrintWriter(new BufferedWriter(new OutputStreamWriter(System.err, UTF_8)), true),
fileManager,
collector,
options,
ImmutableList.of(),
fileManager.getJavaFileObjectsFromPaths(inputs));
task.setProcessors(processors);
return task;
}

/** Normalizes and stringifies a collection of class files. */
Expand Down
16 changes: 15 additions & 1 deletion javatests/com/google/turbine/lower/testdata/type_anno_cstyle_array_dims.test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,27 @@ import java.lang.annotation.ElementType;
import java.lang.annotation.Target;
import java.util.Map;

@Target(ElementType.TYPE_USE) @interface A {}
@Target(ElementType.TYPE_USE) @interface A {
int value() default 0;
}
@Target(ElementType.TYPE_USE) @interface B {}
@Target(ElementType.TYPE_USE) @interface C {}
@Target(ElementType.TYPE_USE) @interface D {}
@Target(ElementType.TYPE_USE) @interface E {}
@Target(ElementType.TYPE_USE) @interface F {}
@Target(ElementType.TYPE_USE) @interface G {}
@Target(ElementType.TYPE_USE) @interface H {}
@Target(ElementType.TYPE_USE) @interface J {}
@Target(ElementType.TYPE_USE) @interface K {}

class Test {
int [] x = {}, y @B @C [] @D @E [] = {{{1}}};

@A int @B [] @C [] @D [] z @E [] @F [] @G [];

void log(@A Object [] params @B @C [] @D @E []) {}

@A int @B [] @C [] @D [] f(@A Object @B [] @C [] @D [] params @E [] @F [] @G []) @D [] @E [] @F [] {
return null;
}
}
38 changes: 36 additions & 2 deletions javatests/com/google/turbine/parse/ParseErrorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,9 +240,9 @@ public void arrayDot() {
.hasMessageThat()
.isEqualTo(
lines(
"<>:1: error: unexpected token: <", //
"<>:1: error: expected token <identifier>", //
"enum\te{p;ullt[].<~>>>L\0",
" ^"));
" ^"));
}

@Test
Expand Down Expand Up @@ -441,6 +441,40 @@ public void sEscape_windowsLineEnding() {
" ^"));
}

@Test
public void typeAnnotationAfterDims() {
String input =
lines(
"class T {", //
" int[] @A a;",
"}");
TurbineError e = assertThrows(TurbineError.class, () -> Parser.parse(input));
assertThat(e)
.hasMessageThat()
.isEqualTo(
lines(
"<>:2: error: unexpected identifier 'a'", //
" int[] @A a;",
" ^"));
}

@Test
public void typeAnnotationBeforeParam() {
String input =
lines(
"class T {", //
" void f(int @A a) {}",
"}");
TurbineError e = assertThrows(TurbineError.class, () -> Parser.parse(input));
assertThat(e)
.hasMessageThat()
.isEqualTo(
lines(
"<>:2: error: unexpected identifier 'a'", //
" void f(int @A a) {}",
" ^"));
}

private static String lines(String... lines) {
return Joiner.on(System.lineSeparator()).join(lines);
}
Expand Down
Loading

0 comments on commit a1c7f7e

Please sign in to comment.