Initial handling of constructor diamond operators #1464
+426
−29
WIP
- Loading branch information
commit 03ce93165d762451cc43fee76a7610ac4e86cf89
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |
| import com.sun.source.tree.Tree; | ||
| import com.sun.source.tree.VariableTree; | ||
| import com.sun.source.util.TreePath; | ||
| import com.sun.source.util.TreePathScanner; | ||
| import com.sun.source.util.TreeScanner; | ||
| import com.sun.tools.javac.code.Attribute; | ||
| import com.sun.tools.javac.code.Symbol; | ||
|
|
@@ -36,6 +37,7 @@ | |
| import com.sun.tools.javac.code.Type; | ||
| import com.sun.tools.javac.code.Types; | ||
| import com.sun.tools.javac.tree.JCTree; | ||
| import com.sun.tools.javac.tree.TreeInfo; | ||
| import com.sun.tools.javac.util.Name; | ||
| import com.sun.tools.javac.util.Names; | ||
| import com.uber.nullaway.CodeAnnotationInfo; | ||
|
|
@@ -434,14 +436,27 @@ private void reportInvalidOverridingMethodParamTypeError( | |
| } | ||
| return result; | ||
| } | ||
| if (tree instanceof NewClassTree newClassTree) { | ||
| if (newClassTree.getClassBody() != null | ||
| && newClassTree.getIdentifier() instanceof JCTree idTree | ||
| && TreeInfo.isDiamond(idTree)) { | ||
| // Keep existing behavior for diamond anonymous classes, which are not yet fully supported. | ||
| return null; | ||
| } | ||
| if (newClassTree.getIdentifier() instanceof ParameterizedTypeTree paramTypedTree | ||
| && !(newClassTree.getIdentifier() instanceof JCTree idTree && TreeInfo.isDiamond(idTree)) | ||
| && !paramTypedTree.getTypeArguments().isEmpty()) { | ||
| return typeWithPreservedAnnotations(paramTypedTree); | ||
| } | ||
| if (hasInferredClassTypeArguments(newClassTree)) { | ||
| // For constructor calls with inferred class type arguments, infer from assignment context | ||
| // when possible so we preserve explicit nullability annotations from the target type. | ||
| Type fromAssignmentContext = getDiamondTypeFromContext(newClassTree, state); | ||
| if (fromAssignmentContext != null) { | ||
| return fromAssignmentContext; | ||
| } | ||
| } | ||
| return ASTHelpers.getType(tree); | ||
| } else if (tree instanceof NewArrayTree | ||
| && ((NewArrayTree) tree).getType() instanceof AnnotatedTypeTree) { | ||
| return typeWithPreservedAnnotations(tree); | ||
|
|
@@ -522,6 +537,144 @@ private void reportInvalidOverridingMethodParamTypeError( | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Gets the type of a constructor call using a diamond operator from its assignment context, if | ||
| * available. | ||
| */ | ||
| private @Nullable Type getDiamondTypeFromContext(NewClassTree tree, VisitorState state) { | ||
| Type fromCurrentPathContext = getDiamondTypeFromCurrentPath(tree, state); | ||
| if (fromCurrentPathContext != null) { | ||
| return fromCurrentPathContext; | ||
| } | ||
| TreePath treePath = findPathToSubtree(state.getPath(), tree); | ||
| if (treePath == null) { | ||
| return null; | ||
| } | ||
| TreePath parentPath = treePath.getParentPath(); | ||
| if (parentPath == null) { | ||
| return null; | ||
| } | ||
| return getDiamondTypeFromParentContext(tree, state, parentPath); | ||
| } | ||
|
|
||
| private @Nullable Type getDiamondTypeFromCurrentPath(NewClassTree tree, VisitorState state) { | ||
| TreePath currentPath = state.getPath(); | ||
| if (currentPath == null) { | ||
| return null; | ||
| } | ||
| if (currentPath.getLeaf() == tree) { | ||
| return null; | ||
| } | ||
| return getDiamondTypeFromParentContext(tree, state, currentPath); | ||
| } | ||
|
|
||
| private @Nullable Type getDiamondTypeFromParentContext( | ||
| NewClassTree tree, VisitorState state, TreePath parentPath) { | ||
| Tree parent = parentPath.getLeaf(); | ||
| while (parent instanceof ParenthesizedTree) { | ||
| parentPath = parentPath.getParentPath(); | ||
| if (parentPath == null) { | ||
| return null; | ||
| } | ||
| parent = parentPath.getLeaf(); | ||
| } | ||
| if (parent instanceof VariableTree variableTree) { | ||
| Tree declaredTypeTree = variableTree.getType(); | ||
| return declaredTypeTree == null | ||
| ? getTreeType(parent, state) | ||
| : typeWithPreservedAnnotations(declaredTypeTree); | ||
| } | ||
| if (parent instanceof AssignmentTree assignmentTree) { | ||
| return getTreeType(assignmentTree.getVariable(), state); | ||
| } | ||
| if (parent instanceof ReturnTree) { | ||
| TreePath enclosingMethodOrLambda = | ||
| NullabilityUtil.findEnclosingMethodOrLambdaOrInitializer(parentPath); | ||
| if (enclosingMethodOrLambda != null | ||
| && enclosingMethodOrLambda.getLeaf() instanceof MethodTree methodTree) { | ||
| Tree returnTypeTree = methodTree.getReturnType(); | ||
| if (returnTypeTree != null) { | ||
| return typeWithPreservedAnnotations(returnTypeTree); | ||
| } | ||
| } | ||
| return null; | ||
|
msridhar marked this conversation as resolved.
|
||
| } | ||
| if (parent instanceof MethodInvocationTree parentInvocation) { | ||
| Type methodType = ASTHelpers.getType(parentInvocation.getMethodSelect()); | ||
| if (methodType == null) { | ||
| return null; | ||
| } | ||
| AtomicReference<@Nullable Type> formalParamTypeRef = new AtomicReference<>(); | ||
| new InvocationArguments(parentInvocation, methodType.asMethodType()) | ||
| .forEach( | ||
| (arg, pos, formalParamType, unused) -> { | ||
| if (ASTHelpers.stripParentheses(arg) == tree) { | ||
| formalParamTypeRef.set(formalParamType); | ||
| } | ||
| }); | ||
| return formalParamTypeRef.get(); | ||
| } | ||
| if (parent instanceof NewClassTree parentConstructorCall) { | ||
| Type parentCtorType = ASTHelpers.getType(parentConstructorCall.getIdentifier()); | ||
| if (parentCtorType == null) { | ||
| return getTargetTypeForDiamond(state, parentPath); | ||
| } | ||
| AtomicReference<@Nullable Type> formalParamTypeRef = new AtomicReference<>(); | ||
| new InvocationArguments(parentConstructorCall, parentCtorType.asMethodType()) | ||
| .forEach( | ||
| (arg, pos, formalParamType, unused) -> { | ||
| if (ASTHelpers.stripParentheses(arg) == tree) { | ||
| formalParamTypeRef.set(formalParamType); | ||
| } | ||
| }); | ||
| return formalParamTypeRef.get(); | ||
| } | ||
|
Comment on lines
+598
to
+611
Contributor
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
🛡️ 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 |
||
| return getTargetTypeForDiamond(state, parentPath); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| private static @Nullable Type getTargetTypeForDiamond(VisitorState state, TreePath treePath) { | ||
| com.google.errorprone.util.TargetType targetType = | ||
| com.google.errorprone.util.TargetType.targetType(state.withPath(treePath)); | ||
| return targetType == null ? null : targetType.type(); | ||
| } | ||
|
msridhar marked this conversation as resolved.
|
||
|
|
||
| private static @Nullable TreePath findPathToSubtree(@Nullable TreePath rootPath, Tree target) { | ||
| if (rootPath == null) { | ||
| return null; | ||
| } | ||
| return new TreePathScanner<@Nullable TreePath, Object>() { | ||
| @Override | ||
| public @Nullable TreePath scan(Tree tree, @Nullable Object unused) { | ||
| if (tree == target) { | ||
| return getCurrentPath(); | ||
| } | ||
| return super.scan(tree, null); | ||
| } | ||
| }.scan(rootPath, null); | ||
| } | ||
|
|
||
| private static VisitorState withPathToSubtree(VisitorState state, Tree subtree) { | ||
| TreePath subtreePath = findPathToSubtree(state.getPath(), subtree); | ||
| return subtreePath == null ? state : state.withPath(subtreePath); | ||
| } | ||
|
|
||
| private static boolean hasInferredClassTypeArguments(NewClassTree newClassTree) { | ||
| if (newClassTree.getClassBody() != null) { | ||
| // For anonymous classes, javac does not preserve all nullability details for the inferred | ||
| // type arguments. Keep legacy behavior for now. | ||
| return false; | ||
| } | ||
| if (newClassTree.getIdentifier() instanceof ParameterizedTypeTree paramTypedTree | ||
| && newClassTree.getIdentifier() instanceof JCTree idTree | ||
| && !TreeInfo.isDiamond(idTree) | ||
| && !paramTypedTree.getTypeArguments().isEmpty()) { | ||
| // explicit class type arguments in source | ||
| return false; | ||
| } | ||
| Type newClassType = ASTHelpers.getType(newClassTree); | ||
| return newClassType != null && !newClassType.getTypeArguments().isEmpty(); | ||
| } | ||
|
|
||
| /** | ||
| * Gets the inferred type of lambda parameter, if the lambda was passed to a generic method and | ||
| * its type was inferred previously | ||
|
|
@@ -606,7 +759,7 @@ public void checkTypeParameterNullnessForAssignability(Tree tree, VisitorState s | |
| && isAssignmentToField(tree)) { | ||
| maybeStoreLambdaTypeFromTarget(lambdaExpressionTree, lhsType); | ||
| } | ||
| Type rhsType = getTreeType(rhsTree, withPathToSubtree(state, rhsTree)); | ||
| if (rhsType != null) { | ||
| if (isGenericCallNeedingInference(rhsTree)) { | ||
| rhsType = | ||
|
|
@@ -1154,7 +1307,7 @@ public void checkTypeParameterNullnessForFunctionReturnType( | |
| // bail out of any checking involving raw types for now | ||
| return; | ||
| } | ||
| Type returnExpressionType = getTreeType(retExpr, withPathToSubtree(state, retExpr)); | ||
|
Collaborator
There was a problem hiding this comment. Can we maintain these paths manually? Should we? I worry that
Collaborator
Author
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 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. |
||
| if (returnExpressionType != null) { | ||
| if (isGenericCallNeedingInference(retExpr)) { | ||
| returnExpressionType = | ||
|
|
@@ -1304,7 +1457,21 @@ public void compareGenericTypeParameterNullabilityForCall( | |
| return; | ||
| } | ||
| Type invokedMethodType = methodSymbol.type; | ||
| Type enclosingType = null; | ||
| if (tree instanceof NewClassTree newClassTree) { | ||
| if (hasInferredClassTypeArguments(newClassTree)) { | ||
| TreePath currentPath = state.getPath(); | ||
| if (currentPath != null && ASTHelpers.stripParentheses(currentPath.getLeaf()) == tree) { | ||
| TreePath parentPath = currentPath.getParentPath(); | ||
| if (parentPath != null) { | ||
| enclosingType = getDiamondTypeFromParentContext(newClassTree, state, parentPath); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if (enclosingType == null) { | ||
| enclosingType = getEnclosingTypeForCallExpression(methodSymbol, tree, null, state, false); | ||
| } | ||
|
msridhar marked this conversation as resolved.
|
||
| if (enclosingType != null) { | ||
| invokedMethodType = | ||
| TypeSubstitutionUtils.memberType(state.getTypes(), enclosingType, methodSymbol, config); | ||
|
|
@@ -1335,7 +1502,8 @@ public void compareGenericTypeParameterNullabilityForCall( | |
| if (inferredPolyType != null) { | ||
| actualParameterType = inferredPolyType; | ||
| } else { | ||
| actualParameterType = | ||
| getTreeType(currentActualParam, withPathToSubtree(state, currentActualParam)); | ||
| } | ||
| if (actualParameterType != null) { | ||
| if (isGenericCallNeedingInference(currentActualParam)) { | ||
|
|
@@ -1840,14 +2008,25 @@ public Nullness getGenericParameterNullnessAtInvocation( | |
| false, | ||
| calledFromDataflow); | ||
| } else { | ||
| enclosingType = getTreeType(receiver, withPathToSubtree(state, receiver)); | ||
| } | ||
| } | ||
| } else { | ||
| Verify.verify(tree instanceof NewClassTree); | ||
| NewClassTree newClassTree = (NewClassTree) tree; | ||
| if (hasInferredClassTypeArguments(newClassTree)) { | ||
| Type typeFromAssignmentContext = | ||
| getDiamondTypeFromContext(newClassTree, withPathToSubtree(state, tree)); | ||
| if (typeFromAssignmentContext != null) { | ||
| return typeFromAssignmentContext; | ||
| } | ||
| } | ||
| // for a constructor invocation, the type from the invocation itself is the "enclosing type" | ||
| // for the purposes of determining type arguments | ||
| enclosingType = ASTHelpers.getType(newClassTree.getIdentifier()); | ||
| if (enclosingType == null) { | ||
| enclosingType = getTreeType(tree, withPathToSubtree(state, tree)); | ||
| } | ||
| } | ||
| return enclosingType; | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,7 +34,7 @@ void testNegative() { | |
| Bar<@Nullable Void> b = new Bar<>(Foo.make()); | ||
| } | ||
| void testPositive() { | ||
| // BUG: Diagnostic contains: incompatible types: Foo<@Nullable String> cannot be converted to Foo<String> | ||
| Bar<String> b = new Bar<>(Foo.makeNullableStr()); | ||
|
Collaborator
There was a problem hiding this comment. Just out of curiosity, does: Also report an error? (In general, that thread about
Collaborator
Author
There was a problem hiding this comment. Yes, this does lead to an error being reported (4babffe). This |
||
| } | ||
| } | ||
|
|
@@ -68,7 +68,7 @@ static class Bar<T extends @Nullable Object> { | |
| return new Bar<>(Foo.make()); | ||
| } | ||
| Bar<String> testPositive() { | ||
| // BUG: Diagnostic contains: incompatible types: Foo<@Nullable String> cannot be converted to Foo<String> | ||
| return new Bar<>(Foo.makeNullableStr()); | ||
| } | ||
| } | ||
|
|
@@ -104,7 +104,7 @@ void testNegative() { | |
| takeNullableVoid(new Bar<>(Foo.make())); | ||
| } | ||
| void testPositive() { | ||
| // BUG: Diagnostic contains: incompatible types: Foo<@Nullable String> cannot be converted to Foo<String> | ||
| takeStr(new Bar<>(Foo.makeNullableStr())); | ||
| } | ||
| } | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this last case? Just
newwithout 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'tnew ...?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)