Enable inference failure warnings in experimental mode, and fix related bug in library models - #1649
Conversation
|
This change is part of the following stack:
Change managed by git-spice. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1649 +/- ##
============================================
- Coverage 88.06% 88.04% -0.02%
- Complexity 3070 3074 +4
============================================
Files 105 105
Lines 10342 10351 +9
Branches 2091 2095 +4
============================================
+ Hits 9108 9114 +6
- Misses 581 582 +1
- Partials 653 655 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a9e1661 to
2191a1c
Compare
bc10772 to
035a576
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughJSpecify mode now enables the experimental compiler option and inference-failure warnings by default. Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2191a1c to
faaed55
Compare
035a576 to
5aa2bd9
Compare
faaed55 to
4276a87
Compare
5aa2bd9 to
ad563fd
Compare
4276a87 to
e39330a
Compare
6864b90 to
a7b561f
Compare
a7b561f to
c2ca171
Compare
msridhar
changed the title
msridhar
changed the title
lazaroclapp
left a comment
lazaroclapp
left a comment
There was a problem hiding this comment.
A few questions below:
| } | ||
|
|
||
| /** Updates method types based on nested annotation information from library models. */ | ||
| /** Updates method types based on top-level parameter and nested annotation library models. */ |
There was a problem hiding this comment.
Wonder if expanding this comment to note how this is specifically for matching type parameters / generics eventually is worth it. I was a bit confused about why we would discard explicitlyNullableParameters when that is not necessarily related to JSpecify / handling type parameters. But, it turns out this handler method is only ever used inside nullaway/src/main/java/com/uber/nullaway/generics/*, because that's the only place where we care if the method type was overriden.
Actually, given that, do we want to have this check for isJSpecifyMode() or just rely on the callers to know why they are asking about the method type? Am I missing something that happens without the config.isJSpecifyMode() check?
There was a problem hiding this comment.
I'm happy to update the comment to indicate that this is currently only relevant for generics in JSpecify mode, and to check for isJSpecifyMode() inside the body and fail if it's off.
In the (distant?) future where JSpecify mode is the default, and maybe we don't want to support anything else, there is a lot of code that could be cleaned up...
| if (explicitlyNullableParameters.contains(index)) { | ||
| updatedArgType = applyTopLevelNullableAnnotation(updatedArgType, state); | ||
| } | ||
| if (!annotationsForArg.isEmpty()) { |
There was a problem hiding this comment.
If we don't guard on config.isJSpecifyMode() for the explicitlyNullableParameters case, and we are considering potential future callers outside com.uber.nullaway.generics, then we probably want to check that config flag here instead.
There was a problem hiding this comment.
I've now asserted at the top of this method that we're in JSpecify mode
| */ | ||
| public record NestedAnnotationInfo(Annotation annotation, ImmutableList<TypePathEntry> typePath) { | ||
|
|
||
| public NestedAnnotationInfo { |
There was a problem hiding this comment.
Remind me why we have the duplication between this and library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/NestedAnnotationInfo.java ...
There was a problem hiding this comment.
Yeah 😐 It's to avoid a dependence from the library model generation code on NullAway and vice versa. I didn't want NullAway to depend on library model code via a separate artifact that needs to be shipped to Maven Central, etc. Making the lib model generation code depend on NullAway led to all kinds of Gradle unhappiness, if I recall correctly.
Anyway, I've forgotten all the details but we should fix this, and I've opened #1657 on that
There was a problem hiding this comment.
Ok @lazaroclapp FYI this is fixed in #1659
2 participants
See #1551
For now, we only enable inference failure warnings in experimental mode, so we can further fix bugs before enabling by default.
We also enable these warnings for all JSpecify mode tests (and remove test helper methods that enabled them explicitly) and for building NullAway itself. This exposed a bug in our use of library models: in
LibraryModelsHandler.onOverrideMethodType, we were not adding top-level@Nullableannotations on parameters captured by theLibraryModels.explicitlyNullableParameters()method. We fix that issue here, and also disallow representation of such annotations as a nested annotation with an empty path, to avoid confusion and redundancy. Eventually we may want to settle on a single mechanism for expressing top-level or nested parameter annotations, but that is for a future cleanup / PR.The PR may look large, but most changes relate to renaming
makeHelperWithInferenceFailureWarningto justmakeHelper.Summary by CodeRabbit
Summary by CodeRabbit
New Features
@Nullableto method parameter types using library models.Bug Fixes
Tests