Generated equals(Object) method parameter should be @Nullable

[pettermahlen]

In AutoValue,

  @Override
  public boolean equals(Object o) {
  // ...

should be

  @Override
  public boolean equals(@Nullable Object o) {
  // ...

I think it's better form, and without it, AutoValue doesn't play nicely with Guava's AbstractPackageSanityTests.

[gk5885]

I agree - this is entirely in line with the philosophy of "what you would have written". That said, I think this is one of the few cases where a processor option is appropriate because I wouldn't want to force users into an additional compile-time option.

Any thoughts on this @eamonnmcmanus?

[cgruber]

I don't think there's any actual reason to avoid this. It feels like
something that could easily be always on, unless the issue is one of
dependency propagation. But I thought we had made it that you can
include auto-value's processor as optional because you only need the
annotation to be present. This means we could include the
javax.annotations dep and it would not increase the dep load of the
user's runtime, only their compile-time.

[kevinb9n]

I'm strongly against AutoValue saddling users with a compile-time dep that
they didn't provably already have or explicitly ask for.

On Mon, Feb 10, 2014 at 10:52 AM, Christian Edward Gruber <
notifications@github.com> wrote:

I don't think there's any actual reason to avoid this. It feels like
something that could easily be always on, unless the issue is one of
dependency propagation. But I thought we had made it that you can
include auto-value's processor as optional because you only need the
annotation to be present. This means we could include the
javax.annotations dep and it would not increase the dep load of the
user's runtime, only their compile-time.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/73#issuecomment-34666882
.

Kevin Bourrillion | Java Librarian | Google, Inc. | kevinb@google.com

[cgruber]

Why? Quite honestly here, why? They didn't ask for a Guava dependency
at compile-time, and we're giving them that.

[kevinb9n]

I don't think that's true. We don't use Guava anywhere in the processor.
Even if we did, it would only be in the self-contained processor artifact,
as opposed to telling people that to use AutoValue they must both use our
processor and make sure that a certain JAR is on the compile classpath.

On Mon, Feb 10, 2014 at 11:32 AM, Christian Edward Gruber <
notifications@github.com> wrote:

Why? Quite honestly here, why? They didn't ask for a Guava dependency
at compile-time, and we're giving them that.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/73#issuecomment-34672316
.

Kevin Bourrillion | Java Librarian | Google, Inc. | kevinb@google.com

[cgruber]

Ah. I see the point. I guess having not worked without a
dependency-aware build system since the early 2000's, I simply don't get
as worked up about asking someone to have a compile-time dependency.
Runtime-dependency yes, as that bloats things, but not a package of
annotations. I do see what your saying, and you're right, the
difference between an internal implementation of guava vs. something in
their generated code is significant. I think we just differ as to the
degree of that significance.

[eamonnmcmanus]

I confirm that the AutoValue processor does not use Guava, either in its own code or in the code it generates.

I would not like to generalize from Christian's never needing to worry about dependencies to saying that nobody should. Additionally, the point about AbstractPackageSanityTests seems flimsy to me. I don't think NullPointerTester should ever require equals(Object) to throw an exception if given a null argument, regardless of the presence or absence of @Nullable. I've logged a bug to that effect.

As an additional wrinkle, we recently changed the AutoValue processor so it would understand a @Nullable annotation (on the abstract property methods) from any package, not just javax.annotation. There are surprisingly many such @Nullable variants in the wild. If your code uses one, would we have to give you a way to use it in equals(Object) instead of the javax.annotation one?

In conclusion, I don't think it would be a good idea to make this change.

[gk5885]

@eamonnmcmanus, what would you think about a compiler flag to allow user to set the FQN of the Nullable annotation they would (or wouldn't) want to use? NullPointerTester aside, it seems reasonable to want to put the correct annotation for your project on the generated code without relying on special casing in the tooling.

[cgruber]

My bad, Eamonn. I do have a tendency to mix the auto projects a bit, and I did think that autovalue used Guava in the processor. But no matter. The view of this group is clearly against permitting this, and I can understand it.

[eamonnmcmanus]

@gk5885, we don't currently include @Nullable in the generated code. I think we reasonably could on constructor parameters, if the corresponding abstract method in the @AutoValue class is marked @Nullable. But in that case we would simply use the same @Nullable as that method. I agree that here NullPointerTester would be doing the right thing to expect nullability of parameters to be specified by @Nullable, although it currently expects exactly @javax.annotation.Nullable and would also have to be changed to support @Nullable from other packages.

We could also put @Nullable on the generated implementations of the property methods, of course, though that would be less obviously useful.

[pettermahlen]

I would agree with @cgruber regarding a dependency on a package with the @Nullable annotation not being very significant, though it is of course a slippery slope and I understand that you don't wish to add dependencies on behalf of your users. As one of those, I'd like to stress, though, that the lack of consistency between @AutoValue and NullPointerTester is a pretty large usability issue. It's the sort of thing you'd expect to 'just work', or at least 'just work with a little tweak somewhere'. Having to add exceptions or special testNulls methods in your package sanity tests for all @AutoValue generated classes is not ideal. It seems like exactly the kind of boilerplate that both the tools strive to eliminate?

[tbroyer]

My 2c. I agree with Éamonn; accepting nulls is part of Object#equals'
contract, so the bug is in Guava-test.
Le 11 févr. 2014 08:39, "Petter Måhlén" notifications@github.com a écrit :

I would agree with @cgruber https://github.com/cgruber regarding a
dependency on a package with the @nullable annotation not being very
significant, though it is of course a slippery slope and I understand that
you don't wish to add dependencies on behalf of your users. As one of
those, I'd like to stress, though, that the lack of consistency between
@autovalue and NullPointerTester is a pretty large usability issue. It's
the sort of thing you'd expect to 'just work', or at least 'just work with
a little tweak somewhere'. Having to add exceptions or special testNullsmethods in your package sanity tests for all
@autovalue generated classes is not ideal. It seems like exactly the kind
of boilerplate that both the tools strive to eliminate?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/73#issuecomment-34732212
.

[pettermahlen]

From the javadocs of Object.equals(Object):

For any non-null reference value x, x.equals(null) should return false.

So, it's very clear (of course) that the parameter to equals is nullable. I'm not quite convinced that that means it's clear that the @Nullable annotation should not be used in equalsmethods, as that would mean equals needs to be special-cased. How many other such special cases would there have to be?

As a user, I'm fine with NullPointerTester special-casing, but I'm personally also fine with a JSR-305 compile-time dependency, since I tend to want that anyway. Special-casing already happens in the testing library (in the form of magic test method and class names), so that may be the lesser evil, I'm not sure.

[cgruber]

Another approach is to simply test for the presence of javax.annotation.Nullable in the processing environment - if it's there, use it, if not, don't. This leaves the signaling as a simple matter of "add it to your classpath if you care" and that takes care of the build dep issue also.

[eamonnmcmanus]

That's an interesting suggestion, @cgruber. I'm a bit hesitant to make the output depend on whether a class is available. Elsewhere in Java I don't think we ever have a situation where you can get one successful compiler output if a class is present and a different successful compiler output if the only difference is that is not. [Puzzler?] To come back to the original reason for this discussion, it seems strange that the result of AbstractPackageSanityTests.testsNulls() would depend on what had been in the classpath when the package under test was compiled.

[cgruber]

I think annotations are the only place you could get away with such, @eamonnmcmanus, since they need only be present in the compile (and are guaranteed to be so in this case) but can be elided in the runtime environment if they do not need to be processed in the runtime environment (or can be processed reflectively). I have no strong commentary on AbstractPackageSantityTests, sorry.

[pettermahlen]

I think it may be hard to figure out whether the user added @Nullable to the classpath, since AutoValue does. It's excluded from the packaged artifact's classpath because AutoValue is a 'provided' dependency. At least, that's the state as I understand it with the code currently on github - I don't see the changes to support other @Nullables than javax.annotations, so I guess it's not the very latest. It seems like in order to use the classpath solution, AutoValue might have to use string names for the annotation classes it supports rather than the actual annotations themselves, which would be bad in itself.

Another note is that I would say that the argument in favour of supporting generating code with @Nullable (probably in more places than just generated equals() methods) is based on helping ensure code correctness. The counter-argument about dependencies, while very valid and strong, is based on (in-)convenience. So it's an argument based in 'accidental complexity' due to the unfortunate fact that JSR-305 isn't included in JDK. That doesn't make the argument invalid in my eyes - I agree it would be bad for AutoValue to force a runtime dependency on users. But it's the reason why I think it would be nice to be able to somehow be able to get those annotations in the generated code.

Was there an argument against the use of a processor option for this? Such a processor option could specify the fully qualified class name of the annotation to use.

[gk5885]

I'm -1 for any such auto-detection.