Repository navigation
fix(semantic_db): local a, b = f() gives each target its own slot - #1359
mareklangiewicz wants to merge 1 commit into
Conversation
`decl_type` typed every target of a multi-target assignment as the whole
initializer, ignoring the decl's `multi_return_index`. The model's VM path
picked the slot, but anything reading the database type got the full list:
the model's fallback when the VM gave up on a call (`b` of `two('x')` was
`(string,number)`), and member types built from such a local
(`payload.output = output` expected `(any,string?,string)`). `TypeShell` gets
`result_slot`, generalizing `first_result`; a target past a single value's
only slot is `any`, as on the VM path.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018NxUUQmc3YFNEXrUcrVPxV
There was a problem hiding this comment.
Code Review
1. query.rs — decl_type slot fallback logic
Issue: any fallback may mask legitimate nil.
shell = if slot.is_unknown() && !shell.is_unknown() {
TypeShell::from_name("any")
} else {
slot
};The comment says "A target past a single value's only slot is an undeclared missing value: any". But result_slot already returns nil for a finite list past its end (see types.rs). So this branch only triggers when the whole shell is known but the specific slot came back unknown. That's a narrow case, but the semantics deserve scrutiny:
- If
shellis a plain (non-multi) value,result_slot(1)returnsunknown(sincefirst_resultof a plain value is itself, but forindex > 0a plain candidate falls into_ => {}). Solocal a, b = 5would giveb = any. Is that intended? In Lua,local a, b = 5givesb = nil, notany. This looks like a behavioral regression — the fallback should probably benilfor plain values, andanyonly for genuinely unknown/opaque multi-return sources.
Recommendation: Distinguish "plain value, no slot" (→ nil) from "unknown multi-return source" (→ any). Consider having result_slot itself return nil for plain candidates when index > 0, and reserve the any fallback for shell.is_unknown()-adjacent cases.
Issue: from_name("any") vs from_primitive. Elsewhere the code uses Self::from_primitive(PrimitiveType::Nil). Using from_name("any") is inconsistent and may not produce the canonical any primitive candidate. Prefer Self::from_primitive(PrimitiveType::Any) if available.
2. types.rs — result_slot
Issue: Variadic handling ignores index for non-tuple candidates.
TypeCandidate::Variadic(base) => {
for candidate in &base.candidates {
match candidate {
TypeCandidate::Tuple(types) => {
if let Some(slot) = types.get(index) { out.merge(slot); }
}
_ => out.merge(&Self { candidates: vec![candidate.clone()] }),
}
}
}For a non-tuple variadic base (e.g. number...), every index returns number — correct. But for a Tuple base, if index >= types.len(), nothing is merged, leaving out as unknown. Combined with the query.rs fallback, this becomes any. For a variadic tuple [string, number]..., slot 2 should arguably be string (wrapping around) or at least nil/any — currently it silently yields unknown. The test only checks slot 1 (TUPLE_VARIADIC2), so this gap is untested.
Recommendation: Add a test for tuple_variadic slot ≥ tuple length, and decide the intended semantics (wrap-around vs. nil vs. any).
Issue: Multi tail expansion check is fragile.
let expands = tail.candidates.iter().any(|candidate| {
matches!(candidate, TypeCandidate::Variadic(_) | TypeCandidate::Repeat(_) | TypeCandidate::Multi(_))
});This duplicates the "is this an expanding candidate" predicate that likely exists elsewhere. If the set of expanding variants changes, this silently drifts. Extract a helper (e.g. TypeCandidate::is_expanding()).
Issue: Repeat ignores index entirely.
TypeCandidate::Repeat(base) => out.merge(base),For Repeat, every slot returns base — presumably correct for number...-style repeats, but worth a comment since it's non-obvious that index is intentionally unused.
Issue: Multi with empty slots silently continues, leaving out as unknown. An empty Multi is likely a bug upstream; consider debug_assert! or documenting.
Minor: checked_sub(1) on slots.len() — slots.len() is usize; checked_sub is fine but slots.len() - 1 after an is_empty guard would be clearer. Not a bug.
3. Tests
Good coverage of the main shapes. Gaps:
- No test for
local a, b = 5(plain value, slot 1) — would expose theanyvsnilquestion above. - No test for
tuple_variadicslot ≥ 2. - No test for
local a, b = f()wherefreturns a single non-tuple value (the_ => {}branch inVariadic). test_field_from_multi_return_local_has_slot_typeuses---@return any resultthen assertsoutputisstring— fine, but doesn't exercise theanyfallback path.
Recommendation: Add the plain-value and out-of-range tuple-variadic cases to pin down intended semantics.
Summary
| Severity | Location | Issue |
|---|---|---|
| High | query.rs |
any fallback likely wrong for plain values (local a, b = 5 → b should be nil) |
| Medium | types.rs |
Variadic + Tuple past end yields unknown (→ any), untested |
| Medium | types.rs |
Duplicated "expanding candidate" predicate; extract helper |
| Low | query.rs |
from_name("any") inconsistent with from_primitive usage |
| Low | types.rs |
Repeat ignores index without comment; empty Multi silently skipped |
| Low | tests | Missing plain-value and out-of-range tuple-variadic cases |
Summary
When the VM gives up on a call, every target of a multi-target assignment gets the first return:
type_of_declfalls back to the database'sdecl_type, which ignoresmulti_return_index. It now picks the slot, using a newTypeShell::result_slotthat generalizesfirst_result:if let Some(value_expr_syntax) = decl.value_expr_syntax { - let shell = expr_type_of(db, file, config, value_expr_syntax); + let mut shell = expr_type_of(db, file, config, value_expr_syntax); + if let Some(index) = decl.multi_return_index { + let slot = shell.result_slot(index); + shell = if slot.is_unknown() && !shell.is_unknown() { any } else { slot }; + }A target past a single value's only slot is
any, as on the VM path.Evidence
Targets of a mismatched call, read through a global:
mainstring, number, booleanstringnumberstring, number, booleanstringbooleanstring, numberstringnilstring, number ...stringnumberreturn true, ...(...isnumber)booleannumber[string, number]anymainde89376):test_mismatched_call_assigns_return_slotsandtest_field_from_multi_return_local_has_slot_typefail.test_mismatched_call_slots_by_result_shape. Fullemmylua_code_analysissuite: 1705 passed.clippyreports nothing new.result_slotin turn, and each removal fails a test.Merge Danger
Door: two-way
Blast Radius: inference
This changes the type of a multi-target
localwhereverdecl_typeis read, including members built from one (payload.output = output). Diagnostics that depended on the old type can change.emmylua_checkfrommainon three real Lua codebases, against this branch alone and with #1361 and #1360:mainWhy this is a draft: alone, this PR adds 2 false reports in the first codebase. They come from two existing bugs, not from this change: on
mainthe local holds the whole return list, so it never reaches them. Both reproduce on plainmainwithout this PR, and #1361 and #1360 fix them. With both applied those 2 are gone. Of the 4 added in the second codebase, 1 is amissing-fieldsreport that already fires elsewhere and 3 are the known cases described in those PRs.Written with help from Claude Code.
🤖 Generated with Claude Code
https://claude.ai/code/session_018NxUUQmc3YFNEXrUcrVPxV