From c65dfc65f55fe4f4183fb81ffe8af54f9819bcbe Mon Sep 17 00:00:00 2001 From: petrovo-as Date: Wed, 19 Aug 2026 16:32:26 +0200 Subject: [PATCH] Dim the code that cannot be reached MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A statement after one that always leaves the block never runs, and nothing said so. `return`, `throw`, `exit`, `die`, `continue`, `break`, and an `if` whose every branch does one of those all end a block; an `if` without an `else` never does, since a path through it always falls through. Reported as a Hint tagged `Unnecessary`, so editors grey the text out rather than underlining it — dead code is tidying, not a defect, and it is rendered the way an unused import already is. The check reads the shape of the statement list and nothing else, which keeps it in the fast phase alongside the syntax and unused-symbol checks. Two things only look dead. A declaration at the top level of a file is hoisted, so it holds whether or not control reaches the line it is written on, while the same declaration inside a function body is created by running the statement and after a `return` never comes into being at all — the two are told apart by where they sit rather than by what they are. A `goto` label is an entry point, so it ends the dead run rather than being swallowed by it. Either can sit in the middle of otherwise dead code, so one block may hold several runs and each is reported on its own. A call to a function declared `never` also ends a block. Recognising it takes the type engine, which would move this check into the expensive phase to catch a case the reader can already see, so it is left to the narrowing code that already has the types in hand. Co-Authored-By: Claude Opus 5 --- docs/CHANGELOG.md | 1 + docs/cli.md | 1 + docs/todo.md | 3 +- docs/todo/diagnostics.md | 101 +-- examples/php/diagnostics.php | 84 +++ src/diagnostics/mod.rs | 12 + src/diagnostics/unreachable_code.rs | 445 +++++++++++ .../diagnostics_unreachable_code.rs | 695 ++++++++++++++++++ tests/integration/main.rs | 1 + 9 files changed, 1299 insertions(+), 44 deletions(-) create mode 100644 src/diagnostics/unreachable_code.rs create mode 100644 tests/integration/diagnostics_unreachable_code.rs diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 9d9e256f7..6bb8611c0 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -206,6 +206,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 #### Diagnostics - **A `private` or `protected` member reached from outside is reported where you write it.** PHP resolves a member access to a declaration first and enforces that declaration's visibility second, so reading `$account->pin` on a class that keeps `$pin` private is a fatal error rather than a missing property — but it used to be reported as neither. Properties, methods, class constants, and static properties are all checked, against the class that *declares* the member rather than the one the access happened to go through, so a member inherited from a shared parent stays reachable from every branch below it while one declared on a sibling does not. A parent's private member, which PHP does not inherit at all, is now named as the access violation it is instead of looking like a member that does not exist. The check stands down wherever PHP itself would not fail: a class whose magic methods answer for members the caller cannot reach directly is left alone, a trait's members belong to whichever class uses it, and a `@see` tag documents a member rather than reading one. Contributed by @petrovo-as. +- **Code that cannot be reached is dimmed.** A statement after a `return`, `throw`, `exit`, `die`, `continue`, `break`, or `goto` — or after an `if` whose every branch does one of those — never runs, and is now greyed out the way an unused import is, since dead code is tidying rather than a defect. The check reads the shape of the statement list alone, so it keeps up with typing. What only looks dead is left alone: a declaration at the top level of a file is hoisted and holds wherever it sits, while the same declaration inside a function body is created by running the statement and really is dead, and a `goto` label is an entry point that ends the dead run rather than being swallowed by it. Contributed by @petrovo-as. - **Two new diagnostics: illegal `readonly` writes and self-contradicting docblocks.** A write to a `readonly` property from anywhere PHP forbids one, and a `@param` or `@return` tag that contradicts the nullability of the declaration it documents, are now reported where you write them rather than when the code runs. Every form the readonly write can take is checked, including the ones that are easy to overlook (`unset()`, a `foreach` or destructuring target, taking a reference), and the writes the language allows are left alone. - **Four new declaration diagnostics.** An enum whose cases do not agree with its backing, a redeclaration that drops `static` from an inherited return type, an abstract trait method nothing implements (with the "Implement missing methods" code action stubbing it alongside the rest), and a `match` arm whose literal can never equal the subject. Contributed by @calebdw. diff --git a/docs/cli.md b/docs/cli.md index 536c88a27..1797920fe 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -256,6 +256,7 @@ Each has a rule identifier shown below the message. | `scalar_member_access` | Error | Member access on a scalar type (int, string, etc.) | | `invalid_member_access` | Error | `private` or `protected` member reached from outside | | `unused_import` | Hint | `use` statement with no references in the file | +| `unreachable_code` | Hint | Statements after a `return`, `throw`, `exit`, or `break` | | `deprecated` | Hint | Reference to a `@deprecated` symbol | --- diff --git a/docs/todo.md b/docs/todo.md index 5a027808d..b086b726d 100644 --- a/docs/todo.md +++ b/docs/todo.md @@ -100,7 +100,6 @@ unlikely to move the needle for most users. | B320 | [An unclosed echo swallows the `@end…` of the block it sits in](todo/bugs.md#b320-an-unclosed-echo-swallows-the-end-of-the-block-it-sits-in) | Low-Medium | Medium | | B321 | [Echo-delimiter hover fires on `{{` that is not an echo](todo/bugs.md#b321-echo-delimiter-hover-fires-on--that-is-not-an-echo) | Low | Low | | | **[Diagnostics](todo/diagnostics.md)** | | | -| D6 | [Unreachable code diagnostic](todo/diagnostics.md#d6-unreachable-code-diagnostic) | Low-Medium | Medium | | D16 | [`unreachable_match_arm` ignores literal subject types](todo/diagnostics.md#d16-unreachable_match_arm-ignores-literal-subject-types) | Low-Medium | Medium | | D5 | [External tool diagnostic suppression actions](todo/diagnostics.md#d5-external-tool-diagnostic-suppression-actions) | Low | Low | | D15 | [Unused parameter diagnostic](todo/diagnostics.md#d15-unused-parameter-diagnostic) | Low | Medium | @@ -111,6 +110,8 @@ unlikely to move the needle for most users. | D21 | [A union of an unreachable and a missing member is reported by neither check](todo/diagnostics.md#d21-a-union-of-an-unreachable-and-a-missing-member-is-reported-by-neither-check) | Low | Medium-High | | D22 | [Member provenance is recomputed instead of recorded](todo/diagnostics.md#d22-member-provenance-is-recomputed-instead-of-recorded) | Medium | Medium-High | | D23 | [A rebound closure's scope is added to the lexical one rather than replacing it](todo/diagnostics.md#d23-a-rebound-closures-scope-is-added-to-the-lexical-one-rather-than-replacing-it) | Low-Medium | Medium | +| D24 | ["Remove unreachable code" is wired to PHPStan only](todo/diagnostics.md#d24-remove-unreachable-code-is-wired-to-phpstan-only) | Low-Medium | Medium | +| D25 | [`namespace` and `declare` bodies break the reachability flow](todo/diagnostics.md#d25-namespace-and-declare-bodies-break-the-reachability-flow) | Low | Low-Medium | | | **[Code Actions](todo/actions.md)** | | | | A40 | [Generate method from call](todo/actions.md#a40-generate-method-from-call) | Medium-High | Medium-High | | A28 | [Explicit nullable parameter type](todo/actions.md#a28-explicit-nullable-parameter-type-php-84-deprecation) (PHP 8.4 deprecation) | Medium | Low | diff --git a/docs/todo/diagnostics.md b/docs/todo/diagnostics.md index ba8e1f4e6..6cb0e2326 100644 --- a/docs/todo/diagnostics.md +++ b/docs/todo/diagnostics.md @@ -43,49 +43,6 @@ proxies: --- -## D6. Unreachable code diagnostic - -**Impact: Low-Medium · Complexity: Medium** - -Dim code that appears after unconditional control flow exits: -`return`, `throw`, `exit`, `die`, `continue`, `break`. This is a -Phase 1 (fast) diagnostic since it requires only AST structure, not -type resolution. - -### Behaviour - -| Scenario | Rendering | -| -------------------------------------------------- | ----------------------------------- | -| Code after `return $x;` in same block | Dimmed (DiagnosticTag::UNNECESSARY) | -| Code after `throw new \Exception()` | Dimmed | -| Code after `exit(1)` or `die()` | Dimmed | -| Code after `continue` or `break` in a loop | Dimmed | -| Code after `if (...) { return; } else { return; }` | Dimmed (both branches exit) | - -Severity: **Hint** with `DiagnosticTag::UNNECESSARY` so editors dim -the text rather than underlining it. This matches how unused imports -are rendered. - -### Implementation - -Walk the AST statement list. After encountering a statement that -unconditionally exits the current scope (return, throw, expression -statement containing `exit`/`die`), mark all subsequent statements in -the same block as unreachable. The span covers from the start of the -first unreachable statement to the end of the last statement in the -block. - -Phase 1 only handles the simple single-block case. Whole-branch -analysis (both if/else branches exit) is a future refinement. - -### Debugging value - -When our type engine silently resolves a method to a `never` return -type (e.g. an incorrectly resolved overload), unreachable code after -the call becomes visible, signalling the bug. - ---- - ## D10. PHPMD diagnostic proxy **Impact: Low · Complexity: Medium** @@ -400,3 +357,61 @@ member is out of reach. it, and let it replace the enclosing class rather than joining it. Inferring a binding from the spelling of the subject is guessing at something the type engine has already decided. +## D24. "Remove unreachable code" is wired to PHPStan only + +**Impact: Low-Medium · Complexity: Medium** + +The action reads `phpstan_tool.last_diags` and nothing else +(`code_actions/phpstan/remove_unreachable.rs`), so the native +`unreachable_code` diagnostic never offers it. Adding the code to the +trigger is not enough on its own: the resolve step deletes from the +diagnostic's line to the next closing brace rather than using the +diagnostic's own range, which + +- has nothing to delete for a dead run at the top level of a file, where + no closing brace follows; +- ignores the reported span, so it would remove more than was dimmed; +- can swallow a hoisted declaration or a `goto` label sitting inside the + run, both of which the diagnostic deliberately leaves reachable. + +**Fix:** Take the range from the diagnostic and carry it through to the +resolve payload, and let the action accept a native diagnostic rather +than only a proxied one. Moving the file out of `phpstan/` is the +smallest part of it. + +--- + +## D25. `namespace` and `declare` bodies break the reachability flow + +**Impact: Low · Complexity: Low-Medium** + +`unreachable_code` treats a braced `namespace` and a `declare` body as +fresh statement lists rather than as the transparent wrappers they are, +so reachability neither flows into them nor out of them: + +```php +, ) { + // Four of these parse the file. Without a shared cache each would + // parse it again, on every keystroke; the guard makes them reuse one + // AST. A nested guard is a no-op, so the workspace and analyze paths + // that already hold one are unaffected. + let _parse_guard = crate::parser::with_parse_cache(content); + self.collect_syntax_error_diagnostics(uri_str, content, out); self.collect_unused_import_diagnostics(uri_str, content, out); self.collect_unused_variable_diagnostics(uri_str, content, out); self.collect_namespace_mismatch_diagnostics(uri_str, content, out); self.collect_class_name_mismatch_diagnostics(uri_str, content, out); self.collect_docblock_native_mismatch_diagnostics(uri_str, content, out); + self.collect_unreachable_code_diagnostics(uri_str, content, out); } /// Collect Phase 2 (slow) diagnostics: unknown class/member/function, diff --git a/src/diagnostics/unreachable_code.rs b/src/diagnostics/unreachable_code.rs new file mode 100644 index 000000000..564698ea9 --- /dev/null +++ b/src/diagnostics/unreachable_code.rs @@ -0,0 +1,445 @@ +//! Unreachable code diagnostics. +//! +//! Statements that follow one which always leaves the block cannot run: +//! +//! ```php +//! function f(): void { +//! return; +//! echo 'never'; // dimmed +//! } +//! ``` +//! +//! Reported as a `Hint` carrying [`DiagnosticTag::UNNECESSARY`], so editors +//! grey the text out rather than underlining it. Dead code is tidying, not +//! a defect, and it is rendered the way unused imports already are. +//! +//! This is a Phase 1 check: it reads the shape of the statement list and +//! nothing else, so it needs no type resolution and runs on every keystroke +//! alongside the syntax and unused-symbol checks. +//! +//! ## What counts as leaving the block +//! +//! `return`, `throw`, `exit`, `die`, `continue`, `break`, and `goto`, plus a +//! block or an `if` whose every branch does one of those — an `if` without an +//! `else` always has a path that falls through, so it never qualifies. +//! +//! A call to a function declared `never` leaves too, and is deliberately not +//! recognised here: knowing that takes the type engine, which would move this +//! check into the expensive phase to catch a case the reader can already see. +//! [`crate::type_engine`] has its own answer to this question for narrowing, +//! where the types are already in hand. +//! +//! ## What is not dead +//! +//! A declaration at the top level of a file is hoisted — PHP binds it before +//! the script runs, so it exists whether or not control reaches the line it is +//! written on. The same declaration nested inside a function body is not +//! hoisted: it is created by executing that statement, so after a `return` it +//! really is dead. The two cases are told apart by where they sit, not by +//! what kind of statement they are. Imports and the tags around them declare +//! nothing at runtime and hold wherever they sit. +//! +//! A `goto` label is an entry point. Code after one is reachable by jumping +//! to it, so a label ends the dead run rather than being swallowed by it — +//! whether any `goto` actually targets it is not something this check tries to +//! establish. For the same reason a `goto` ends a run only inside the list it +//! is written in: a jump to a label in the same block leaves the block running, +//! and without resolving the label the two cannot be told apart. +//! +//! Since both of those can sit in the middle of otherwise dead code, one block +//! can hold several separate dead runs, and each is reported on its own. + +use mago_span::HasSpan; +use mago_syntax::cst::*; +use tower_lsp::lsp_types::*; + +use crate::Backend; +use crate::parser::with_parsed_program; + +use super::helpers::make_diagnostic; + +/// Diagnostic code for code that cannot be reached. +pub(crate) const UNREACHABLE_CODE_CODE: &str = "unreachable_code"; + +/// A run of statements that cannot be reached, as byte offsets. +struct DeadRun { + start: u32, + end: u32, +} + +impl Backend { + /// Collect unreachable-code diagnostics for a single file. + /// + /// Appends diagnostics to `out`. The caller is responsible for + /// publishing them. + pub fn collect_unreachable_code_diagnostics( + &self, + uri: &str, + content: &str, + out: &mut Vec, + ) { + let runs = with_parsed_program(content, "unreachable_code", |program, _content| { + let mut runs = Vec::new(); + collect_from_program(program, &mut runs); + collect_from_function_expressions(program, &mut runs); + runs + }); + + for run in runs { + let Some(range) = + self.offset_range_to_lsp_range(uri, content, run.start as usize, run.end as usize) + else { + continue; + }; + let mut diagnostic = make_diagnostic( + range, + DiagnosticSeverity::HINT, + UNREACHABLE_CODE_CODE, + "Unreachable code".to_string(), + ); + diagnostic.tags = Some(vec![DiagnosticTag::UNNECESSARY]); + out.push(diagnostic); + } + } +} + +/// Walk a file's top-level statements. +/// +/// A `return` here ends the script, so the top level has dead runs like any +/// block — it differs only in that its declarations are hoisted. +fn collect_from_program(program: &Program<'_>, runs: &mut Vec) { + scan_statements(program.statements.iter(), Scope::TopLevel, runs); +} + +/// Walk the bodies that hang off expressions rather than statements. +/// +/// A closure or an anonymous class is written inside an expression, so the +/// statement recursion above never reaches it — and a callback with dead code +/// in it is ordinary PHP. The walker finds them wherever they are nested, +/// including inside each other, and the two passes cannot overlap because the +/// statement recursion never descends into an expression. +fn collect_from_function_expressions(program: &Program<'_>, runs: &mut Vec) { + let collector = FunctionExpressionBodies; + mago_syntax::walker::Walker::walk_program(&collector, program, runs); +} + +struct FunctionExpressionBodies; + +impl<'ast, 'arena> mago_syntax::walker::Walker<'ast, 'arena, Vec> + for FunctionExpressionBodies +{ + fn walk_in_closure(&self, closure: &'ast Closure<'arena>, runs: &mut Vec) { + scan_block(&closure.body, runs); + } + + fn walk_in_anonymous_class( + &self, + class: &'ast AnonymousClass<'arena>, + runs: &mut Vec, + ) { + scan_members(&class.members, runs); + } +} + +/// Whether a statement list is the file's top level, which decides only one +/// thing: whether a declaration in it is hoisted. +#[derive(Clone, Copy, PartialEq, Eq)] +enum Scope { + TopLevel, + Nested, +} + +/// Scan one statement list for dead runs, then descend into every statement +/// it holds. +fn scan_statements<'a>( + statements: impl Iterator>, + scope: Scope, + runs: &mut Vec, +) { + let statements: Vec<&Statement<'_>> = statements.collect(); + + let mut dead_from: Option = None; + for (index, statement) in statements.iter().enumerate() { + match dead_from { + // Already past a statement that left the block. A label makes + // what follows reachable again, so it closes the run it + // interrupts rather than joining it. + Some(start) => { + if matches!(statement, Statement::Label(_)) { + push_run(&statements[start..index], scope, runs); + dead_from = None; + } + } + None => { + if ends_run(statement) && index + 1 < statements.len() { + dead_from = Some(index + 1); + } + } + } + } + if let Some(start) = dead_from { + push_run(&statements[start..], scope, runs); + } + + for statement in statements { + descend(statement, runs); + } +} + +/// Record one contiguous dead run, minus anything in it that is not dead. +/// +/// A hoisted declaration inside the run keeps its own meaning, so the run is +/// split around it instead of covering it — dimming a class that PHP has +/// already bound would be a lie about what the code does. +fn push_run(statements: &[&Statement<'_>], scope: Scope, runs: &mut Vec) { + let mut segment_start: Option = None; + let mut segment_end: u32 = 0; + + for statement in statements { + if is_hoisted(statement, scope) { + if let Some(start) = segment_start.take() { + runs.push(DeadRun { + start, + end: segment_end, + }); + } + continue; + } + let span = statement.span(); + if segment_start.is_none() { + segment_start = Some(span.start.offset); + } + segment_end = span.end.offset; + } + + if let Some(start) = segment_start { + runs.push(DeadRun { + start, + end: segment_end, + }); + } +} + +/// Whether a statement is bound before the code around it runs, and so keeps +/// its meaning wherever it is written. +/// +/// Imports and the tags around them declare nothing at runtime and hold +/// wherever they sit. A `function` or `class` is hoisted only at the top +/// level of a file: nested inside a function body it is created by executing +/// the statement, so after a `return` it never comes into being at all. +fn is_hoisted(statement: &Statement<'_>, scope: Scope) -> bool { + if matches!( + statement, + Statement::Use(_) + | Statement::Namespace(_) + | Statement::Declare(_) + | Statement::OpeningTag(_) + | Statement::ClosingTag(_) + | Statement::Noop(_) + ) { + return true; + } + scope == Scope::TopLevel + && matches!( + statement, + Statement::Function(_) + | Statement::Class(_) + | Statement::Interface(_) + | Statement::Trait(_) + | Statement::Enum(_) + | Statement::Constant(_) + ) +} + +/// Whether this statement ends the run of reachable code in its own list. +/// +/// That is [`leaves_block`] plus `goto`, which hands control to its label. +/// `goto` counts only here, never when the result is propagated outwards by +/// [`leaves_block`]: a jump whose label sits in the same block leaves the +/// block running perfectly well, and without resolving the label there is no +/// way to tell which kind it is. Treating it as an exit out there would dim +/// live code, so it is treated as one only where a label can end the run. +fn ends_run(statement: &Statement<'_>) -> bool { + matches!(statement, Statement::Goto(_)) || leaves_block(statement) +} + +/// Whether every path through this statement leaves the enclosing block. +/// +/// Mirrors the syntactic half of +/// [`crate::type_engine`]'s `statement_unconditionally_exits`, which answers +/// the same question with types in hand so it can also recognise a call to a +/// `never`-returning function. Sharing one implementation would drag the +/// resolver into a check that deliberately runs without it. +fn leaves_block(statement: &Statement<'_>) -> bool { + match statement { + Statement::Return(_) | Statement::Continue(_) | Statement::Break(_) => true, + // `throw`, `exit`, and `die` are expressions in PHP, so they reach + // here wrapped in an expression statement. + Statement::Expression(expression) => { + matches!( + expression.expression, + Expression::Throw(_) + | Expression::Construct(Construct::Exit(_)) + | Expression::Construct(Construct::Die(_)) + ) + } + Statement::Block(block) => list_leaves_block(block.statements.iter()), + Statement::If(if_statement) => if_leaves_block(&if_statement.body), + _ => false, + } +} + +/// Whether an `if` leaves the block down every branch. +/// +/// Needs the then-branch, every `elseif`, and an `else` that exists — without +/// the `else` there is a path that runs none of them and falls through. +fn if_leaves_block(body: &IfBody<'_>) -> bool { + match body { + IfBody::Statement(body) => { + leaves_block(body.statement) + && body + .else_if_clauses + .iter() + .all(|clause| leaves_block(clause.statement)) + && body + .else_clause + .as_ref() + .is_some_and(|clause| leaves_block(clause.statement)) + } + IfBody::ColonDelimited(body) => { + list_leaves_block(body.statements.iter()) + && body + .else_if_clauses + .iter() + .all(|clause| list_leaves_block(clause.statements.iter())) + && body + .else_clause + .as_ref() + .is_some_and(|clause| list_leaves_block(clause.statements.iter())) + } + } +} + +/// Whether control is gone by the end of a statement list. +/// +/// Not the same question as "does anything in here leave": a `goto` label +/// after the exit is an entry point, so control can be back in the list and +/// run out of its end. The list has to be walked for that to show, and a +/// list that ends reachable leaves whatever encloses it running — which is +/// why asking `any` here dims the live code after a block whose exit was +/// jumped over. +/// +/// A `goto` itself does not end anything here. It ends the run it sits in +/// (see [`ends_run`]), but where its label lives is unknown, so counting it +/// as an exit would claim the enclosing block never falls through when the +/// jump may simply land further down the same list. +fn list_leaves_block<'a>(statements: impl Iterator>) -> bool { + let mut reachable = true; + for statement in statements { + if matches!(statement, Statement::Label(_)) { + reachable = true; + } else if reachable && leaves_block(statement) { + reachable = false; + } + } + !reachable +} + +/// Descend into every statement list a statement contains. +/// +/// Each body is scanned in its own right: a `return` inside one loop says +/// nothing about the statements after that loop. +fn descend(statement: &Statement<'_>, runs: &mut Vec) { + match statement { + Statement::Block(block) => scan_block(block, runs), + Statement::Namespace(namespace) => { + // A namespace declaration does not run, so what it holds is still + // the top level as far as hoisting is concerned. + scan_statements(namespace.statements().iter(), Scope::TopLevel, runs); + } + Statement::Function(function) => scan_block(&function.body, runs), + Statement::Class(class) => scan_members(&class.members, runs), + Statement::Interface(interface) => scan_members(&interface.members, runs), + Statement::Trait(r#trait) => scan_members(&r#trait.members, runs), + Statement::Enum(r#enum) => scan_members(&r#enum.members, runs), + Statement::If(if_statement) => descend_if(if_statement, runs), + Statement::Try(r#try) => { + scan_block(&r#try.block, runs); + for catch in r#try.catch_clauses.iter() { + scan_block(&catch.block, runs); + } + if let Some(finally) = &r#try.finally_clause { + scan_block(&finally.block, runs); + } + } + Statement::Foreach(foreach) => match &foreach.body { + ForeachBody::Statement(statement) => descend(statement, runs), + ForeachBody::ColonDelimited(body) => { + scan_statements(body.statements.iter(), Scope::Nested, runs) + } + }, + Statement::For(r#for) => match &r#for.body { + ForBody::Statement(statement) => descend(statement, runs), + ForBody::ColonDelimited(body) => { + scan_statements(body.statements.iter(), Scope::Nested, runs) + } + }, + Statement::While(r#while) => match &r#while.body { + WhileBody::Statement(statement) => descend(statement, runs), + WhileBody::ColonDelimited(body) => { + scan_statements(body.statements.iter(), Scope::Nested, runs) + } + }, + Statement::DoWhile(do_while) => descend(do_while.statement, runs), + Statement::Switch(switch) => { + for case in switch.body.cases() { + scan_statements(case.statements().iter(), Scope::Nested, runs); + } + } + Statement::Declare(declare) => match &declare.body { + DeclareBody::Statement(statement) => descend(statement, runs), + DeclareBody::ColonDelimited(body) => { + scan_statements(body.statements.iter(), Scope::Nested, runs) + } + }, + _ => {} + } +} + +fn scan_block(block: &Block<'_>, runs: &mut Vec) { + scan_statements(block.statements.iter(), Scope::Nested, runs); +} + +/// Walk the bodies of a class-like declaration's methods. +fn scan_members(members: &Sequence<'_, ClassLikeMember<'_>>, runs: &mut Vec) { + for member in members.iter() { + if let ClassLikeMember::Method(method) = member + && let MethodBody::Concrete(block) = &method.body + { + scan_block(block, runs); + } + } +} + +fn descend_if(if_statement: &If<'_>, runs: &mut Vec) { + match &if_statement.body { + IfBody::Statement(body) => { + descend(body.statement, runs); + for clause in body.else_if_clauses.iter() { + descend(clause.statement, runs); + } + if let Some(clause) = &body.else_clause { + descend(clause.statement, runs); + } + } + IfBody::ColonDelimited(body) => { + scan_statements(body.statements.iter(), Scope::Nested, runs); + for clause in body.else_if_clauses.iter() { + scan_statements(clause.statements.iter(), Scope::Nested, runs); + } + if let Some(clause) = &body.else_clause { + scan_statements(clause.statements.iter(), Scope::Nested, runs); + } + } + } +} diff --git a/tests/integration/diagnostics_unreachable_code.rs b/tests/integration/diagnostics_unreachable_code.rs new file mode 100644 index 000000000..782f2f129 --- /dev/null +++ b/tests/integration/diagnostics_unreachable_code.rs @@ -0,0 +1,695 @@ +use crate::common::create_test_backend; +use tower_lsp::lsp_types::*; + +// ─── Helpers ──────────────────────────────────────────────────────────────── + +fn collect(php: &str) -> Vec { + let backend = create_test_backend(); + let uri = "file:///test.php"; + backend.update_ast(uri, php); + let mut out = Vec::new(); + backend.collect_unreachable_code_diagnostics(uri, php, &mut out); + out.retain(|d| { + d.code + .as_ref() + .is_some_and(|c| matches!(c, NumberOrString::String(s) if s == "unreachable_code")) + }); + out +} + +/// The dead text each diagnostic covers, so a test can assert on what was +/// dimmed rather than on line numbers. +fn dimmed(php: &str) -> Vec { + let lines: Vec<&str> = php.lines().collect(); + collect(php) + .into_iter() + .map(|d| { + let first = lines[d.range.start.line as usize].trim(); + let last = lines[d.range.end.line as usize].trim(); + if d.range.start.line == d.range.end.line { + first.to_string() + } else { + format!("{first} … {last}") + } + }) + .collect() +} + +// ─── Each way of leaving a block ──────────────────────────────────────────── + +#[test] +fn code_after_return_is_dead() { + let php = r#"\nThis text is never emitted.\n"; + assert!( + !collect(php).is_empty(), + "inline output is a runtime statement, not a declaration" + ); +} + +// ─── Spans ────────────────────────────────────────────────────────────────── + +#[test] +fn consecutive_dead_statements_are_one_diagnostic() { + let php = r#" 0) { + break; + echo 'never in while'; + } + + for ($i = 0; $i < $n; $i++) { + continue; + echo 'never in for'; + } + + do { + break; + echo 'never in do'; + } while ($n > 0); +} +"#; + assert_eq!( + dimmed(php), + vec![ + "echo 'never in while';", + "echo 'never in for';", + "echo 'never in do';" + ] + ); +} + +// ─── Alternative syntax ───────────────────────────────────────────────────── + +#[test] +fn a_colon_delimited_if_is_scanned() { + let php = r#"