Skip to content

fix(flow): keep named casts scoped to their targets - #1285

Merged
CppCXY merged 3 commits into
EmmyLuaLs:mainfrom
hgschmie:fix/named-cast-expression-scope
Oct 14, 2026
Merged

CppCXY merged 3 commits into
EmmyLuaLs:mainfrom
hgschmie:fix/named-cast-expression-scope

Conversation

@hgschmie

@hgschmie hgschmie commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

A named cast immediately before an indexed expression can replace the type of the indexed value with the cast's target type. For example:

---@param values string[]
---@param index integer?
local function get_value(values, index)
    ---@cast index integer
    local value = values[index] -- inferred as integer instead of string?
    return value
end

The inline-expression cast handler sees the named cast through the expression's flow binding and applies it without checking its target. Keep named casts scoped to their targets. Declaration/member flow tracing handles targets with a unique identity; for ambiguous union members without a unique member ID, the inline handler applies an immediate cast only when the full static member path and resolved root declaration match. Also retain the syntax of dynamic keys in the inference VM so member-key casts such as ---@cast position.index integer narrow the key before indexing.

The VM represents index operands as Static(member) or Dynamic(syntax), so the stack shape and key metadata cannot disagree. Debug assertions check the operand count while release builds retain the existing unknown-type fallback.

Flow queries can compute on their first call; they are not lookup-only. type_of_expr_at_impl caches results by file, syntax and offset, and ordinary inference guards in-progress dependencies. The VM asks about the key subtree, not the enclosing index. A bounded regression covers 64 nested computed keys and a recursive key dependency.

Keep the Unknown | Any fallback: nil and never can be legitimate cast results, and treating them as inference failures would incorrectly restore an array element type. Tests cover both. The named owner cast ---@cast values string[] remains target-aware, while a keyless statement cast such as ---@cast -? still applies to the expression.

Eleven regression tests cover local/member/owner casts, aliases, unrelated targets, cast operators, nested indexing, assert, inline and keyless statement casts, nil/never keys, recursive dependencies and both strict.arrayIndex settings. They also cover ambiguous union members, equivalent dot/string-literal paths, unrelated owners and paths, shadowing, owner/member reassignment, and a 16-level recursive member path. Path/root matching runs before member resolution and excludes dynamic keys. The local-type helper rejects duplicate names rather than silently selecting a shadowed declaration. Each strictness case keeps an isolated workspace; this small suite takes less than one second.

The ambiguous-member fallback preserves the existing immediate-expression behavior. Cast persistence across intervening statements for members without a unique identity remains a pre-existing limitation; this change does not introduce a new flow walker.

Validation: all 2,395 workspace tests with all features, workspace Clippy with warnings denied, and rustfmt pass. The two original regressions fail before the fix. A direct LSP reproduction also returns the correct nullable element type and nonnullable assert result.

@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.

Code Review

Overview

The change fixes a bug where named casts (---@cast index integer) were incorrectly narrowing the indexed value (values[index]) instead of only the key expression (index). The fix adds a key_syntax field to track the key expression's syntax node and applies flow-sensitive type narrowing to the key at index time.

Issues & Suggestions

1. key_syntax field is redundant with dynamic (minor design smell)
In Instr::IndexMember, dynamic: bool and key_syntax: Option<LuaSyntaxId> are always set in lockstep:

  • dynamic: false → key_syntax: None
  • dynamic: true → key_syntax: Some(...)

This invariant is not enforced by the type system. Consider collapsing them into a single enum, e.g.:

enum IndexKey {
    Static(LuaMemberKey),
    Dynamic(LuaSyntaxId),
}

This eliminates the possibility of an inconsistent state and removes the unwrap_or fallback ambiguity.

2. Silent fallback masks compiler bugs

let mut key_value = self.stack.pop().unwrap_or(Value::plain(LuaType::Unknown));
let owner_value = self.stack.pop().unwrap_or(Value::plain(LuaType::Unknown));

The pre-existing unwrap_or pattern is retained. If the stack is misaligned, this silently produces Unknown rather than surfacing a VM bug. Not introduced by this PR, but worth flagging since the new code path depends on stack ordering being correct.

3. type_of_expr_at may re-enter inference (potential recursion / perf concern)

let flow_ty = self.model.type_of_expr_at(syntax, syntax.get_range().start());

This is called on every dynamic index operation. If type_of_expr_at itself triggers inference (or re-runs the VM), this could cause recursion or significant overhead in hot paths. Worth confirming it's a cached/lookup-only call, or documenting the assumption.

4. Unknown | Any guard may be too narrow

if !matches!(flow_ty, LuaType::Unknown | LuaType::Any) {
    key_value.ty = flow_ty;
}

If type_of_expr_at returns LuaType::Nil or an error/never type for an unresolvable expression, the key type will be overwritten with something less useful than the original inferred key type. Consider only overriding when the flow type is strictly more specific, or explicitly listing the "don't override" cases.

5. Test helper duplicates logic and may be brittle

fn local_type(ws: &VirtualWorkspace, file: FileId, name: &str) -> LuaType {
    ...
    .find(|local| ... token.get_name_text() == name)
  • Uses .find() on all LuaLocalName descendants — if two locals share a name (shadowing), the test silently picks the first. Tests here don't shadow, but the helper is fragile.
  • expect("...") panics give poor diagnostics on failure; consider returning Result or using expect with the name interpolated.

6. Test coverage gap: no negative test for the key_syntax guard
The fix relies on cast.get_key_expr().is_some() to skip named casts in apply_expr_casts. There's no test asserting that a non-named cast (e.g. ---@cast values string[]) still applies to the indexed value. The inline_expression_casts_still_apply test covers inline casts, but not statement-level casts without a key expr. Consider adding one.

7. Comment accuracy

// Named casts narrow their target through trace_decl/trace_member.
// Sharing that flow node does not make another expression a target.

The second sentence is grammatically awkward and its meaning is unclear ("Sharing that flow node does not make another expression a target"). Consider rewording to explain why skipping is correct, e.g.: "Named casts are applied via trace_decl/trace_member on the key expression itself; applying them here would incorrectly narrow the indexed value."

8. strict.array_index loop in tests

for strict in [false, true] {
    let mut ws = VirtualWorkspace::new_with_init_std_lib();
    ...
}

Each iteration rebuilds the workspace. Fine for correctness, but if these tests are slow, consider parameterizing via #[gtest] cases or a shared fixture. Minor.

Summary

The core fix is sound and well-targeted. Main recommendations:

  • Collapse dynamic + key_syntax into a single enum to enforce the invariant.
  • Verify type_of_expr_at is non-recursive/cached.
  • Reconsider the Unknown | Any guard for robustness.
  • Add a negative test for non-named statement-level casts.
  • Clarify the explanatory comment.

Exclude named casts from inline expression casting so a cast on an index does not replace the type of the indexed value. Use flow-sensitive dynamic key types to retain narrowing of member keys.

Cover named local and member casts, unrelated targets, cast operators, nested indexing, assert calls, inline casts and both strict array-index settings.
@hgschmie
hgschmie force-pushed the fix/named-cast-expression-scope branch from 4dbecfe to 599281a Compare October 14, 2026 05:42
@hgschmie

Copy link
Copy Markdown
Contributor Author

rebased to current main

@CppCXY
CppCXY merged commit d81543b into EmmyLuaLs:main Oct 14, 2026
16 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.

2 participants