perf: avoid rebuilding large inferred unions - #1217
Conversation
Large inferred unions were repeatedly copied and merged one member at a time. Inspect nested union members in place and use a faster merge when no special type rules are needed. Keep the existing safe merge for combinations such as number and integer, and add regression coverage for both paths. Fixes EmmyLuaLs#1192 Assisted-by: Codex
There was a problem hiding this comment.
I've reviewed the code changes. Here are my observations:
Issues Found
1. Logic Bug in StructuralUnionState::add
File: union_type.rs
The early rejection logic has changed behavior. In the original code, the function would return false immediately when a conflict was detected. In the new code, the add method returns !(conflict_condition) at the end, but this only checks the current state after adding the current type.
Critical issue: When processing a LuaType::Union, the code recursively adds members but doesn't propagate the failure correctly:
LuaType::Union(union) => return union.all_members(|typ| self.add(typ)),If self.add(typ) returns false for one member, all_members will short-circuit and return false, which is correct. However, the state mutations from partially processed members are not rolled back. This could lead to incorrect results if the same StructuralUnionState is reused.
2. Incorrect Early Exit Semantics
File: union_type.rs
The original code had an early exit pattern:
if has_number && has_number_variant || ... {
return false;
}The new code checks the condition at the end of add(). While this appears equivalent for a single pass, the recursive union handling changes the semantics. Consider:
- A union
number | integeris processed - After processing,
has_number=trueandhas_number_variant=true - The function returns
falsecorrectly
But what about a union like number | string | integer? The original code would return false as soon as it sees number and integer. The new code processes all members first, then checks. This is functionally equivalent but less efficient.
3. Potential Performance Regression
File: union_type.rs
The new all_members method on LuaUnionType is called for every union type encountered. For deeply nested unions, this could lead to exponential recursion in worst-case scenarios. The original code was O(n) for flat unions; the new code could be O(n²) for nested unions.
4. Missing Nil Handling in all_members
File: complex.rs
LuaUnionType::Nullable(ty) => predicate(ty) && predicate(&LuaType::Nil),This creates a temporary LuaType::Nil on each call. While this is correct, it could be optimized by checking predicate(&LuaType::Nil) once and reusing it if the predicate is pure.
Recommendations
- Fix the state rollback issue: Consider making
addtransactional or using a different approach:
fn add(&mut self, typ: &LuaType) -> bool {
let mut temp_state = self.clone(); // or use a snapshot approach
let result = temp_state.add_internal(typ);
if result {
*self = temp_state;
}
result
}- Add early exit for nested unions: When processing a
LuaType::Union, check if the current state already violates constraints before processing all members:
LuaType::Union(union) => {
if self.is_conflicted() {
return false;
}
union.all_members(|typ| self.add(typ))
}- Consider caching
LuaType::Nil: Inall_members, avoid creating a newLuaType::Nileach time:
LuaUnionType::Nullable(ty) => {
let nil = LuaType::Nil;
predicate(ty) && predicate(&nil)
}- Add more test cases: The tests added are good, but consider adding:
- Deeply nested unions
- Unions with mixed types that trigger multiple conflict conditions
- Edge cases with empty unions
Summary
The refactoring improves code organization by extracting the state into a struct, but introduces potential issues with state management in recursive scenarios. The core logic appears correct for the common cases, but the recursive union handling needs careful review for edge cases and performance implications.
Problem
Large inferred unions were repeatedly copied and merged one member at a time.
Flow narrowing also cloned the full union whenever it compared literal types.
In the reproduction from #1192, this made a check of 20 Lua files take about
35 seconds.
Solution
number | integer,references, and callable types
Results
Tests
cargo test -p emmylua_code_analysiscargo clippy -p emmylua_code_analysis --all-targets --all-features --locked -- -D warningscargo fmt --checkgit diff --checkFixes #1192