Skip to content

fix(generic): preserve variadic return tails - #1229

Open
lewis6991 wants to merge 1 commit into
EmmyLuaLs:mainfrom
lewis6991:issue1227
Open

fix(generic): preserve variadic return tails#1229
lewis6991 wants to merge 1 commit into
EmmyLuaLs:mainfrom
lewis6991:issue1227

Conversation

@lewis6991

Copy link
Copy Markdown
Collaborator

Problem

T... variadic return tails were unwrapped to one T when they followed
fixed return values. Deeper assignment slots therefore became unknown,
including for @return_overload.

Solution

  • Keep instantiated unbounded base variadics wrapped in multi-return rows.
  • Continue expanding finite multi-return substitutions.

Tests

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

Fixes #1227

Keep instantiated base variadics wrapped after fixed return values.
Unwrapping them reduced `T...` to one `T`, so deeper assignment slots
became unknown.

Cover both return annotations and return overloads.

Fixes EmmyLuaLs#1227
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. Potential Infinite Loop / Recursion Risk (Critical)

File: crates/emmylua_code_analysis/src/semantic/generic/instantiate_type/mod.rs

The change replaces unwrapping a VariadicType::Base with pushing the variadic type itself:

// Before:
VariadicType::Base(base) => new_types.push(base.clone()),
// After:
VariadicType::Base(_) => new_types.push(LuaType::Variadic(variadic)),

This could cause infinite recursion if the same variadic type is processed again in a loop. The comment says "A base variadic is unbounded; unwrapping it turns T... into one T", but pushing the variadic back into new_types might cause it to be processed repeatedly without termination.

2. Missing Test Coverage for Edge Cases

The new tests only cover the happy path. Consider adding tests for:

  • Empty variadic arguments (e.g., calling some_func() with no arguments)
  • Multiple generic parameters with variadic returns
  • Nested generic functions with variadic returns

3. Inconsistent Behavior Between Regular and Overload Variants

The two tests are nearly identical but test different code paths:

  • test_generic_variadic_return_after_fixed_return_keeps_deep_slots tests regular return annotations
  • test_generic_variadic_return_overload_after_fixed_return_keeps_deep_slots tests @return_overload

This suggests the fix might need to be applied in multiple places. Verify that the fix covers all code paths where variadic types are instantiated.

Recommendations

  1. Add a termination guard in the variadic instantiation logic to prevent potential infinite loops:
VariadicType::Base(_) => {
    if !new_types.contains(&LuaType::Variadic(variadic.clone())) {
        new_types.push(LuaType::Variadic(variadic));
    }
}
  1. Add more comprehensive tests:
#[test]
fn test_generic_variadic_return_with_no_arguments() {
    // Test calling with no args
}

#[test]
fn test_generic_variadic_return_with_multiple_generics() {
    // Test multiple generic parameters
}
  1. Consider documenting the behavior change in the code comment more thoroughly, explaining why preserving the variadic wrapper is necessary and what the implications are for type inference.

  2. Verify performance impact - if this code path is in a hot loop, pushing variadic types could increase memory usage. Consider whether there's a more efficient approach.

The fix appears to address a real bug (the tests demonstrate the issue), but the implementation needs careful review for potential recursion issues and should be accompanied by more comprehensive test coverage.

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.

Generic Variadic returns don't work combined with any other return defined

1 participant