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.

Sign up for GitHub

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

Jump to bottom

Consider adopting NullAway into the build process #1680

Closed
trustin opened this issue Mar 26, 2019 · 7 comments · Fixed by #5820 or #5830
Closed

Consider adopting NullAway into the build process #1680

trustin opened this issue Mar 26, 2019 · 7 comments · Fixed by #5820 or #5830
Labels
cleanup

Comments

@trustin
Copy link
Member

trustin commented Mar 26, 2019

See: https://github.com/uber/NullAway
@trustin trustin mentioned this issue Apr 8, 2019
Closed
43 tasks
@trustin trustin mentioned this issue Jun 2, 2020
Open
@anuraaga
Copy link
Collaborator

anuraaga commented Jun 2, 2020

Ah yeah now I remember this issue. Seems NullAway is a faster but potentially less precise checker good enough for almost all cases. But can't find any good resource listing out what cases it misses vs checker framework. Do you have any idea?

@trustin
Copy link
Member Author

trustin commented Jun 2, 2020

No. 😄 I didn't take a close look at all. IIRC JetBrains had some contract annotations, too. Not sure which is the best 🤔

@ikhoon
Copy link
Contributor

ikhoon commented Jun 4, 2020 •
edited
Loading

I tried to test and compare NullAway and the Checker Framework.

The Checker Framework - It does not support Java 13 yet.

> org.checkerframework.javacutil.UserError: 
The Checker Framework cannot be run with JDK 13+.  You are using version 13. Please use JDK 8 or JDK 11.

So I don't think we need to go any further now.

NullAway

It produces some useful error messages. But it could not cover all of our null checking code.
For example, it does not recognize null checking in checkArgument(). uber/NullAway#47

armeria/core/src/main/java/com/linecorp/armeria/common/SessionProtocol.java

Lines 87 to 88 in bd28eb2

checkArgument(value != null, "unknown session protocol: ", uriText);
return value; 

SerializationFormat.java:131: error: [NullAway] returning @Nullable expression from method with @NonNull return type

I am not sure that we can put NullAway into the build process.
Anyway, I am going to make a PR to fix True Positive errors detected by NullAway. 😀

@anuraaga
Copy link
Collaborator

anuraaga commented Jun 5, 2020

@ikhoon Since checker framework also can't be run together with error prone from what I understand, we'd have two builds anyways, and I guess with some configuration tweaks that could be set to Java 11. Probably not worth the trouble though, but I'm curious whether checker framework would catch checkArgument. Do you have a branch I can play with (or you can try playing with Java 11? :D)

@ikhoon
Copy link
Contributor

ikhoon commented Jun 5, 2020 •
edited
Loading

Since checker framework also can't be run together with error prone from what I understand,

It looks resolved in Error Prone 2.4.0
https://github.com/kelloggm/checkerframework-gradle-plugin#incompatibility-with-error-prone

Do you have a branch I can play with (or you can try playing with Java 11? :D)

Yeah, let me push the branch tested locally.

@ikhoon
Copy link
Contributor

ikhoon commented Jun 5, 2020

@anuraaga This is https://github.com/ikhoon/armeria/tree/nullness 😀

ikhoon added a commit to ikhoon/armeria that referenced this issue Jun 5, 2020
@ikhoon
Apply NullAway
e029800 
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
@ikhoon ikhoon mentioned this issue Jun 5, 2020
Merged
trustin pushed a commit that referenced this issue Jun 8, 2020
@ikhoon
Fix the errors reported by NullAway (#2776)
8d59ec2 
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
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this issue Sep 19, 2020
@ikhoon
Fix the errors reported by NullAway (line#2776)
57f3647 
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
trustin added a commit to trustin/armeria that referenced this issue Jul 22, 2024
@trustin
Enable NullAway for :core
1922f90 
Motivation:

It would be nice if we can fail our build if there is any potential
`null` dereference, so our users have much less chance of getting
`NullPointerException`.

Modifications:

- Updated the build so that NullAway plugin is enabled for `:core`
  - Note that the root `build.gradle` was modified so that we can
    enable NullAway for other projects in the future.
  - Other caveats:
    - NullAway is disabled for MR-JAR sources because it seems to make
      the build fail with unrelated errors (e.g. no symbol)
- Added `assert` sentences and `@Nullable` annotations wherever
  applicable
- Fixed a few minor potential `null` dereferences

Result:

- We're more confident about our `@Nullable` usages in our API.
- Theoretically, there should be no chance of `NullPointerException` at
  least in the `:core`.
  - However, note that some `NullPointerException`s might have become
    `AssertionError`s, since we added a bunch of assertions in this PR,
    so please review carefully :-)
- Partially resolves line#1680 and line#5184
@trustin trustin mentioned this issue Jul 22, 2024
Merged
@ikhoon ikhoon closed this as completed in 2367de2 Jul 25, 2024
trustin added a commit to trustin/armeria that referenced this issue Jul 25, 2024
@trustin
Enable NullAway for all non-test sources
d564251 
Motivation:

This is a follow-up PR for line#5820

It would be nice if we can fail our build if there is any potential
`null` dereference, so our users have much less chance of getting
`NullPointerException`.

Modifications:

- Enabled NullAway for all projects
- Added `assert` sentences and `@Nullable` annotations wherever
  applicable
- Fixed a few minor potential `null` dereferences

Result:

- Theoretically, there should be no chance of `NullPointerException`
  anymore.
  - However, note that some NullPointerExceptions might have become
    `AssertionError`s, since we added a bunch of assertions in this PR,
    so please review carefully :-)
- Closes line#1680
- Closes line#5184
@trustin trustin mentioned this issue Jul 25, 2024
Merged
@trustin
Copy link
Member Author

trustin commented Jul 26, 2024

#5830 needs to be merged to close this.

@trustin trustin reopened this Jul 26, 2024
@ikhoon ikhoon closed this as completed in 0960d09 Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
cleanup
Projects
None yet
Milestone
No milestone
3 participants
@trustin @anuraaga @ikhoon