Fix problem with dataflow inside lambdas #7316
Conversation
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
Outdated
Show resolved
Hide resolved
| if (prevReturnedExpressionTypes != null) { | ||
| for (int i = 0; i < prevReturnedExpressionTypes.size(); i++) { | ||
| if (!prevReturnedExpressionTypes.get(i).equals(returnedExpressionTypes.get(i))) { | ||
| anyLambdaResultChanged = true; | ||
| break; | ||
| } | ||
| } | ||
| } else { | ||
| anyLambdaResultChanged = true; | ||
| } |
There was a problem hiding this comment.
Can you just use returnedExpressionTypes.equals(prevReturnedExpressionTypes?
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java (4)
1439-1451: Duplicate local variable ‘cfg’ across switch cases causes compile error; scope each case.Declaring
ControlFlowGraph cfgin both VARIABLE and BLOCK cases without braces creates “duplicate local variable” in a switch.Apply scoped blocks:
@@ - case VARIABLE: - VariableTree vt = (VariableTree) m; + case VARIABLE: { + VariableTree vt = (VariableTree) m; @@ - postAnalyze(cfg); + postAnalyze(cfg); Value initializerValue = flowResult.getValue(initializer); if (initializerValue != null) { fieldValues.add( new FieldInitialValue<>(fieldExpr, declaredValue, initializerValue)); break; } } fieldValues.add(new FieldInitialValue<>(fieldExpr, declaredValue, null)); - break; + break; + } @@ - case BLOCK: - BlockTree b = (BlockTree) m; + case BLOCK: { + BlockTree b = (BlockTree) m; @@ - postAnalyze(cfg); - break; + postAnalyze(cfg); + break; + }Also applies to: 1462-1474
1485-1486: Wrong class context used; passct(current class) instead ofclassTree.Methods/lambdas here belong to
ct. UsingclassTreecan skew defaults/initialization context.- performFlowAnalysisForMethod(classTree, method, classQueue, fieldValues, capturedStore); + performFlowAnalysisForMethod(ct, method, classQueue, fieldValues, capturedStore); @@ - analyze( + analyze( classQueue, lambdaQueue, - new CFGLambda(lambdaPair.first, classTree, mt), + new CFGLambda(lambdaPair.first, ct, mt), fieldValues, null, false, false, false, lambdaPair.second);Also applies to: 1494-1504
1778-1786: Javadoc @see param type mismatch: useStore, notCFAbstractStore.The analyze signature ends with
@Nullable Store; Javadoc still saysCFAbstractStore.- * analyzed by {@link #analyze(Queue, Queue, UnderlyingAST, List, ControlFlowGraph, boolean, - * boolean, boolean, CFAbstractStore)}. If the CFG is analyzed more than once, this method is + * analyzed by {@link #analyze(Queue, Queue, UnderlyingAST, List, ControlFlowGraph, boolean, + * boolean, boolean, Store)}. If the CFG is analyzed more than once, this method is @@ - * @see #analyze(Queue, Queue, UnderlyingAST, List, ControlFlowGraph, boolean, boolean, boolean, - * CFAbstractStore) + * @see #analyze(Queue, Queue, UnderlyingAST, List, ControlFlowGraph, boolean, boolean, boolean, + * Store)
1524-1615: Lambda result comparison can throw IndexOutOfBounds and miss reanalysis on size change.Currently iterates up to previous size and indexes new list, risking IOBE and missing size changes.
- List<AnnotationMirrorSet> prevReturnedExpressionTypes = lambdaResultTypeMap.get(lambda); - if (prevReturnedExpressionTypes != null) { - for (int i = 0; i < prevReturnedExpressionTypes.size(); i++) { - if (!prevReturnedExpressionTypes.get(i).equals(returnedExpressionTypes.get(i))) { - anyLambdaResultChanged = true; - break; - } - } - } else { - anyLambdaResultChanged = true; - } + List<AnnotationMirrorSet> prevReturnedExpressionTypes = lambdaResultTypeMap.get(lambda); + if (prevReturnedExpressionTypes != null) { + if (prevReturnedExpressionTypes.size() != returnedExpressionTypes.size()) { + anyLambdaResultChanged = true; + } else { + for (int i = 0; i < prevReturnedExpressionTypes.size(); i++) { + if (!prevReturnedExpressionTypes.get(i).equals(returnedExpressionTypes.get(i))) { + anyLambdaResultChanged = true; + break; + } + } + } + } else { + anyLambdaResultChanged = true; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java (4)
javacutil/src/main/java/org/checkerframework/javacutil/TreePathUtil.java (1)
TreePathUtil(33-505)javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (1)
TreeUtils(116-2967)javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java (1)
TypesUtils(55-1555)framework/src/main/java/org/checkerframework/framework/flow/CFCFGBuilder.java (1)
CFCFGBuilder(38-220)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (inference_part1_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
- GitHub Check: typetools.checker-framework (nonjunit_jdk25)
🔇 Additional comments (2)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java (2)
432-432: Good change: route preprocessing through performFlowAnalysisForClassOnce.Makes flow execution idempotent per class.
2091-2103: Good guard: perform flow once per class.Prevents repeated analysis; matches new workflow.
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java (2)
1439-1473: Duplicate local variablecfgcauses compile error; wrap case bodies in braces.The variable
cfgis declared in both theVARIABLEcase (line 1439) and theBLOCKcase (line 1462) within the same switch statement without scoping. This creates a duplicate local variable compile error.Apply this diff to wrap each case body in braces:
case VARIABLE: + { VariableTree vt = (VariableTree) m; ExpressionTree initializer = vt.getInitializer(); AnnotatedTypeMirror declaredType = getAnnotatedTypeLhs(vt); Value declaredValue = analysis.createAbstractValue(declaredType); FieldAccess fieldExpr = (FieldAccess) JavaExpression.fromVariableTree(vt); // analyze initializer if present if (initializer != null) { boolean isStatic = vt.getModifiers().getFlags().contains(Modifier.STATIC); ControlFlowGraph cfg = analyze( classQueue, lambdaQueue, new CFGStatement(vt, ct), fieldValues, null, true, true, isStatic, capturedStore); postAnalyze(cfg); Value initializerValue = flowResult.getValue(initializer); if (initializerValue != null) { fieldValues.add( new FieldInitialValue<>(fieldExpr, declaredValue, initializerValue)); break; } } fieldValues.add(new FieldInitialValue<>(fieldExpr, declaredValue, null)); break; + } case BLOCK: + { BlockTree b = (BlockTree) m; ControlFlowGraph cfg = analyze( classQueue, lambdaQueue, new CFGStatement(b, ct), fieldValues, null, true, true, b.isStatic(), capturedStore); postAnalyze(cfg); break; + }
1768-1782: Fix Javadoc reference to match actual parameter typeStore.The
@seetag referencesCFAbstractStorebut the actual method signature usesStore.Apply this diff:
/** * Perform any additional operations on a CFG. Called once per CFG, after the CFG has been - * analyzed by {@link #analyze(Queue, Queue, UnderlyingAST, List, ControlFlowGraph, boolean, - * boolean, boolean, CFAbstractStore)}. If the CFG is analyzed more than once, this method is + * analyzed by {@link #analyze(Queue, Queue, UnderlyingAST, List, ControlFlowGraph, boolean, + * boolean, boolean, Store)}. If the CFG is analyzed more than once, this method is * still only called one time after the last time the CFG is analyzed. This method can be used to * initialize additional state or to perform any analyzes that are easier to perform on the CFG * instead of the AST. * * @param cfg the CFG - * @see #analyze(Queue, Queue, UnderlyingAST, List, ControlFlowGraph, boolean, boolean, boolean, - * CFAbstractStore) + * @see #analyze(Queue, Queue, UnderlyingAST, List, ControlFlowGraph, boolean, boolean, boolean, + * Store) */
♻️ Duplicate comments (2)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java (2)
1485-1485: Usectinstead ofclassTreefor correct class context.The method being analyzed belongs to
ct(the current class in the loop), notclassTree(the original parameter). PassingclassTreecan cause incorrect dataflow analysis and initialization context.Apply this diff:
- performFlowAnalysisForMethod(classTree, method, classQueue, fieldValues, capturedStore); + performFlowAnalysisForMethod(ct, method, classQueue, fieldValues, capturedStore);
1661-1670: Add@Nullableannotation to queue parameter types for type correctness.Queue entries can contain null stores (e.g., for unreachable lambdas as seen at line 1387). The parameter types should reflect this to avoid nullness mismatches.
Apply this diff:
protected ControlFlowGraph analyze( - Queue<IPair<ClassTree, Store>> classQueue, - Queue<IPair<LambdaExpressionTree, Store>> lambdaQueue, + Queue<IPair<ClassTree, @Nullable Store>> classQueue, + Queue<IPair<LambdaExpressionTree, @Nullable Store>> lambdaQueue, UnderlyingAST ast, List<FieldInitialValue<Value>> fieldValues, @Nullable ControlFlowGraph cfg, boolean isInitializationCode, boolean updateInitializationStore, boolean isStatic, @Nullable Store capturedStore) {Note: Also update the
performFlowAnalysisForMethodsignature at lines 1534-1539 and all local queue declarations to use@Nullable Storeconsistently.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java (3)
javacutil/src/main/java/org/checkerframework/javacutil/TreePathUtil.java (1)
TreePathUtil(33-505)javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java (1)
TypesUtils(55-1555)framework/src/main/java/org/checkerframework/framework/flow/CFCFGBuilder.java (1)
CFCFGBuilder(38-220)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
- GitHub Check: typetools.checker-framework (inference_part1_jdk25)
- GitHub Check: typetools.checker-framework (nonjunit_jdk25)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java (2)
1768-1777: Verify Javadoc @see reference matches actual signature.Past review comment (marked "Addressed in commit 1c886a3") noted that the Javadoc
@seetag referenced theanalyzemethod with parameter typeCFAbstractStore, but the actual signature usesStore. Please confirm the Javadoc on lines 1768-1769 now correctly referencesStorerather thanCFAbstractStore, as the visible code here doesn't show the complete Javadoc text.Based on learnings
1494-1504: Critical: Wrong class context for lambda analysis (regression).Past review comment marked this as addressed, but the code still uses
classTreewherectshould be used. The lambdas inlambdaQueuewere discovered while analyzing classct, notclassTree. Both theCFGLambdaconstructor (line 1497) and theanalyzecall (line 1499) should receivectas the class context.Apply this diff:
ControlFlowGraph cfg = analyze( classQueue, lambdaQueue, - new CFGLambda(lambdaPair.first, classTree, mt), + new CFGLambda(lambdaPair.first, ct, mt), fieldValues, - classTree, + ct, null, false, false, false, lambdaPair.second);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java (3)
javacutil/src/main/java/org/checkerframework/javacutil/TreePathUtil.java (1)
TreePathUtil(33-505)javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java (1)
TreeUtils(116-2967)javacutil/src/main/java/org/checkerframework/javacutil/TypesUtils.java (1)
TypesUtils(55-1555)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: typetools.checker-framework (nonjunit_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
- GitHub Check: typetools.checker-framework (inference_part1_jdk25)
🔇 Additional comments (4)
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java (4)
1524-1538: LGTM: Well-documented helper for iterative lambda analysis.The new
performFlowAnalysisForMethodhelper encapsulates the iterative analysis of a method and its contained lambdas, addressing the core issue of dataflow inside lambdas during type inference.
1539-1561: Approve: Correct initialization and iterative method analysis.The iterative analysis correctly reuses the method's CFG across iterations (line 1557) while analyzing lambdas with fresh CFGs on each iteration. This supports type refinement for lambda return types.
1606-1608: LGTM: Correct post-analysis finalization.The method correctly calls
postAnalyzeonce for the method CFG and each lambda CFG after the iterative analysis completes, matching the new API design where callers are responsible for finalization.
1659-1678: LGTM: Correct analyze() signature and CFG reuse logic.The updated
analyze()method correctly:
- Returns the
ControlFlowGraphfor caller-controlled finalization- Accepts an optional
@Nullable ControlFlowGraph cfgfor reuse across iterations- Uses properly annotated
@Nullable Storein queue generics- Creates a new CFG only when needed (line 1669-1678)
This design enables the iterative lambda reanalysis that fixes issue #6629.
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
Show resolved
Hide resolved
2 participants
Fixes #6629