fix: stop repeated assignments from hanging#1115
Conversation
There was a problem hiding this comment.
Code Review Summary
Issues Found:
-
Potential Infinite Loop Risk (Critical)
- The new
binary_assignment_operator_typefunction returnsSome(LuaType::Number)for arithmetic operators andSome(LuaType::String)for concatenation. However, the condition at line 1074-1083 only checks forInteger,Number, orStringtypes. If a custom type with@operator addreturns a different type (e.g.,Counter), the fallback toLuaType::Numberwill be incorrect.
- The new
-
Inconsistent Type Inference (Medium)
- In
test_binary_assignment_infer_error_keeps_previous_type, the test expects"string"when assigningconfig.pic + 1to a string variable. This behavior might be confusing - silently keeping the previous type instead of reporting an error could mask real issues.
- In
-
Missing Error Handling (Low)
- The
try_infer_expr_no_flowcall at line 1074 could returnErr, but the match arm at line 1077 treatsErrthe same asOk(None)forresult_slot != 0. This might hide legitimate inference errors.
- The
Recommendations:
- Add type checking for custom operator types:
// After the existing condition, add:
&& !binary_expr.get_exprs().is_some_and(|(left_expr, right_expr)| {
[left_expr, right_expr].iter().any(|operand| {
try_infer_expr_no_flow(self.db, self.cache, operand.clone())
.ok()
.flatten()
.is_some_and(|typ| typ.any_type(LuaType::is_custom_type))
})
})-
Consider adding a warning or error for type mismatch in binary assignments rather than silently preserving the previous type.
-
Improve error handling for
try_infer_expr_no_flow:
// Instead of treating Err as None, log or propagate the error
Err(e) => {
log::warn!("Inference error in binary assignment: {:?}", e);
None
}-
Add more test coverage for edge cases:
- Custom types with
@operatorthat return non-primitive types - Mixed operator types (e.g.,
string + number) - Nested binary expressions in assignments
- Custom types with
-
Consider performance implications - The
repeat(512)in tests might be excessive. Consider reducing to a smaller number that still reproduces the issue (e.g., 100-200).
There was a problem hiding this comment.
Code Review
This pull request optimizes flow replay for self-dependent binary assignments (such as repeated additions or string concatenations) by introducing a shortcut that resolves them to fallback primitive types, preventing excessive recursion. It also adds several unit tests to verify these scenarios. The review feedback is highly constructive, pointing out that the type matching is too restrictive and should support constant variants of primitive types, and identifying an unused import.
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.
| if let Some(expr_type) = expr_type | ||
| && matches!( | ||
| expr_type, | ||
| LuaType::Integer | LuaType::Number | LuaType::String | ||
| ) | ||
| { | ||
| return Ok(self.finish_walk(walk, expr_type)); | ||
| } |
There was a problem hiding this comment.
The pattern match matches!(expr_type, LuaType::Integer | LuaType::Number | LuaType::String) is too restrictive because it only matches the base primitive types. It will not match constant types like LuaType::IntegerConst, LuaType::FloatConst, LuaType::StringConst, LuaType::DocIntegerConst, or LuaType::DocStringConst.
Using the helper methods is_integer(), is_number(), and is_string() on LuaType will correctly match both the base types and their constant variants, ensuring the shortcut is correctly applied when constant types are inferred.
| if let Some(expr_type) = expr_type | |
| && matches!( | |
| expr_type, | |
| LuaType::Integer | LuaType::Number | LuaType::String | |
| ) | |
| { | |
| return Ok(self.finish_walk(walk, expr_type)); | |
| } | |
| if let Some(expr_type) = expr_type | |
| && (expr_type.is_integer() || expr_type.is_number() || expr_type.is_string()) | |
| { | |
| return Ok(self.finish_walk(walk, expr_type)); | |
| } |
| use crate::{ | ||
| CacheEntry, DbIndex, FlowId, FlowNode, FlowNodeKind, FlowTree, InferFailReason, LuaDeclId, | ||
| LuaInferCache, LuaMemberId, LuaSignatureId, LuaType, TypeOps, check_type_compact, | ||
| LuaInferCache, LuaMemberId, LuaSignatureId, LuaType, LuaTypeNode, TypeOps, check_type_compact, |
There was a problem hiding this comment.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a performance optimization to prevent deep recursive flow replays when analyzing self-dependent assignment chains (such as x = x + 1 or wnd = wnd .. value), resolving Issue 1114. It adds helper functions to determine the type of built-in operators directly and includes several test cases. The review feedback points out that the optimization's type matching is too restrictive because it only checks for base primitive types and misses constant variants (e.g., IntegerConst, StringConst), which could cause the optimization to be bypassed for constant expressions and lead to hangs.
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.
| matches!( | ||
| expr_type, | ||
| LuaType::Boolean | LuaType::Integer | LuaType::Number | LuaType::String | ||
| ) | ||
| .then_some(expr_type) |
There was a problem hiding this comment.
The current matches! check only matches the base primitive types (Boolean, Integer, Number, String). However, try_infer_expr_no_flow can return constant variants of these types (such as BooleanConst, IntegerConst, FloatConst, StringConst, or DocBooleanConst) if the operands can be partially or fully evaluated/inferred as constants.
If a constant type is returned, the shortcut will not be taken, and the engine will fall back to a full flow replay. This can cause hangs or timeouts on long assignment chains of constant expressions (e.g., total = -total or x = x + 0).
We should expand the matches! check to also include these constant variants to ensure the performance shortcut is reliably taken.
matches!(
expr_type,
LuaType::Boolean
| LuaType::BooleanConst(_)
| LuaType::DocBooleanConst(_)
| LuaType::Integer
| LuaType::IntegerConst(_)
| LuaType::Number
| LuaType::FloatConst(_)
| LuaType::String
| LuaType::StringConst(_)
)
.then_some(expr_type)Statements like `text = text .. value` or `total = total + value` could make type analysis revisit the same variable over and over, which stalled semantic model building. Handle that case directly for simple built-in operations. Other assignments still use the normal flow analysis, including custom operators. Fixes EmmyLuaLs#1114 Assisted-by: Codex
Problem
Repeated assignments like this could make semantic model building hang:
The same issue can happen with other simple binary operators:
Flow replay tries to infer the right-hand side by querying the values it depends on. For
x = x <op> unresolved, that can queryxat the same assignment path over and over.Solution
When an assignment RHS is a self-dependent built-in binary expression, return the primitive result type directly instead of starting another flow replay.
The shortcut stays narrow:
Tests
cargo test -p emmylua_code_analysis test_assignment_rhs_keeps_flow_dependent -- --nocapturecargo test -p emmylua_code_analysis issue_1114 -- --nocapturecargo test -p emmylua_code_analysis test_assignment_binary_rhs_replays_non_self_dependency -- --nocapturecargo test -p emmylua_code_analysis test_binary_assignment_infer_error_keeps_previous_type -- --nocapturecargo test -p emmylua_code_analysis flowcargo fmt --all --checkgit diff --check