palantir / safe-logging

Interfaces and utilities for safe log messages
Apache License 2.0
14 stars 20 forks source link

Add `@Nullable` annotations to exception causes #869

Closed ash211 closed 1 year ago

ash211 commented 1 year ago

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[bot] commented 1 year ago

Generate changelog in changelog-dir>`changelog/@unreleased`</changelog-dir

Type

- [ ] Feature - [x] Improvement - [ ] Fix - [ ] Break - [ ] Deprecation - [ ] Manual task - [ ] Migration

Description

Add `@Nullable` annotations to exception causes **Check the box to generate changelog(s)** - [x] Generate changelog entry
svc-autorelease commented 1 year ago

Released 3.5.0

carterkozak commented 1 year ago

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 commented 1 year ago

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!