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 @Nullable annotations to exception causes #869

Merged
merged 2 commits into from
May 30, 2023
Merged

Conversation

ash211
Copy link
Contributor

@ash211 ash211 commented May 26, 2023

Before this PR

Baseline's nullaway plugin blocks calling these constructors with a null cause:

 warning: [NullAway] passing @Nullable parameter 'last' where @NonNull is required
        throw new SafeRuntimeException("Unable to execute call even after exponential backoff.", last);
                                                                                                 ^
    (see http://t.uber.com/nullaway )
error: warnings found and -Werror specified

But null causes are explicitly allowed in the JDK for this parameter:

https://github.com/openjdk/jdk/blob/46c4da7fddb8103934f2a90b4456a5ce6ed3467c/src/java.base/share/classes/java/lang/Exception.java#L79-L81

So we need to tell nullaway that this pattern is nullable, and the standard way to do that is with the javax.annotation.Nullable annotation.

After this PR

==COMMIT_MSG==
Add @Nullable annotations to exception causes
==COMMIT_MSG==

@changelog-app
Copy link

changelog-app bot commented May 26, 2023

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Add @Nullable annotations to exception causes

Check the box to generate changelog(s)

  • Generate changelog entry

Copy link
Contributor

@carterkozak carterkozak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

@bulldozer-bot bulldozer-bot bot merged commit 0004410 into develop May 30, 2023
@bulldozer-bot bulldozer-bot bot deleted the aash/nullable branch May 30, 2023 14:39
@svc-autorelease
Copy link
Collaborator

Released 3.5.0

@carterkozak
Copy link
Contributor

fwiw, null causes allowed by the jdk aren't sufficient reason to allow null in our APIs, however in this case I think the convenience of matching broader norms outweigh the avoidance of nulls (plus, we tend to be more permissive around errors such that fewer things go wrong when things begin to go sideways)

@ash211
Copy link
Contributor Author

ash211 commented May 30, 2023

Good point that JDK being null-accepting doesn't mean our exceptions must be as well. But being permissive in error scenarios makes sense too. Thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants