Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
1 change: 1 addition & 0 deletions docs/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |

---
Expand Down
3 changes: 2 additions & 1 deletion docs/todo.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand All @@ -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 |
Expand Down
101 changes: 58 additions & 43 deletions docs/todo/diagnostics.md
Original file line number Diff line number Diff line change
Expand Up @@ -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**
Expand Down Expand Up @@ -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
<?php
namespace First {
return; // ends the whole file
}

namespace Second {
echo 'never'; // not reported
}
```

and, inside a function:

```php
return;

declare(ticks=1) {
echo 'never'; // not reported
}
```

Neither wrapper is itself a runtime statement, so neither should be
dimmed — but the state on either side of it has to carry through.

**Fix:** Thread the reachable/unreachable state through both wrappers
instead of restarting the scan inside them.
84 changes: 84 additions & 0 deletions examples/php/diagnostics.php
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,90 @@ public function fromOutside(): void
}


// ── Diagnostic: Unreachable Code ────────────────────────────────────────────
// A statement after one that always leaves the block never runs. These are
// dimmed rather than underlined: dead code is tidying, not a defect. The
// check reads the shape of the statement list only, so it needs no type
// resolution and keeps up with typing.

class UnreachableCodeDemo
{
public function afterReturn(): void
{
return;

// Dimmed — nothing after the return can run:
echo 'never';
}

public function everyBranchLeaves(bool $flag): string
{
if ($flag) {
return 'yes';
} elseif ($flag) {
throw new \RuntimeException('impossible');
} else {
return 'no';
}

// Dimmed — all three branches leave, so nothing follows them:
echo 'never';
}

public function oneBranchFallsThrough(bool $flag): void
{
if ($flag) {
return;
}

// No diagnostic — an `if` with no `else` always has a path that
// reaches here:
echo 'reached when the flag is false';
}

public function insideALoop(array $items): void
{
foreach ($items as $item) {
if ($item === null) {
continue;

// Dimmed — `continue` leaves the iteration:
echo 'never';
}
}

// No diagnostic — the loop body may never run at all, so what
// follows the loop is reachable:
echo 'reached';
}

public function nestedDeclaration(): void
{
return;

// Dimmed — a declaration inside a function body is created by
// running the statement, so this one never comes into being.
// A `function` or `class` written at the top level of a file is
// hoisted instead, and is left alone there.
function neverDeclared(): void {}
}

public function jumpTarget(): void
{
goto tail;

// Dimmed — unreachable by falling through:
echo 'never';

tail:

// No diagnostic — a label is an entry point, so the dead run ends
// here:
echo 'reached by the jump';
}
}


// ── Diagnostic: Docblock Contradicts the Type Hint ──────────────────────────
// A `@param` or `@return` tag refines the native declaration; it must not
// disagree with it. When the signature admits `null` and the tag does not,
Expand Down
12 changes: 12 additions & 0 deletions src/diagnostics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@
//! from its path.
//! - **Class name mismatch diagnostics** — report a single-class file
//! whose class name disagrees with the name PSR-4 expects for its path.
//! - **Unreachable code diagnostics** — dim the statements that follow one
//! which always leaves the block (`return`, `throw`, `exit`, `die`,
//! `continue`, `break`, or an `if` whose every branch does). Reads the
//! shape of the statement list only, so it needs no type resolution.
//!
//! ## Phase 2 — slow (require type resolution)
//!
Expand Down Expand Up @@ -250,6 +254,7 @@ pub(crate) mod undefined_variables;
pub(crate) mod unknown_classes;
pub(crate) mod unknown_functions;
pub(crate) mod unknown_members;
pub(crate) mod unreachable_code;
pub(crate) mod unresolved_member_access;
mod unused_imports;
pub(crate) mod unused_variables;
Expand Down Expand Up @@ -313,12 +318,19 @@ impl Backend {
content: &str,
out: &mut Vec<Diagnostic>,
) {
// 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,
Expand Down
Loading
Loading