Repository navigation
fix(flow): keep named casts scoped to their targets - #1285
Conversation
There was a problem hiding this comment.
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: Nonedynamic: 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 allLuaLocalNamedescendants — 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 returningResultor usingexpectwith 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_syntaxinto a single enum to enforce the invariant. - Verify
type_of_expr_atis non-recursive/cached. - Reconsider the
Unknown | Anyguard 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.
4dbecfe to
599281a
Compare
|
rebased to current main |
A named cast immediately before an indexed expression can replace the type of the indexed value with the cast's target type. For example:
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 integernarrow the key before indexing.The VM represents index operands as
Static(member)orDynamic(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_implcaches 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 | Anyfallback:nilandnevercan 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 bothstrict.arrayIndexsettings. 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
assertresult.