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

Fix the errors reported by NullAway #2776

Merged
merged 3 commits into from
Jun 8, 2020
Merged

Fix the errors reported by NullAway #2776

merged 3 commits into from
Jun 8, 2020

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Jun 5, 2020

Motivation:

Before adopting NullAway(#1680) or other static analysis tools.
Manually run it and clean up errors.

Modifications:

  • Fix all true positives reported by NullAway

Result:

Less NullAway errors

@ikhoon ikhoon added the cleanup label Jun 5, 2020
@ikhoon ikhoon requested review from minwoox and trustin as code owners June 5, 2020 04:08
@ikhoon ikhoon added this to the 0.99.7 milestone Jun 5, 2020
@anuraaga
Copy link
Collaborator

anuraaga commented Jun 5, 2020

Looks like the majority are not inheriting the annotation from a base. Behavior for this doesn't seem well defined.

I'm curious how others treat this, for example does Kotlin treat all these methods as non-null? If so then adding the annotation is good, if not though maybe NullAway interpreting too strictly?

core/src/main/java/com/linecorp/armeria/client/Client.java Outdated Show resolved Hide resolved
@@ -220,14 +220,14 @@

private static <T extends Annotation> void findMetaAnnotations(
Builder<T> builder, Annotation annotation,
Class<T> annotationType, Class<? extends Annotation> containerType) {
Class<T> annotationType, @Nullable Class<? extends Annotation> containerType) {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@trustin
Copy link
Member

trustin commented Jun 5, 2020

I'm curious how others treat this, for example does Kotlin treat all these methods as non-null? If so then adding the annotation is good, if not though maybe NullAway interpreting too strictly?

/cc Kotlin users - @gary-lo @tobias- @rfkm @okue

@ikhoon
Copy link
Contributor Author

ikhoon commented Jun 5, 2020

Kotlin also inherits @Nullable annotation.

// in Java
interface Super {
    @Nullable
    String foo();
}
interface NullableSub extends Super {
    @Override
    String foo();
}
interface NonNullSub extends Super {
    @Nonnull
    @Override
    String foo();
}

// in Kotlin
// Nullable is inherited
val a: NullableSub = ...
val fooA: String? = a.foo()

val b: NonNullSub = ...
val fooB: String = b.foo()

@ikhoon
Copy link
Contributor Author

ikhoon commented Jun 5, 2020

Kotlin treats non-null by default if it does not have @Nullable.

// in Java
class Foo {
    @Nullable
    String bar() { ... }
    String baz() { ... }
}

// in Kotlin
val foo = Foo()
val bar: String? = foo.bar()
val baz: String = foo.baz()

@tobias-
Copy link
Contributor

tobias- commented Jun 5, 2020

We've had issues with the Kotlin compiler in IntelliJ and the one used for Gradle exhibiting different behaviours on nonnull. Rule of thumb is that if the Gradle one accepts it, the IntelliJ one does as well. IntelliJ is more likely to accept DefaultNullable and thus causing discrepancies between the two compilations.

I will check in an hour or so.

Only read this on my phone. What @ikhoon said

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @ikhoon 😄

@trustin
Copy link
Member

trustin commented Jun 5, 2020

Oh, it seems like we are getting assertion errors during the build. Perhaps the new asserts we added is the problem? 😅

@ikhoon
Copy link
Contributor Author

ikhoon commented Jun 6, 2020

Oh, it seems like we are getting assertion errors during the build. Perhaps the new asserts we added is the problem? 😅

Oops...

@codecov
Copy link

codecov bot commented Jun 6, 2020

Codecov Report

Merging #2776 into master will increase coverage by 0.08%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2776      +/-   ##
============================================
+ Coverage     72.78%   72.87%   +0.08%     
- Complexity    11845    11878      +33     
============================================
  Files          1042     1042              
  Lines         46090    46159      +69     
  Branches       5748     5764      +16     
============================================
+ Hits          33546    33637      +91     
+ Misses         9603     9571      -32     
- Partials       2941     2951      +10     
Impacted Files Coverage Δ Complexity Δ
...corp/armeria/client/RefreshingAddressResolver.java 76.35% <0.00%> (-0.68%) 20.00 <0.00> (-1.00)
...in/java/com/linecorp/armeria/common/MediaType.java 97.29% <ø> (ø) 96.00 <0.00> (ø)
...eria/internal/common/DefaultTimeoutController.java 86.17% <ø> (ø) 31.00 <0.00> (ø)
...ria/internal/common/util/ObjectCollectingUtil.java 93.33% <ø> (ø) 6.00 <0.00> (ø)
...ria/internal/server/annotation/AnnotationUtil.java 86.86% <ø> (-3.65%) 50.00 <0.00> (-2.00)
...a/com/linecorp/armeria/server/docs/DocService.java 94.21% <ø> (ø) 41.00 <0.00> (ø)
...necorp/armeria/internal/common/brave/SpanTags.java 66.66% <0.00%> (-33.34%) 3.00% <0.00%> (-2.00%)
...n/java/com/linecorp/armeria/server/HttpServer.java 40.00% <0.00%> (-20.00%) 2.00% <0.00%> (-1.00%)
...com/linecorp/armeria/client/brave/BraveClient.java 80.32% <0.00%> (-9.84%) 12.00% <0.00%> (-3.00%)
...rmeria/internal/common/brave/TraceContextUtil.java 95.83% <0.00%> (-4.17%) 5.00% <0.00%> (ø%)
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9716098...257fbdb. Read the comment docs.

Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks!

@trustin trustin changed the title Apply NullAway Fix the errors reported by NullAway Jun 8, 2020
@trustin trustin merged commit 8d59ec2 into line:master Jun 8, 2020
@ikhoon ikhoon deleted the nullaway branch June 22, 2020 05:22
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
Motivation:

Before adopting NullAway(line#1680) or other static analysis tools.
Manually run it and clean up errors.

Modifications:

- Fix all true positives reported by NullAway

Result:

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

Successfully merging this pull request may close these issues.

5 participants