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

Add aliases to chromium nullity annotations #6977

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johnkrah
Copy link

[Problem]
Chromium has implemented copies/extensions of Nullaway annotations which is not
yet recognized by Checker Framework.
See also: https://chromium-review.googlesource.com/c/chromium/src/+/6065417

[Solution]
This change adds aliases of chromium's new non-null and nullable
annotations to Checker Framework.

[Testing]
Building java classes in a derivative module using chromium APIs
correctly exits with no warning when passing a null value to a method
parameter annotated org.chromium.build.annotations.Nullable.

For example, these warnings were emitted after chromium added new
annotation https://source.chromium.org/chromium/chromium/src/+/main:ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java;l=325-326

import org.chromium.build.annotations.Nullable;
...
            @Nullable Float xdpi,
            @Nullable Float ydpi,

and are no longer emitted after this pull request applied to checker.

XyzActivity.java:0: warning: [argument] incompatible argument for parameter xdpi of DisplayAndroid.update.
                null, // Float xdpi
                ^
  found   : null (NullType)
  required: @Initialized @NonNull Float
XyzActivity.java:0: warning: [argument] incompatible argument for parameter ydpi of DisplayAndroid.update.
                null, // Float ydpi
                ^
  found   : null (NullType)
  required: @Initialized @NonNull Float

Added clean-room local re-implementation of the classes, following
recent example given by mernst in 3d01348

Copy link
Member

@mernst mernst left a comment

Choose a reason for hiding this comment

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

Thanks!

I have two comments:

  • It would be nice to include all of the org.chromium annotations in one pull request, so that we don't overlook any of them.

  • It looks to me like one of the annotations already in this pull request is being aliased incorrectly, unless I am mistaken.

@johnkrah
Copy link
Author

the main chromium nullness annotations mapped to checker aliases:

  • Nullable : synonymous
  • OptimizeAsNonNull --> NonNull
  • EnsuresNonNull : synonymous
  • EnsuresNonNullIf : synonymous
  • RequiresNonNull : synonymous

some additional chromium annotations that may be of interest to nullness checker. I noticed checker has an alias list for PolyNull as well but I don't think any of these align to that proof. and I didn't find an equivalent for one of my favorite proofs MonotonicNonNull either.

  • Contract : returns false if null, true if non-null i.e. evaluates param != null
  • NullMarked : means this class is in scope for nullness checker
  • NullUnmarked : means this class is out of scope for nullness checker

the remaining new chromium annotations are probably not nullness related:

  • AlwaysInline
  • CheckDiscard
  • DoNotClassMerge
  • DoNotInline
  • DoNotStripLogs
  • IdentifierNameString
  • Initializer
  • MockedInTests
  • ServiceImpl
  • UsedByReflection
  • UsedReflectively

@johnkrah
Copy link
Author

johnkrah commented Feb 13, 2025

I see there are some tests failing now, taking a look and will update.

I had tried adding aliases for chromium's versions of EnsuresNonNull, EnsuresNonNullIf, RequiresNonNull along with copying their elements, but that seemed to break the purity of checker's EnsuresNonNull annotation itself, so I backed that out for now at least.

@johnkrah
Copy link
Author

hi @mernst checking in for any further comments or questions, thanks for taking a look at this pull req, cheers!

[Problem]
Chromium has implemented copies/extensions of Nullaway annotations which is not
yet recognized by Checker Framework.
See also: https://chromium-review.googlesource.com/c/chromium/src/+/6065417

[Solution]
This change adds aliases of chromium's nullable, non-null,
ensures/requires annotations to Checker Framework.

[Testing]
Building java classes in a derivative module using chromium APIs
correctly exits with no warning when passing a null value to a method
parameter annotated org.chromium.build.annotations.Nullable.

For example, these warnings were emitted after chromium added new
annotation https://source.chromium.org/chromium/chromium/src/+/main:ui/android/java/src/org/chromium/ui/display/DisplayAndroid.java;l=325-326

```
import org.chromium.build.annotations.Nullable;
...
            @nullable Float xdpi,
            @nullable Float ydpi,
```

and are no longer emitted after this pull request applied to checker.

```
XyzActivity.java:0: warning: [argument] incompatible argument for parameter xdpi of DisplayAndroid.update.
                null, // Float xdpi
                ^
  found   : null (NullType)
  required: @initialized @nonnull Float
XyzActivity.java:0: warning: [argument] incompatible argument for parameter ydpi of DisplayAndroid.update.
                null, // Float ydpi
                ^
  found   : null (NullType)
  required: @initialized @nonnull Float
```

Added clean-room local re-implementation of the classes, following
recent example given by mernst in typetools@3d01348
@johnkrah
Copy link
Author

updating pr with rebase on the the recent commit 618b9a0

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

Successfully merging this pull request may close these issues.

2 participants