Skip to content

Commit 77c85a6

Browse files
msridharmsridhar
authored
Address severe performance regression in dataflow analysis (#1328)
Fixes #1327 #1309 introduced a severe performance regression in dataflow analysis, as the result of running the dataflow analysis was no longer being cached. This caused a >100X slowdown in our dataflow micro-benchmark! (Which we really should have measured before landing #1309...) This PR re-introduces the cache for analysis results (by tracking when we have already run the analysis). This fix exposed some other bugs in generic method inference where we weren't passing around the right `TreePath` to enable discovery of appropriate assignment contexts and containing methods; we fix those bugs here too. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * More precise nullness inference for generic method returns in JSpecify mode by using invocation context when available. * **Bug Fixes** * Prevents redundant dataflow analyses by tracking already-run analyses; cache invalidation resets this tracking. * Behavior changes are internal only; public API remains unchanged. * **Tests** * Updated test setup and expected diagnostics to reflect refined generic inference and nullability behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 38a7561 commit 77c85a6

4 files changed

Lines changed: 55 additions & 21 deletions

File tree

‎nullaway/src/main/java/com/uber/nullaway/NullAway.java‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2720,14 +2720,23 @@ private boolean mayBeNullMethodCall(
27202720
if (Nullness.hasNullableAnnotation(exprSymbol, config)) {
27212721
return true;
27222722
}
2723-
// NOTE: we cannot rely on state.getPath() here to get a TreePath to the invocation, since
2724-
// sometimes the invocation is a sub-node of the leaf of the path. So, here if inference runs,
2725-
// it will do so without an assignment context. If this becomes a problem, we can revisit
2726-
if (config.isJSpecifyMode()
2727-
&& genericsChecks
2728-
.getGenericReturnNullnessAtInvocation(exprSymbol, invocationTree, null, state, false)
2729-
.equals(Nullness.NULLABLE)) {
2730-
return true;
2723+
if (config.isJSpecifyMode() && exprSymbol.getReturnType().getKind().equals(TypeKind.TYPEVAR)) {
2724+
// It is important to pass a correct TreePath to getGenericReturnNullnessAtInvocation. So, we
2725+
// do a search under path to find invocationTree. This shouldn't be too costly in the common
2726+
// case, and it's important for correctness.
2727+
TreePath path = state.getPath();
2728+
var invocationPath = TreePath.getPath(path, invocationTree);
2729+
Verify.verify(
2730+
invocationPath != null,
2731+
"%s not found as a descendant of %s",
2732+
invocationTree,
2733+
path.getLeaf());
2734+
if (genericsChecks
2735+
.getGenericReturnNullnessAtInvocation(
2736+
exprSymbol, invocationTree, invocationPath, state, false)
2737+
.equals(Nullness.NULLABLE)) {
2738+
return true;
2739+
}
27312740
}
27322741
return false;
27332742
}

‎nullaway/src/main/java/com/uber/nullaway/dataflow/DataFlow.java‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@
4040
import com.uber.nullaway.NullabilityUtil;
4141
import com.uber.nullaway.dataflow.cfg.NullAwayCFGBuilder;
4242
import com.uber.nullaway.handlers.Handler;
43+
import java.util.HashSet;
44+
import java.util.Set;
4345
import javax.annotation.processing.ProcessingEnvironment;
4446
import org.checkerframework.nullaway.dataflow.analysis.AbstractValue;
4547
import org.checkerframework.nullaway.dataflow.analysis.Analysis;
@@ -93,6 +95,12 @@ public final class DataFlow {
9395
}
9496
});
9597

98+
/**
99+
* Set of {@link AnalysisParams} for which the analysis has already been run to completion. Used
100+
* to avoid re-running analyses needlessly
101+
*/
102+
private final Set<AnalysisParams> alreadyRunAnalyses = new HashSet<>();
103+
96104
private final LoadingCache<CfgParams, ControlFlowGraph> cfgCache =
97105
CacheBuilder.newBuilder()
98106
.maximumSize(MAX_CACHE_SIZE)
@@ -161,11 +169,12 @@ Result<A, S, T> dataflow(
161169
AnalysisParams aparams = AnalysisParams.create(transfer, cfg);
162170
@SuppressWarnings("unchecked")
163171
Analysis<A, S, T> analysis = (Analysis<A, S, T>) analysisCache.getUnchecked(aparams);
164-
if (performAnalysis) {
172+
if (performAnalysis && !alreadyRunAnalyses.contains(aparams)) {
165173
analysis.performAnalysis(cfg);
174+
alreadyRunAnalyses.add(aparams);
166175
}
167176

168-
return new Result<A, S, T>() {
177+
return new Result<>() {
169178
@Override
170179
public Analysis<A, S, T> getAnalysis() {
171180
return analysis;
@@ -313,6 +322,7 @@ ControlFlowGraph getControlFlowGraph(TreePath path, Context context, T transfer)
313322
public void invalidateCaches() {
314323
cfgCache.invalidateAll();
315324
analysisCache.invalidateAll();
325+
alreadyRunAnalyses.clear();
316326
}
317327

318328
@AutoValue

‎nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -519,7 +519,7 @@ public void checkTypeParameterNullnessForAssignability(Tree tree, VisitorState s
519519
if (isGenericCallNeedingInference(rhsTree)) {
520520
rhsType =
521521
inferGenericMethodCallType(
522-
state, (MethodInvocationTree) rhsTree, lhsType, assignedToLocal, false);
522+
state, (MethodInvocationTree) rhsTree, null, lhsType, assignedToLocal, false);
523523
}
524524
boolean isAssignmentValid = subtypeParameterNullability(lhsType, rhsType, state);
525525
if (!isAssignmentValid) {
@@ -551,6 +551,8 @@ private ConstraintSolver makeSolver(VisitorState state, NullAway analysis) {
551551
*
552552
* @param state the visitor state
553553
* @param invocationTree the method invocation tree representing the call to a generic method
554+
* @param path the tree path to the invocationTree if available and possibly distinct from {@code
555+
* state.getPath()}
554556
* @param typeFromAssignmentContext the type being "assigned to" in the assignment context
555557
* @param assignedToLocal true if the method call result is assigned to a local variable, false
556558
* otherwise
@@ -560,6 +562,7 @@ private ConstraintSolver makeSolver(VisitorState state, NullAway analysis) {
560562
private Type inferGenericMethodCallType(
561563
VisitorState state,
562564
MethodInvocationTree invocationTree,
565+
@Nullable TreePath path,
563566
@Nullable Type typeFromAssignmentContext,
564567
boolean assignedToLocal,
565568
boolean calledFromDataflow) {
@@ -572,7 +575,7 @@ private Type inferGenericMethodCallType(
572575
result =
573576
runInferenceForCall(
574577
state,
575-
null,
578+
path,
576579
invocationTree,
577580
typeFromAssignmentContext,
578581
assignedToLocal,
@@ -935,7 +938,7 @@ public void checkTypeParameterNullnessForFunctionReturnType(
935938
if (isGenericCallNeedingInference(retExpr)) {
936939
returnExpressionType =
937940
inferGenericMethodCallType(
938-
state, (MethodInvocationTree) retExpr, formalReturnType, false, false);
941+
state, (MethodInvocationTree) retExpr, null, formalReturnType, false, false);
939942
}
940943
boolean isReturnTypeValid =
941944
subtypeParameterNullability(formalReturnType, returnExpressionType, state);
@@ -1080,7 +1083,7 @@ public void compareGenericTypeParameterNullabilityForCall(
10801083
return;
10811084
}
10821085
Type invokedMethodType = methodSymbol.type;
1083-
Type enclosingType = getEnclosingTypeForCallExpression(methodSymbol, tree, state, false);
1086+
Type enclosingType = getEnclosingTypeForCallExpression(methodSymbol, tree, null, state, false);
10841087
if (enclosingType != null) {
10851088
invokedMethodType =
10861089
TypeSubstitutionUtils.memberType(state.getTypes(), enclosingType, methodSymbol, config);
@@ -1118,6 +1121,7 @@ public void compareGenericTypeParameterNullabilityForCall(
11181121
inferGenericMethodCallType(
11191122
state,
11201123
(MethodInvocationTree) currentActualParam,
1124+
null,
11211125
formalParameter,
11221126
false,
11231127
false);
@@ -1271,7 +1275,7 @@ public Nullness getGenericMethodReturnTypeNullness(
12711275
*
12721276
* @param invokedMethodSymbol symbol for the invoked method
12731277
* @param tree the tree for the invocation
1274-
* @param path the path to the invocation tree, or null if not available
1278+
* @param path the path to the invocation tree
12751279
* @param state the visitor state
12761280
* @param calledFromDataflow whether this method is being called from dataflow analysis
12771281
* @return Nullness of invocation's return type, or {@code NONNULL} if the call does not invoke an
@@ -1280,7 +1284,7 @@ public Nullness getGenericMethodReturnTypeNullness(
12801284
public Nullness getGenericReturnNullnessAtInvocation(
12811285
Symbol.MethodSymbol invokedMethodSymbol,
12821286
MethodInvocationTree tree,
1283-
@Nullable TreePath path,
1287+
TreePath path,
12841288
VisitorState state,
12851289
boolean calledFromDataflow) {
12861290
// If the return type is not a type variable, just return NONNULL (explicit @Nullable should
@@ -1304,7 +1308,8 @@ public Nullness getGenericReturnNullnessAtInvocation(
13041308
}
13051309

13061310
Type enclosingType =
1307-
getEnclosingTypeForCallExpression(invokedMethodSymbol, tree, state, calledFromDataflow);
1311+
getEnclosingTypeForCallExpression(
1312+
invokedMethodSymbol, tree, path, state, calledFromDataflow);
13081313
if (enclosingType == null) {
13091314
return Nullness.NONNULL;
13101315
} else {
@@ -1475,6 +1480,7 @@ private InvocationAndContext getInvocationAndContextForInference(
14751480
getEnclosingTypeForCallExpression(
14761481
ASTHelpers.getSymbol(parentInvocation),
14771482
parentInvocation,
1483+
parentPath,
14781484
state,
14791485
calledFromDataflow);
14801486
} else {
@@ -1556,7 +1562,8 @@ public Nullness getGenericParameterNullnessAtInvocation(
15561562
}
15571563
}
15581564

1559-
Type enclosingType = getEnclosingTypeForCallExpression(invokedMethodSymbol, tree, state, false);
1565+
Type enclosingType =
1566+
getEnclosingTypeForCallExpression(invokedMethodSymbol, tree, null, state, false);
15601567
if (enclosingType == null) {
15611568
return Nullness.NONNULL;
15621569
}
@@ -1571,13 +1578,15 @@ public Nullness getGenericParameterNullnessAtInvocation(
15711578
*
15721579
* @param invokedMethodSymbol symbol for the invoked method
15731580
* @param tree the tree for the invocation
1581+
* @param path the path to the invocation tree, or null if not available
15741582
* @param state the visitor state
15751583
* @param calledFromDataflow whether this method is being called from dataflow analysis
15761584
* @return the enclosing type for the method call, or null if it cannot be determined
15771585
*/
15781586
private @Nullable Type getEnclosingTypeForCallExpression(
15791587
Symbol.MethodSymbol invokedMethodSymbol,
15801588
Tree tree,
1589+
@Nullable TreePath path,
15811590
VisitorState state,
15821591
boolean calledFromDataflow) {
15831592
Type enclosingType = null;
@@ -1599,9 +1608,15 @@ public Nullness getGenericParameterNullnessAtInvocation(
15991608
ExpressionTree receiver =
16001609
ASTHelpers.stripParentheses(((MemberSelectTree) methodSelect).getExpression());
16011610
if (isGenericCallNeedingInference(receiver)) {
1611+
var receiverPath = path == null ? null : new TreePath(path, receiver);
16021612
enclosingType =
16031613
inferGenericMethodCallType(
1604-
state, (MethodInvocationTree) receiver, null, false, calledFromDataflow);
1614+
state,
1615+
(MethodInvocationTree) receiver,
1616+
receiverPath,
1617+
null,
1618+
false,
1619+
calledFromDataflow);
16051620
} else {
16061621
enclosingType = getTreeType(receiver, state);
16071622
}

‎nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1271,7 +1271,7 @@ public void nullableWildcardFromCaffeine() {
12711271
/** various cases where dataflow analysis forces inference to run for a generic method call */
12721272
@Test
12731273
public void inferenceFromDataflow() {
1274-
makeHelperWithInferenceFailureWarning()
1274+
makeHelper()
12751275
.addSourceLines(
12761276
"Test.java",
12771277
"import org.jspecify.annotations.NullMarked;",
@@ -1299,7 +1299,7 @@ public void inferenceFromDataflow() {
12991299
" // to ensure that dataflow runs",
13001300
" Object x = new Object(); x.toString();",
13011301
" Object y = null;",
1302-
" // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type",
1302+
" // BUG: Diagnostic contains: passing @Nullable parameter 'y' where @NonNull is required",
13031303
" return (((id(y))));",
13041304
" }",
13051305
" static Object testReturnNested() {",

0 commit comments

Comments
 (0)