fix(flow): preserve self-assignment antecedent types#1182
Conversation
Resolve self-dependent assignment reads through the flow scheduler instead of injecting unknown. Hoist only unavoidable self reads so repeated replay stays linear without bypassing skippable index dependencies. Add regressions for the minimal assignment and original while-loop cases. Fixes EmmyLuaLs#1180 Assisted-by: Codex
There was a problem hiding this comment.
Code Review Summary
Issues Identified:
-
Logic Error in
has_unskippable_dependencymethod- The method iterates through dependencies but only checks if
query.var_ref_id == *var_ref_id && query.flow_id == flow_id. This doesn't account for the case where the same variable appears at different flow IDs, potentially causing false positives. - Recommendation: Add a check to ensure the dependency is actually a self-reference (i.e., the variable being assigned to itself) before returning
true.
- The method iterates through dependencies but only checks if
-
Potential Infinite Loop Risk
- The
AssignmentSelfAntecedentcontinuation creates a recursive pattern where resolving a self-assignment could trigger another self-assignment resolution, potentially leading to infinite recursion. - Recommendation: Add a recursion depth counter or visited set to prevent infinite loops.
- The
-
Missing Error Handling for
has_unskippable_dependency- If
has_unskippable_dependencyreturnstruebut the antecedent type resolution fails, the error handling inresume_assignment_self_antecedentcould leave the system in an inconsistent state. - Recommendation: Add proper cleanup or fallback logic when antecedent resolution fails.
- If
-
Removed
contains_short_circuit_binary_exprFunction- The function was removed without replacement. This function was previously used to handle
and/oroperators specially. The new logic might not correctly handle short-circuit operators. - Recommendation: Verify that short-circuit operators are still handled correctly in the new implementation.
- The function was removed without replacement. This function was previously used to handle
-
Test Coverage Gap
- The new tests only cover basic integer self-assignment cases. Missing edge cases:
- Self-assignment with short-circuit operators
- Nested self-assignments
- Self-assignment in different scopes
- Recommendation: Add more comprehensive test cases.
- The new tests only cover basic integer self-assignment cases. Missing edge cases:
Suggestions for Improvement:
-
Add documentation explaining the new
AssignmentSelfAntecedentcontinuation and its purpose. -
Consider performance impact: The new logic adds additional queries for self-assignments. Profile to ensure no significant performance regression.
-
Simplify
has_unskippable_dependency: The current implementation is complex. Consider using a simpler approach like checking if the variable appears in the RHS expression directly. -
Add defensive checks: In
resume_assignment_self_antecedent, verify that the resolved antecedent type is compatible with expected types before proceeding.
Overall Assessment:
The changes address the issue of self-assignment type inference but introduce complexity and potential edge cases. The core logic appears sound but needs better error handling and more comprehensive testing.
There was a problem hiding this comment.
Code Review
This pull request resolves issue #1180 by improving the type inference of self-dependent assignments (such as index = index - 1) during flow analysis. Instead of defaulting self-reads in RHS expressions to Unknown, the engine now queries the variable's pre-assignment (antecedent) type first and resumes the RHS replay once that type is resolved. This is implemented via a new Continuation::AssignmentSelfAntecedent variant, the has_unskippable_dependency helper, and corresponding engine methods. Unit tests have been added to verify that self-assignments and loops correctly preserve and replay their antecedent types. No review comments were provided, and I have no additional feedback to offer.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
I think you can also take a look at this #1178 |
|
Thanks, I checked #1178. This PR fixes the original example: position remains a Vector after the self-assignment, so The other example in the comments still fails, but it does not involve self-assignment and looks like a separate custom-operator issue. I think that should be handled separately. |
Problem
Self-dependent assignments replaced the RHS read with
unknownto avoidrecursive replay. For expressions such as
index = index - 1, this discardedthe pre-assignment
integertype and widened the result tonumber.Solution
VarRefIdandFlowIddependencies
unknowninjection and itsand/orexceptionTests
cargo test -p emmylua_code_analysis issue_1180cargo test -p emmylua_code_analysis test_issue_1114_repeated_self_dependent_assignments_build_semantic_modelcargo test -p emmylua_code_analysis test_issue_1116_generic_call_index_replay_builds_semantic_modelcargo test -p emmylua_code_analysis(1,079 tests)cargo fmt --all --checkgit diff upstream/main...HEAD --checkFixes #1180