Add @Nullable annotations to Arg and subtypes #97
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jsr305 is a compile-only dependency in order to provide a minimal footprint while allowing validation in projects which provide the dependency.
|
@cakofony I forgot to mention I had created develop...schlosna:ds/null-away but didn't create a PR. Interestingly the checks flagged something in the |
bulldozer-bot bot
pushed a commit
that referenced
this pull request
Jan 7, 2019
<!-- PR title should start with '[fix]', '[improvement]' or '[break]' if this PR would cause a patch, minor or major SemVer bump. Omit the prefix if this PR doesn't warrant a standalone release. --> ## Before this PR <!-- Describe the problem you encountered with the current state of the world (or link to an issue) and why it's important to fix now. --> The safe-logging library did not explicitly annotate `Arg` and its subtypes, so consumers of this library would receive false-positive warnings when using static analysis checks such as [NullAway](https://github.com/uber/NullAway) or IntelliJ nullability checks, for example the below as seen in palantir/tritium#152 where [use of `UnsafeArg.of` with a `@Nullable` value](https://github.com/palantir/tritium/blob/develop/tritium-core/src/main/java/com/palantir/tritium/event/CompositeInvocationEventHandler.java#L142) is mistakenly flagged: ``` warning: [NullAway] passing @nullable parameter 'context' where @nonnull is required UnsafeArg.of("context", context), ^ ``` ## After this PR <!-- Describe at a high-level why this approach is better. --> Enables [NullAway](https://github.com/uber/NullAway) gradle build checks and appropriately annotates Arg and its subtypes to support proper nullability analysis. <!-- Reference any existing GitHub issues, e.g. 'fixes #000' or 'relevant to #000' --> Relevant to palantir/tritium#152 and supersedes #97 The NullAway checks identified one possible issue in safe-logging's `LoggableExceptionAssert`, and this has been corrected. * Handle null LoggableExceptionAssert arg, addresses NullAway warnings: ``` > Task :preconditions-assertj:compileJava /Volumes/git/safe-logging/preconditions-assertj/src/main/java/com/palantir/logsafe/testing/LoggableExceptionAssert.java:30: warning: [NullAway] initializer method does not guarantee @nonnull field argsAssert is initialized along all control-flow paths (remember to check for exceptions or early returns). public LoggableExceptionAssert(T actual) { ^ (see http://t.uber.com/nullaway ) /Volumes/git/safe-logging/preconditions-assertj/src/main/java/com/palantir/logsafe/testing/LoggableExceptionAssert.java:33: warning: [NullAway] assigning @nullable expression to @nonnull field argsAssert = actual == null ? null : new ArgsAssert(actual.getArgs()); ^ (see http://t.uber.com/nullaway ) 2 warnings ``` Another error-prone warning identified a potential issue with `Arg.equals` implementation * Arg.equals checks isSafeForLogging, addresses EqualsGetClass error-prone finding: ``` > Task :safe-logging:compileJava /Volumes/git/safe-logging/safe-logging/src/main/java/com/palantir/logsafe/Arg.java:51: warning: [EqualsGetClass] Overriding Object#equals in a non-final class by using getClass rather than instanceof breaks substitutability of subclasses. public final boolean equals(Object other) { ^ (see https://errorprone.info/bugpattern/EqualsGetClass) Did you mean 'if (!(other instanceof Arg)) {'? 1 warning ```
|
Thanks @schlosna! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
jsr305 is a compile-only dependency in order to provide a minimal
footprint while allowing validation in projects which provide the
dependency.