Type use annotations on receiver not copied to generated code

[msridhar]

Consider the following example:

@Target({ElementType.TYPE_USE, ElementType.TYPE_PARAMETER})
public @interface TypeUseTest {
  String[] value() default {};
}

import com.google.auto.value.AutoValue;

@AutoValue
abstract class Animal {
  abstract String name();
  abstract int numberOfLegs();

  static Builder builder() {
    return new AutoValue_Animal.Builder();
  }

  @AutoValue.Builder
  abstract static class Builder {
    abstract Builder setName(@TypeUseTest({"hi"}) String value);
    abstract Builder setNumberOfLegs(int value);
    abstract Animal build(@TypeUseTest({"hi", "bye"}) Builder this);
  }
}

In AutoValue_Animal.Builder, I see the @TypeUseTest annotation copied down to the implementation of setName. But, I do not see the @TypeUseTest annotation on this copied down for the implementation of build.

This may be a bit of a corner case, but it would be great if it were supported! If the maintainers are interested and can point me roughly where the code should go, I can look into opening a PR.

[eamonnmcmanus]

Is this an issue you have run into in practice? It seems to me that if we are going to deal with it seriously we should copy receiver annotations whenever we are implementing an abstract method, so also in the implementations of name(), numberOfLegs(), setName, setNumberOfLegs, and the toBuilder() method if there is one. That's certainly possible, but I'd be interested in seeing the motivation.

[msridhar]

Is this an issue you have run into in practice?

Yes. We are working on using the Checker Framework to build a generic checker that can ensure some properties about the order in which methods are called on builder-like objects, e.g., that required setters are called before build(). To specify required methods we add a type use annotation on the this argument of build(). I could see this being relevant to other checkers in the future. Also, I think that for consistency all type use annotations on parameters should get copied down, though admittedly annotating this is less common.

I'll also note that we are not blocked on this issue at the moment. For our checker, the only problem is that if any client code directly uses the generated Builder subclass, we can miss bugs; but that should be rare. For other (hypothetical) checkers, not copying down the receiver annotation could cause a compile-time error.

[wesalvaro]

I have a different (perhaps larger) issue although I'm surprised hasn't come up...
My annotations are getting copied but copied in an incompatible way.

import com.my.package.Foo.Bar;
import com.my.package.Foo.Baz;
import org.checkerframework.checker.nullness.qual.Nullable;

@AutoFactory
class MyClass {
  MyClass(
    @Nullable Bar bar,
    @Nullable Baz baz,
    ...
  ) { ... }
}

The above seems to be generating:

import com.my.package.Foo;
import org.checkerframework.checker.nullness.qual.Nullable;

class MyClassFactory {
  MyClassFactory(
    @Nullable Foo.Bar bar,
    @Nullable Foo.Baz baz,
    ...
  ) { ... }
}

Because the build dies with this error:

 scoping construct cannot be annotated with type-use annotation: @org.checkerframework.checker.nullness.qual.Nullable
      @Nullable Foo.Bar bar,
 scoping construct cannot be annotated with type-use annotation: @org.checkerframework.checker.nullness.qual.Nullable
      @Nullable Foo.Baz baz,

Why isn't the generated code using the same scoping setup as the source?

[eamonnmcmanus]

@wesalvaro can you open a separate issue for that? It looks like we should be generating Foo.@Nullable Bar rather than @Nullable Foo.Bar, right?

Also, that's a bug in AutoFactory, not AutoValue like the earlier bug.