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.
|
I am about to merge the salsa3 branch within the next one to two weeks, so I do not plan to merge anything before that, as there will be a lot of conflicts. However, if things do not proceed as expected, I will merge this PR and defer the salsa update. |
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