More generics inference fixes#1255
More generics inference fixes#1255msridhar merged 21 commits intogenerics-type-constraint-solverfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## generics-type-constraint-solver #1255 +/- ##
==================================================================
Coverage 88.18% 88.19%
- Complexity 2417 2424 +7
==================================================================
Files 92 92
Lines 7981 8002 +21
Branches 1587 1593 +6
==================================================================
+ Hits 7038 7057 +19
Misses 491 491
- Partials 452 454 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lazaroclapp
left a comment
There was a problem hiding this comment.
Very minor comments below. Would personally prefer a clearer title for the PR, but I know this will land squashed with the previous PR, so it's less of an issue :)
nullaway/src/main/java/com/uber/nullaway/generics/ConstraintSolverImpl.java
Show resolved
Hide resolved
| @@ -435,6 +436,8 @@ public void checkTypeParameterNullnessForAssignability(Tree tree, VisitorState s | |||
| } else { | |||
There was a problem hiding this comment.
Maybe an else if (tree instanceof AssignmentTree) { with a error handling } else { branch? Or some precondition check? I know the contract is the function takes only those two types, but just in terms of defensive programming.
An argument type like AssignmentTree | VariableTree tree would be great, if Java supported that 😅
There was a problem hiding this comment.
Sure, I can do an else if here to give a cleaner error message if something weird happens 🙂
| " }", | ||
| "}") | ||
| .doTest(); | ||
| } |
There was a problem hiding this comment.
Test case might benefit from a quick explanation of what's being tested, since no reports are expected. From looking at the test alone, the answer is that we are testing constraint resolution when method invocations are passed as parameters to other methods with some nested type variable on the signature of both methods. But given the PR description, it is a bit more specific than that, no? We are making sure we avoid conflating T and @Nullable T, right?
There was a problem hiding this comment.
Yeah, I'll add a comment. This code was reduced from some code in Spring that was giving weird inference failures, and yes, it was due to not properly distinguishing T and @Nullable T
| " result = make();", | ||
| " }", | ||
| "}") | ||
| .doTest(); |
There was a problem hiding this comment.
Not that it needs docs or anything, but I am curious here... did this fail before this PR in some way?
There was a problem hiding this comment.
It did fail! Before, we didn't properly mark when AssignmentTrees like result = make() were assigning to local variables (we only handled initializers in VariableTrees). So, when inference ran, it thought that the result variable had to be @NonNull, which then caused inference to fail for the make() due to the constraint @Nullable U <: @NonNull Object. Now, we properly detect this is an assignment to a local variable, so we do not generate that constraint (since types of locals are inferred). I'll add a comment on this one too.
Three fixes:
TypeVariableobjects representing the same underlying type variable due to differences in annotations (e.g.,Tvs@Nullable T). Switch our maps key'd onTypeVariableto use the underlyingElementinstead, which should uniquely identify the type variable.@Nullable Tas if they were the same as justT; this was wrong. Now, only treatTypeVariableobjects with no nullness annotations as actual type variables whose nullability must be inferred.AssignmentTreewas assigning to a local variable.For testing, we also add a new config flag
WarnOnGenericInferenceFailureto allow for a warning to be emitted when inference fails for a generic method call. This way, we can test the expectation that generic inference fails for certain calls. This is off by default; I'm still unsure how best to expose these inference-related errors to the user.