[lint] Create a new lint check for unreachable code warning#19485
[lint] Create a new lint check for unreachable code warning#19485
Conversation
vineethk
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Differential Security Review — PR #19485: unreachable_code lint check
Scope: e65a8d1..38fb1ce — 31 files, +464 / -0 lines
Modules touched: move-compiler-v2/src/lib.rs, move-model/src/model.rs, move-linter/src/stackless_bytecode_lints{.rs, /unreachable_code.rs}, 27 test/golden files
Strategy: Full trace — small change set, all files examined
Executive Summary
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| HIGH | 0 |
| MEDIUM | 0 |
| LOW | 2 |
Overall risk: Low. This is a diagnostics-only addition to the Move compiler v2 lint pipeline. It has no effect on on-chain execution, consensus, VM, or transaction validation. The two LOW findings are a performance characteristic and a fragile implicit pipeline dependency — neither is exploitable.
Recommendation: APPROVE WITH NOTES
What Changed
| File | Change | Risk |
|---|---|---|
move-model/src/model.rs |
New public accessor inlined_from_loc() on Loc |
Low |
move-compiler-v2/src/lib.rs |
Wire UnreachableCodeProcessor into lint-check pipeline |
Low |
move-linter/src/stackless_bytecode_lints.rs |
Register UnreachableCode checker in default pipeline |
Low |
move-linter/src/stackless_bytecode_lints/unreachable_code.rs |
New lint implementation (99 lines) | Low |
| 27 test/golden files | New tests + updated golden for true-positive in existing test | Low |
The change is entirely confined to the static analysis tool path. The LINT_CHECKS experiment defaults to false, so the new processor only runs when the linter is explicitly invoked.
Findings
[LOW] Finding 1 — O(N²) scan in check() for functions with large dead regions
File: third_party/move/tools/move-linter/src/stackless_bytecode_lints/unreachable_code.rs:60
Blast radius: 1 call site (within the lint tool only)
Test coverage: No adversarial size test
The filter that suppresses compiler-synthesized scaffolding performs a linear scan of reachable_locs for every dead instruction:
if reachable_locs.iter().any(|r| encloses_by_span(&loc, r)) {
continue;
}reachable_locs is a BTreeSet that accumulates one entry per reachable instruction plus one per inlining call-site. For a function with N bytecode instructions, the set can hold up to 2N entries. Since .iter().any() is O(|set|), this loop is O(N²) per function. For heavily inlined or macro-generated functions, or functions with many dead blocks, this degrades proportionally.
Concrete impact: No current exploit. The lint tool has no security boundary. Compiler limits on function body size bound the worst case. However, a developer running the linter on unusually large generated code (e.g., large match expansions with dead arms) may experience noticeable slowdown.
[LOW] Finding 2 — Silent no-op when ReachableStateAnnotation is absent
File: third_party/move/tools/move-linter/src/stackless_bytecode_lints/unreachable_code.rs:26–28
Blast radius: All functions checked by the UnreachableCode lint when LINT_CHECKS is on
let Some(annotation) = target.get_annotations().get::<ReachableStateAnnotation>() else {
return;
};If UnreachableCodeProcessor is ever removed from the lint pipeline (or inserted after LintProcessor by accident), this early return silently swallows every function — the lint produces zero warnings with no indication that analysis was skipped. The contract between UnreachableCodeProcessor and UnreachableCode is purely positional/implicit; there is no assertion or defensive check that the prerequisite was satisfied.
The symmetric pattern in UnreachableCodeRemover (in the optimization pipeline) uses the same ?-return idiom, so this is consistent with the existing codebase convention. Nonetheless, for the lint pipeline specifically, silent omission of warnings is harder to notice than a silent skip of an optimization.
Concrete impact: No current exploit. The PR correctly inserts the processor before the lint checker (lib.rs:629). This finding is a robustness gap, not a present bug.
Test Coverage Assessment
| Changed Function | Coverage | Notes |
|---|---|---|
UnreachableCode::check |
Well-tested | 10 Move test cases covering abort, return, loops, labels, inlining, multi-region, skip attributes |
call_site_loc |
Well-tested | Covered via unreachable_code_inlined.{move,exp} — verifies no false positive on inlined abort |
encloses_by_span |
Partially tested | Happy paths (loop scaffolding, merge labels) covered; no test for cross-file file_id mismatch; no adversarial size test |
UnreachableCode::flush |
Well-tested | Empty-run guard verified implicitly |
Loc::inlined_from_loc |
Tested indirectly | Exercised through call_site_loc |
Updated golden (cyclomatic_complexity_warn.exp) |
Validated | Confirms the one true positive found when running on framework code (complex_state_machine at line 451 has an unreachable counter after an infinite loop) |
No coverage gaps with security implications.
Blast Radius
The new UnreachableCodeProcessor insertion in lib.rs:629 is the only structural pipeline change. Callers of stackless_bytecode_check_pipeline:
lib.rs:121—run_checker(compiler check mode)lib.rs:218—run_compiler_for_analysis(analysis mode)
Both are already gated on LINT_CHECKS. The optimization pipeline already had its own independent UnreachableCodeProcessor instance (lib.rs:676) under DEAD_CODE_ELIMINATION; the two pipelines are independent and do not share state.
Loc::inlined_from_loc() is a pure accessor with a single caller (call_site_loc in the lint). No existing callers are affected.
Notable Design Observations (non-findings)
Two-pass architecture is correct. The first pass collecting reachable_locs (including call-site locs for inlined instructions) before the second pass filtering dead instructions is necessary. Without it, synthesized jump/label instructions that reuse a parent AST node's Loc would generate false positives, because a dead synthesized back-jump could share the Loc of an enclosing reachable loop body. The comment at lines 31–39 explains this clearly.
Loc::enclosing behavior with mixed-file slices. enclosing silently ignores locs from files other than the first location in the slice. In practice a single bytecode function's instructions always belong to the same file, so this cannot affect flush. However, the function's contract is subtly incomplete for the general case.
Inlining-chain acyclicity. call_site_loc traverses the inlined_from_loc chain with a while let loop. Move disallows recursive inline functions (the compiler rejects them), so this chain is guaranteed finite — no infinite-loop risk.
True positive found on framework code. The updated cyclomatic_complexity_warn.exp golden confirms the lint correctly identifies counter at line 451 of the test file as unreachable (it follows an infinite loop {} with no break). This is a real user-visible dead-code pattern that the lint correctly catches.
Sent by Cursor Automation: Security Review Bot
vineethk
requested review from
calintat,
Copilot,
georgemitenkov,
rahxephon89,
vgao1996 and
wrwg
There was a problem hiding this comment.
Pull request overview
This PR adds a new default-on unreachable_code lint to the Move linter, leveraging compiler v2’s stackless-bytecode reachability analysis to warn only on user-authored unreachable regions (while filtering compiler-synthesized/inlined artifacts).
Changes:
- Introduces a new stackless-bytecode lint
unreachable_codebacked byReachableStateAnnotation. - Wires
UnreachableCodeProcessorinto the compiler v2 lint pipeline so the lint has the required reachability annotations. - Adds comprehensive golden tests for suppression (
#[lint::skip]), multiple dead regions, loops/labels, inlining artifacts, and control expressions as terminators; updates one existing expected-output baseline to include the new warning.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| third_party/move/tools/move-linter/src/stackless_bytecode_lints/unreachable_code.rs | Implements the new unreachable-code lint on stackless bytecode using reachability annotations and Loc-span filtering. |
| third_party/move/tools/move-linter/src/stackless_bytecode_lints.rs | Registers the new lint in the default stackless-bytecode lint pipeline. |
| third_party/move/move-model/src/model.rs | Adds Loc::inlined_from_loc() accessor to traverse inlining chains for accurate reporting/filtering. |
| third_party/move/move-compiler-v2/src/lib.rs | Adds UnreachableCodeProcessor before LintProcessor when LINT_CHECKS is enabled to provide prerequisites. |
| third_party/move/tools/move-linter/tests/model_ast_lints/cyclomatic_complexity_warn.exp | Updates baseline output to include the new unreachable_code warning. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_skip_module.move | Test: module-level #[lint::skip(unreachable_code)] suppresses warnings across functions. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_skip_module.exp | Baseline: no warnings expected with module-level skip. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_skip_function.move | Test: function-level skip suppresses only the annotated function’s warning. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_skip_function.exp | Baseline: warning only for non-skipped function. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_return_in_binop.move | Test: return in expression/binop contexts makes following code unreachable. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_return_in_binop.exp | Baseline: single span warning covering the unreachable tail region. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_no_warning.move | Test: functions without unreachable code produce no diagnostics. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_no_warning.exp | Baseline: no warnings expected. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_multiple_regions.move | Test: two disjoint dead regions should yield two warnings. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_multiple_regions.exp | Baseline: two distinct warnings/spans. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_loop_only.move | Test: loop {} makes subsequent code unreachable. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_loop_only.exp | Baseline: warning on the unreachable expression after the loop. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_loop_labels.move | Test: labeled/unlabeled nested loops and control flow produce dead regions. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_loop_labels.exp | Baseline: warnings for unreachable break sites. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_inlined.move | Test: inlining artifacts should not produce unreachable-code warnings. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_inlined.exp | Baseline: no warnings expected for inlining-only dead artifacts. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_control_exp_as_term.move | Test: control expressions as terminators inside expressions/arguments. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_control_exp_as_term.exp | Baseline: warning span across the unreachable tail region. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_conditional_loop.move | Test: unreachable regions inside conditional loops (two dead regions). |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_conditional_loop.exp | Baseline: two warnings for the two dead regions. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_break_unreachable.move | Test: unreachable statements after break/continue, plus unreachable after conditional. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_break_unreachable.exp | Baseline: warnings for unreachable statements/regions after control flow. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_after_abort.move | Test: unreachable code after abort. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_after_abort.exp | Baseline: warning for unreachable expression after abort. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_abort_or_return_always.move | Test: if where both branches terminate makes subsequent code unreachable. |
| third_party/move/tools/move-linter/tests/default-only/unreachable_code_abort_or_return_always.exp | Baseline: warning on the unreachable trailing expression. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wrwg
left a comment
wrwg
left a comment
There was a problem hiding this comment.
Nice. IIRC the reason why we didn't create some compiler errors is because of inline functions, but you seem to have managed this one. This could be BTW also a compiler error, but for avoiding breaking existing code, linter is better choice.
| if annotation.is_definitely_not_reachable(offset as CodeOffset) { | ||
| let loc = target.get_bytecode_loc(instr.get_attr_id()); | ||
| // Skip code from inlining — not actionable at this site. | ||
| if loc.is_inlined() { | ||
| continue; | ||
| } | ||
| // Skip Locs of compiler-synthesized scaffolding (merge labels, | ||
| // back-jumps, trailing `Ret`): they reuse a parent AST node's | ||
| // Loc that physically wraps a reachable sibling instruction. | ||
| if reachable_locs.iter().any(|r| encloses_by_span(&loc, r)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The continue in the inlined and scaffolding skip paths intentionally doesn't flush current_run, so dead instructions separated only by skipped instructions merge into one warning. This seems correct — the user perceives them as one dead region. But a reader might wonder whether flush should happen before continue. Worth a one-line comment like:
// NB: `continue` (not `flush`) — skipped instructions are transparent
// to run-merging so the user sees one contiguous warning.
| } | ||
|
|
||
| /// Span-only enclosure (ignores `inlined_from_loc`, unlike `Loc::is_enclosing`). | ||
| fn encloses_by_span(outer: &Loc, inner: &Loc) -> bool { | ||
| outer.file_id() == inner.file_id() | ||
| && inner.span().start() >= outer.span().start() | ||
| && inner.span().end() <= outer.span().end() | ||
| } |
There was a problem hiding this comment.
This heuristic relies on a compiler invariant: synthesized bytecode instructions (merge labels, back-jumps, trailing Ret) inherit the Loc of their parent AST node rather than getting a distinct Loc. The filter detects this by checking whether a dead instruction's Loc physically wraps a reachable instruction's Loc.
Consider adding a brief doc note about this assumption, e.g.:
/// This works because the compiler assigns synthesized instructions the
/// Loc of their enclosing AST node; if that ever changes, this heuristic
/// will need updating.
This would help future maintainers understand why the filter exists and when it might break.
| public fun caller() { | ||
| terminator(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Nice test for the false-positive prevention on inlined terminators. Consider adding a complementary test where the inline function itself has genuinely dead code in its body, e.g.:
inline fun dead_in_body(): u64 {
abort 0;
42 // genuinely dead, but won't warn because it's inlined at call site
}
public fun caller2(): u64 { dead_in_body() }Currently the is_inlined() skip means such dead code is a known false negative. Documenting that in a test expectation would make the design decision explicit and prevent a future maintainer from treating it as a bug.
vineethk
force-pushed
the
vk/unreachable-warning
branch
from
084fed8 to
866244e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
An
unreachable_codelint that warns on unreachable user-written code (i.e., what existing unreachable code analysis pass on stackless bytecode can prove is dead). Uses location spans of what is reachable and not reachable to disambiguate compiler inserted code vs. user code.This will improve as more intelligence is added to unreachable code analysis in the compiler.
We may eventually make this a compiler warning.
Closes #11707.
How Has This Been Tested?
Added lots of tests and ported some from the compiler suite.
Ran on framework code and found one true positive.
Type of Change
Which Components or Systems Does This Change Impact?
Note
Medium Risk
Adds a new default-enabled linter warning and wires additional analysis into the compiler v2 lint pipeline, which can change diagnostics and potentially impact build/test baselines.
Overview
Adds a new
unreachable_codestackless-bytecode lint that warns on definitely unreachable user-written code using reachability annotations, with heuristics to avoid flagging compiler-synthesized and inlined regions.Wires
UnreachableCodeProcessorinto the compiler v2LINT_CHECKSpipeline as a prerequisite for linting, extendsLocwithinlined_from_loc()to walk inlining chains, and adds a comprehensive set of linter golden tests covering abort/return, loops/break/continue, expression-terminators, inlining behavior, and skip attributes.Reviewed by Cursor Bugbot for commit 866244e. Bugbot is set up for automated code reviews on this repo. Configure here.