Adresses #615

Not sure this is all we need. It would be nice,

1. to test it using an auto value. Does incremental annotation work using gradle 4.7 ?
2. add (at least) a unit test to make sure we pass the right originating elements tothe filer when we generate a __AutoValue file. (it seems to be the case: auto value passes the type containing the @AutoAnnotation annotation to the filer)

It would be really helpful, especially in constrained environments like Android, to have the Annotations and the Processor separated into different dependencies. The current structure leads to unnecessarily bloated output binaries.

Currently on Android, if the user includes AutoValue using compile 'com.google.auto.value:auto-value:1.2' then all of the processing classes and all of the shaded classes are included in the final binary. There are ways around this, but they are hacky and unclear.

Separating the annotations from the processor would allow something like the following:

compile 'com.google.auto.value:auto-value-annotation:1.2'
apt `com.google.auto.value:auto-value:1.2

This naming would allow existing users to update smoothly (assuming auto-value depends on auto-value-annotation)

This would only include the minimal set of classes (annotations) required in the output binary and ensure that all other classes, including shaded classes, aren't unnecessarily included.

Issues #87, #162, and #201 are examples of use cases that are a bit too specialized to put into the core of AutoValue, but which are still important when they apply. We should define an extensibility API so that users can build extensions that run at the same time as AutoValue. The general idea would be that there would be an interface, say AutoValueExtension, and that the AutoValue processor would use ServiceLoader to find implementations of this interface. So if you include on your -processorpath a jar that contains such an implementation, AutoValue will invoke it when it runs.

A possible sketch of the interface is this:

public interface AutoValueExtension {
  interface Context {
    ProcessingEnvironment processingEnvironment();
    String packageName();
    TypeElement autoValueClass();
    Map<String, ExecutableElement> properties();
  }

  boolean applicable(Context context);

  String generateClass(Context context, String classToExtend, String classToImplement);
}

The idea is that every extension gets its applicable method called, and those that return true get their generateClass method called. That method can return a String that is an implementation of the class called classToImplement that extends the class called classToExtend; or it can return null (for example if it has generated some related code but doesn't need to intervene in the AutoValue hierarchy). So if you have two extensions, @AutoValue class Foo might result in a hierarchy AutoValue_Foo extends $AutoValue_Foo extends $$AutoValue_Foo extends Foo, where AutoValue_Foo is generated by one of the extensions, $AutoValue_Foo is generated by the other, and $$AutoValue_Foo contains the logic generated by the AutoValue processor. This means that extensions can override the implementations of toString() etc.

This is a first pass implementation of issue #421, adding read only builder properties to the Extensions API.

The AutoValueExtension interface has been updated to overload the generateClass method to include an Optional<BuilderContext> parameter which is populated with information about the Builder, if there is one. The default implementation for this new method will call the older deprecated method if it is not overridden.

The AutoValueExtension.Context interface has been updated with a hasBuilder method. An Extension may require a Builder and should be informed by the applicable callback. We cannot provide a valid BuilderContext to the extension before the call generateClass, because we cannot validate the builder until all extensions have specified which methods they consume.

I'd like to have "optional" value properties but instead of annotating my properties with @Nullable and then checking for null when using the property I'd rather use java.util.Optional<T> and/or com.google.common.base.Optional<T>.

Here's what I'm doing today:

import com.google.auto.value.AutoValue;

import java.util.Optional;

class Example {
  @AutoValue
  abstract static class Animal {
    static Builder builder() {
      return new AutoValue_Example_Animal.Builder()
          .name(Optional.empty)
          .numberOfLegs(Optional.empty());
    }

    abstract Optional<String> name();
    abstract Optional<Integer> numberOfLegs();

    @AutoValue.Builder
    abstract static class Builder {
      Builder name(String s) { return name(Optional.ofNullable(s)); }
      abstract Builder name(Optional<String> s);
      Builder name(Integer n) { return name(Optional.ofNullable(n)); }
      abstract Builder numberOfLegs(Optional<Integer> n);
      abstract Animal build();
    }
  }
}

Here's what I want to do:

import com.google.auto.value.AutoValue;

import java.util.Optional;

class Example {
  @AutoValue
  abstract static class Animal {
    static Builder builder() {
      return new AutoValue_Example_Animal.Builder();
    }

    abstract Optional<String> name();
    abstract Optional<Integer> numberOfLegs();

    @AutoValue.Builder
    abstract static class Builder {
      abstract Builder name(String s);
      abstract Builder numberOfLegs(Integer n);
      abstract Animal build();
    }
  }
}

i.e. I want Optional<T> properties to default to Optional.empty()/Optional.absent() instead of null and to be able to define Builder methods (or constructors) that use T instead of Optional<T>. I'm using Optional.ofNullable(T)/Optional.fromNullable(T) instead of Optional.of(T)/Optional.of(T) so that I can still set a property to Optional.empty()/Optional.absent() by passing in null to the appropriate Builder method.

Gradle 4.7 provides support for incremental annotation processing. https://docs.gradle.org/nightly/userguide/java_plugin.html#sec:incremental_annotation_processing

It would be nice to see auto (at least auto-value) supporting it. The change is not that big as auto value is an isolating AP, and even simpler as it doesn't handle inheritance.

Is there a good way to direct generated output to a different directory than [build_folder]/intermediates/classes.... This gets a bit messy from using it in the IDE and linking to a build output folder as a src directory.

Suppose I have a set of base arguments that I need to share with two classes, and I define the following base class.

abstract class BaseArgs {
  abstract Object getFoo();
  abstract Object getBar();

  abstract static class Builder<T extends Builder<T>>{
    abstract T setFoo(Object foo);
    abstract T setBar(Object bar);
  }
}

Now I want to add baz for class A and qux for class B.

@AutoValue
abstract class ArgsA extends BaseArgs {
  abstract Object getBaz();

  @AutoValue.Builder
  abstract static class Builder extends BaseArgs.Builder<Builder> {
    abstract Builder setBaz(Object baz);

    abstract build();
  }
}

And similarly for ArgsB. However this fails with: Setter methods must return ArgsA.Builder, despite the generic annotation.

Is this something sane to do? Could it be fixed in the way the types are being compared to support the generic annotation? I could have optional parameters in BaseArgs but I would like to keep separate definitions of these arguments, and reuse any validations in the base class.

For now my only solution is to rewrite the builder setters in each of the two "concrete" classes.

This is a first pass implementation of issue #202, providing two options for extending AutoValue through the use of compile time plugins.

The first method, is by having your extension create an intermediate class between the abstract template class and the final AutoValue implementation of the class. The provides a good mechanism for adding functionality to an AutoValue class, while still keeping the implementation segregated.

The second method is to alter the final AutoValue class itself. This was required to support Android Parcelable implementations, as Parcels are required to have static factory objects, and this isn't achievable via inheritance. Using this method, extensions have to option of adding imports, interfaces and code to the resulting AutoValue class implementation. You can see a working example of one such plugin here.

You can view a test Android app that makes use of the auto-parcel extension here.

We get the following error message from time to time in our CI builds: warning: An exception occurred while looking for AutoValue extensions. No extensions will function. This may be due to a corrupt jar file in the compiler's classpath. Exception: java.util.ServiceConfigurationError: com.google.auto.value.extension.AutoValueExtension: Error reading configuration file This has no discernible pattern, but of course leads to breaking builds.

We're currently using com.google.auto.value:auto-value:1.6.3 and the latest module that failed randomly was also using com.google.auto.factory:auto-factory:1.0-beta5 (which also has beta6, but it looks like it's essentially the same version?) The JDK version is 1.8.0_162-b12 (which does not suffer from the dreaded "zip file closed" issue).

Is there any way to include more stack trace in the failure? Or is there any other clue to what's going on?

Since compile-testing doesn't support checking warnings, I've simply tested for the inverse here, ensuring that invalid properties fail with the appropriate error. This move should also remove the warning for "abstract methods other than property getters and builder converters" for consumed methods.

Following the discussion at https://github.com/google/auto/discussions/1340, I believe changing the defer policy to individual rejected elements as opposed to their enclosing TypeElement (if not PackageElement) is a sound and beneficial choice for the following reasons.

For my convenience whenever I refer to an element, please assume that I am talking about a non-package element.

Before I explain I want to note that in my solution superficial validation for each element is still done starting from its enclosing TypeElement; therefore, the deferral policy when it comes to well-informedness has not changed. Deferral policy is changed only for those elements that are rejected by a processor which are guaranteed to be well-informed.

Is it backward compatible?

I believe it should be. Unless someone uses some hacks to overcome similar (rare) difficulties like that of the discussion.

Why is it a sound choice?

1. The main problem in our discussion was that according to process() in Processor Javadoc (and only here) the input is TypeElements originating from the prior round and not any Elements. (Oddly enough, as it is verified in the discussion in practicality this is not the behavior of the checked Java compilers, more on this later.) I believe this is still the case especially if the current behavior of BasicAnnotationProcessor is acceptable.

   Presently, the aforementioned process() is overridden, and within it another process() method Is called. To distinguish between them let's call the latter process2(). The input of process2() is not restricted to TypeElements and in fact it gets any Element in general. With the current policy. the step deferred elements are only considered again in process2() and have nothing to do with process(). The only exception is when in process() the missing elements are to be reported, where now only enclosing types are reported but with my solution, the exact problematic elements are reported. I can only report the violating TypeElements as well if required. For this reason, I believe the solution does not violate the aforementioned Javadoc.

2. As hinted before, the checked processing tools (like OpenJDK) do not adhere to only sending the TypeElements and in that case, the new solution is the more appropriate behavior to have.

Why beneficial?

As discussed in the discussion because of the current behavior of common processing tools (like OpenJDK) if the annotation target something other than Types, it may happen that some elements correctly get processed and just because one other element within the same enclosing TypeElement gets rejected, all of these elements will be sent to be processed again. This is not optimized and also causes some headaches for the users (including me :)) of Google's BasicAnnotationProcessor to handle multiple processing scenarios.

Asking for some favors :)

1. I would greatly appreciate it if you check the Javadoc of ElementName and let me know if I am correct and give me your opinion. I think the main purpose of ElementName instead of Element is the information presented here.
2. For the unit test I could not figure out the API to say more than 1 file is created with the processor. I would appreciate your help in this regard as well.

Edits:

Now the retrieval of overloaded executable elements is done correctly. For now to gain access to erasedParameterTypes() I have added an instance of ExplicitOverrides in ExecutableElementName. While this is not bad it is subjective and for me is less than ideal, if this proposal gets accepted please let me know your opinion about a slight change in structure so that this method is more generally accessible.

I would still greatly appreciate your comments about the questions that I have asked before.

Thank you!

Migrate from legacy com.google.gwt to org.gwtproject.

org.gwtproject starts being available at the 2.10.0 release, so upgrade to that where necessary.

See https://github.com/google/auto/pull/1342#issuecomment-1165230080

RELNOTES=n/a

Bumps gwt from 2.9.0 to 2.10.0.

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

Summary of issue: A field that is nullable is nondeterministically having a null check in the generated code, depending on what other build tasks occurred.

See https://lists.apache.org/thread/cdw4y50r3hl37z8y7x470m2mddclkqgv (from here you can gather more info from the thread)

Inline, paraphrasing the thread...

To reproduce:

git clone https://github.com/apache/beam
cd beam
git checkout 4ffeae4d2b800f2df36d2ea2eab549f2204d5691~1
./gradlew :runners:direct-java:compileJava
less
./runners/direct-java/build/generated/sources/annotationProcessor/java/main/org/apache/beam/runners/direct/AutoValue_ImmutableListBundleFactory_CommittedImmutableListBundle.java

git checkout 4ffeae4d2b800f2df36d2ea2eab549f2204d5691
./gradlew :runners:direct-java:compileJava
less
./runners/direct-java/build/generated/sources/annotationProcessor/java/main/org/apache/beam/runners/direct/AutoValue_ImmutableListBundleFactory_CommittedImmutableListBundle.java

./gradlew :runners:direct-java:compileJava --rerun-tasks
less
./runners/direct-java/build/generated/sources/annotationProcessor/java/main/org/apache/beam/runners/direct/AutoValue_ImmutableListBundleFactory_CommittedImmutableListBundle.java

The class at https://github.com/apache/beam/blob/1dff59b4ff26310f88f927edfcf44709ed6ea9c2/runners/direct-java/src/main/java/org/apache/beam/runners/direct/ImmutableListBundleFactory.java#L130

abstract static class CommittedImmutableListBundle<T> implements CommittedBundle<T> {
    public static <T> CommittedImmutableListBundle<T> create(
        @Nullable PCollection<T> pcollection,
        StructuralKey<?> key,
        Iterable<WindowedValue<T>> committedElements,
        Instant minElementTimestamp,
        Instant synchronizedCompletionTime) {
      return new AutoValue_ImmutableListBundleFactory_CommittedImmutableListBundle<>(
          pcollection, key, committedElements, minElementTimestamp, synchronizedCompletionTime);
    }
  ...
}

extends https://github.com/apache/beam/blob/1dff59b4ff26310f88f927edfcf44709ed6ea9c2/runners/direct-java/src/main/java/org/apache/beam/runners/direct/CommittedBundle.java#L35

interface CommittedBundle<T> extends Bundle<T, PCollection<T>> {
  /** Returns the PCollection that the elements of this bundle belong to. */
  @Override
  @Nullable
  PCollection<T> getPCollection();

  ...
}

In all cases the PCollection field should be nullable.

In a "good" build, the AutoValue generated code constructor does not check for null

AutoValue_ImmutableListBundleFactory_CommittedImmutableListBundle(
    @Nullable PCollection<T> PCollection,
    StructuralKey<?> key,
    Iterable<WindowedValue<T>> elements,
    Instant minimumTimestamp,
    Instant synchronizedProcessingOutputWatermark) {
  this.PCollection = PCollection;

in a 'bad' build, the autovalue java is re-generated, but has an added nullness check:

AutoValue_ImmutableListBundleFactory_CommittedImmutableListBundle(
    PCollection<T> PCollection,
    StructuralKey<?> key,
    Iterable<WindowedValue<T>> elements,
    Instant minimumTimestamp,
    Instant synchronizedProcessingOutputWatermark) {
  if (PCollection == null) {
    throw new NullPointerException("Null PCollection");
  }
  this.PCollection = PCollection;

and then with --rerun-tasks it gets re-re-generated without the nullness check.

Discovered during apache/beam#16721 here is a summary example:

@AutoValue
public abstract class ValueInSingleWindow<T> {
  T getValue();
  ... other fields ...
}

It is valid to create ValueInSingleWindow<@Nullable String> in the usual way via ValueInSingleWindow.<@Nullable String>of(null, ...).

However, autovalue creates a check that the value is non-null, rejecting type-safe code.

It would be useful to have the @CheckReturnValue annotation automatically be added to the AutoValue generated getters and builder methods. Calling these without using their return value (i.e. writing them as statement) is probably always an error (e.g. configuring a builder, but never actually building the final object). The CheckReturnValue annotation is validated at least by errorprone and spotbugs. To my knowledge both tools care only about the simple class name, therefore the annotation could be cloned into the AutoValue annotations namespace, to not create a dependency to any of those tools.

Currently the developer can add the annotation to the AutoValue fields and they will be copied (so there is a workaround for the getters). However, it seems impossible to have the annotation at the generated builder methods. With our without using @AutoValue.CopyAnnotations they are missing from the generated code.