Repository navigation
fix(flow): keep inline @as casts on their value expression - #1288
Conversation
Inline @as casts used shared flow bindings and changed unrelated types. For example: local value = 1 --[[@as integer]] print(value) The analyzer treated print as an integer and reported an invalid call. A cast on an assignment could also change its other values: first, second = 'text', {} --[[@as Item]] Only second must get the Item type. first must remain a string. Use the comment owner to identify the cast expression. Read its cast when inferring initializers and assignments, before selecting call return slots. Remove @as from declaration and member path casts so later callees, arguments, and unrelated variables keep their own types. Keep diagnostics for invalid calls and arguments.
There was a problem hiding this comment.
Code Review: Inline @as Cast Scoping Changes
Summary
This change introduces an ExprCastMode enum (All / AsOnly) to distinguish between two categories of inline casts:
All: applies both---@cast(named casts) and--[[@as T]](inline assertion casts)AsOnly: applies only--[[@as T]]casts
The intent is to prevent named ---@cast statements from leaking into initializer/assignment reads, while preserving inline @as behavior. The design is reasonable, but there are several concerns.
Issues & Concerns
1. ExprCastMode is pub(crate) but apply_expr_casts is pub(super) — visibility mismatch
pub(crate) enum ExprCastMode { All, AsOnly }
pub(super) fn apply_expr_casts(..., mode: ExprCastMode) -> LuaTypeapply_expr_casts is called from types.rs (a sibling module under semantic_model), which requires at least pub(crate) visibility. pub(super) restricts it to the parent of flow — verify this actually compiles. If types.rs is not a child of flow's parent, this will fail. Given the diff shows calls from types.rs, this looks like it should be pub(crate).
Recommendation: Make apply_expr_casts pub(crate) to match its call sites, or confirm the module hierarchy.
2. ExprCastMode derives only Clone, Copy — missing Debug, PartialEq
#[derive(Clone, Copy)]
pub(crate) enum ExprCastMode { All, AsOnly }For a public-ish enum used across modules, Debug and PartialEq/Eq are conventional and useful for assertions/logging.
Recommendation: Add #[derive(Clone, Copy, Debug, PartialEq, Eq)].
3. use ExprCastMode::{All, AsOnly}; — non-idiomatic import placement
use ExprCastMode::{All, AsOnly};This is placed after the rowan import and before the crate:: imports, breaking the conventional grouping (external crates → local crate). Also, importing enum variants directly can hurt readability at call sites (All vs ExprCastMode::All).
Recommendation: Move into the crate:: import block and consider using qualified paths, or at least group it with local imports.
4. Behavior change: AsCast flow nodes no longer collected in trace_decl / trace_member
The diff removes:
FlowNodeKind::AsCast(as_ptr) => {
if options.casts {
collect_decl_as_cast(model, decl, as_ptr, path);
}
}and the corresponding collect_member_as_cast. Instead, @as is now applied via apply_expr_casts at query time.
This is a significant semantic shift. Previously, @as was a path cast (affecting the decl/member along the flow path); now it's an expression-level cast. The new tests cover many cases, but consider:
- Ordering/interaction with other path casts: Previously
@aswas pushed intopath.castsand could interact with---@castordering. Now it's applied last (inapply_expr_casts). Verify no regression where a---@castafter an@asshould override it. options.castsgating removed: The old code respectedoptions.casts; the newapply_expr_castspath does not appear to check any equivalent option. Confirm this is intentional.
5. target.is_none_or(...) — verify MSRV
if target.is_none_or(|expr| expr.get_syntax_id() != expr_syntax) {
continue;
}Option::is_none_or was stabilized in Rust 1.82. If the project's MSRV is lower, this breaks the build.
Recommendation: Confirm MSRV, or use target.map_or(true, |expr| ...).
6. apply_expr_casts now silently skips TagCast in AsOnly mode
FlowEffect::TagCast(cast_ptr) if matches!(mode, All) => { ... }When mode == AsOnly, the TagCast arm is skipped by the guard. But there's no _ => {} fallback shown for the TagCast variant — ensure the match is exhaustive (the guard means TagCast falls through to... what?). If the match has no catch-all, this won't compile. If it does have a catch-all, fine — but the logic is subtle.
Recommendation: Add an explicit FlowEffect::TagCast(_) => {} arm for clarity, or a comment explaining the fallthrough.
7. assigned_value_type refactor changes multi-return semantics
// before
let expanded = model.infer_expr_list_types(&[LuaExpr::CallExpr(call)], Some(reference.return_index + 1));
expanded.get(reference.return_index).map(|(ty, _)| ty.clone())
// after
type_of_expr_with_cast(model, call.get_syntax_id(), AsOnly)
.get_result_slot_type(reference.return_index)This replaces infer_expr_list_types with get_result_slot_type. These may not be equivalent:
infer_expr_list_typeswithSome(n)may truncate/expand differently thanget_result_slot_type.- The
AsOnlycast is now applied to the call expression as a whole, then a slot extracted — but the@ascast was attached to the call, not to the individual slot. Applying a cast to a multi-return call before slot extraction could produce incorrect results if the cast type is not a tuple.
Recommendation: Add a test for local a, b = f() --[[@as SomeTuple]] and verify slot extraction still works. Confirm get_result_slot_type handles the cast-applied type correctly.
8. Duplicated cast-application logic in types.rs
let ty = if let Some(facts) = self.file_facts_of(member_file)
&& let Some(member_def) = facts.member_by_id(member)
&& member_def.doc_type_syntax.is_none()
&& let Some(value_syntax) = member_def.value_syntax
{
flow::apply_expr_casts(self, value_syntax, ty, flow::ExprCastMode::AsOnly)
} else {
ty
};This is a third place applying AsOnly casts. The condition doc_type_syntax.is_none() is a subtle guard — worth a comment explaining why doc-typed members skip the cast (presumably the doc type already wins).
9. Test test_inline_as_does_not_cast_later_call is weak
assert!(ws.has_no_diagnostic(
DiagnosticCode::CallNonCallable,
r#"
local function run()
local value = 1 --[[@as integer]]
print(value)
end
run()
"#,
));This only asserts the absence of a diagnostic. It doesn't verify the positive behavior (that value is integer). The test name suggests it's checking that the cast doesn't leak to print, but print is callable regardless. Consider asserting the type of value explicitly.
10. Missing test for ---@cast + --[[@as]] interaction
The core motivation is separating named casts from inline @as. There's no test where both appear on the same expression/statement, verifying precedence and that ---@cast doesn't leak while @as does.
Recommendation: Add a test like:
---@cast x integer
local y = x --[[@as string]]and assert y is string while x remains integer.
Minor / Style
- The doc comment on
type_of_expr_with_castwas rewritten but the old comment mentioned "closure return inference" — ensure the new comment accurately reflects all call sites (now includes initializers, assignments, member reads). pub(super) fn apply_expr_casts— the doc comment above it still says "Apply inline casts bound to this expression (e.g.A:get(1) --[[@cast -?]])" but the function now handles bothTagCastandAsCastwith mode gating. Update the doc to mentionmode.
Verdict
The overall direction (scoping @as casts to the expression they annotate, rather than leaking through flow paths) is sound and the test coverage is good. However, the following should be addressed before merge:
- Verify visibility of
apply_expr_casts(pub(super)vs call sites intypes.rs). - Confirm MSRV for
is_none_or. - Validate
assigned_value_typerefactor preserves multi-return slot semantics (add a test). - Add a test for
---@cast+--[[@as]]interaction on the same expression. - Clarify the
TagCastguard fallthrough in the match.
Inline
@ascasts can change unrelated expressions. The analyzer treatsprintas an integer in this example and reports an invalid call:Use the comment owner to apply the cast only to its value expression.
Read that result in initializers and assignments, before selecting call
return slots. Remove
@asfrom declaration and member path casts so latercallees, arguments, and unrelated values keep their own types.
For example, only
secondgets theItemtype here;firststays a string:The new reads apply only
@as. Named-cast scope handling stays with #1285,which edits the same cast helper. Its current patch applies without a
textual conflict. Nil-guard fixes stay with #1282.