Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Nullable as a TYPE_USE annotation #1193

Closed
tbroyer opened this issue Jun 21, 2018 · 6 comments
Closed

Add support for Nullable as a TYPE_USE annotation #1193

tbroyer opened this issue Jun 21, 2018 · 6 comments

Comments

@tbroyer
Copy link
Contributor

tbroyer commented Jun 21, 2018

I wanted to switch from JSR305 to Checker Framework @Nullable, but it breaks my @Nullable injection points:

1) null returned by binding at […]
 but the 1st parameter of […].<init>([…]) is not @Nullable

Apparently, this is because Guice only looks at field and parameter annotations, not TYPE_USE annotations:

Annotation[] parameterAnnotations = paramterAnnotations[index];
Key<?> key = Annotations.getKey(parameterType, member, parameterAnnotations, errors);
dependencies.add(newDependency(key, Nullability.allowsNull(parameterAnnotations), index));

Note: I couldn't use the Java 7-compatible annotations from the Checker Framework either because it's named @NullableDecl, and Guice only looks at annotations named @Nullable.

@tbroyer
Copy link
Contributor Author

tbroyer commented Jun 21, 2018

Possible workaround: in our case, I could easily switch to OptionalBinder and Optional<T> instead of @Nullable T.

@ronshapiro
Copy link
Collaborator

FWIW, we decided (both Guice and Dagger) to ignore type annotations for qualifiers at least for the time being because of the complications that they may introduce into the key/type system that weren't previously considered.

This may be entirely different, as I'm not familiar with this part of Guice, but there's always the consideration of something like this:

class Foo<T> {
  @Inject Foo(T t) {}
}

class Bar {
  @Inject Bar(Foo<@Nullable String> fooOfNullableString) {} 
}

@nresare
Copy link

nresare commented Apr 9, 2020

To clarify what I believe is happening here, the annotations that describe annotations and the reflection API to get information about annotations was extended in jdk8. Annotations such as @Nullable can now have a @Target({TYPE_USE}) and this does indeed make sense to use for something like @Nullable. This is what for example he checkerframework Nullable uses. To extract such annotation from a constructor, one need to use constructor.getAnnotatedParameterTypes()...getAnnotations() which was introduced in java8.

nresare pushed a commit to nresare/guice that referenced this issue Apr 9, 2020
Java support for annotation metadata has evolved, and projects providing
implementations of @nullable has been adopting those new features. This
has had the side effect of some commonly used @nullable annotations not
working as expected with Guice. This change uses reflection to extract
new style @nullable annotations as well as old style ones. Addresses
the issue described in google#1193
@cpovirk
Copy link
Member

cpovirk commented Jan 25, 2021

There are a lot of things that "Guice supports @Nullable type annotations" could mean, but 1a410a8 accomplished the main one. I assume there hasn't been a release yet, though.

@cpovirk
Copy link
Member

cpovirk commented Oct 25, 2021

It looks to me like 5.0.0 (though use 5.0.1 instead), released earlier this year, contains the change that lets you write @Nullable Foo foo for type-use annotations as well as for declaration annotations.

As discussed above, Guice could theoretically do more with type-use annotations, but this at least makes them not worse than declaration annotations. Possibly that's enough to close this issue.

Anyone who migrates from declaration annotations to type-use annotations may need to move some annotations along the way:

  • @Nullable Foo[] foo -> Foo @Nullable [] foo
  • @Nullable Foo.Bar bar -> Foo.@Nullable Bar bar

(That's just how type-use-annotation syntax works.)

(I don't see mention of this in the release notes. Maybe it should be added?)

@sameb
Copy link
Member

sameb commented Apr 19, 2023

This was fixed in 1a410a8.

@sameb sameb closed this as completed Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants