feat(gapic-generator): Add Nullable annotation to generated classes#13558
Conversation
nnicolee
added
the
do not merge
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces @Nullable annotations to various generated classes and generator code in the Java GAPIC generator. Specifically, it updates the composer classes to annotate settings fields, getter methods, non-required resource name helper arguments, parse methods, toStringList generic types, and equals method arguments, with corresponding updates to the golden files. A review comment highlights a fragile workaround in ResourceNameHelperClassComposer.java where the @Nullable annotation is hacked into the VaporReference name string, warning that this could lead to issues with type comparisons, fully qualified names, AST manipulation, and missing imports, and suggests extending the AST classes to properly support type-use annotations.
nnicolee
added
the
kokoro:run
yoshi-kokoro
removed
the
kokoro:run
nnicolee
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
| public final Room getRoom(RoomName name) { | ||
| GetRoomRequest request = | ||
| GetRoomRequest.newBuilder().setName(name == null ? null : name.toString()).build(); | ||
| GetRoomRequest request = GetRoomRequest.newBuilder().setName(name.toString()).build(); |
There was a problem hiding this comment.
qq, I don't think we should change this. Perhaps better to mark the param as nullable?
There was a problem hiding this comment.
+1 the new logic may affect this, which is a good way to catch additional cases where @Nullable is needed.
There was a problem hiding this comment.
Discussed with @nnicolee, looks like setName is a proto class, which we don't have control over, and is being marked by NullAway with:
[ERROR] /usr/local/google/home/nicolejlee/google-cloud-java/java-showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/MessagingClient.java:[1822,64] error: [NullAway] passing @Nullable parameter 'parent == null ? null : parent.toString()' where @NonNull is required
If it was possible to configure ErrorProne to ignore proto classes in these cases that would be great. That could be a good first step.
On the other hand, looks like Proto classes actually can take nulls (since the code's been like this), which can be worked around by adding a warning suppression as Nicole suggested.
There was a problem hiding this comment.
Yes, currently we're ignoring this error
96dffb8 to
30ce14a
Compare
96dffb8 to
0a438e0
Compare
c245fbc to
8084981
Compare
nnicolee
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
…-cloud-java into feat/jspecify-nullable
|
|
…-cloud-java into feat/jspecify-nullable
…-cloud-java into feat/jspecify-nullable
nnicolee
removed
the
do not merge
The `check-version` job in showcase is not running in PRs that do not modify the specific paths. Update the job to use a filter job to skip the job on non-relevant PRs. Example: #13558
5 participants
This PR updates the gapic-generator-java to add the JSpecify @nullable annotation to all generated class declarations. This is the second phase in onboarding the generated client libraries to compile-time safety validation, see design doc for more details: go/sdk:java-jspecify-null-annotations-gapic
Classes Annotated:
Implementation Changes:
Verification/Testing: