patch for "AutoValue should respect @Nullable annotations on types #283"

[brychcy]

The patch invokes the required java8-method via reflection, so everything should work with earlier java version (verified with java 7). The tests for java 8 functionality are ignored with earlier java versions, too.

What currently doesn't work (because of javac limitations):

• @nullable primitive arrays (I added an @ignored test case for this)
• @nullable in type annotation position, e.g.
  abstract java.lang.@nullable String anString()

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[eamonnmcmanus]

Thanks for this contribution! If I could make a couple of requests:

• We use two-space indentation at Google rather than four.
• Rather than having a Java 8 version of CompilationTest, I think it would be more maintainable to have a Java 8 version of AutoValueTest. I'd like to minimize the amount of code that tests the exact code generated, because it's a pain to update it all whenever we change the details of code generation.

Since the main motivation for this change is to ensure that null values are allowed if a property is @Nullable it should be straightforward to test that that is happening even for a type-targeted @Nullable.

FYI the way our code flow works we won't be merging this PR. Instead, we'll submit it into Google's internal code base and it will appear at our next push as a later PR that references this one. We do need the CLA to be signed before we can start that.

[brychcy]

"Rather than having a Java 8 version of CompilationTest, I think it would be more maintainable to have a Java 8 version of AutoValueTest. I'd like to minimize the amount of code that tests the exact code generated, because it's a pain to update it all whenever we change the details of code generation."

I felt a bit of the pain while creating the test, so I really understand what you mean. The problem is that TYPE_USE annotations don't compile at all with java 7 or older, so the integration tests wouldn't even compile and I currently have no idea how to exclude the tests on the maven level depending on the jdk.

I'll have a look at the indentation and CLA.

[tbroyer]

Wrt the test, you'd create a new integration-test project (e.g. src/it/functional-java8) and put an invoker.properties in there with invoker.java.version=1.8+. That way the integration-test project would only ever be invoked when the build is done with a JDK 8.
See https://maven.apache.org/plugins/maven-invoker-plugin/examples/selector-conditions.html
You'd also add a JDK 8 to the .travis.yml.

There probably are ways to get it run with Java 8 while Maven is invoked with a JDK 7, using Maven Toolchains, but AFAICT it's a PITA to get it working and would require everyone willing to build the project to configure his Maven toolchains.

[brychcy]

I signed it!

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

[brychcy]

I signed it!

[googlebot]

CLAs look good, thanks!

[brychcy]

Thanks for the hints regarding maven, I'll try to create a java 8 version of AutoValueTest.

[brychcy]

It looks like AutoValueTest currently doesn't get executed at all.

The files value/src/it/functional/invoker.properties and value/src/it/functional/pom.xml were deleted in commit 2c4bf22

Maybe this was not intended?

[eamonnmcmanus]

Yow! It looks as if something pretty bad happened in that commit. We'll need to investigate. (Paging @cgruber...)

[cgruber]

Fixed. Please fetch the remote origin branch and rebase this pull
requests' branch against origin/master. For some reason a README/javadoc
editing commit got mis-merged (without conflict) and the mis-merge deleted
the two files needed to invoke the integration tests. That is crazy weird,
but I've fixed it by partially reverting that commit (just the relevant
bits). It seems likely it was bad behavior from our syncing tool, and we
missed it. Apologies.

On Tue, 1 Dec 2015 at 08:38 Éamonn McManus notifications@github.com wrote:

Yow! It looks as if something pretty bad happened in that commit. We'll
need to investigate. (Paging @cgruber https://github.com/cgruber...)

—
Reply to this email directly or view it on GitHub
#293 (comment).

[brychcy]

Updated.

Some notes:

1. The "eclipse" profile in value/src/it/functional/pom.xml doesn't really seem to do anything different than the other profile. Support for com.google.auto.value.EclipseHackTest has been removed in 4b8ccc5. The commit comment suggests that there is a real test with the Eclipse compiler, but i cannot see how it works (maybe Google-internal somehow?). I have therefore manually tested with eclipse (and have added some code to TypeSimplifier)

2. I have copied functional to functional-java8 as suggested. You can compare the directories, to see what had to be changed. Note that I have disabled the NullPointerTester tests (because it doesn't understand type annotations) and some tests in the functional-java8 version of AutoValueTest.java because of the following limitations (repeated from the commit comment):

Limitations: The following cases currently don't work with javac (i.e.
the annotation processors don't see the @nullable so a null check is
generated in the constructor):

public abstract int @nullable [] getInts()
This works with eclipse JDT and the null check is ommited in the
generated code, but note that the @nullable is in the wrong position in
the generated code (i.e. @nullable int[] instead of int @nullable[])),
which might lead to compiler warnings

For generic type T:
@nullable
public abstract T getT()
(works in JDT)

Also @nullable won't work for types which can't be imported because of
naming conflicts, because the @nullable is generated at the wrong place,
i.e. "@nullable some.package.Map" instead of "some.package.@nullable
Map" which won't compile at all for TYPE_USE @nullable.

[brychcy]

Regarding the javac limitations: openjdk bug 8031744 is fixed for java 9 as of JEP 217.

I have tried the it/functional-java8 test with the Early Access Java 9 Build 95 and the annotations are now visible to the Annotations processor, so the int @Nullable [] case works. What still doesn't work is @Nullable T, because for it, the TypeMirror#toString() which is used via com.google.auto.value.processor.TypeSimplifier.ToStringTypeVisitor.defaultAction(TypeMirror, StringBuilder) now includes the Annotation, so the generated code contains the Annotations twice:

@Nullable
@Override
public @com.google.auto.value.annotation.Nullable T t() {
    return t;
}

[eamonnmcmanus]

Thanks for this detailed investigation!

I'm not sure whether JDK 9 should be including annotations in the toString. Maybe. Regardless, avoiding the problem seems straightforward, by overriding visitTypeVariable in TypeSimplifier.ToStringTypeVisitor, and using typeVariable.asElement().getSimpleName().

This makes me think that ToStringTypeVisitor should document exactly where it is used so that it will be more obvious what string should be output for each kind of type. But that's an issue for me.

[eamonnmcmanus]

This code has been incorporated into the latest snapshot. Thanks!

Our processes mean that we do this in Google's internal repository rather than merging the code here, so I'm closing the PR.