fix(generic): preserve variadic return tails - #1229
Conversation
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
There was a problem hiding this comment.
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_slotstests regular return annotationstest_generic_variadic_return_overload_after_fixed_return_keeps_deep_slotstests@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
- 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));
}
}- 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
}-
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.
-
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.
Problem
T...variadic return tails were unwrapped to oneTwhen they followedfixed return values. Deeper assignment slots therefore became unknown,
including for
@return_overload.Solution
Tests
cargo test -p emmylua_code_analysiscargo fmt --all --checkcargo clippy -p emmylua_code_analysis --all-targets --all-features --locked -- -D warningsFixes #1227