Better type annotation disambiguation

Given the method signature `void f(java.lang.@A String x);`, `@A` is
unambiguously a type annotation. Currently this isn't handled correctly
because the bound model doesn't distinguish between `java.lang.@A
String` and `@A String`, so if `@A` can target parameters it gets added
to the parameter declaration.

To handle this better, the parser now collects all ambiguous annotations
into the declaration annotation list. In practice, the only case that
changes is method declaration annotations that occur after the formal
type parameter list, e.g. previously `@A` was classified as a type
annotation in `<T> @A void f();`.

Finally, during disambiguation annotations are now only moved from
declarations to types, and never from types to declarations.

MOE_MIGRATED_REVID=139402105

11 files changed, 109 insertions, 73 deletions

diff --git a/java/com/google/turbine/binder/DisambiguateTypeAnnotations.java b/java/com/google/turbine/binder/DisambiguateTypeAnnotations.java
index 2b5f029..e39c7ee 100644
--- a/java/com/google/turbine/binder/DisambiguateTypeAnnotations.java
+++ b/java/com/google/turbine/binder/DisambiguateTypeAnnotations.java
@@ -158,7 +158,7 @@ public class DisambiguateTypeAnnotations {
         declarationAnnotations.add(anno);
       }
     }
-    return fixAnnotations(env, type, typeAnnotations.build(), declarationAnnotations);
+    return addAnnotationsToType(type, typeAnnotations.build());
   }
 
   private static ImmutableList<FieldInfo> bindFields(
@@ -189,22 +189,16 @@ public class DisambiguateTypeAnnotations {
    * <p>Note: the second case means that type annotation disambiguation has to occur on nested types
    * before they are canonicalized.
    */
-  private static Type fixAnnotations(
-      Env<ClassSymbol, TypeBoundClass> env,
-      Type type,
-      ImmutableList<AnnoInfo> extra,
-      Builder<AnnoInfo> removed) {
+  private static Type addAnnotationsToType(Type type, ImmutableList<AnnoInfo> extra) {
     switch (type.tyKind()) {
       case PRIM_TY:
         PrimTy primTy = (PrimTy) type;
-        return new Type.PrimTy(
-            primTy.primkind(), fixAnnotations(env, primTy.annos(), extra, removed));
+        return new Type.PrimTy(primTy.primkind(), appendAnnotations(primTy.annos(), extra));
       case CLASS_TY:
         ClassTy classTy = (ClassTy) type;
         SimpleClassTy base = classTy.classes.get(0);
         SimpleClassTy simple =
-            new SimpleClassTy(
-                base.sym(), base.targs(), fixAnnotations(env, base.annos(), extra, removed));
+            new SimpleClassTy(base.sym(), base.targs(), appendAnnotations(base.annos(), extra));
         return new Type.ClassTy(
             ImmutableList.<SimpleClassTy>builder()
                 .add(simple)
@@ -212,11 +206,10 @@ public class DisambiguateTypeAnnotations {
                 .build());
       case ARRAY_TY:
         ArrayTy arrayTy = (ArrayTy) type;
-        return new ArrayTy(
-            fixAnnotations(env, arrayTy.elementType(), extra, removed), arrayTy.annos());
+        return new ArrayTy(addAnnotationsToType(arrayTy.elementType(), extra), arrayTy.annos());
       case TY_VAR:
         TyVar tyVar = (TyVar) type;
-        return new Type.TyVar(tyVar.sym(), fixAnnotations(env, tyVar.annos(), extra, removed));
+        return new Type.TyVar(tyVar.sym(), appendAnnotations(tyVar.annos(), extra));
       case VOID_TY:
         return type;
       case WILD_TY:
@@ -226,22 +219,9 @@ public class DisambiguateTypeAnnotations {
     }
   }
 
-  private static ImmutableList<AnnoInfo> fixAnnotations(
-      Env<ClassSymbol, TypeBoundClass> env,
-      ImmutableList<AnnoInfo> annos,
-      ImmutableList<AnnoInfo> extra,
-      Builder<AnnoInfo> removed) {
-    ImmutableList.Builder<AnnoInfo> result = ImmutableList.builder();
-    for (AnnoInfo anno : annos) {
-      Set<ElementType> target = env.get(anno.sym()).annotationMetadata().target();
-      if (target.contains(ElementType.TYPE_USE)) {
-        result.add(anno);
-      } else {
-        removed.add(anno);
-      }
-    }
-    result.addAll(extra);
-    return result.build();
+  private static ImmutableList<AnnoInfo> appendAnnotations(
+      ImmutableList<AnnoInfo> annos, ImmutableList<AnnoInfo> extra) {
+    return ImmutableList.<AnnoInfo>builder().addAll(annos).addAll(extra).build();
   }
 
   /**
diff --git a/java/com/google/turbine/parse/ConstExpressionParser.java b/java/com/google/turbine/parse/ConstExpressionParser.java
index 4a3b297..59253d7 100644
--- a/java/com/google/turbine/parse/ConstExpressionParser.java
+++ b/java/com/google/turbine/parse/ConstExpressionParser.java
@@ -153,7 +153,7 @@ public class ConstExpressionParser {
         return primitiveClassLiteral(TurbineConstantTypeKind.BOOLEAN);
       case VOID:
         eat();
-        return finishClassLiteral(position, new Tree.VoidTy(position, ImmutableList.of()));
+        return finishClassLiteral(position, new Tree.VoidTy(position));
       case AT:
         return annotation();
       default:
diff --git a/java/com/google/turbine/parse/Parser.java b/java/com/google/turbine/parse/Parser.java
index e25ccb5..4e1dfb3 100644
--- a/java/com/google/turbine/parse/Parser.java
+++ b/java/com/google/turbine/parse/Parser.java
@@ -470,12 +470,14 @@ public class Parser {
       typaram = typarams();
     }
 
-    ImmutableList<Anno> typeAnnos = maybeAnnos();
+    if (token == Token.AT) {
+      annos = ImmutableList.<Anno>builder().addAll(annos).addAll(maybeAnnos()).build();
+    }
 
     switch (token) {
       case VOID:
         {
-          result = new Tree.VoidTy(position, typeAnnos);
+          result = new Tree.VoidTy(position);
           next();
           name = eatIdent();
           return memberRest(access, annos, typaram, result, name);
@@ -489,7 +491,7 @@ public class Parser {
       case DOUBLE:
       case FLOAT:
         {
-          result = referenceType(typeAnnos);
+          result = referenceType(ImmutableList.of());
           name = eatIdent();
           return memberRest(access, annos, typaram, result, name);
         }
@@ -510,7 +512,7 @@ public class Parser {
                         Optional.<ClassTy>absent(),
                         ident,
                         ImmutableList.<Type>of(),
-                        typeAnnos);
+                        ImmutableList.of());
                 name = eatIdent();
                 return memberRest(access, annos, typaram, result, name);
               }
@@ -523,24 +525,16 @@ public class Parser {
                         Optional.<ClassTy>absent(),
                         ident,
                         ImmutableList.<Type>of(),
-                        typeAnnos);
-                typeAnnos = maybeAnnos();
-                eat(Token.LBRACK);
-                do {
-                  result = new ArrTy(position, typeAnnos, result);
-                  eat(Token.RBRACK);
-                  typeAnnos = maybeAnnos();
-                } while (maybe(Token.LBRACK));
+                        ImmutableList.of());
+                result = maybeDims(maybeAnnos(), result);
                 break;
               }
             case LT:
               {
                 result =
-                    new ClassTy(position, Optional.<ClassTy>absent(), ident, tyargs(), typeAnnos);
-                while (maybe(Token.LBRACK)) {
-                  eat(Token.RBRACK);
-                  result = new ArrTy(position, typeAnnos, result);
-                }
+                    new ClassTy(
+                        position, Optional.<ClassTy>absent(), ident, tyargs(), ImmutableList.of());
+                result = maybeDims(maybeAnnos(), result);
                 break;
               }
             case DOT:
@@ -550,7 +544,7 @@ public class Parser {
                       Optional.<ClassTy>absent(),
                       ident,
                       ImmutableList.<Type>of(),
-                      typeAnnos);
+                      ImmutableList.of());
               break;
             default:
               throw error(token);
@@ -562,11 +556,8 @@ public class Parser {
             next();
             // TODO(cushon): is this cast OK?
             result = classty((ClassTy) result);
-            while (maybe(Token.LBRACK)) {
-              eat(Token.RBRACK);
-              result = new ArrTy(position, typeAnnos, result);
-            }
           }
+          result = maybeDims(maybeAnnos(), result);
           name = eatIdent();
           switch (token) {
             case LPAREN:
@@ -591,16 +582,15 @@ public class Parser {
   }
 
   private ImmutableList<Anno> maybeAnnos() {
-    ImmutableList<Anno> typeAnnos = ImmutableList.of();
-    if (token == Token.AT) {
-      ImmutableList.Builder<Anno> builder = ImmutableList.builder();
-      while (token == Token.AT) {
-        next();
-        builder.add(annotation());
-      }
-      typeAnnos = builder.build();
+    if (token != Token.AT) {
+      return ImmutableList.of();
+    }
+    ImmutableList.Builder<Anno> builder = ImmutableList.builder();
+    while (token == Token.AT) {
+      next();
+      builder.add(annotation());
     }
-    return typeAnnos;
+    return builder.build();
   }
 
   private ImmutableList<Tree> memberRest(
@@ -790,18 +780,14 @@ public class Parser {
 
   private VarDecl formalParam() {
     ImmutableList.Builder<Anno> annos = ImmutableList.builder();
-    EnumSet<TurbineModifier> access = modifiers(annos);
-    Type ty = referenceType(maybeAnnos());
+    EnumSet<TurbineModifier> access = modifiersAndAnnotations(annos);
+    Type ty = referenceType(ImmutableList.of());
     ImmutableList<Anno> typeAnnos = maybeAnnos();
     if (maybe(Token.ELLIPSIS)) {
       access.add(TurbineModifier.VARARGS);
       ty = new ArrTy(position, typeAnnos, ty);
-    }
-    while (token == Token.LBRACK) {
-      eat(Token.LBRACK);
-      eat(Token.RBRACK);
-      ty = new ArrTy(position, typeAnnos, ty);
-      typeAnnos = maybeAnnos();
+    } else {
+      ty = maybeDims(typeAnnos, ty);
     }
     // the parameter name is `this` for receiver parameters, and a qualified this expression
     // for inner classes
@@ -1040,14 +1026,20 @@ public class Parser {
       default:
         throw error(token);
     }
+    ty = maybeDims(maybeAnnos(), ty);
+    return ty;
+  }
+
+  private Type maybeDims(ImmutableList<Anno> typeAnnos, Type ty) {
     while (maybe(Token.LBRACK)) {
       eat(Token.RBRACK);
       ty = new ArrTy(position, typeAnnos, ty);
+      typeAnnos = maybeAnnos();
     }
     return ty;
   }
 
-  private EnumSet<TurbineModifier> modifiers(ImmutableList.Builder<Anno> annos) {
+  private EnumSet<TurbineModifier> modifiersAndAnnotations(ImmutableList.Builder<Anno> annos) {
     EnumSet<TurbineModifier> access = EnumSet.noneOf(TurbineModifier.class);
     while (true) {
       switch (token) {
diff --git a/java/com/google/turbine/tree/Tree.java b/java/com/google/turbine/tree/Tree.java
index 9ba6a70..6f46df5 100644
--- a/java/com/google/turbine/tree/Tree.java
+++ b/java/com/google/turbine/tree/Tree.java
@@ -203,8 +203,8 @@ public abstract class Tree {
       return visitor.visitVoidTy(this, input);
     }
 
-    public VoidTy(int position, ImmutableList<Anno> annos) {
-      super(position, annos);
+    public VoidTy(int position) {
+      super(position, ImmutableList.of());
     }
   }
 
diff --git a/javatests/com/google/turbine/lower/LowerIntegrationTest.java b/javatests/com/google/turbine/lower/LowerIntegrationTest.java
index 036689e..91c7d86 100644
--- a/javatests/com/google/turbine/lower/LowerIntegrationTest.java
+++ b/javatests/com/google/turbine/lower/LowerIntegrationTest.java
@@ -276,6 +276,10 @@ public class LowerIntegrationTest {
       "innerclassanno.test",
       "type_anno_parameter_index.test",
       "anno_const_scope.test",
+      "type_anno_ambiguous_qualified.test",
+      "type_anno_array_bound.test",
+      "type_anno_return.test",
+      "type_anno_order.test",
     };
     List<Object[]> tests =
         ImmutableList.copyOf(testCases).stream().map(x -> new Object[] {x}).collect(toList());
diff --git a/javatests/com/google/turbine/lower/testdata/type_anno_ambiguous_qualified.test b/javatests/com/google/turbine/lower/testdata/type_anno_ambiguous_qualified.test
new file mode 100644
index 0000000..30cb7c1
--- /dev/null
+++ b/javatests/com/google/turbine/lower/testdata/type_anno_ambiguous_qualified.test
@@ -0,0 +1,13 @@
+=== Test.java ===
+import static java.lang.annotation.ElementType.PARAMETER;
+import static java.lang.annotation.ElementType.TYPE_USE;
+import java.lang.annotation.Target;
+import java.util.List;
+
+@Target({TYPE_USE, PARAMETER}) @interface A {}
+@Target({TYPE_USE, PARAMETER}) @interface B {}
+
+class Test {
+  List<? extends @A Integer @B []> xs;
+}
+
diff --git a/javatests/com/google/turbine/lower/testdata/type_anno_array_bound.test b/javatests/com/google/turbine/lower/testdata/type_anno_array_bound.test
new file mode 100644
index 0000000..2c035a7
--- /dev/null
+++ b/javatests/com/google/turbine/lower/testdata/type_anno_array_bound.test
@@ -0,0 +1,13 @@
+=== Test.java ===
+import static java.lang.annotation.ElementType.PARAMETER;
+import static java.lang.annotation.ElementType.TYPE_USE;
+import java.lang.annotation.Target;
+import java.util.List;
+
+@Target({TYPE_USE, PARAMETER}) @interface A {}
+@Target({TYPE_USE, PARAMETER}) @interface B {}
+
+class Test {
+  void f(final java.lang.@A String x) {}
+}
+
diff --git a/javatests/com/google/turbine/lower/testdata/type_anno_array_dims.test b/javatests/com/google/turbine/lower/testdata/type_anno_array_dims.test
index c577264..805e82f 100644
--- a/javatests/com/google/turbine/lower/testdata/type_anno_array_dims.test
+++ b/javatests/com/google/turbine/lower/testdata/type_anno_array_dims.test
@@ -1,10 +1,12 @@
 === Test.java ===
 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 B {}
 
 class Test<T> {
   private T @A [] [] @B [] values;
+  Map.Entry<?, ?> @A [] @B [] entries;
 }
diff --git a/javatests/com/google/turbine/lower/testdata/type_anno_order.test b/javatests/com/google/turbine/lower/testdata/type_anno_order.test
new file mode 100644
index 0000000..8ac125f
--- /dev/null
+++ b/javatests/com/google/turbine/lower/testdata/type_anno_order.test
@@ -0,0 +1,15 @@
+=== Test.java ===
+import static java.lang.annotation.ElementType.METHOD;
+import static java.lang.annotation.ElementType.PARAMETER;
+import static java.lang.annotation.ElementType.TYPE_PARAMETER;
+import static java.lang.annotation.ElementType.TYPE_USE;
+
+import java.lang.annotation.Target;
+import java.util.function.Function;
+
+@Target({TYPE_USE, TYPE_PARAMETER, METHOD, PARAMETER}) @interface A {}
+
+interface Test {
+  public <T extends @A Object> @A Function<T, @A Throwable> a(
+      final @A Function<T, @A Throwable> delegate);
+}
diff --git a/javatests/com/google/turbine/lower/testdata/type_anno_return.test b/javatests/com/google/turbine/lower/testdata/type_anno_return.test
new file mode 100644
index 0000000..be10eb6
--- /dev/null
+++ b/javatests/com/google/turbine/lower/testdata/type_anno_return.test
@@ -0,0 +1,15 @@
+=== Test.java ===
+import static java.lang.annotation.ElementType.METHOD;
+import static java.lang.annotation.ElementType.PARAMETER;
+import static java.lang.annotation.ElementType.TYPE_PARAMETER;
+import static java.lang.annotation.ElementType.TYPE_USE;
+
+import java.lang.annotation.Target;
+import java.util.function.Function;
+
+@Target({TYPE_USE, TYPE_PARAMETER, METHOD, PARAMETER}) @interface A {}
+
+interface Test {
+  public <T extends Object> @A Function<T, Throwable> c(
+      final Function<T, Throwable> delegate);
+}
diff --git a/javatests/com/google/turbine/parse/testdata/type_annotations.input b/javatests/com/google/turbine/parse/testdata/type_annotations.input
index 1564821..f5cc7d6 100644
--- a/javatests/com/google/turbine/parse/testdata/type_annotations.input
+++ b/javatests/com/google/turbine/parse/testdata/type_annotations.input
@@ -1,5 +1,7 @@
 public class Test<@A T extends @B List<@C ?>> extends @D Object implements @E I {
-  <@A X extends @A Object> @A @B X f(int @C [] @D [] @E [] x) throws @E Exception {}
+  @A
+  @B
+  <@A X extends @A Object> X f(int @C [] @D [] @E [] x) throws @E Exception {}
   List<@A ? extends @B Object> x;
   List<@A ? super @B Object> y;
   List<@A ?> z;