Skip to content

fix(flow): keep inline @as casts on their value expression - #1288

Merged
CppCXY merged 1 commit into
EmmyLuaLs:mainfrom
lewis6991:rewrite-inline-as
Oct 14, 2026
Merged

CppCXY merged 1 commit into
EmmyLuaLs:mainfrom
lewis6991:rewrite-inline-as

Conversation

@lewis6991

Copy link
Copy Markdown
Collaborator

Inline @as casts can change unrelated expressions. The analyzer treats
print as an integer in this example and reports an invalid call:

local function run()
  local value = 1 --[[@as integer]]
  print(value)
end
run()

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 @as from declaration and member path casts so later
callees, arguments, and unrelated values keep their own types.

For example, only second gets the Item type here; first stays a string:

first, second = 'text', {} --[[@as Item]]

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.

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.

@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: 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) -> LuaType

apply_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 @as was pushed into path.casts and could interact with ---@cast ordering. Now it's applied last (in apply_expr_casts). Verify no regression where a ---@cast after an @as should override it.
  • options.casts gating removed: The old code respected options.casts; the new apply_expr_casts path 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_types with Some(n) may truncate/expand differently than get_result_slot_type.
  • The AsOnly cast is now applied to the call expression as a whole, then a slot extracted — but the @as cast 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_cast was 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 both TagCast and AsCast with mode gating. Update the doc to mention mode.

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:

  1. Verify visibility of apply_expr_casts (pub(super) vs call sites in types.rs).
  2. Confirm MSRV for is_none_or.
  3. Validate assigned_value_type refactor preserves multi-return slot semantics (add a test).
  4. Add a test for ---@cast + --[[@as]] interaction on the same expression.
  5. Clarify the TagCast guard fallthrough in the match.

@CppCXY
CppCXY merged commit a0ce151 into EmmyLuaLs:main Oct 14, 2026
17 checks passed
@lewis6991
lewis6991 deleted the rewrite-inline-as branch October 14, 2026 06:49
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