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

[improvement] Enable NullAway gradle checks #99

Merged
merged 4 commits into from
Jan 7, 2019

Conversation

schlosna
Copy link
Contributor

@schlosna schlosna commented Jan 2, 2019

Before this PR

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 or IntelliJ nullability checks, for example the below as seen in palantir/tritium#152 where use of UnsafeArg.of with a @Nullable value is mistakenly flagged:

warning: [NullAway] passing @Nullable parameter 'context' where @NonNull is required
                    UnsafeArg.of("context", context),
                                            ^

After this PR

Enables NullAway gradle build checks and appropriately annotates Arg and its subtypes to support proper nullability analysis.

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

Include appropriate @nullable and @nonnull annotations on Arg and its
subtypes to properly support NullAway checks.
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
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
@schlosna schlosna requested a review from a team as a code owner January 2, 2019 21:25
@@ -52,12 +59,13 @@ public final boolean equals(Object other) {
if (this == other) {
return true;
}
if (other == null || getClass() != other.getClass()) {
if (!(other instanceof Arg)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I tried using IntelliJ's regenerate equals method and it gave us exactly this.

return false;
}
Arg<?> arg = (Arg<?>) other;
return Objects.equals(name, arg.name)
&& Objects.equals(value, arg.value);
&& Objects.equals(value, arg.value)
&& (isSafeForLogging() == arg.isSafeForLogging());
Copy link
Contributor

Choose a reason for hiding this comment

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

this shouldn't change any behaviour because we don't expect people to make their own subclasses of Arg, but better safe than sorry!

@bulldozer-bot bulldozer-bot bot merged commit 6841f17 into palantir:develop Jan 7, 2019
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.

2 participants