Initial handling of constructor diamond operators#1464
Initial handling of constructor diamond operators#1464
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1464 +/- ##
============================================
- Coverage 88.46% 88.40% -0.06%
- Complexity 2757 2780 +23
============================================
Files 99 99
Lines 9196 9275 +79
Branches 1847 1871 +24
============================================
+ Hits 8135 8200 +65
- Misses 521 524 +3
- Partials 540 551 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
msridhar
changed the title
WalkthroughExtends GenericsChecks with path- and context-aware inference for Java diamond ( Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 1465-1479: If hasInferredClassTypeArguments is true and you detect
the NewClassTree via the current path, avoid re-running diamond inference twice:
when getDiamondTypeFromParentContext(newClassTree, state, parentPath) returns
null, immediately compute and assign the tree's type via
ASTHelpers.getType(withPathToSubtree(state, tree)) (or getTreeType(tree,
withPathToSubtree(state, tree))) to enclosingType instead of falling straight
through; only call getEnclosingTypeForCallExpression(methodSymbol, tree, null,
state, false) if enclosingType is still null. This short-circuits the redundant
re-entry into getDiamondTypeFromParentContext while preserving behavior.
- Around line 610-621: Replace the AtomicReference + forEach pattern in
getFormalParameterTypeForArgument with a simple indexed loop over the
InvocationArguments so you can compare ASTHelpers.stripParentheses(arg) to
argumentTree and return the matching formalParamType immediately; specifically,
construct new InvocationArguments(invocationTree, invocationType), iterate over
its arguments by index (or an iterator if provided), check the stripped argument
against argumentTree, and return the corresponding formalParamType instead of
storing it in an AtomicReference to allow an early return and avoid the extra
allocation.
lazaroclapp
left a comment
lazaroclapp
left a comment
There was a problem hiding this comment.
Did a pass. I am a bit concerned about the performance implications of the traversal in withPathToSubtree elsewhere, either now or when more and more code is using paths that go through that method, but maybe I am over-estimating the cost, I know we have performance measuring...
| } | ||
| void testPositive() { | ||
| // BUG: Diagnostic contains: incompatible types: Foo<@Nullable String> cannot be converted to Foo<String> | ||
| Bar<String> b = new Bar<>(Foo.makeNullableStr()); |
There was a problem hiding this comment.
Just out of curiosity, does:
Bar<Void> b = new Bar<>(Foo.make());
Also report an error?
(In general, that thread about @Nullable Void vs Void still feels like a weird gotcha in the type system regardless of what decision was taken, "nothing or null" is a silly type, but I understand the wish for consistency)
There was a problem hiding this comment.
Yes, this does lead to an error being reported (4babffe). This @Nullable Void point came up again in a recent discussion and I did not sense any appetite to re-open the question :-) We are not yet forcing users to write @Nullable Void everywhere, though probably we should do it at some point soon (in JSpecify mode)
| && !paramTypedTree.getTypeArguments().isEmpty()) { | ||
| return typeWithPreservedAnnotations(paramTypedTree); | ||
| } | ||
| return ASTHelpers.getType(tree); |
There was a problem hiding this comment.
What is this last case? Just new without either generics or a diamond operator, right? Do we need any of the handling we have below for raw types or that's only relevant to invocations that aren't new ...?
There was a problem hiding this comment.
Good point! We had tests for raw constructor invocations that passed without any checking here (probably due to defensive checking for raw types elsewhere). But better to be consistent; fixed in 78501f2 (and added a related helper function)
nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| /** Finds the path to {@code target} within {@code rootPath}, or null when not found. */ | ||
| private static @Nullable TreePath findPathToSubtree(TreePath rootPath, Tree target) { |
There was a problem hiding this comment.
Same worry here. These sort of traversals, as opposed to keeping the current path in state somehow, is one of the reasons Checker Framework was slower than FBInfer/NullAway in the past, no? I guess this is not a traversal from the compilation unit root, but still feels like a bit of a potential smell.
| return; | ||
| } | ||
| Type returnExpressionType = getTreeType(retExpr, state); | ||
| Type returnExpressionType = getTreeType(retExpr, withPathToSubtree(state, retExpr)); |
There was a problem hiding this comment.
Can we maintain these paths manually? Should we? I worry that withPathToSubtree is long term a dangerous method to have from a performance standpoint, but not sure exactly how bad maintaining a separate tree-path for each of these use cases would get.
There was a problem hiding this comment.
@lazaroclapp thanks for pushing me on this. I dug in and rewrote the code so we don't need withPathToSubtree or findPathToSubtree anymore, by maintaining paths manually. In particular, I added a method com.uber.nullaway.NullabilityUtil#stripParensAndUpdateTreePath that both strips parentheses from an expression and also updates a VisitorState to have a path to the nested expression (without the parens).
There might be some unhandled cases, but all the tests pass. In the future we might find another case that needs handling, but we should then augment the manual path construction to do so.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 555-607: getDiamondTypeFromParentContext currently misses
ConditionalExpressionTree parents, so diamond-in-ternary cases fall through to
null; add a handler for parent instanceof ConditionalExpressionTree in
getDiamondTypeFromParentContext that detects whether the NewClassTree `tree` is
the true or false expression, then obtain the type of the opposite arm (use
getTreeType(...) on the other expression or, if the other arm is also a
NewClassTree with a diamond, try to recursively call
getDiamondTypeFromParentContext on that arm) and return that as the inferred
context type; if the opposite arm yields no useful type, fall back to the
existing behavior (return null).
---
Duplicate comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 623-644: The override in findPathToSubtree uses getCurrentPath()
which is declared nullable; make the non-null assumption explicit to avoid
accidental NPEs by wrapping the call with castToNonNull(getCurrentPath()) when
constructing the new TreePath inside the scan override of the anonymous
TreePathScanner; this keeps the existing logic but documents the invariant and
silences nullness warnings in the scan method.
- Around line 596-603: ASTHelpers.getSymbol(parentConstructorCall) can return
null for NewClassTree, so guard the result (parentCtorSymbol) before passing it
to TypeSubstitutionUtils.memberType to avoid an NPE; if parentCtorSymbol is
null, return a sensible fallback (e.g., call getFormalParameterTypeForArgument
with a null-safe/unknown method type or skip substitution and derive the formal
parameter type directly from parentClassType) instead of calling
TypeSubstitutionUtils.memberType, updating the logic around parentCtorSymbol,
parentCtorType, and the call to getFormalParameterTypeForArgument to handle the
null case safely.
- Around line 1635-1649: The diamond-inference path does redundant work: after
calling getDiamondTypeFromParentContext(newClassTree, state, parentPath) if it
returns null, immediately obtain the tree's type once via
ASTHelpers.getType(tree) and assign it to enclosingType (instead of leaving it
null) and then pass that enclosingType into
getEnclosingTypeForCallExpression(methodSymbol, tree, enclosingType, state,
false); this avoids re-entering
getDiamondTypeFromContext/getDiamondTypeFromParentContext a second time. Ensure
you reference NewClassTree newClassTree, enclosingType,
getDiamondTypeFromParentContext, ASTHelpers.getType, and
getEnclosingTypeForCallExpression when making the change.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nullaway/src/test/java/com/uber/nullaway/jspecify/GenericDiamondTests.java`:
- Around line 12-45: The test repeats identical inner class skeletons (Foo<T>,
Bar<T>, and Baz<T>) across methods assignToLocal, returnDiamond, paramPassing,
parenthesizedDiamond, and nestedDiamondConstructors; extract that common class
header into a private static String CLASSES_HEADER in GenericDiamondTests and
replace the duplicated multi-line literal in each makeHelper().addSourceLines
call with CLASSES_HEADER concatenated (or passed as the first source line) so
each test appends only its method-specific code; ensure CLASSES_HEADER contains
the exact inner class definitions for Foo and Bar (and Baz where used) so edits
to those types are made in one place.
- Around line 104-114: Add the missing positive test in GenericDiamondTests for
the paramPassing case: inside the paramPassing.testPositive block add the call
takeStr(new Bar<>(Foo.make())) so the test also checks the Bar<Void> +
Foo.make() scenario and triggers the expected diagnostic "incompatible types:
Foo<@Nullable Void> cannot be converted to Foo<Void>" (mirror the second
positive case present for assignToLocal); ensure the new line is
annotated/placed alongside the existing takeStr(new
Bar<>(Foo.makeNullableStr())) call.
- Around line 187-212: The test diamondSubclassPassedToGenericMethod in
GenericDiamondTests.java lacks a negative (error-expected) case and a TODO
documenting the known limitation; add a commented-out or error-expected variant
(e.g., a static <V> List<V> testNegativeNotYetEnforced(Class<V> cls)) that calls
make(new FooImpl<>(cls)) to show the intended incompatible-types behavior once
diamond-inference for generic methods is fully supported, and precede it with a
TODO comment referencing the limitation and the involved symbols (FooImpl, make,
test) so future work can track enabling the check.
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericDiamondTests.java
Show resolved
Hide resolved
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericDiamondTests.java
Show resolved
Hide resolved
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericDiamondTests.java
Show resolved
Hide resolved
|
Main Branch: With This PR: |
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 573-583: getDiamondTypeFromParentContext currently returns null
for NewClassTree diamonds inside lambdas (expression or block) because the
ReturnTree/LambdaExpressionTree cases only handle enclosing MethodTree; add
TODOs and fix points: in getDiamondTypeFromParentContext and the
ReturnTree/LambdaExpressionTree handling (referencing symbols ReturnTree,
LambdaExpressionTree, NewClassTree, getDiamondTypeFromParentContext, and
NullabilityUtil.findEnclosingMethodOrLambdaOrInitializer) note that if enclosing
context is a lambda rather than a MethodTree we should attempt to derive the
target type from the lambda's target (expression lambda owner or block lambda
enclosing LambdaExpressionTree) and preserve nullability annotations instead of
falling back to javac; add TODO comments where the method currently returns null
indicating to implement extraction of the lambda target type and propagate
nullability, and link to the existing MethodTree branch as the intended
resolution pattern.
- Around line 598-611: The block handling parent instanceof NewClassTree must
null-check the constructor symbol: after obtaining Symbol parentCtorSymbol =
ASTHelpers.getSymbol(parentConstructorCall) guard if (parentCtorSymbol == null)
and return/fallback (e.g., return null) instead of calling
TypeSubstitutionUtils.memberType with a null symbol; specifically add the null
check before invoking TypeSubstitutionUtils.memberType and only call
getFormalParameterTypeForArgument(parentConstructorCall,
parentCtorType.asMethodType(), tree) when parentCtorSymbol is non-null.
---
Duplicate comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 1613-1627: The diamond inference integration in
compareGenericTypeParameterNullabilityForCall (the new handling around
NewClassTree/newClassTree and enclosingType) is correct and requires no code
changes—keep the current logic that checks hasInferredClassTypeArguments, guards
with state.getPath()/ASTHelpers.stripParentheses, calls
getDiamondTypeFromParentContext(newClassTree, state, parentPath) to set
enclosingType, and falls back to getEnclosingTypeForCallExpression(methodSymbol,
tree, null, state, false) when null; proceed to merge as-is.
| if (parent instanceof NewClassTree parentConstructorCall) { | ||
| // get the type returned by the parent constructor call | ||
| Type parentClassType = getTreeType(parentConstructorCall, state.withPath(parentPath)); | ||
| if (parentClassType != null) { | ||
| Symbol parentCtorSymbol = ASTHelpers.getSymbol(parentConstructorCall); | ||
| // get the proper type for the constructor, as a member of the type returned by the | ||
| // constructor | ||
| Type parentCtorType = | ||
| TypeSubstitutionUtils.memberType( | ||
| state.getTypes(), parentClassType, parentCtorSymbol, config); | ||
| return getFormalParameterTypeForArgument( | ||
| parentConstructorCall, parentCtorType.asMethodType(), tree); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ASTHelpers.getSymbol usage with NewClassTree in the codebase
rg -n --type=java 'ASTHelpers\.getSymbol\(' nullaway/src/main/java/ | head -50Repository: uber/NullAway
Length of output: 7770
🏁 Script executed:
#!/bin/bash
# Check the specific file and surrounding context for how getSymbol is used with NewClassTree
# Also look for other NewClassTree getSymbol usages
rg -n --type=java -B2 -A2 'NewClassTree' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java | grep -A5 -B5 'getSymbol'Repository: uber/NullAway
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Look at the exact context in the file at lines 598-611 and check for null checks
sed -n '590,620p' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javaRepository: uber/NullAway
Length of output: 1324
🏁 Script executed:
#!/bin/bash
# Check TypeSubstitutionUtils.memberType implementation to see if it handles null
rg -n --type=java 'public.*memberType' nullaway/src/main/java/com/uber/nullaway/Repository: uber/NullAway
Length of output: 218
🏁 Script executed:
#!/bin/bash
# Search for TypeSubstitutionUtils.memberType to understand its signature
rg -n --type=java -A10 'static.*memberType' nullaway/src/main/java/com/uber/nullaway/Repository: uber/NullAway
Length of output: 1493
🏁 Script executed:
#!/bin/bash
# Check if there are other places in the codebase where getSymbol(NewClassTree) is used
# and how they handle potential null
rg -n --type=java -B3 -A3 'NewClassTree' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java | head -100Repository: uber/NullAway
Length of output: 4962
🏁 Script executed:
#!/bin/bash
# Check the implementation details and null handling in TypeSubstitutionUtils.memberType
cat -n nullaway/src/main/java/com/uber/nullaway/generics/TypeSubstitutionUtils.java | sed -n '67,77p'Repository: uber/NullAway
Length of output: 690
🏁 Script executed:
#!/bin/bash
# Look for any defensive null checks after getSymbol(NewClassTree) in the codebase
rg -n --type=java -B2 -A5 'NewClassTree.*=' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java | grep -A5 'getSymbol'Repository: uber/NullAway
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check if there are other instances where getSymbol result from NewClassTree is used
# and whether null checks are applied
rg -n --type=java 'instanceof NewClassTree' nullaway/src/main/java/ -A10 | grep -A10 'getSymbol'Repository: uber/NullAway
Length of output: 1358
🏁 Script executed:
#!/bin/bash
# Check Error Prone's ASTHelpers documentation or implementation for getSymbol behavior
# First, let's see if there's any documentation in NullAway about this
rg -n --type=java 'getSymbol.*NewClass' nullaway/src/main/java/ -B5 -A10Repository: uber/NullAway
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check if there are any null checks or error handling after getSymbol(NewClassTree)
# in the entire codebase
rg -n --type=java 'getSymbol\(' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java | grep -A2 -B2 '602'Repository: uber/NullAway
Length of output: 436
🏁 Script executed:
#!/bin/bash
# Look at how getSymbol is typically used after being called
# Check patterns of null checking around line 522 where memberSelectSymbol is retrieved
sed -n '515,530p' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javaRepository: uber/NullAway
Length of output: 925
🏁 Script executed:
#!/bin/bash
# Let's see the context around line 522 more clearly to confirm null checking pattern
sed -n '520,530p' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javaRepository: uber/NullAway
Length of output: 639
🏁 Script executed:
#!/bin/bash
# Look at how other similar cases handle getSymbol - check line 502 (assignmentTree)
sed -n '500,510p' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javaRepository: uber/NullAway
Length of output: 542
🏁 Script executed:
#!/bin/bash
# Check if there are Error Prone docs or other references about getSymbol nullability
# Let's look for any Nullable annotations on the method
rg -n --type=java '@Nullable.*getSymbol|getSymbol.*@Nullable' nullaway/src/Repository: uber/NullAway
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Final confirmation: check if there are any explicit null-safety guarantees in Error Prone
# by looking at how other overloads of getSymbol are documented/used in NullAway
rg -n --type=java 'ASTHelpers\.getSymbol\(' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java | head -20Repository: uber/NullAway
Length of output: 1665
🏁 Script executed:
#!/bin/bash
# Check the broader pattern: do all other Symbol assignments from getSymbol have null checks?
rg -n --type=java 'Symbol.*=.*ASTHelpers\.getSymbol' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java | wc -lRepository: uber/NullAway
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Count how many of these have immediate null checks following them
# Let's look at a few more examples to establish the pattern
sed -n '645,680p' nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.javaRepository: uber/NullAway
Length of output: 1860
Add null guard for parentCtorSymbol to prevent potential NPE.
ASTHelpers.getSymbol(parentConstructorCall) can return null. The codebase pattern shows null checks for getSymbol() results on MemberSelectTree (line 522) and AssignmentTree (line 502). If parentCtorSymbol is null, TypeSubstitutionUtils.memberType() at line 606 will dereference sym.type and throw an NPE.
🛡️ Proposed null guard
if (parent instanceof NewClassTree parentConstructorCall) {
// get the type returned by the parent constructor call
Type parentClassType = getTreeType(parentConstructorCall, state.withPath(parentPath));
if (parentClassType != null) {
Symbol parentCtorSymbol = ASTHelpers.getSymbol(parentConstructorCall);
+ if (parentCtorSymbol == null) {
+ return null;
+ }
// get the proper type for the constructor, as a member of the type returned by the
// constructor
Type parentCtorType =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java` around
lines 598 - 611, The block handling parent instanceof NewClassTree must
null-check the constructor symbol: after obtaining Symbol parentCtorSymbol =
ASTHelpers.getSymbol(parentConstructorCall) guard if (parentCtorSymbol == null)
and return/fallback (e.g., return null) instead of calling
TypeSubstitutionUtils.memberType with a null symbol; specifically add the null
check before invoking TypeSubstitutionUtils.memberType and only call
getFormalParameterTypeForArgument(parentConstructorCall,
parentCtorType.asMethodType(), tree) when parentCtorSymbol is non-null.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java`:
- Around line 598-611: The code calls
ASTHelpers.getSymbol(parentConstructorCall) which can return null; add a null
guard after obtaining parentCtorSymbol and bail out (e.g., return null or
continue) before calling TypeSubstitutionUtils.memberType to avoid an NPE.
Specifically, in the block handling parent instanceof NewClassTree, check if
parentCtorSymbol is null and handle it (returning null from this method or
skipping the substitution and subsequent getFormalParameterTypeForArgument call)
so TypeSubstitutionUtils.memberType and parentCtorSymbol.type are never
dereferenced when parentCtorSymbol is null.
|
Main Branch: With This PR: |
2 participants
Fixes #1451
This adds partial support for diamond operators, sufficient to fix #1451. We still need to fully support diamonds in generic method inference.
Summary by CodeRabbit
New Features
Enhancements
Tests