Skip to content

perf: avoid rebuilding large inferred unions - #1217

Merged
CppCXY merged 1 commit into
EmmyLuaLs:mainfrom
lewis6991:issue1192
Aug 12, 2026
Merged

perf: avoid rebuilding large inferred unions#1217
CppCXY merged 1 commit into
EmmyLuaLs:mainfrom
lewis6991:issue1192

Conversation

@lewis6991

Copy link
Copy Markdown
Collaborator

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

  • inspect nested union members in place
  • use the structural merge when no special type rules are needed
  • keep the existing safe merge for combinations such as number | integer,
    references, and callable types
  • compare union members during flow narrowing without cloning them
  • cover both the fast path and semantic fallback with regression tests

Results

  • full reproduction: 34.68s to 4.75s
  • workspace analysis: 12.54s to 1.36s
  • all 678 diagnostic codes, ranges, and severities are unchanged

Tests

  • cargo test -p emmylua_code_analysis
  • cargo clippy -p emmylua_code_analysis --all-targets --all-features --locked -- -D warnings
  • cargo fmt --check
  • git diff --check

Fixes #1192

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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | integer is processed
  • After processing, has_number=true and has_number_variant=true
  • The function returns false correctly

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

  1. Fix the state rollback issue: Consider making add transactional 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
}
  1. 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))
}
  1. Consider caching LuaType::Nil: In all_members, avoid creating a new LuaType::Nil each time:
LuaUnionType::Nullable(ty) => {
    let nil = LuaType::Nil;
    predicate(ty) && predicate(&nil)
}
  1. 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.

@CppCXY
CppCXY merged commit 6d7b41f into EmmyLuaLs:main Aug 12, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stuck loading

2 participants