-
Notifications
You must be signed in to change notification settings - Fork 3
Add some missing @Nullable annotations.
#129
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
Conversation
Most of these came to my attention during the testing of [RedundantNullCheck](google/error-prone#5121). Notes: - For `BufferedInputStream` and `FilterInputStream`, see #128. - For `URL`, the docs for `getAuthority()` don't mention `null`, but it's easy enough to demonstrate that `null` is a possible return value: ```java var u = new URL("mailto:foo@bar.com"); System.out.println("authority " + u.getAuthority()); ``` - `AlgorithmParameters.toString()` is gross but documented as such, as eamonnmcmanus points out. - `Matcher.group()` is unfortunate: At one point, it could not return `null`, but that changed, as documented since [JDK-8274663](https://bugs.openjdk.org/browse/JDK-8274663). The change is a consequence of [`usePattern`](https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/util/regex/Matcher.html#usePattern(java.util.regex.Pattern)), which was added all the way back in Java 5. I am a little tempted to lie here and leave the type as non-null. - In `Component`, I also removed an annotation on the package-private helper method used by `getName()`. - In `ResultSet`, `getBlob` and `getObject` have no documentatino about `null` returns. Gemini [wrote me some code that demonstrates the possible null returns](https://g.co/gemini/share/776a7669ae47). - Also in `ResultSet`, I simplified some type-parameter declarations. This ensures that the `Class<T>` parameter has its type argument in bounds, and it makes no difference to the result type, which is always nullable, regardless of the type argument. Fixes #128
| * @since 1.1 | ||
| */ | ||
| public String getName() { | ||
| public @Nullable String getName() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could see also adding @Nullable on the parameter of setName, but I didn't give it much thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would seem consistent to make the setName parameter @Nullable - the check for nameExplicitlySet helps to account for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thanks, I hadn't paid attention to the nameExplicitlySet check. That does suggest that upstream is at minimum resigned to supporting null :) Done.
wmdietl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. If you want, make setName consistent.
| * if this parameter object has not been initialized. | ||
| */ | ||
| public final String toString() { | ||
| public final @Nullable String toString() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really ugly, as you note. It breaks behavioral subtyping, contradicting the contract from java.lang.Object#toString.
jspecify/jspecify#111 would say that this annotation should cause a distinct warning.
But as this is what the code is clearly doing, the annotation matches the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep :(
One note for anyone in the future who might be interested: It's not really fair of me to call this annotation "missing" in the way that the others are: This class is not @NullMarked, so JSpecify would say that the return type is unspecified. (Of course, I could hardly criticize a tool for still treating it as non-null, given everything you've said about toString().) Thus, the @Nullable annotation here is "missing" only in the sense that all unannotated code is "missing" tons of annotations. That is in contrast to the other edits I'm making, which are in null-marked classes, since those are changing types from non-null to nullable.
| * @since 1.1 | ||
| */ | ||
| public String getName() { | ||
| public @Nullable String getName() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would seem consistent to make the setName parameter @Nullable - the check for nameExplicitlySet helps to account for that.
cpovirk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
I have a sinking feeling that I'll have some work to do before I can import this one into Google. I may end up locally undoing the Matcher.group change on the grounds that it's more valuable to land the rest than to be strictly correct there.
| * @since 1.1 | ||
| */ | ||
| public String getName() { | ||
| public @Nullable String getName() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thanks, I hadn't paid attention to the nameExplicitlySet check. That does suggest that upstream is at minimum resigned to supporting null :) Done.
| * if this parameter object has not been initialized. | ||
| */ | ||
| public final String toString() { | ||
| public final @Nullable String toString() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep :(
One note for anyone in the future who might be interested: It's not really fair of me to call this annotation "missing" in the way that the others are: This class is not @NullMarked, so JSpecify would say that the return type is unspecified. (Of course, I could hardly criticize a tool for still treating it as non-null, given everything you've said about toString().) Thus, the @Nullable annotation here is "missing" only in the sense that all unannotated code is "missing" tons of annotations. That is in contrast to the other edits I'm making, which are in null-marked classes, since those are changing types from non-null to nullable.
Most of these came to my attention during the testing of
RedundantNullCheck.
Notes:
BufferedInputStreamandFilterInputStream, seeFilterInputStreamandBufferedInputStreamprotectedfields should likely be@Nullable#128.URL, the docs forgetAuthority()don't mentionnull, butit's easy enough to demonstrate that
nullis a possible returnvalue:
AlgorithmParameters.toString()is gross but documented as such, aseamonnmcmanus points out.
Matcher.group()is unfortunate: At one point, it could not returnnull, but that changed, as documented sinceJDK-8274663. The change
is a consequence of
usePattern,which was added all the way back in Java 5. I am a little tempted to
lie here and leave the type as non-null.
Component, I also removed an annotation on the package-privatehelper method used by
getName().ResultSet,getBlobandgetObjecthave no documentatino aboutnullreturns. Gemini wrote me some code that demonstrates thepossible null returns.
ResultSet, I simplified some type-parameter declarations.This ensures that the
Class<T>parameter has its type argument inbounds, and it makes no difference to the result type, which is always
nullable, regardless of the type argument.
Fixes #128