Skip to content

fix(diagnostics): function signatures, save-gated propagation, external tool scheduling, and @phpstan-ignore parsing#196

Open
calebdw wants to merge 5 commits into
PHPantom-dev:mainfrom
calebdw:calebdw/push-uqvsqztpwypy
Open

fix(diagnostics): function signatures, save-gated propagation, external tool scheduling, and @phpstan-ignore parsing#196
calebdw wants to merge 5 commits into
PHPantom-dev:mainfrom
calebdw:calebdw/push-uqvsqztpwypy

Conversation

@calebdw

@calebdw calebdw commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Five diagnostic improvements, each in its own commit:

1. Function signature change detection (update_ast)

Root cause: update_ast only tracked class signature changes. When a standalone function's parameter type changed (e.g. bar(null $x) to bar(string $x)), update_ast returned false, so schedule_diagnostics_for_open_files was never called and other open files kept showing stale diagnostics.

  • Add FunctionInfo::signature_eq() for comparing function signatures
  • Track function signature changes in update_ast: snapshot old functions before re-parse, compare signatures, clean up stale entries
  • Add did_save handler as a reliable diagnostic refresh point
  • Register save notification capability in TextDocumentSyncOptions
  • 23 tests covering signature_eq and update_ast change detection

Closes #123

2. Cross-file diagnostics only on save

Move schedule_diagnostics_for_open_files out of the didChange handler so that editing a function or class signature in one file no longer immediately refreshes diagnostics in other open files. This avoids propagating intermediate/incomplete edits to other buffers. Same-file diagnostics continue to update on every keystroke.

3. External tools run on save only, no debounce

External tool workers (PHPStan, PHPCS, Mago lint, Mago analyze) are now scheduled exclusively from did_save. They are expensive (seconds per run) and serialized (one process at a time), so running them on every keystroke would block save-triggered runs. All external tool debounce constants and logic have been removed — workers fire immediately on save.

Also adds workspace/diagnostic/refresh after each external tool worker caches new results, so editors using pull diagnostics see updates without requiring a didChange event.

4. Disable workspace diagnostics capability

Set workspace_diagnostics to false. The workspace/diagnostic handler currently only returns cached diagnostics for open files — the same data textDocument/diagnostic provides per-file. Advertising the capability causes editors to send redundant workspace-wide pull requests. Can be flipped to true when the server gains project-wide analysis (e.g. whole-project PHPStan).

5. Parse @phpstan-ignore with per-identifier reasons

The stale diagnostic filter failed to match @phpstan-ignore comments with the per-identifier reason syntax:

/** @phpstan-ignore return.type (reason text here) */

The parser stopped at */ before checking for (, so the parenthesized reason text was included in the identifier list. When the reason contained commas, identifiers split incorrectly and none matched. Fix: split by comma first, then strip the optional (reason) suffix from each entry. Three new tests cover this.

@calebdw calebdw force-pushed the calebdw/push-uqvsqztpwypy branch from 42f2bf0 to 4b6003e Compare July 5, 2026 01:43
@codecov-commenter

codecov-commenter commented Jul 5, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 93.70079% with 24 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/diagnostics/mod.rs 79.31% 24 Missing ⚠️

📢 Thoughts on this report? Let us know!

@Disservin

Copy link
Copy Markdown

hi @calebdw this already works much better! however the diagnostics now change immediately, even before the file is saved

for example in my bug report if you fix test2.php but you don't save the file the error from test.php will still disappear, which I consider to be wrong

i'd expect the following:

If the two functions are inside the same php file and bar gets fixed then the error in foo should immediately disappear.. however if the definition is coming from a different file it should only propagate this once the file is saved.

@calebdw

calebdw commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

@Disservin, thanks for the feedback! Please try now---it was existing behaviour, but I agree that cross-file diagnostics should only happen on save and not every change

@calebdw calebdw force-pushed the calebdw/push-uqvsqztpwypy branch from e811b66 to f3e1657 Compare July 6, 2026 21:53
@calebdw calebdw changed the title fix(diagnostics): update after function signature change and on save fix(diagnostics): function signatures, save-gated propagation, external tool scheduling, and @phpstan-ignore parsing Jul 7, 2026
@calebdw calebdw force-pushed the calebdw/push-uqvsqztpwypy branch 6 times, most recently from 8a9d7fe to efdc9ae Compare July 7, 2026 03:36
@Disservin

Copy link
Copy Markdown

looks good imo

@calebdw calebdw requested a review from AJenbo July 7, 2026 15:47
@calebdw calebdw force-pushed the calebdw/push-uqvsqztpwypy branch from efdc9ae to 6415738 Compare July 7, 2026 17:48
calebdw added 5 commits July 7, 2026 16:17
Root cause: update_ast only tracked class signature changes. When a
standalone function's parameter type changed (e.g. bar(null $x) to
bar(string $x)), update_ast returned false, so
schedule_diagnostics_for_open_files was never called and other open
files kept showing stale diagnostics.

Changes:
- Add FunctionInfo::signature_eq() for comparing function signatures
- Track function signature changes in update_ast: snapshot old
  functions before re-parse, compare signatures, clean up stale
  entries, and return true when any function signature changed
- Add did_save handler as a reliable diagnostic refresh point
  (critical for Neovim which relies on save events)
- Register save notification capability in TextDocumentSyncOptions
- Add 23 tests covering signature_eq and update_ast change detection

Closes PHPantom-dev#123
Move `schedule_diagnostics_for_open_files` out of the `didChange`
handler so that editing a function or class signature in one file no
longer immediately refreshes diagnostics in other open files.
Cross-file diagnostic invalidation now only triggers on `didSave`,
which avoids propagating intermediate/incomplete edits to other
buffers.  Same-file diagnostics continue to update on every keystroke
via `schedule_diagnostics`.
External tool workers (PHPStan, PHPCS, Mago lint, Mago analyze) are
now scheduled exclusively from `did_save`.  These tools read from
disk, not from the in-memory buffer, so running them before save
produces stale results.

This removes all external tool debounce constants and logic.  The
workers wake on save, take the pending URI, and run immediately.
Native diagnostics (type errors, unused variables, etc.) continue
to update on every keystroke via `schedule_diagnostics`.

Also adds `workspace/diagnostic/refresh` after each external tool
worker caches new results, so editors using pull diagnostics see
updates without requiring a `didChange` event.
Set `workspace_diagnostics` to `false` in the diagnostic provider
options.  The `workspace/diagnostic` handler currently only returns
cached diagnostics for files the user has already opened — the same
data `textDocument/diagnostic` provides per-file.  Advertising the
capability causes editors to send redundant workspace-wide pull
requests that duplicate per-document pulls with no additional value.

This can be flipped to `true` when the server gains the ability to
report diagnostics for unopened files (e.g. project-wide PHPStan
analysis).
The `@phpstan-ignore` stale diagnostic filter failed to match when
the comment used the per-identifier reason syntax:

    /** @PHPStan-Ignore return.type (reason text here) */

The parser stopped at `*/` before checking for ` (`, so the entire
content between `@phpstan-ignore` and `*/` was treated as the
identifier list — including the parenthesized reason text.  When the
reason contained commas the identifiers split incorrectly and none
matched.

Fix: split by comma first, then strip the optional ` (reason)` suffix
from each entry before comparing against the diagnostic identifier.
This correctly handles:
- Single identifier with reason: `@phpstan-ignore id (reason)`
- Multiple identifiers with reasons: `@phpstan-ignore id1 (r1), id2 (r2)`
- Reason text containing commas: `@phpstan-ignore id (a, b, and c)`
@calebdw calebdw force-pushed the calebdw/push-uqvsqztpwypy branch from 6415738 to 022109d Compare July 7, 2026 21:17
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.

diagnostics don't update

3 participants