diff --git a/README.md b/README.md index 1fcc576..e50d9f7 100644 --- a/README.md +++ b/README.md @@ -64,6 +64,7 @@ mindmap rename willRenameFiles codeAction + bound-error fix-it codeLens Understand hover @@ -77,6 +78,8 @@ mindmap duplicate-template undefined-bareword constructor-arg-mismatch + argument-mismatch + cross-file broadcast Find completion completionItem/resolve @@ -85,7 +88,7 @@ mindmap stub cache tolerant-parse UTF-16 columns - short-name tie-break + proximity FQN resolution lint mode ``` diff --git a/docs/roadmap.md b/docs/roadmap.md index b0601c9..a7e2d1a 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -41,23 +41,52 @@ Features grouped by theme, not chronological or priority ordering: ```mermaid timeline section Shipped - Navigation: definition: typeDefinition: references: implementation: call hierarchy: type hierarchy: documentSymbol: workspaceSymbol: documentHighlight - Editing: rename: willRenameFiles: codeAction + resolve: codeLens + resolve - Understanding: hover: signatureHelp: inlayHint: foldingRange: semanticTokens - Validation: parse: bound: duplicate-template: undefined-bareword: ctor-arg-mismatch + Navigation: definition: typeDefinition: references (incl. constructor usages): implementation: call hierarchy: type hierarchy: documentSymbol: workspaceSymbol: documentHighlight (read/write) + Editing: rename: willRenameFiles: codeAction + resolve: codeLens + resolve: bound-error fix-its + Understanding: hover: signatureHelp: inlayHint: foldingRange: semanticTokens (interpolation + non-ASCII) + Validation: parse: bound: duplicate-template: undefined-bareword: ctor-arg-mismatch: arg-mismatch (method/static/function): cross-file broadcast Completion: type-arg + member + static + variable: scope-aware insertText: completionItem/resolve - Performance: warm AST cache: stub cache: tolerant parse: UTF-16 columns: short-name tie-break: lint mode + Performance: warm AST cache: stub cache: tolerant parse: UTF-16 columns: proximity-aware FQN resolution: lint mode section Planned Editing: prepareRename: selectionRange Navigation: documentLink - Validation: argument-type checker V2: cross-file broadcast section Exploratory Editing: bound name hover/jump: formatting: documentColor - Understanding: lowering preview: specialization explorer: instantiation inlay hints: bound-error fix-its: demangle FQN to source template + Understanding: lowering preview: specialization explorer: instantiation inlay hints: demangle FQN to source template ``` --- +## Recently shipped + +Moved out of Planned / Exploratory since the last revision (exercised by the +test suite; full descriptions to fold into [`README.md`](../README.md#features)): + +- **Argument-type checker V2** -- a new `xphp.arg-mismatch` diagnostic extends + the constructor check to `$obj->m(...)`, `Cls::m(...)`, and `freeFn(...)`, with + conservative "simple-locals" inference for `$var` arguments assigned from a + literal / `new` earlier in the same scope. +- **Cross-file diagnostic broadcast** -- after a workspace pass, diagnostics are + re-published for every *other* open document whose results changed + (per-URI signature-guarded against edit storms), so a dependent flags / clears + without being touched. +- **Bound-error fix-its** -- a `Generic bound violated` diagnostic now offers + "Add `implements \Bound` to ``" (cross-file edit on the concrete class) + and "Change type argument to ``" (bound-satisfying workspace types, + scalars included). +- **Proximity-aware FQN resolution** -- the filesystem index covers the whole + tree (the `test/fixture` exclusion is gone) and resolves a duplicated FQN to + the declaration nearest the requesting file; the bound-check hierarchy is + single-sourced the same way. +- **Constructor usages** -- `new X(...)` is tracked as a reference to + `X::__construct` in Find Usages, the code lens, and document-highlight. +- **Semantic tokens** -- interpolated `"… $x …"` strings split into string + + variable spans, and token lengths are UTF-16 code units (non-ASCII-correct). +- **Document highlight** -- read vs. write kind (declaration / assignment = + write, use site = read). + +--- + ## Planned Known shape, no open design questions. Listed in rough priority order. @@ -76,29 +105,12 @@ cursor. PhpStorm and VS Code both bind Ctrl+W / Ctrl+Shift+W to it. Implementation is a tree walk producing `SelectionRange { range, parent }` per anchor. -### Argument-type checker V2 -- methods, statics, free functions (M) - -`xphp.ctor-arg-mismatch` (cosntructor arguments mismatch) V1 covers `new C(...)` -and `new C(...)` only. V2 extends the same diagnostic to `$obj->m(...)`, -`Cls::m(...)`, and `freeFn(...)`. The hard parts (receiver-type resolution, -substitution through generics) already work for hover / signature help; the V2 -checker reuses them and emits the same diagnostic shape as V1. - ### `documentLink` -- clickable URLs in comments (S) `textDocument/documentLink` returns ranges + URIs for URLs and PSR-4-style references inside comments / docblocks. Editors underline them and `Cmd+Click` opens. Low value compared to the above, listed for completeness. -### Cross-file diagnostic broadcast (M) - -Today: editing `Box.xphp` re-publishes `Box.xphp`'s diagnostics only; `Use.xphp` -(which instantiates `Box`) catches up the next time it's touched. The fix is -straightforward -- after each diagnostic pass, also re-publish for every file -whose AST cites the changed file's templates. The remaining work is the right -batching window so a rapid edit storm doesn't flood every consumer on every -keystroke. - --- ## Exploratory @@ -200,36 +212,6 @@ IntelliJ; Rust's `rustc-demangle` for symbol names. method `xphp.demangle`. Prototype consumption in PhpStorm's "Analyze stacktrace" dialog as a transformation pass. -### Bound-error fix-its -- "implement missing interface" / "swap type-arg" - -**What it'd do.** Today, a `Generic bound violated` diagnostic shows -the explanation but no quick fix. The fix-it would offer: - -1. "Add `implements \Stringable` to `class App\Models\User`" -- one - text edit on the supplied concrete type's declaration. -2. "Swap type-arg to ``" -- show the list of project classes - that already satisfy the bound, picked via the existing bound- - aware completion filter. - -**Open questions.** - -- For (1), insertion of `implements` is a straightforward parser - walk; the leading `\` qualification is the same problem - `ClassNameImportContext` solves and can be reused. -- For (2), candidate ranking: alphabetical, by frequency in the - project, or by current import status? -- Cross-file edits: the type-arg site and the class declaration - may live in different files. LSP `WorkspaceEdit` handles this - in the protocol, but the UI feedback in PhpStorm for multi-file - actions is uneven. - -**Prior art.** TypeScript LSP's "Add missing properties" quick -fix; Rust analyzer's "Implement trait" assist. - -**Initial step.** Fix (2) wired end-to-end first (single-file -edit, reuses existing completion infrastructure). Fix (1) deferred -until the multi-file edit story is ironed out via the rename loop. - ### Hover / jump on bound names in template headers **What it'd do.** Cursor on `Stringable` inside diff --git a/features/README.md b/features/README.md index ff7d564..1182e17 100644 --- a/features/README.md +++ b/features/README.md @@ -14,9 +14,12 @@ features/ ├── navigate/ definition, type-definition, references, implementation, │ document & workspace symbols, document highlight, │ call hierarchy, type hierarchy -├── edit/ rename, code actions, code lens, willRenameFiles +├── edit/ rename, code actions (import, optimize, bound fix-its), +│ code lens, willRenameFiles ├── understand/ hover, signature help, inlay hints, folding ranges, semantic tokens -├── validate/ diagnostics (parse, undefined-name, bound, ctor-arg, duplicate) +├── validate/ diagnostics (parse, undefined-name, bound, duplicate, +│ argument-type mismatch: ctor / method / static / function), +│ cross-file broadcast (push-mode re-publish of dependents) └── find/ completion, completion-item resolve ``` @@ -52,13 +55,46 @@ Behat is installed in an isolated tooling dir (`tools/behat/`) because Behat 3.x caps `symfony/console` at `^7` while the project pins `^8` via `xphp-lang/xphp`. `make test/behat` bootstraps it on first run. +## Coverage boundary — what is *not* covered here, and why + +The suite is **100% in-memory**: `World` builds the server with +`new InitializeParams(new ClientCapabilities())` — **no `rootUri`/`rootPath`** — +so the filesystem walk is empty and every scenario is open-document-only (files +arrive via `textDocument/didOpen`, never from disk). That guarantee is what +keeps the suite isolated and parallel-safe, but it puts whole categories of +behavior structurally out of reach. Those are covered by **PHPUnit unit tests** +instead (named below), not Behat — driving them here would require writing real +files to a real `rootUri`, which would break the in-memory / no-disk / +parallel-safe invariant. + +**Filesystem layer (unit-tested, never Behat):** +- The FQN **filesystem index** + **proximity-aware resolution** and its + per-request origin anchor (`OriginTrackingMiddleware`) — duplicate FQNs across + on-disk files resolved by nearness to the requesting document. + → `test/Reflection/FqnIndexTest.php`, `test/Dispatcher/OriginTrackingMiddlewareTest.php` +- Cross-file go-to-definition / hover into **closed** (on-disk, not open) files. +- The warmers (`FqnIndexWarmer`, `ParsedDocumentCacheWarmer`). +- Bound-check hierarchy single-sourcing across duplicate-FQN packages. + → `test/Diagnostics/XphpDiagnosticsProviderTest.php` +- `workspace/didChangeWatchedFiles` (file-watcher index invalidation). + +**Other unit-only behaviors:** +- Non-ASCII semantic-token **length** (UTF-16 code units): the Behat token + decoder is byte-based, so the length is pinned in + `test/Handler/SemanticTokens/AstVisitorTest.php` instead. + +**In-memory-drivable but currently not scripted (low value):** +- Document-lifecycle notifications `textDocument/didClose` / `didSave` / + `willSave` / `willSaveWaitUntil` (Behat only drives `didOpen` / `didChange`). +- The `codeAction/resolve` round-trip (providers attach edits eagerly, so the + resolve step is a no-op in practice). + +Everything else — all of navigate / edit / understand / validate / find — is +exercised end-to-end through the real dispatcher here. + ## @todo scenarios -Deferred behavior is written as `@todo` scenarios that document the desired +Deferred behavior can be written as `@todo` scenarios that document the desired outcome but are skipped (via the gherkin tag filter in `behat.dist.yml`), so the -suite stays green on what's expected to work. Current `@todo`s: - -- go-to-definition through a generic **method** call (navigate/definition) -- **duplicate-template** diagnostic on the edited file — the per-file pull - provider canonicalizes the edited file, so it surfaces on the other file; - needs the roadmap's cross-file diagnostic broadcast (validate/diagnostics) +suite stays green on what's expected to work. There are currently none — every +scenario runs. diff --git a/features/edit/bound_fixes.feature b/features/edit/bound_fixes.feature new file mode 100644 index 0000000..2a31f71 --- /dev/null +++ b/features/edit/bound_fixes.feature @@ -0,0 +1,50 @@ +Feature: Quick-fixes for generic bound violations + As a developer editing xphp + I want lightbulb fixes when a type argument violates a generic bound + + Background: + Given the file at "/Stringy.xphp" contains the following lines: + """ + { public T $item; } + """ + And the FQN index has been warmed on initialize + + Scenario: Offer to swap a scalar type argument for a bound-satisfying type + Given the file at "/Use.xphp" contains the following lines: + """ + (); + """ + When I request code actions for the "xphp.bound" diagnostic in "/Use.xphp" + Then a code action titled "Change type argument to Stringy" is offered + And the "Change type argument to Stringy" action has kind "quickfix" + And the "Change type argument to Stringy" action replaces "int" with "Stringy" + + Scenario: Offer to implement the bound on the offending concrete class + Given the file at "/Money.xphp" contains the following lines: + """ + (); + """ + When I request code actions for the "xphp.bound" diagnostic in "/Use.xphp" + Then a code action titled "Add implements \Stringable to Money" is offered + And the "Add implements \Stringable to Money" action inserts "implements \Stringable" + And a code action titled "Change type argument to Stringy" is offered diff --git a/features/edit/code_lens.feature b/features/edit/code_lens.feature index 69ad096..fb58d39 100644 --- a/features/edit/code_lens.feature +++ b/features/edit/code_lens.feature @@ -29,3 +29,23 @@ Feature: Code lens And I resolve the first code lens Then the resolved lens reads "2 usages" And the resolved lens carries the reference locations + + Scenario: A constructor lens counts "new" instantiation sites + Given the file at "/Gadget.xphp" contains the following lines: + """ + rename(42); + """ + And the FQN index has been warmed on initialize + When I analyze "/Use.xphp" for diagnostics + Then a "xphp.arg-mismatch" diagnostic is reported saying "expects string, got int" + And the "xphp.arg-mismatch" diagnostic underlines "42" + + Scenario: Flag a scalar literal passed to a static method + Given the file at "/Factory.xphp" contains the following lines: + """ + + { + public function add(T $item): void {} + } + """ + And the file at "/User.xphp" contains the following lines: + """ + (); + $c->add(new Tag()); + """ + And the FQN index has been warmed on initialize + When I analyze "/Use.xphp" for diagnostics + Then a "xphp.arg-mismatch" diagnostic is reported saying "App\User" + And the "xphp.arg-mismatch" diagnostic underlines "new Tag()" + + Scenario: Flag a locally-assigned variable whose type can't satisfy the parameter + Given the file at "/User.xphp" contains the following lines: + """ + rename($n); + """ + And the FQN index has been warmed on initialize + When I analyze "/Use.xphp" for diagnostics + Then a "xphp.arg-mismatch" diagnostic is reported saying "expects string, got int" + And the "xphp.arg-mismatch" diagnostic underlines "$n" + + Scenario: Accept a correct instance-method call without complaint + Given the file at "/User.xphp" contains the following lines: + """ + rename('alice'); + """ + And the FQN index has been warmed on initialize + When I analyze "/Use.xphp" for diagnostics + Then no diagnostics are reported diff --git a/features/validate/broadcast.feature b/features/validate/broadcast.feature new file mode 100644 index 0000000..6d15a1e --- /dev/null +++ b/features/validate/broadcast.feature @@ -0,0 +1,27 @@ +Feature: Cross-file diagnostic broadcast + As a developer editing xphp + I want a dependent file's diagnostics to refresh when I edit a file it + depends on -- without having to open or touch the dependent myself + + Scenario: Adding a bound to a template re-flags its open dependents + Given the file at "/Box.xphp" contains the following lines: + """ + { public T $item; } + """ + And the file at "/Use.xphp" contains the following lines: + """ + (); + """ + And the FQN index has been warmed on initialize + And the diagnostics service is running + When I change the file at "/Box.xphp" to contain the following lines: + """ + { public T $item; } + """ + Then a "xphp.bound" diagnostic is published for "/Use.xphp" without re-requesting it diff --git a/features/validate/diagnostics.feature b/features/validate/diagnostics.feature index ee77f1b..0b6e12b 100644 --- a/features/validate/diagnostics.feature +++ b/features/validate/diagnostics.feature @@ -36,11 +36,7 @@ Feature: Diagnostics Then a "xphp.undefined-name" diagnostic is reported saying "nul" And the "xphp.undefined-name" diagnostic underlines "nul" - # Deferred: the per-file pull provider treats the analyzed file as canonical, - # so the duplicate is flagged on the OTHER open file. Surfacing it on the - # edited file needs the roadmap's cross-file diagnostic broadcast. - @todo - Scenario: Report a duplicate template declaration + Scenario: Report a duplicate template declaration on the edited file Given the file at "/BoxOne.xphp" contains the following lines: """ (…)` → `xphp.ctor-arg-mismatch` (V1) + * - `$obj->m(…)` → `xphp.arg-mismatch` (V2) + * - `C::m(…)` / `C::m(…)` → `xphp.arg-mismatch` (V2) + * - `fn(…)` / `fn(…)` → `xphp.arg-mismatch` (V2) + * + * Argument type inference is intentionally narrow -- only the cases + * where the AST alone tells us the type: + * - `new ClassName(...)` → ClassName FQN + * - string / int / float literals → the obvious scalar + * - `true` / `false` / `null` const fetch → bool / null + * - array literal `[…]` → array + * - a `$var` assigned EXACTLY ONCE, earlier in the same function + * scope, from one of the above ("simple locals"). + * + * Anything else (function-call results, properties, ternaries, + * reassigned or forward-assigned variables) is SKIPPED to avoid false + * positives. The same simple-locals binding map is what lets an + * instance method call's receiver type be inferred + * (`$users = new Collection(); $users->add(42);`). + * + * Comparison rules: see {@see isSatisfied()}. Class ancestry defers to + * the workspace `TypeHierarchy`; unknown ancestry is treated as OK so we + * never false-positive on types missing from the index. + */ +final readonly class CallArgumentChecker +{ + /** Scalar param types the checker can compare against literals. */ + private const SCALARS = ['string' => true, 'int' => true, 'float' => true, 'bool' => true, 'array' => true]; + + /** Pseudo / supertype params that accept anything. */ + private const PERMISSIVE_TYPES = [ + 'mixed' => true, + 'object' => true, + 'callable' => true, + 'iterable' => true, + 'void' => true, + 'never' => true, + 'self' => true, + 'static' => true, + 'parent' => true, + ]; + + /** + * @param array, source: string}> $files + * @return array> diagnostics keyed by URI/path + */ + public function check(array $files, TypeHierarchy $hierarchy): array + { + $classIndex = $this->indexClassesByFqn($files); + $functionIndex = $this->indexFunctionsByFqn($files); + $diagnosticsByFile = array_fill_keys(array_keys($files), []); + + foreach ($files as $path => $entry) { + $positionMap = new PositionMap($entry['source']); + $context = self::extractNamespaceAndUseMap($entry['ast']); + $bindings = $this->collectScopeBindings( + $entry['ast'], + $context['namespace'], + $context['useMap'], + ); + $this->walkCalls( + $entry['ast'], + $classIndex, + $functionIndex, + $bindings, + $hierarchy, + $positionMap, + $context['namespace'], + $context['useMap'], + $diagnosticsByFile[$path], + ); + } + return $diagnosticsByFile; + } + + /** + * Extract the file's enclosing namespace + the `use Foo\Bar [as + * Baz]` map needed to resolve bare `Name` nodes to fully-qualified + * class names without relying on nikic's NameResolver (which the + * LSP's per-file Analyzer doesn't run). + * + * Handles both `Use_` and `GroupUse` (only the TYPE_NORMAL slots + * -- function / const uses go through separate symbol tables and + * don't bind class-like aliases). + * + * @param list $ast + * @return array{namespace: string, useMap: array} + */ + private static function extractNamespaceAndUseMap(array $ast): array + { + $namespace = ''; + $useMap = []; + $topLevelStmts = $ast; + foreach ($ast as $stmt) { + if ($stmt instanceof Node\Stmt\Namespace_) { + $namespace = $stmt->name === null ? '' : $stmt->name->toString(); + $topLevelStmts = $stmt->stmts; + break; + } + } + foreach ($topLevelStmts as $stmt) { + if ($stmt instanceof Node\Stmt\Use_) { + foreach ($stmt->uses as $useUse) { + $type = $useUse->type !== Node\Stmt\Use_::TYPE_UNKNOWN + ? $useUse->type + : $stmt->type; + if ($type !== Node\Stmt\Use_::TYPE_NORMAL) { + continue; + } + $useMap[$useUse->getAlias()->toString()] = $useUse->name->toString(); + } + continue; + } + if ($stmt instanceof Node\Stmt\GroupUse) { + $prefix = $stmt->prefix->toString(); + foreach ($stmt->uses as $useUse) { + $type = $useUse->type !== Node\Stmt\Use_::TYPE_UNKNOWN + ? $useUse->type + : $stmt->type; + if ($type !== Node\Stmt\Use_::TYPE_NORMAL) { + continue; + } + $useMap[$useUse->getAlias()->toString()] = $prefix . '\\' . $useUse->name->toString(); + } + } + } + return ['namespace' => $namespace, 'useMap' => $useMap]; + } + + /** + * Resolve a `Name` node to an FQN given the file's namespace and + * use map. Handles the three nikic-classified shapes: + * + * - fully-qualified `\App\Foo` → strip leading slash; + * - relative `namespace\Foo` → prepend file namespace; + * - unqualified / qualified `Foo` / `Foo\Bar` → consult use map + * for the head segment, otherwise prepend file namespace. + * + * @param array $useMap + */ + public function resolveNameToFqn(Name $name, string $namespace, array $useMap): string + { + if ($name->isFullyQualified()) { + return ltrim($name->toString(), '\\'); + } + $parts = $name->getParts(); + if ($parts === []) { + return ''; + } + $head = $parts[0]; + if (isset($useMap[$head])) { + $tail = array_slice($parts, 1); + return $tail === [] + ? $useMap[$head] + : $useMap[$head] . '\\' . implode('\\', $tail); + } + $local = implode('\\', $parts); + return $namespace !== '' ? $namespace . '\\' . $local : $local; + } + + /** + * Index every named ClassLike in the workspace by FQN, carrying its + * declared methods (keyed by lowercased name), the owning ClassLike + * (so callers can read its ATTR_GENERIC_PARAMS) and the declaring + * file's import context (params resolve in the OWNER's namespace). + * + * @param array, source: string}> $files + * @return array, namespace: string, useMap: array}> + */ + private function indexClassesByFqn(array $files): array + { + $byFqn = []; + foreach ($files as $entry) { + $context = self::extractNamespaceAndUseMap($entry['ast']); + foreach (self::collectClassLikesWithNamespace($entry['ast']) as [$namespace, $cls]) { + if ($cls->name === null) { + continue; + } + $shortName = $cls->name->toString(); + $fqn = $namespace !== '' ? $namespace . '\\' . $shortName : $shortName; + $methods = []; + foreach ($cls->stmts as $member) { + if ($member instanceof ClassMethod) { + $methods[strtolower($member->name->toString())] = $member; + } + } + $byFqn[$fqn] = [ + 'owner' => $cls, + 'methods' => $methods, + 'namespace' => $namespace, + 'useMap' => $context['useMap'], + ]; + } + } + return $byFqn; + } + + /** + * Index every named free function in the workspace by FQN. + * + * @param array, source: string}> $files + * @return array}> + */ + private function indexFunctionsByFqn(array $files): array + { + $byFqn = []; + foreach ($files as $entry) { + $context = self::extractNamespaceAndUseMap($entry['ast']); + foreach (self::collectFunctionsWithNamespace($entry['ast']) as [$namespace, $fn]) { + $shortName = $fn->name->toString(); + $fqn = $namespace !== '' ? $namespace . '\\' . $shortName : $shortName; + $byFqn[$fqn] = [ + 'func' => $fn, + 'namespace' => $namespace, + 'useMap' => $context['useMap'], + ]; + } + } + return $byFqn; + } + + /** + * Recursively walk the top-level statement list collecting every + * `ClassLike` paired with its enclosing namespace string (empty + * when the file has no `namespace` declaration). Handles both + * the "bracketed" form (`namespace App { ... }`) and the + * "semicolon" form (`namespace App; ...`). + * + * @param list $stmts + * @return list + */ + private static function collectClassLikesWithNamespace(array $stmts): array + { + $out = []; + foreach ($stmts as $stmt) { + if ($stmt instanceof Node\Stmt\Namespace_) { + $ns = $stmt->name === null ? '' : $stmt->name->toString(); + foreach ($stmt->stmts as $inner) { + if ($inner instanceof ClassLike) { + $out[] = [$ns, $inner]; + } + } + continue; + } + if ($stmt instanceof ClassLike) { + $out[] = ['', $stmt]; + } + } + return $out; + } + + /** + * Companion of {@see collectClassLikesWithNamespace()} for free + * functions. + * + * @param list $stmts + * @return list + */ + private static function collectFunctionsWithNamespace(array $stmts): array + { + $out = []; + foreach ($stmts as $stmt) { + if ($stmt instanceof Node\Stmt\Namespace_) { + $ns = $stmt->name === null ? '' : $stmt->name->toString(); + foreach ($stmt->stmts as $inner) { + if ($inner instanceof Function_) { + $out[] = [$ns, $inner]; + } + } + continue; + } + if ($stmt instanceof Function_) { + $out[] = ['', $stmt]; + } + } + return $out; + } + + /** + * Build the per-scope "simple locals" binding map: for each function + * scope (keyed by the FunctionLike's object id; 0 = file top level) + * a `$var -> binding` map of variables assigned EXACTLY ONCE from an + * inferable literal / `new` expression. Variables assigned more than + * once in a scope are dropped (ambiguous). Each binding records the + * assignment's byte offset so lookups can require assign-before-use. + * + * @param array $useMap + * @return array, offset: int}>> + */ + private function collectScopeBindings(array $ast, string $namespace, array $useMap): array + { + $checker = $this; + $visitor = new class($checker, $namespace, $useMap) extends NodeVisitorAbstract { + /** @var list stack of enclosing FunctionLike object ids */ + private array $scopeStack = []; + + /** @var array, offset: int}>> */ + public array $bindings = [0 => []]; + + /** @var array> scope -> vars seen more than once */ + private array $ambiguous = [0 => []]; + + /** @param array $useMap */ + public function __construct( + private readonly CallArgumentChecker $checker, + private readonly string $namespace, + private readonly array $useMap, + ) { + } + + public function enterNode(Node $node): null + { + if ($node instanceof FunctionLike) { + $id = spl_object_id($node); + $this->scopeStack[] = $id; + $this->bindings[$id] ??= []; + $this->ambiguous[$id] ??= []; + return null; + } + if (!$node instanceof Assign || !$node->var instanceof Variable || !is_string($node->var->name)) { + return null; + } + $scope = $this->scopeStack === [] ? 0 : $this->scopeStack[count($this->scopeStack) - 1]; + $name = $node->var->name; + if (isset($this->ambiguous[$scope][$name])) { + return null; + } + if (isset($this->bindings[$scope][$name])) { + // Second assignment in the same scope -> ambiguous; drop it. + unset($this->bindings[$scope][$name]); + $this->ambiguous[$scope][$name] = true; + return null; + } + $binding = $this->checker->bindingForExpr($node->expr, $this->namespace, $this->useMap); + if ($binding === null) { + return null; + } + $offset = $node->getStartFilePos(); + $this->bindings[$scope][$name] = $binding + ['offset' => $offset >= 0 ? $offset : 0]; + return null; + } + + public function leaveNode(Node $node): null + { + if ($node instanceof FunctionLike && $this->scopeStack !== []) { + array_pop($this->scopeStack); + } + return null; + } + }; + + $traverser = new NodeTraverser(); + $traverser->addVisitor($visitor); + $traverser->traverse($ast); + + return $visitor->bindings; + } + + /** + * Compute the binding for an assignment RHS, or null when its type + * isn't statically inferable. Mirrors {@see inferArgType()} but also + * captures the class FQN + generic args needed to type a receiver. + * + * @param array $useMap + * @return array{type: ?string, fqn: ?string, args: ?list}|null + */ + public function bindingForExpr(Expr $expr, string $namespace, array $useMap): ?array + { + if ($expr instanceof New_ && $expr->class instanceof Name) { + $fqn = $this->resolveTargetClassFqn($expr->class, $namespace, $useMap); + $args = $expr->class->getAttribute(XphpSourceParser::ATTR_GENERIC_ARGS); + return [ + 'type' => $fqn !== '' ? $fqn : null, + 'fqn' => $fqn !== '' ? $fqn : null, + 'args' => is_array($args) ? $args : null, + ]; + } + $scalar = $this->inferArgType($expr, $namespace, $useMap, []); + if ($scalar === null) { + return null; + } + return ['type' => $scalar, 'fqn' => null, 'args' => null]; + } + + /** + * Resolve the target class FQN of a `new C(…)` expression. + * Prefers the xphp-parser-attached `ATTR_TEMPLATE_FQN` (set for + * generic `new C(…)` shapes), then resolves bare names via the + * call site's namespace + use map. Returns the empty string when + * nothing can be resolved. + * + * @param array $useMap + */ + public function resolveTargetClassFqn(Name $classExpr, string $namespace, array $useMap): string + { + $generic = $classExpr->getAttribute(XphpSourceParser::ATTR_TEMPLATE_FQN); + if (is_string($generic) && $generic !== '') { + return ltrim($generic, '\\'); + } + return $this->resolveNameToFqn($classExpr, $namespace, $useMap); + } + + /** + * Pair a template's declared TypeParams with a list of call-site + * TypeRefs into a `name -> TypeRef` substitution map. Returns an + * empty map when the shapes don't line up (nothing to substitute). + * + * @param array|null $params declared type params (TypeParam[]) + * @param array|null $args supplied type args (TypeRef[]) + * @return array + */ + private function pairSubstitution(?array $params, ?array $args): array + { + if (!is_array($params) || !is_array($args) || $params === [] || count($params) !== count($args)) { + return []; + } + $names = self::extractTypeParamNames($params); + if (count($names) !== count($args)) { + return []; + } + $substitution = []; + foreach ($names as $i => $paramName) { + $arg = $args[$i]; + if ($arg instanceof TypeRef) { + $substitution[$paramName] = $arg; + } + } + return $substitution; + } + + /** + * @param array $params + * @return list + */ + private static function extractTypeParamNames(array $params): array + { + $names = []; + foreach ($params as $p) { + if (is_object($p) && property_exists($p, 'name') && is_string($p->name)) { + $names[] = $p->name; + } + } + return $names; + } + + /** + * Single traversal that checks every call site. `New_` keeps the V1 + * `xphp.ctor-arg-mismatch` surface; the three V2 shapes emit + * `xphp.arg-mismatch`. Method-call receivers are typed via the + * scope binding map (so `$x->m(…)` only checks when `$x`'s class is + * locally known). + * + * @param array, namespace: string, useMap: array}> $classIndex + * @param array}> $functionIndex + * @param array, offset: int}>> $bindings + * @param array $useMap + * @param list $diagnostics + */ + private function walkCalls( + array $ast, + array $classIndex, + array $functionIndex, + array $bindings, + TypeHierarchy $hierarchy, + PositionMap $positionMap, + string $namespace, + array $useMap, + array &$diagnostics, + ): void { + $checker = $this; + $visitor = new class($checker, $classIndex, $functionIndex, $bindings, $hierarchy, $positionMap, $namespace, $useMap, $diagnostics) extends NodeVisitorAbstract { + /** @var list */ + private array $scopeStack = []; + + /** + * @param array, namespace: string, useMap: array}> $classIndex + * @param array}> $functionIndex + * @param array, offset: int}>> $bindings + * @param array $useMap + * @param list $diagnostics + */ + public function __construct( + private readonly CallArgumentChecker $checker, + private readonly array $classIndex, + private readonly array $functionIndex, + private readonly array $bindings, + private readonly TypeHierarchy $hierarchy, + private readonly PositionMap $positionMap, + private readonly string $namespace, + private readonly array $useMap, + public array &$diagnostics, + ) { + } + + public function enterNode(Node $node): null + { + if ($node instanceof FunctionLike) { + $this->scopeStack[] = spl_object_id($node); + return null; + } + $scope = $this->scopeStack === [] ? 0 : $this->scopeStack[count($this->scopeStack) - 1]; + $scopeBindings = $this->bindings[$scope] ?? []; + + if ($node instanceof New_) { + $this->checker->checkNew($node, $this->classIndex, $scopeBindings, $this->hierarchy, $this->positionMap, $this->namespace, $this->useMap, $this->diagnostics); + } elseif ($node instanceof MethodCall) { + $this->checker->checkMethodCall($node, $this->classIndex, $scopeBindings, $this->hierarchy, $this->positionMap, $this->namespace, $this->useMap, $this->diagnostics); + } elseif ($node instanceof StaticCall) { + $this->checker->checkStaticCall($node, $this->classIndex, $scopeBindings, $this->hierarchy, $this->positionMap, $this->namespace, $this->useMap, $this->diagnostics); + } elseif ($node instanceof FuncCall) { + $this->checker->checkFuncCall($node, $this->functionIndex, $scopeBindings, $this->hierarchy, $this->positionMap, $this->namespace, $this->useMap, $this->diagnostics); + } + return null; + } + + public function leaveNode(Node $node): null + { + if ($node instanceof FunctionLike && $this->scopeStack !== []) { + array_pop($this->scopeStack); + } + return null; + } + }; + + $traverser = new NodeTraverser(); + $traverser->addVisitor($visitor); + $traverser->traverse($ast); + } + + /** + * `new C(…)` / `new C(…)`. + * + * @param array, namespace: string, useMap: array}> $classIndex + * @param array, offset: int}> $scopeBindings + * @param list $diagnostics + */ + public function checkNew( + New_ $node, + array $classIndex, + array $scopeBindings, + TypeHierarchy $hierarchy, + PositionMap $positionMap, + string $namespace, + array $useMap, + array &$diagnostics, + ): void { + if (!$node->class instanceof Name) { + return; + } + $fqn = $this->resolveTargetClassFqn($node->class, $namespace, $useMap); + if ($fqn === '' || !isset($classIndex[$fqn]['methods']['__construct'])) { + return; + } + $entry = $classIndex[$fqn]; + $substitution = $this->pairSubstitution( + $entry['owner']->getAttribute(XphpSourceParser::ATTR_GENERIC_PARAMS), + $node->class->getAttribute(XphpSourceParser::ATTR_GENERIC_ARGS), + ); + $this->checkArguments( + $node->args, + $entry['methods']['__construct']->params, + $substitution, + $entry['namespace'], + $entry['useMap'], + $namespace, + $useMap, + $scopeBindings, + $hierarchy, + $positionMap, + 'Constructor argument', + $fqn, + DiagnosticCode::ConstructorArgumentMismatch, + $diagnostics, + ); + } + + /** + * `$obj->m(…)` -- receiver typed via the scope binding map. + * + * @param array, namespace: string, useMap: array}> $classIndex + * @param array, offset: int}> $scopeBindings + * @param list $diagnostics + */ + public function checkMethodCall( + MethodCall $node, + array $classIndex, + array $scopeBindings, + TypeHierarchy $hierarchy, + PositionMap $positionMap, + string $namespace, + array $useMap, + array &$diagnostics, + ): void { + if (!$node->name instanceof Identifier) { + return; + } + $receiver = $this->receiverBinding($node->var, $node, $scopeBindings); + if ($receiver === null || $receiver['fqn'] === null) { + return; + } + $entry = $classIndex[$receiver['fqn']] ?? null; + if ($entry === null) { + return; + } + $method = $entry['methods'][strtolower($node->name->toString())] ?? null; + if ($method === null) { + return; + } + // Class-level generics from the receiver's `new C<...>()`, plus any + // method-level generics on the call (`$x->m(…)`). + $substitution = $this->pairSubstitution( + $entry['owner']->getAttribute(XphpSourceParser::ATTR_GENERIC_PARAMS), + $receiver['args'], + ); + $substitution += $this->pairSubstitution( + $method->getAttribute(XphpSourceParser::ATTR_METHOD_GENERIC_PARAMS), + $node->getAttribute(XphpSourceParser::ATTR_METHOD_GENERIC_ARGS), + ); + $this->checkArguments( + $node->args, + $method->params, + $substitution, + $entry['namespace'], + $entry['useMap'], + $namespace, + $useMap, + $scopeBindings, + $hierarchy, + $positionMap, + 'Argument', + $receiver['fqn'] . '::' . $node->name->toString() . '()', + DiagnosticCode::ArgumentMismatch, + $diagnostics, + ); + } + + /** + * `C::m(…)` / `C::m(…)`. Class-level generics can't be bound in a + * static context, so only method-level generics are substituted; + * unsubstituted class type-params stay as bare names and never + * false-positive (unknown class ancestry is treated as OK). + * + * @param array, namespace: string, useMap: array}> $classIndex + * @param array, offset: int}> $scopeBindings + * @param list $diagnostics + */ + public function checkStaticCall( + StaticCall $node, + array $classIndex, + array $scopeBindings, + TypeHierarchy $hierarchy, + PositionMap $positionMap, + string $namespace, + array $useMap, + array &$diagnostics, + ): void { + if (!$node->class instanceof Name || !$node->name instanceof Identifier) { + return; + } + $fqn = $this->resolveTargetClassFqn($node->class, $namespace, $useMap); + $entry = $classIndex[$fqn] ?? null; + if ($entry === null) { + return; + } + $method = $entry['methods'][strtolower($node->name->toString())] ?? null; + if ($method === null) { + return; + } + $substitution = $this->pairSubstitution( + $method->getAttribute(XphpSourceParser::ATTR_METHOD_GENERIC_PARAMS), + $node->getAttribute(XphpSourceParser::ATTR_METHOD_GENERIC_ARGS), + ); + $this->checkArguments( + $node->args, + $method->params, + $substitution, + $entry['namespace'], + $entry['useMap'], + $namespace, + $useMap, + $scopeBindings, + $hierarchy, + $positionMap, + 'Argument', + $fqn . '::' . $node->name->toString() . '()', + DiagnosticCode::ArgumentMismatch, + $diagnostics, + ); + } + + /** + * `fn(…)` / `fn(…)`. + * + * @param array}> $functionIndex + * @param array, offset: int}> $scopeBindings + * @param list $diagnostics + */ + public function checkFuncCall( + FuncCall $node, + array $functionIndex, + array $scopeBindings, + TypeHierarchy $hierarchy, + PositionMap $positionMap, + string $namespace, + array $useMap, + array &$diagnostics, + ): void { + if (!$node->name instanceof Name) { + return; + } + $fqn = $this->resolveNameToFqn($node->name, $namespace, $useMap); + $entry = $functionIndex[$fqn] ?? null; + if ($entry === null) { + return; + } + $substitution = $this->pairSubstitution( + $entry['func']->getAttribute(XphpSourceParser::ATTR_METHOD_GENERIC_PARAMS), + $node->getAttribute(XphpSourceParser::ATTR_METHOD_GENERIC_ARGS), + ); + $this->checkArguments( + $node->args, + $entry['func']->params, + $substitution, + $entry['namespace'], + $entry['useMap'], + $namespace, + $useMap, + $scopeBindings, + $hierarchy, + $positionMap, + 'Argument', + $fqn . '()', + DiagnosticCode::ArgumentMismatch, + $diagnostics, + ); + } + + /** + * Resolve a call's receiver to its scope binding. Only plain + * `$var` receivers assigned before the call are typed (conservative); + * `$this`, chained calls, and properties are skipped. + * + * @param array, offset: int}> $scopeBindings + * @return array{type: ?string, fqn: ?string, args: ?list, offset: int}|null + */ + private function receiverBinding(Expr $receiver, Node $call, array $scopeBindings): ?array + { + if (!$receiver instanceof Variable || !is_string($receiver->name)) { + return null; + } + $binding = $scopeBindings[$receiver->name] ?? null; + if ($binding === null) { + return null; + } + $callOffset = $call->getStartFilePos(); + if ($callOffset >= 0 && $binding['offset'] > $callOffset) { + // Assigned after this call -- don't trust it. + return null; + } + return $binding; + } + + /** + * Compare each positional argument against its (substituted) callee + * parameter type, emitting one Diagnostic per mismatch. + * + * @param array $argNodes + * @param list $params + * @param array $substitution + * @param array $ownerUseMap + * @param array $callerUseMap + * @param array, offset: int}> $scopeBindings + * @param list $diagnostics + */ + private function checkArguments( + array $argNodes, + array $params, + array $substitution, + string $ownerNamespace, + array $ownerUseMap, + string $callerNamespace, + array $callerUseMap, + array $scopeBindings, + TypeHierarchy $hierarchy, + PositionMap $positionMap, + string $noun, + string $calleeLabel, + DiagnosticCode $code, + array &$diagnostics, + ): void { + foreach ($argNodes as $i => $arg) { + if (!$arg instanceof Arg) { + continue; + } + $param = self::paramAtIndex($params, $i); + if ($param === null) { + continue; + } + $expectedType = $this->extractParamType($param, $substitution, $ownerNamespace, $ownerUseMap); + if ($expectedType === null) { + continue; + } + $actualType = $this->inferArgType($arg->value, $callerNamespace, $callerUseMap, $scopeBindings); + if ($actualType === null) { + continue; + } + if (self::isSatisfied($actualType, $expectedType, $hierarchy)) { + continue; + } + $diagnostics[] = self::buildMismatchDiagnostic( + $arg->value, + $positionMap, + $i + 1, + $param, + $expectedType, + $actualType, + $noun, + $calleeLabel, + $code, + ); + } + } + + /** + * Resolve the param record for a given positional argument index, + * honoring variadics (last param consumes all trailing args). + * + * @param list $params + */ + private static function paramAtIndex(array $params, int $index): ?Param + { + if (isset($params[$index])) { + return $params[$index]; + } + $last = $params[count($params) - 1] ?? null; + if ($last !== null && $last->variadic) { + return $last; + } + return null; + } + + /** + * Extract the param's declared type as a normalized display string, + * applying type-arg substitution when the param type references a + * generic type parameter. Returns null when the param has no + * type hint. + * + * For union / intersection types, returns the rendered form + * (`A|B` / `A&B`). Nullable types are rendered with the leading + * `?`. + * + * The `$namespace` + `$useMap` are the OWNER's (the declaring + * class's), not the call site's -- non-generic class-type params + * resolve in the declaring file's import context. + * + * @param array $substitution + * @param array $useMap + */ + private function extractParamType(Param $param, array $substitution, string $namespace, array $useMap): ?string + { + $type = $param->type; + if ($type === null) { + return null; + } + return $this->renderType($type, $substitution, $namespace, $useMap); + } + + /** + * @param array $substitution + * @param array $useMap + */ + private function renderType(Node $type, array $substitution, string $namespace, array $useMap): string + { + if ($type instanceof NullableType) { + return '?' . $this->renderType($type->type, $substitution, $namespace, $useMap); + } + if ($type instanceof Node\UnionType) { + $parts = array_map(fn (Node $t): string => $this->renderType($t, $substitution, $namespace, $useMap), $type->types); + return implode('|', $parts); + } + if ($type instanceof Node\IntersectionType) { + $parts = array_map(fn (Node $t): string => $this->renderType($t, $substitution, $namespace, $useMap), $type->types); + return implode('&', $parts); + } + if ($type instanceof Identifier) { + return $type->toString(); + } + if ($type instanceof Name) { + $raw = ltrim($type->toString(), '\\'); + // Generic type-param substitution: param `T $x` resolves + // to whatever the instantiation passed for T. + if (isset($substitution[$raw])) { + return ltrim($substitution[$raw]->name, '\\'); + } + // Bare scalar / reserved type names (`string`, `int`, + // `self`, etc.) stay as-is -- no FQN resolution. + if ($type->isUnqualified() && self::isReservedTypeName($raw)) { + return $raw; + } + return $this->resolveNameToFqn($type, $namespace, $useMap); + } + return ''; + } + + /** + * Recognises PHP's reserved scalar / pseudo type names that + * shouldn't be FQN-resolved against the use map. + */ + private static function isReservedTypeName(string $name): bool + { + $lower = strtolower($name); + return isset(self::SCALARS[$lower]) + || isset(self::PERMISSIVE_TYPES[$lower]) + || $lower === 'null' + || $lower === 'true' + || $lower === 'false'; + } + + /** + * AST-only argument type inference, extended with the scope binding + * map for plain `$var`s. Returns null when the static type isn't + * visible from the expression (or binding) alone. + * + * @param array $useMap + * @param array, offset: int}> $scopeBindings + */ + private function inferArgType(Expr $expr, string $namespace, array $useMap, array $scopeBindings): ?string + { + if ($expr instanceof New_) { + if (!$expr->class instanceof Name) { + return null; + } + // Generic instantiation: ATTR_TEMPLATE_FQN gives the + // template FQN; for the satisfaction check we compare + // against the template name, not the mangled + // specialization name -- that lines up with the param's + // pre-substitution type. + $templateFqn = $expr->class->getAttribute(XphpSourceParser::ATTR_TEMPLATE_FQN); + if (is_string($templateFqn) && $templateFqn !== '') { + return ltrim($templateFqn, '\\'); + } + return $this->resolveNameToFqn($expr->class, $namespace, $useMap); + } + if ($expr instanceof String_) { + return 'string'; + } + if ($expr instanceof Int_) { + return 'int'; + } + if ($expr instanceof Float_) { + return 'float'; + } + if ($expr instanceof Array_) { + return 'array'; + } + if ($expr instanceof ConstFetch) { + $name = strtolower($expr->name->toString()); + if ($name === 'true' || $name === 'false') { + return 'bool'; + } + if ($name === 'null') { + return 'null'; + } + } + if ($expr instanceof Variable && is_string($expr->name)) { + $binding = $scopeBindings[$expr->name] ?? null; + if ($binding !== null) { + $offset = $expr->getStartFilePos(); + if ($offset < 0 || $binding['offset'] <= $offset) { + return $binding['type']; + } + } + } + return null; + } + + /** + * Check whether `$actual` satisfies `$expected`. See the class + * docblock for the supported shapes. + */ + private static function isSatisfied(string $actual, string $expected, TypeHierarchy $hierarchy): bool + { + $expected = ltrim($expected, '\\'); + $actual = ltrim($actual, '\\'); + + if ($expected === '' || $actual === '') { + return true; + } + // Nullable param: null is always OK; non-null is checked + // against the inner type. + if (str_starts_with($expected, '?')) { + if ($actual === 'null') { + return true; + } + return self::isSatisfied($actual, substr($expected, 1), $hierarchy); + } + if ($actual === 'null') { + // Non-nullable param with null arg: explicit mismatch. + return false; + } + if (str_contains($expected, '|')) { + foreach (explode('|', $expected) as $arm) { + if (self::isSatisfied($actual, $arm, $hierarchy)) { + return true; + } + } + return false; + } + if (str_contains($expected, '&')) { + foreach (explode('&', $expected) as $arm) { + if (!self::isSatisfied($actual, $arm, $hierarchy)) { + return false; + } + } + return true; + } + $expectedLower = strtolower($expected); + if (isset(self::PERMISSIVE_TYPES[$expectedLower])) { + return true; + } + if ($expected === $actual) { + return true; + } + // Scalar param: arg type must match exactly. `int -> float` + // promotion is technically allowed by PHP but reporting it + // is rarely useful as a warning -- accept the literal `int` + // for a `float` param to avoid false positives. + if (isset(self::SCALARS[$expectedLower])) { + if ($expectedLower === 'float' && $actual === 'int') { + return true; + } + if (isset(self::SCALARS[strtolower($actual)])) { + return $expectedLower === strtolower($actual); + } + // Scalar expected, class supplied → mismatch. + return false; + } + // Class-like param with a known scalar argument: a PHP scalar + // never satisfies a class type hint (no autoboxing), so this is + // an unambiguous mismatch -- don't defer to the hierarchy (which + // returns "unknown" for a scalar-vs-class query). + if (isset(self::SCALARS[strtolower($actual)])) { + return false; + } + // Both sides are class-like. Defer to the workspace's + // TypeHierarchy. Unknown ancestry -> assume OK (don't false- + // positive on closed-source vendor types). + $is = $hierarchy->isSubtype($actual, $expected); + return $is !== false; + } + + private static function buildMismatchDiagnostic( + Expr $argExpr, + PositionMap $positionMap, + int $oneBasedIndex, + Param $param, + string $expectedType, + string $actualType, + string $noun, + string $calleeLabel, + DiagnosticCode $code, + ): Diagnostic { + $paramName = $param->var instanceof Variable && is_string($param->var->name) + ? '$' . $param->var->name + : '#' . $oneBasedIndex; + $message = sprintf( + '%s %d (%s) of %s expects %s, got %s.', + $noun, + $oneBasedIndex, + $paramName, + $calleeLabel, + $expectedType, + $actualType, + ); + if ($argExpr->getStartFilePos() >= 0 && $argExpr->getEndFilePos() >= 0) { + [$sl, $sc, $el, $ec] = $positionMap->rangeFromOffsets( + $argExpr->getStartFilePos(), + $argExpr->getEndFilePos() + 1, + ); + } else { + [$sl, $sc, $el, $ec] = $positionMap->fullLineRangeFromNikic($argExpr->getStartLine()); + } + return new Diagnostic( + startLine: $sl, + startCharacter: $sc, + endLine: $el, + endCharacter: $ec, + message: $message, + code: $code, + ); + } +} diff --git a/src/Analyzer/ConstructorArgumentChecker.php b/src/Analyzer/ConstructorArgumentChecker.php deleted file mode 100644 index 22b9e16..0000000 --- a/src/Analyzer/ConstructorArgumentChecker.php +++ /dev/null @@ -1,704 +0,0 @@ -(…)` - * expression in the workspace and emits an `xphp.ctor-arg-mismatch` - * diagnostic when a supplied argument's statically-known type doesn't - * satisfy the constructor parameter's declared type (after type-arg - * substitution for the generic case). - * - * Argument type inference is intentionally narrow -- only the cases - * where the AST alone tells us the type: - * - `new ClassName(...)` → ClassName FQN - * - string / int / float literals → the obvious scalar - * - `true` / `false` / `null` const fetch → bool / null - * - array literal `[…]` → array - * - * Variables, method calls, function calls, ternaries, and any other - * expression whose static type would need flow typing are SKIPPED. - * That avoids false positives while still catching the prod case - * (`new StringableBox(new User('hello'))` → `User` vs `Tag`). - * - * Comparison rules: - * - exact match: param type === arg type → OK. - * - class hierarchy: TypeHierarchy::isSubtype($actual, $expected) - * === true → OK. null (unknown) → OK (don't false-positive on - * types missing from the workspace index). - * - nullable param `?T`: `null` literal is always OK; non-null args - * are checked against T. - * - union `T|U`: OK if actual matches ANY arm. - * - intersection `T&U`: OK if actual matches ALL arms. - * - mixed / object / callable / iterable / void / never: always - * considered satisfied (object accepts any class, mixed accepts - * anything, etc.). No false positives. - */ -final readonly class ConstructorArgumentChecker -{ - /** Scalar param types the checker can compare against literals. */ - private const SCALARS = ['string' => true, 'int' => true, 'float' => true, 'bool' => true, 'array' => true]; - - /** Pseudo / supertype params that accept anything. */ - private const PERMISSIVE_TYPES = [ - 'mixed' => true, - 'object' => true, - 'callable' => true, - 'iterable' => true, - 'void' => true, - 'never' => true, - 'self' => true, - 'static' => true, - 'parent' => true, - ]; - - /** - * @param array, source: string}> $files - * @return array> diagnostics keyed by URI/path - */ - public function check(array $files, TypeHierarchy $hierarchy): array - { - $ctorByFqn = $this->indexConstructorsByFqn($files); - $diagnosticsByFile = array_fill_keys(array_keys($files), []); - - foreach ($files as $path => $entry) { - $positionMap = new PositionMap($entry['source']); - $context = self::extractNamespaceAndUseMap($entry['ast']); - $this->walkNewExpressions( - $entry['ast'], - $ctorByFqn, - $hierarchy, - $positionMap, - $context['namespace'], - $context['useMap'], - $diagnosticsByFile[$path], - ); - } - return $diagnosticsByFile; - } - - /** - * Extract the file's enclosing namespace + the `use Foo\Bar [as - * Baz]` map needed to resolve bare `Name` nodes to fully-qualified - * class names without relying on nikic's NameResolver (which the - * LSP's per-file Analyzer doesn't run). - * - * Handles both `Use_` and `GroupUse` (only the TYPE_NORMAL slots - * -- function / const uses go through separate symbol tables and - * don't bind class-like aliases). - * - * @param list $ast - * @return array{namespace: string, useMap: array} - */ - private static function extractNamespaceAndUseMap(array $ast): array - { - $namespace = ''; - $useMap = []; - $topLevelStmts = $ast; - foreach ($ast as $stmt) { - if ($stmt instanceof Node\Stmt\Namespace_) { - $namespace = $stmt->name === null ? '' : $stmt->name->toString(); - $topLevelStmts = $stmt->stmts; - break; - } - } - foreach ($topLevelStmts as $stmt) { - if ($stmt instanceof Node\Stmt\Use_) { - foreach ($stmt->uses as $useUse) { - $type = $useUse->type !== Node\Stmt\Use_::TYPE_UNKNOWN - ? $useUse->type - : $stmt->type; - if ($type !== Node\Stmt\Use_::TYPE_NORMAL) { - continue; - } - $useMap[$useUse->getAlias()->toString()] = $useUse->name->toString(); - } - continue; - } - if ($stmt instanceof Node\Stmt\GroupUse) { - $prefix = $stmt->prefix->toString(); - foreach ($stmt->uses as $useUse) { - $type = $useUse->type !== Node\Stmt\Use_::TYPE_UNKNOWN - ? $useUse->type - : $stmt->type; - if ($type !== Node\Stmt\Use_::TYPE_NORMAL) { - continue; - } - $useMap[$useUse->getAlias()->toString()] = $prefix . '\\' . $useUse->name->toString(); - } - } - } - return ['namespace' => $namespace, 'useMap' => $useMap]; - } - - /** - * Resolve a `Name` node to an FQN given the file's namespace and - * use map. Handles the three nikic-classified shapes: - * - * - fully-qualified `\App\Foo` → strip leading slash; - * - relative `namespace\Foo` → prepend file namespace; - * - unqualified / qualified `Foo` / `Foo\Bar` → consult use map - * for the head segment, otherwise prepend file namespace. - * - * @param array $useMap - */ - public function resolveNameToFqn(Name $name, string $namespace, array $useMap): string - { - if ($name->isFullyQualified()) { - return ltrim($name->toString(), '\\'); - } - $parts = $name->getParts(); - if ($parts === []) { - return ''; - } - $head = $parts[0]; - if (isset($useMap[$head])) { - $tail = array_slice($parts, 1); - return $tail === [] - ? $useMap[$head] - : $useMap[$head] . '\\' . implode('\\', $tail); - } - $local = implode('\\', $parts); - return $namespace !== '' ? $namespace . '\\' . $local : $local; - } - - /** - * Build a `App\Models\User` -> `{ctor, owner}` map by walking - * every ClassLike across the workspace. Carries the owning - * ClassLike alongside the ClassMethod so the substitution map - * builder can read the template's ATTR_GENERIC_PARAMS without - * re-walking. - * - * FQN derivation: the LSP's per-file Analyzer does NOT run - * nikic's NameResolver, so `namespacedName` isn't attached. We - * compute the FQN manually from the top-level `Namespace_` - * wrapper instead -- cheaper than running NameResolver per-file - * and avoids cloning the AST. - * - * Anonymous classes and classes whose constructor isn't declared - * (the implicit zero-arg ctor) are skipped -- nothing for the - * checker to compare against. - * - * @param array, source: string}> $files - * @return array}> - */ - private function indexConstructorsByFqn(array $files): array - { - $byFqn = []; - foreach ($files as $entry) { - $context = self::extractNamespaceAndUseMap($entry['ast']); - foreach (self::collectClassLikesWithNamespace($entry['ast']) as [$namespace, $cls]) { - if ($cls->name === null) { - continue; - } - $shortName = $cls->name->toString(); - $fqn = $namespace !== '' ? $namespace . '\\' . $shortName : $shortName; - foreach ($cls->stmts as $member) { - if ($member instanceof ClassMethod && strtolower($member->name->toString()) === '__construct') { - $byFqn[$fqn] = [ - 'ctor' => $member, - 'owner' => $cls, - 'namespace' => $namespace, - 'useMap' => $context['useMap'], - ]; - break; - } - } - } - } - return $byFqn; - } - - /** - * Recursively walk the top-level statement list collecting every - * `ClassLike` paired with its enclosing namespace string (empty - * when the file has no `namespace` declaration). Handles both - * the "bracketed" form (`namespace App { ... }`) and the - * "semicolon" form (`namespace App; ...`). - * - * @param list $stmts - * @return list - */ - private static function collectClassLikesWithNamespace(array $stmts): array - { - $out = []; - foreach ($stmts as $stmt) { - if ($stmt instanceof Node\Stmt\Namespace_) { - $ns = $stmt->name === null ? '' : $stmt->name->toString(); - foreach ($stmt->stmts as $inner) { - if ($inner instanceof ClassLike) { - $out[] = [$ns, $inner]; - } - } - continue; - } - if ($stmt instanceof ClassLike) { - $out[] = ['', $stmt]; - } - } - return $out; - } - - /** - * @param list $ast - * @param array $ctorByFqn - * @param array $useMap - * @param list $diagnostics - */ - private function walkNewExpressions( - array $ast, - array $ctorByFqn, - TypeHierarchy $hierarchy, - PositionMap $positionMap, - string $namespace, - array $useMap, - array &$diagnostics, - ): void { - $checker = $this; - $visitor = new class($ctorByFqn, $hierarchy, $positionMap, $namespace, $useMap, $diagnostics, $checker) extends NodeVisitorAbstract { - /** - * @param array $ctorByFqn - * @param array $useMap - * @param list $diagnostics - */ - public function __construct( - private readonly array $ctorByFqn, - private readonly TypeHierarchy $hierarchy, - private readonly PositionMap $positionMap, - private readonly string $namespace, - private readonly array $useMap, - public array &$diagnostics, - private readonly ConstructorArgumentChecker $checker, - ) { - } - - public function enterNode(Node $node): null - { - if (!$node instanceof New_) { - return null; - } - if (!$node->class instanceof Name) { - return null; - } - $fqn = $this->checker->resolveTargetClassFqn($node->class, $this->namespace, $this->useMap); - if ($fqn === '' || !isset($this->ctorByFqn[$fqn])) { - return null; - } - $entry = $this->ctorByFqn[$fqn]; - $substitution = $this->checker->buildSubstitution($node->class, $entry['owner']); - $this->checker->emitMismatchDiagnostics( - $node, - $entry['ctor'], - $substitution, - $this->hierarchy, - $this->positionMap, - $this->namespace, - $this->useMap, - $entry['namespace'], - $entry['useMap'], - $this->diagnostics, - $fqn, - ); - return null; - } - }; - $traverser = new NodeTraverser(); - $traverser->addVisitor($visitor); - $traverser->traverse($ast); - } - - /** - * Resolve the target class FQN of a `new C(…)` expression. - * Prefers the xphp-parser-attached `ATTR_TEMPLATE_FQN` (set for - * generic `new C(…)` shapes), then resolves bare names via the - * call site's namespace + use map. Returns the empty string when - * nothing can be resolved. - * - * @param array $useMap - */ - public function resolveTargetClassFqn(Name $classExpr, string $namespace, array $useMap): string - { - $generic = $classExpr->getAttribute(XphpSourceParser::ATTR_TEMPLATE_FQN); - if (is_string($generic) && $generic !== '') { - return ltrim($generic, '\\'); - } - return $this->resolveNameToFqn($classExpr, $namespace, $useMap); - } - - /** - * Build the type-parameter substitution map for a `new C(…)` - * instantiation by pairing the template's declared TypeParams with - * the call site's TypeRefs. Returns an empty map for non-generic - * calls (no substitution needed). - * - * @return array - */ - public function buildSubstitution(Name $classExpr, ClassLike $owner): array - { - $args = $classExpr->getAttribute(XphpSourceParser::ATTR_GENERIC_ARGS); - if (!is_array($args) || $args === []) { - return []; - } - $params = $owner->getAttribute(XphpSourceParser::ATTR_GENERIC_PARAMS); - if (!is_array($params) || count($params) !== count($args)) { - return []; - } - $names = self::extractTypeParamNames($params); - if (count($names) !== count($args)) { - return []; - } - $substitution = []; - foreach ($names as $i => $paramName) { - $arg = $args[$i]; - if ($arg instanceof TypeRef) { - $substitution[$paramName] = $arg; - } - } - return $substitution; - } - - /** - * @param array $params - * @return list - */ - private static function extractTypeParamNames(array $params): array - { - $names = []; - foreach ($params as $p) { - if (is_object($p) && property_exists($p, 'name') && is_string($p->name)) { - $names[] = $p->name; - } - } - return $names; - } - - /** - * Compare each call argument against its corresponding (substituted) - * constructor parameter type. Emits one Diagnostic per mismatch. - * - * @param array $substitution - * @param array $callerUseMap - * @param array $ownerUseMap - * @param list $diagnostics - */ - public function emitMismatchDiagnostics( - New_ $newExpr, - ClassMethod $ctor, - array $substitution, - TypeHierarchy $hierarchy, - PositionMap $positionMap, - string $callerNamespace, - array $callerUseMap, - string $ownerNamespace, - array $ownerUseMap, - array &$diagnostics, - string $classFqn, - ): void { - $params = $ctor->params; - foreach ($newExpr->args as $i => $arg) { - if (!$arg instanceof Arg) { - continue; - } - $param = self::paramAtIndex($params, $i); - if ($param === null) { - continue; - } - $expectedType = $this->extractParamType($param, $substitution, $ownerNamespace, $ownerUseMap); - if ($expectedType === null) { - continue; - } - $actualType = $this->inferArgType($arg->value, $callerNamespace, $callerUseMap); - if ($actualType === null) { - continue; - } - if (self::isSatisfied($actualType, $expectedType, $hierarchy)) { - continue; - } - $diagnostics[] = self::buildMismatchDiagnostic( - $arg->value, - $positionMap, - $i + 1, - $param, - $expectedType, - $actualType, - $classFqn, - ); - } - } - - /** - * Resolve the param record for a given positional argument index, - * honoring variadics (last param consumes all trailing args). - * - * @param list $params - */ - private static function paramAtIndex(array $params, int $index): ?Param - { - if (isset($params[$index])) { - return $params[$index]; - } - $last = $params[count($params) - 1] ?? null; - if ($last !== null && $last->variadic) { - return $last; - } - return null; - } - - /** - * Extract the param's declared type as a normalized display string, - * applying type-arg substitution when the param type references a - * generic type parameter. Returns null when the param has no - * type hint. - * - * For union / intersection types, returns the rendered form - * (`A|B` / `A&B`). Nullable types are rendered with the leading - * `?`. - * - * The `$namespace` + `$useMap` are the OWNER's (the declaring - * class's), not the call site's -- non-generic class-type params - * resolve in the declaring file's import context. - * - * @param array $substitution - * @param array $useMap - */ - private function extractParamType(Param $param, array $substitution, string $namespace, array $useMap): ?string - { - $type = $param->type; - if ($type === null) { - return null; - } - return $this->renderType($type, $substitution, $namespace, $useMap); - } - - /** - * @param array $substitution - * @param array $useMap - */ - private function renderType(Node $type, array $substitution, string $namespace, array $useMap): string - { - if ($type instanceof NullableType) { - return '?' . $this->renderType($type->type, $substitution, $namespace, $useMap); - } - if ($type instanceof Node\UnionType) { - $parts = array_map(fn (Node $t): string => $this->renderType($t, $substitution, $namespace, $useMap), $type->types); - return implode('|', $parts); - } - if ($type instanceof Node\IntersectionType) { - $parts = array_map(fn (Node $t): string => $this->renderType($t, $substitution, $namespace, $useMap), $type->types); - return implode('&', $parts); - } - if ($type instanceof Node\Identifier) { - return $type->toString(); - } - if ($type instanceof Name) { - $raw = ltrim($type->toString(), '\\'); - // Generic type-param substitution: param `T $x` resolves - // to whatever the instantiation passed for T. - if (isset($substitution[$raw])) { - return ltrim($substitution[$raw]->name, '\\'); - } - // Bare scalar / reserved type names (`string`, `int`, - // `self`, etc.) stay as-is -- no FQN resolution. - if ($type->isUnqualified() && self::isReservedTypeName($raw)) { - return $raw; - } - return $this->resolveNameToFqn($type, $namespace, $useMap); - } - return ''; - } - - /** - * Recognises PHP's reserved scalar / pseudo type names that - * shouldn't be FQN-resolved against the use map. - */ - private static function isReservedTypeName(string $name): bool - { - $lower = strtolower($name); - return isset(self::SCALARS[$lower]) - || isset(self::PERMISSIVE_TYPES[$lower]) - || $lower === 'null' - || $lower === 'true' - || $lower === 'false'; - } - - /** - * AST-only argument type inference. Returns null when the static - * type isn't visible from the expression alone (variables, - * method-call results, etc.). - * - * @param array $useMap - */ - private function inferArgType(Expr $expr, string $namespace, array $useMap): ?string - { - if ($expr instanceof New_) { - if (!$expr->class instanceof Name) { - return null; - } - // Generic instantiation: ATTR_TEMPLATE_FQN gives the - // template FQN; for the satisfaction check we compare - // against the template name, not the mangled - // specialization name -- that lines up with the param's - // pre-substitution type. - $templateFqn = $expr->class->getAttribute(XphpSourceParser::ATTR_TEMPLATE_FQN); - if (is_string($templateFqn) && $templateFqn !== '') { - return ltrim($templateFqn, '\\'); - } - return $this->resolveNameToFqn($expr->class, $namespace, $useMap); - } - if ($expr instanceof String_) { - return 'string'; - } - if ($expr instanceof Int_) { - return 'int'; - } - if ($expr instanceof Float_) { - return 'float'; - } - if ($expr instanceof Array_) { - return 'array'; - } - if ($expr instanceof ConstFetch) { - $name = strtolower($expr->name->toString()); - if ($name === 'true' || $name === 'false') { - return 'bool'; - } - if ($name === 'null') { - return 'null'; - } - } - return null; - } - - /** - * Check whether `$actual` satisfies `$expected`. See the class - * docblock for the supported shapes. - */ - private static function isSatisfied(string $actual, string $expected, TypeHierarchy $hierarchy): bool - { - $expected = ltrim($expected, '\\'); - $actual = ltrim($actual, '\\'); - - if ($expected === '' || $actual === '') { - return true; - } - // Nullable param: null is always OK; non-null is checked - // against the inner type. - if (str_starts_with($expected, '?')) { - if ($actual === 'null') { - return true; - } - return self::isSatisfied($actual, substr($expected, 1), $hierarchy); - } - if ($actual === 'null') { - // Non-nullable param with null arg: explicit mismatch. - return false; - } - if (str_contains($expected, '|')) { - foreach (explode('|', $expected) as $arm) { - if (self::isSatisfied($actual, $arm, $hierarchy)) { - return true; - } - } - return false; - } - if (str_contains($expected, '&')) { - foreach (explode('&', $expected) as $arm) { - if (!self::isSatisfied($actual, $arm, $hierarchy)) { - return false; - } - } - return true; - } - $expectedLower = strtolower($expected); - if (isset(self::PERMISSIVE_TYPES[$expectedLower])) { - return true; - } - if ($expected === $actual) { - return true; - } - // Scalar param: arg type must match exactly. `int -> float` - // promotion is technically allowed by PHP but reporting it - // is rarely useful as a warning -- accept the literal `int` - // for a `float` param to avoid false positives. - if (isset(self::SCALARS[$expectedLower])) { - if ($expectedLower === 'float' && $actual === 'int') { - return true; - } - if (isset(self::SCALARS[strtolower($actual)])) { - return $expectedLower === strtolower($actual); - } - // Scalar expected, class supplied → mismatch. - return false; - } - // Both sides are class-like. Defer to the workspace's - // TypeHierarchy. Unknown ancestry -> assume OK (don't false- - // positive on closed-source vendor types). - $is = $hierarchy->isSubtype($actual, $expected); - return $is !== false; - } - - /** - * @param Param $param - */ - private static function buildMismatchDiagnostic( - Expr $argExpr, - PositionMap $positionMap, - int $oneBasedIndex, - Param $param, - string $expectedType, - string $actualType, - string $classFqn, - ): Diagnostic { - $paramName = $param->var instanceof Node\Expr\Variable && is_string($param->var->name) - ? '$' . $param->var->name - : '#' . $oneBasedIndex; - $message = sprintf( - 'Constructor argument %d (%s) of %s expects %s, got %s.', - $oneBasedIndex, - $paramName, - $classFqn, - $expectedType, - $actualType, - ); - if ($argExpr->getStartFilePos() >= 0 && $argExpr->getEndFilePos() >= 0) { - [$sl, $sc, $el, $ec] = $positionMap->rangeFromOffsets( - $argExpr->getStartFilePos(), - $argExpr->getEndFilePos() + 1, - ); - } else { - [$sl, $sc, $el, $ec] = $positionMap->fullLineRangeFromNikic($argExpr->getStartLine()); - } - return new Diagnostic( - startLine: $sl, - startCharacter: $sc, - endLine: $el, - endCharacter: $ec, - message: $message, - code: DiagnosticCode::ConstructorArgumentMismatch, - ); - } -} diff --git a/src/Analyzer/Diagnostic.php b/src/Analyzer/Diagnostic.php index 896bbb9..26e6b69 100644 --- a/src/Analyzer/Diagnostic.php +++ b/src/Analyzer/Diagnostic.php @@ -15,9 +15,19 @@ * front so editors / users can pattern-match by code (e.g. "downgrade * xphp.parse.internal to info"). The previous freeform `string $code = ''` * encouraged inconsistency between catch sites. + * + * `$data` is an optional structured payload carried through to the LSP + * `Diagnostic.data` field. Clients round-trip it back on + * `textDocument/codeAction`, so a code-action provider can build a quick-fix + * from facts the analyzer already computed (e.g. the bound-violation fix-its + * read the offending type-arg range + candidate types from here) without + * re-deriving them from the message text. */ final readonly class Diagnostic { + /** + * @param array|null $data + */ public function __construct( public int $startLine, public int $startCharacter, @@ -26,6 +36,7 @@ public function __construct( public string $message, public DiagnosticCode $code, public DiagnosticSeverity $severity = DiagnosticSeverity::Error, + public ?array $data = null, ) { } } diff --git a/src/Analyzer/DiagnosticCode.php b/src/Analyzer/DiagnosticCode.php index 1f1421e..847b4a5 100644 --- a/src/Analyzer/DiagnosticCode.php +++ b/src/Analyzer/DiagnosticCode.php @@ -77,6 +77,20 @@ enum DiagnosticCode: string */ case ConstructorArgumentMismatch = 'xphp.ctor-arg-mismatch'; + /** + * V2 of the argument-type checker, extending the constructor case + * (`xphp.ctor-arg-mismatch`) to the other call shapes: instance + * method calls (`$obj->m(…)`), static calls (`C::m(…)`), and + * free-function calls (`fn(…)`). Same conservative inference -- + * literals, `new C()`, and `$var`s locally assigned from those -- + * surfacing a would-be runtime `TypeError` ahead of time. + * + * A separate code (rather than reusing the constructor one) lets an + * editor downgrade/scope the two independently and keeps the + * constructor surface's history untouched. + */ + case ArgumentMismatch = 'xphp.arg-mismatch'; + /** * Map a RuntimeException raised by Registry::recordInstantiation to its * diagnostic code. The Registry doesn't (currently) use a typed exception diff --git a/src/Analyzer/WorkspaceAnalyzer.php b/src/Analyzer/WorkspaceAnalyzer.php index b90e831..2fabd9c 100644 --- a/src/Analyzer/WorkspaceAnalyzer.php +++ b/src/Analyzer/WorkspaceAnalyzer.php @@ -56,43 +56,99 @@ public function analyze(array $files, array $hierarchyAsts = []): array $hierarchy = TypeHierarchy::fromAstPerFile($astPerFile); $registry = new Registry(hierarchy: $hierarchy); - // First pass: definitions. Catch duplicate-declaration RuntimeExceptions and pin - // them on the second declaration's file (which is what the compiler also reports). - // Open files first — their declarations win on URI collision. + // Duplicate-template diagnostics. A duplicate is a property of ALL the + // colliding declarations, not of iteration order, so we flag every open + // file that re-declares a template (each diagnostic naming the others). + // This makes the duplicate surface on whichever file the editor pulls: + // the pull provider forces the current file first, which previously made + // it the "canonical" (clean) one and hid the duplicate. Only open files + // ($files) are considered -- filesystem copies live in $hierarchyAsts, so + // an open file is never flagged as a duplicate of its own on-disk copy. + $declarationsByFqn = []; foreach ($files as $path => $entry) { $positionMap = new PositionMap($entry['source']); - $this->walkDefinitions($entry['ast'], $registry, $path, $positionMap, $diagnosticsByFile[$path]); + foreach (self::collectTemplateDeclarations($entry['ast']) as $decl) { + $declarationsByFqn[$decl['fqn']][] = $decl + ['path' => $path, 'positionMap' => $positionMap]; + } + } + foreach ($declarationsByFqn as $fqn => $declarations) { + if (count($declarations) < 2) { + continue; + } + foreach ($declarations as $index => $decl) { + $others = []; + foreach ($declarations as $otherIndex => $other) { + if ($otherIndex !== $index) { + $others[] = $other['path']; + } + } + $diagnosticsByFile[$decl['path']][] = self::buildDefinitionDiagnostic( + $decl['positionMap'], + $decl['name'], + $decl['line'], + sprintf('Generic template "%s" is already declared (also in %s).', $fqn, implode(', ', $others)), + ); + } + } + + // Register every definition so the bound-check pass below can resolve + // templates. Open files first -- their declarations win the registry on a + // collision; the duplicate throw is swallowed since it's already been + // reported above. Filesystem-only definitions are registered silently so + // Registry::validateBounds finds templates whose defining file isn't open. + foreach ($files as $path => $entry) { + $this->recordDefinitions($entry['ast'], $registry, $path); + } + foreach ($hierarchyAsts as $uri => $ast) { + if (isset($files[$uri])) { + continue; + } + $this->recordDefinitions($ast, $registry, $uri); + } + + // Index every named class declaration so the bound-violation fix-its + // can (a) offer concrete types that satisfy the bound and (b) locate the + // offending concrete class to add an `implements` clause. Open files + // carry source (needed to compute a cross-file edit); filesystem-only + // hierarchy entries contribute candidate FQNs only. + $openClasses = []; + $allClassFqns = []; + foreach ($files as $path => $entry) { + foreach (self::collectClasses($entry['ast']) as $cls) { + $openClasses[$cls['fqn']] = ['uri' => $path, 'node' => $cls['node'], 'source' => $entry['source']]; + $allClassFqns[$cls['fqn']] = true; + } } - // Filesystem-only definitions are silently registered so the - // bound-check lookup in `Registry::validateBounds` succeeds even - // when the template's defining file isn't currently open. Any - // duplicate-template throws (whether against an open file already - // registered or against another filesystem entry) land in a sink - // no caller reads — they aren't actionable for the user since - // the offending file isn't on screen. - $definitionSink = []; foreach ($hierarchyAsts as $uri => $ast) { if (isset($files[$uri])) { continue; } - // Degenerate PositionMap is fine: any diagnostic constructed here - // is discarded via the sink, so the bogus offsets never surface. - $this->walkDefinitions($ast, $registry, $uri, new PositionMap(''), $definitionSink); + foreach (self::collectClasses($ast) as $cls) { + $allClassFqns[$cls['fqn']] = true; + } } // Second pass: instantiations. Bound violations fire here. foreach ($files as $path => $entry) { $positionMap = new PositionMap($entry['source']); - $this->walkInstantiations($entry['ast'], $registry, $positionMap, $diagnosticsByFile[$path]); + $this->walkInstantiations( + $entry['ast'], + $registry, + $positionMap, + $entry['source'], + $hierarchy, + $openClasses, + array_keys($allClassFqns), + $diagnosticsByFile[$path], + ); } - // Third pass: constructor argument-type checking (V1 of the - // post-monomorphization arg type checker). Catches the class - // of bugs where `new C(…)` or plain `new C(…)` is called - // with an arg whose static type can't satisfy the (substituted) - // ctor param's declared type -- a runtime TypeError waiting - // to happen. - $argChecks = (new ConstructorArgumentChecker())->check($files, $hierarchy); + // Third pass: argument-type checking across all call shapes -- + // `new C(…)`, `$obj->m(…)`, `C::m(…)`, `fn(…)`. Catches the + // class of bugs where an argument's static type can't satisfy + // the (substituted) parameter's declared type -- a runtime + // TypeError waiting to happen. + $argChecks = (new CallArgumentChecker())->check($files, $hierarchy); foreach ($argChecks as $path => $diags) { foreach ($diags as $diag) { $diagnosticsByFile[$path][] = $diag; @@ -103,23 +159,19 @@ public function analyze(array $files, array $hierarchyAsts = []): array } /** + * Register every generic-template declaration into the registry so the + * bound-check pass can resolve templates. Duplicate-declaration throws are + * swallowed -- they are surfaced as diagnostics by the dedicated cross-file + * pass in {@see analyze()}, and the registry keeps the first registration. + * * @param list $ast - * @param list $diagnostics */ - private function walkDefinitions( - array $ast, - Registry $registry, - string $sourceFile, - PositionMap $positionMap, - array &$diagnostics, - ): void { - $visitor = new class($registry, $sourceFile, $positionMap, $diagnostics) extends NodeVisitorAbstract { - /** @param list $diagnostics */ + private function recordDefinitions(array $ast, Registry $registry, string $sourceFile): void + { + $visitor = new class($registry, $sourceFile) extends NodeVisitorAbstract { public function __construct( private readonly Registry $registry, private readonly string $sourceFile, - private readonly PositionMap $positionMap, - private array &$diagnostics, ) { } @@ -133,71 +185,108 @@ public function enterNode(Node $node): null if (!is_array($params) || $params === [] || !is_string($fqn)) { return null; } - // Deliberately do NOT pre-check for "already registered" — the whole point - // of running this in the analyzer is to surface the duplicate-declaration - // RuntimeException from Registry::recordDefinition as a diagnostic. try { - $this->registry->recordDefinition( - $fqn, - $node->name->toString(), - $params, - $node, - $this->sourceFile, - ); - } catch (RuntimeException $e) { - $this->diagnostics[] = self::buildDiagnostic( - $this->positionMap, - $node->name, - $node->getStartLine(), - DiagnosticCode::Definition, - $e->getMessage(), - ); + $this->registry->recordDefinition($fqn, $node->name->toString(), $params, $node, $this->sourceFile); + } catch (RuntimeException) { + // Duplicate -- already reported by the cross-file pass; the + // registry keeps the first registration. } return null; } + }; - private static function buildDiagnostic( - PositionMap $positionMap, - ?\PhpParser\Node\Identifier $identifier, - int $fallbackNikicLine, - DiagnosticCode $code, - string $message, - ): Diagnostic { - // Prefer the identifier's actual byte span — squiggles the class - // name, not the whole line. Fall back to the full-line range when - // position info is missing (synthetic nodes etc.). - if ($identifier !== null && $identifier->getStartFilePos() >= 0) { - [$sl, $sc, $el, $ec] = $positionMap->rangeFromOffsets( - $identifier->getStartFilePos(), - $identifier->getEndFilePos() + 1, - ); - } else { - [$sl, $sc, $el, $ec] = $positionMap->fullLineRangeFromNikic($fallbackNikicLine); + $traverser = new NodeTraverser(); + $traverser->addVisitor($visitor); + $traverser->traverse($ast); + } + + /** + * Collect every generic-template declaration (FQN + name node) in a file, + * for the cross-file duplicate-detection pass. + * + * @param list $ast + * @return list + */ + private static function collectTemplateDeclarations(array $ast): array + { + $visitor = new class extends NodeVisitorAbstract { + /** @var list */ + public array $declarations = []; + + public function enterNode(Node $node): null + { + if (!$node instanceof ClassLike || $node->name === null) { + return null; } - return new Diagnostic($sl, $sc, $el, $ec, $message, code: $code); + $params = $node->getAttribute(XphpSourceParser::ATTR_GENERIC_PARAMS); + $fqn = $node->getAttribute(XphpSourceParser::ATTR_TEMPLATE_FQN); + if (!is_array($params) || $params === [] || !is_string($fqn)) { + return null; + } + $this->declarations[] = ['fqn' => $fqn, 'name' => $node->name, 'line' => $node->getStartLine()]; + return null; } }; $traverser = new NodeTraverser(); $traverser->addVisitor($visitor); $traverser->traverse($ast); + + return $visitor->declarations; } /** - * @param list $ast - * @param list $diagnostics + * Build a Definition diagnostic squiggling the class-name span (falling back + * to the full line when byte offsets are missing). + */ + private static function buildDefinitionDiagnostic( + PositionMap $positionMap, + \PhpParser\Node\Identifier $identifier, + int $fallbackNikicLine, + string $message, + ): Diagnostic { + if ($identifier->getStartFilePos() >= 0) { + [$sl, $sc, $el, $ec] = $positionMap->rangeFromOffsets( + $identifier->getStartFilePos(), + $identifier->getEndFilePos() + 1, + ); + } else { + [$sl, $sc, $el, $ec] = $positionMap->fullLineRangeFromNikic($fallbackNikicLine); + } + return new Diagnostic($sl, $sc, $el, $ec, $message, code: DiagnosticCode::Definition); + } + + /** + * @param list $ast + * @param array $openClasses + * @param list $allClassFqns + * @param list $diagnostics */ private function walkInstantiations( array $ast, Registry $registry, PositionMap $positionMap, + string $source, + TypeHierarchy $hierarchy, + array $openClasses, + array $allClassFqns, array &$diagnostics, ): void { - $visitor = new class($registry, $positionMap, $diagnostics) extends NodeVisitorAbstract { - /** @param list $diagnostics */ + $analyzer = $this; + $visitor = new class($registry, $positionMap, $source, $hierarchy, $openClasses, $allClassFqns, $analyzer, $diagnostics) extends NodeVisitorAbstract { + /** + * @param array $openClasses + * @param list $allClassFqns + * @param list $diagnostics + */ public function __construct( private readonly Registry $registry, private readonly PositionMap $positionMap, + private readonly string $source, + private readonly TypeHierarchy $hierarchy, + private readonly array $openClasses, + private readonly array $allClassFqns, + private readonly WorkspaceAnalyzer $analyzer, private array &$diagnostics, ) { } @@ -232,18 +321,33 @@ public function enterNode(Node $node): null } else { [$sl, $sc, $el, $ec] = $this->positionMap->fullLineRangeFromNikic($node->getStartLine()); } + // Registry::recordInstantiation has two error paths + // (bound violation vs. hash collision). The triage helper + // distinguishes them by the message's leading phrase so + // editors / users can act on the right hint (raise + // XPHP_HASH_LENGTH vs. fix the bound). + $code = DiagnosticCode::fromRegistryRecordInstantiationException($e); + $data = $code === DiagnosticCode::BoundViolation + ? $this->analyzer->buildBoundFixData( + $node, + $args, + $fqn, + $this->source, + $this->positionMap, + $this->registry, + $this->hierarchy, + $this->openClasses, + $this->allClassFqns, + ) + : null; $this->diagnostics[] = new Diagnostic( startLine: $sl, startCharacter: $sc, endLine: $el, endCharacter: $ec, message: $e->getMessage(), - // Registry::recordInstantiation has two error paths - // (bound violation vs. hash collision). The triage helper - // distinguishes them by the message's leading phrase so - // editors / users can act on the right hint (raise - // XPHP_HASH_LENGTH vs. fix the bound). - code: DiagnosticCode::fromRegistryRecordInstantiationException($e), + code: $code, + data: $data, ); } return null; @@ -254,4 +358,231 @@ public function enterNode(Node $node): null $traverser->addVisitor($visitor); $traverser->traverse($ast); } + + /** + * Compute the structured fix-it payload for a generic bound violation: + * which type parameter / bound was violated, the offending concrete type, + * the source range of the offending type-argument (for a "swap" fix), + * concrete workspace types that DO satisfy the bound, and -- when the + * concrete type is an open-buffer class -- where to add an `implements` + * clause (for an "implement the interface" fix). + * + * Returns null when the violating param can't be pinned down, leaving a + * plain (fix-less) diagnostic. + * + * @param list $args TypeRef[] + * @param array $openClasses + * @param list $allClassFqns + * @return array|null + */ + public function buildBoundFixData( + Name $node, + array $args, + string $templateFqn, + string $source, + PositionMap $positionMap, + Registry $registry, + TypeHierarchy $hierarchy, + array $openClasses, + array $allClassFqns, + ): ?array { + $definition = $registry->definition($templateFqn); + if ($definition === null) { + return null; + } + $typeParams = $definition->typeParams; + if (count($typeParams) !== count($args)) { + return null; + } + + // Locate the first type-param whose bound the supplied arg violates. + $index = null; + foreach ($typeParams as $i => $param) { + if ($param->boundFqn === null) { + continue; + } + if ($hierarchy->isSubtype($args[$i]->name, $param->boundFqn) !== true) { + $index = $i; + break; + } + } + if ($index === null) { + return null; + } + + $bound = ltrim((string) $typeParams[$index]->boundFqn, '\\'); + $concrete = ltrim((string) $args[$index]->name, '\\'); + $concreteIsScalar = (bool) ($args[$index]->isScalar ?? false); + + // Candidate concrete types that satisfy the bound (for the "swap" fix). + $candidates = []; + foreach ($allClassFqns as $candidateFqn) { + if ($candidateFqn === $concrete) { + continue; + } + if ($hierarchy->isSubtype($candidateFqn, $bound) === true) { + $short = strrpos($candidateFqn, '\\') !== false + ? substr($candidateFqn, strrpos($candidateFqn, '\\') + 1) + : $candidateFqn; + $candidates[$short] = true; + } + } + $candidateNames = array_keys($candidates); + sort($candidateNames); + $candidateNames = array_slice($candidateNames, 0, 3); + + return [ + 'kind' => 'bound', + 'param' => $typeParams[$index]->name, + 'bound' => $bound, + 'concrete' => $concrete, + 'concreteIsScalar' => $concreteIsScalar, + 'typeArgRange' => self::typeArgRange($source, $node->getEndFilePos() + 1, $index, $positionMap), + 'candidates' => $candidateNames, + 'implementsInsert' => $concreteIsScalar + ? null + : self::implementsInsert($openClasses[$concrete] ?? null, $bound), + ]; + } + + /** + * Resolve the LSP range of the type-argument at `$index` in the `<…>` + * clause that follows `$fromOffset` in the original source. Generic + * clauses are stripped to equal-length whitespace before nikic parses, so + * byte offsets are 1:1 with the original text the PositionMap was built on. + * + * @return array{startLine: int, startCharacter: int, endLine: int, endCharacter: int}|null + */ + private static function typeArgRange(string $source, int $fromOffset, int $index, PositionMap $positionMap): ?array + { + $len = strlen($source); + $i = $fromOffset; + while ($i < $len && ctype_space($source[$i])) { + $i++; + } + if ($i >= $len || $source[$i] !== '<') { + return null; + } + $i++; + $depth = 0; + $segmentStart = $i; + $segments = []; + for (; $i < $len; $i++) { + $ch = $source[$i]; + if ($ch === '<') { + $depth++; + } elseif ($ch === '>') { + if ($depth === 0) { + $segments[] = [$segmentStart, $i]; + break; + } + $depth--; + } elseif ($ch === ',' && $depth === 0) { + $segments[] = [$segmentStart, $i]; + $segmentStart = $i + 1; + } + } + if (!isset($segments[$index])) { + return null; + } + [$start, $end] = $segments[$index]; + while ($start < $end && ctype_space($source[$start])) { + $start++; + } + while ($end > $start && ctype_space($source[$end - 1])) { + $end--; + } + if ($end <= $start) { + return null; + } + [$sl, $sc, $el, $ec] = $positionMap->rangeFromOffsets($start, $end); + return ['startLine' => $sl, 'startCharacter' => $sc, 'endLine' => $el, 'endCharacter' => $ec]; + } + + /** + * Compute where to insert an `implements \Bound` clause on the concrete + * class so it satisfies the violated bound. Only open-buffer `class` + * declarations are supported (the edit needs the file's source). Returns + * null when the concrete type isn't an editable open class or already + * implements the bound. + * + * @param array{uri: string, node: ClassLike, source: string}|null $entry + * @return array{uri: string, line: int, character: int, newText: string}|null + */ + private static function implementsInsert(?array $entry, string $bound): ?array + { + if ($entry === null || !$entry['node'] instanceof Node\Stmt\Class_) { + return null; + } + $class = $entry['node']; + $boundShort = strrpos($bound, '\\') !== false ? substr($bound, strrpos($bound, '\\') + 1) : $bound; + foreach ($class->implements as $impl) { + $parts = $impl->getParts(); + if (end($parts) === $boundShort) { + return null; // already implements it + } + } + + if ($class->implements !== []) { + $anchor = $class->implements[count($class->implements) - 1]; + $newText = ', \\' . $bound; + } elseif ($class->extends !== null) { + $anchor = $class->extends; + $newText = ' implements \\' . $bound; + } elseif ($class->name !== null) { + $anchor = $class->name; + $newText = ' implements \\' . $bound; + } else { + return null; + } + $insertOffset = $anchor->getEndFilePos() + 1; + if ($insertOffset <= 0) { + return null; + } + [$line, $character] = (new PositionMap($entry['source']))->offsetToPosition($insertOffset); + return ['uri' => $entry['uri'], 'line' => $line, 'character' => $character, 'newText' => $newText]; + } + + /** + * Every named-class FQN declared in an AST. Used by the diagnostics + * provider to keep the bound-check hierarchy free of duplicate-FQN + * collisions: a filesystem file enters the hierarchy only when it's the + * nearest declarer of a class it defines. + * + * @param list $ast + * @return list + */ + public static function classFqnsIn(array $ast): array + { + return array_map(static fn (array $cls): string => $cls['fqn'], self::collectClasses($ast)); + } + + /** + * Collect every named ClassLike in an AST paired with its computed FQN + * (namespace + short name). Mirrors collectTemplateDeclarations but for + * ALL classes, not just generic templates. + * + * @param list $ast + * @return list + */ + private static function collectClasses(array $ast): array + { + $out = []; + foreach ($ast as $stmt) { + if ($stmt instanceof Node\Stmt\Namespace_) { + $ns = $stmt->name === null ? '' : $stmt->name->toString(); + foreach ($stmt->stmts as $inner) { + if ($inner instanceof ClassLike && $inner->name !== null) { + $short = $inner->name->toString(); + $out[] = ['fqn' => $ns !== '' ? $ns . '\\' . $short : $short, 'node' => $inner]; + } + } + continue; + } + if ($stmt instanceof ClassLike && $stmt->name !== null) { + $out[] = ['fqn' => $stmt->name->toString(), 'node' => $stmt]; + } + } + return $out; + } } diff --git a/src/Diagnostics/DiagnosticTranslator.php b/src/Diagnostics/DiagnosticTranslator.php index 234db7a..95fba7b 100644 --- a/src/Diagnostics/DiagnosticTranslator.php +++ b/src/Diagnostics/DiagnosticTranslator.php @@ -28,6 +28,11 @@ public static function toLsp(Diagnostic $d): LspDiagnostic $lsp->severity = $d->severity->value; $lsp->code = $d->code->value; $lsp->source = 'xphp'; + // Structured fix-it payload (e.g. bound-violation quick-fix facts). + // Clients echo `data` back on textDocument/codeAction. + if ($d->data !== null) { + $lsp->data = $d->data; + } return $lsp; } } diff --git a/src/Diagnostics/XphpDiagnosticsProvider.php b/src/Diagnostics/XphpDiagnosticsProvider.php index 6f6e434..7491661 100644 --- a/src/Diagnostics/XphpDiagnosticsProvider.php +++ b/src/Diagnostics/XphpDiagnosticsProvider.php @@ -8,6 +8,7 @@ use Amp\Promise; use Amp\Success; use Phpactor\LanguageServer\Core\Diagnostics\DiagnosticsProvider; +use Phpactor\LanguageServer\Core\Server\ClientApi; use Phpactor\LanguageServer\Core\Workspace\Workspace as PhpactorWorkspace; use Phpactor\LanguageServerProtocol\Diagnostic as LspDiagnostic; use Phpactor\LanguageServerProtocol\TextDocumentItem; @@ -20,33 +21,55 @@ * * Strategy: * 1. Parse the document being linted via the per-file Analyzer. If it has syntax - * errors we surface those and skip the workspace pass (the AST is unusable). + * errors we surface those and skip the workspace pass for it (the AST is unusable). * 2. Parse every other currently-open document in the phpactor workspace via the - * same Analyzer. Drop any that fail to parse — they get their own diagnostics - * when they're individually re-linted. - * 3. Run the WorkspaceAnalyzer over the parsed set. It catches bound violations, - * duplicate templates, and hash collisions. - * 4. Return only the diagnostics keyed to the document we were asked about. - * Cross-file violations on OTHER documents will get re-published when those - * documents themselves are re-linted (debounce-driven; an explicit broadcast - * pass is a follow-up). + * same Analyzer. + * 3. Run the WorkspaceAnalyzer over the parsed set ONCE, producing a per-file + * diagnostic map for every open document. It catches bound violations, + * duplicate templates, hash collisions, and argument-type mismatches. + * 4. Return the diagnostics keyed to the document we were asked about. + * 5. Cross-file broadcast (push path only): because a single workspace pass + * already computes diagnostics for EVERY open document, editing `Box.xphp` + * can re-publish the now-stale diagnostics of dependents like `Use.xphp` + * without waiting for the user to touch them. We push a + * `textDocument/publishDiagnostics` for each OTHER open document whose + * diagnostics changed since we last published them. The document being + * linted is left to the engine (it publishes that one itself). * * Translates framework-neutral Diagnostic → LSP-wire Diagnostic at the boundary * via DiagnosticTranslator. */ final class XphpDiagnosticsProvider implements DiagnosticsProvider { + /** + * Per-URI signature of the diagnostics last broadcast for that document, + * so a re-lint that doesn't change a dependent's diagnostics doesn't + * re-publish them (avoids flooding the client on rapid edit storms). + * + * @var array + */ + private array $lastBroadcast = []; + public function __construct( private readonly ParsedDocumentCache $cache, private readonly WorkspaceAnalyzer $workspaceAnalyzer, private readonly PhpactorWorkspace $workspace, private readonly FqnIndex $fqnIndex, + /** + * Client handle used to push diagnostics for dependent documents. Null in + * pull-mode / unit contexts that don't broadcast; when null, step 5 above + * is skipped and the provider behaves exactly as the per-document linter. + */ + private readonly ?ClientApi $clientApi = null, ) { } public function provideDiagnostics(TextDocumentItem $textDocument, CancellationToken $cancel): Promise { - return new Success($this->analyzeSync($textDocument)); + $byUri = $this->computeAllOpenDiagnostics($textDocument); + $this->broadcastDependents($textDocument->uri, $byUri); + + return new Success($byUri[$textDocument->uri] ?? []); } public function name(): string @@ -57,11 +80,25 @@ public function name(): string /** * Sync entry-point shared by the push-mode `provideDiagnostics` * (above) and the pull-mode `textDocument/diagnostic` handler. - * Both flows want the same analysis without the Promise wrap. + * Pull mode never broadcasts -- it returns the requested document's + * diagnostics and nothing else. * * @return list */ public function analyzeSync(TextDocumentItem $textDocument): array + { + return $this->computeAllOpenDiagnostics($textDocument)[$textDocument->uri] ?? []; + } + + /** + * Run a single workspace pass and return the merged (per-file syntax + + * cross-file) LSP diagnostics for EVERY open document, keyed by URI. The + * document being linted is included like any other; callers pick the + * entries they need. + * + * @return array> + */ + private function computeAllOpenDiagnostics(TextDocumentItem $textDocument): array { $currentUri = $textDocument->uri; @@ -73,40 +110,48 @@ public function analyzeSync(TextDocumentItem $textDocument): array $textDocument->version, $textDocument->text, ); - $perFileDiagnostics = array_map( - static fn ($d) => DiagnosticTranslator::toLsp($d), - $currentResult->diagnostics, - ); - - // If the document didn't parse, the workspace pass would skip it anyway — - // and feeding it through TypeHierarchy::fromAstPerFile() with a null AST - // is meaningless. Return the syntax diagnostics alone. - if ($currentResult->ast === null) { - return $perFileDiagnostics; - } - // Build the {uri: {ast, source}} map for the workspace pass: the current - // document plus every OTHER open document that we can parse cleanly. - $parsedFiles = [ - $currentUri => ['ast' => $currentResult->ast, 'source' => $textDocument->text], + // Per-file (syntax) diagnostics + the {uri: {ast, source}} map for the + // workspace pass, covering the current document plus every OTHER open + // document. Documents that fail to parse contribute their syntax + // diagnostics but are kept out of the workspace pass (unusable AST). + $perFileByUri = [ + $currentUri => array_map( + static fn ($d): LspDiagnostic => DiagnosticTranslator::toLsp($d), + $currentResult->diagnostics, + ), ]; + $parsedFiles = []; + if ($currentResult->ast !== null) { + $parsedFiles[$currentUri] = ['ast' => $currentResult->ast, 'source' => $textDocument->text]; + } foreach ($this->workspace as $uri => $item) { if ($uri === $currentUri) { continue; } $otherResult = $this->cache->getOrParse($uri, $item->version, $item->text); - if ($otherResult->ast === null) { - continue; + $perFileByUri[$uri] = array_map( + static fn ($d): LspDiagnostic => DiagnosticTranslator::toLsp($d), + $otherResult->diagnostics, + ); + if ($otherResult->ast !== null) { + $parsedFiles[$uri] = ['ast' => $otherResult->ast, 'source' => $item->text]; } - $parsedFiles[$uri] = ['ast' => $otherResult->ast, 'source' => $item->text]; } - // Enrich the bound-check hierarchy with every filesystem-indexed file the + // Enrich the bound-check hierarchy with filesystem-indexed files the // ParsedDocumentCacheWarmer has already parsed. Without this, the workspace // pass only sees open buffers — so `new Box(…)` in an open file dependent // on a Tag class that's on disk but not open fires a spurious - // "concrete type is not in the source set" diagnostic. Open-buffer entries - // already in $parsedFiles take precedence and are skipped here. + // "concrete type is not in the source set" diagnostic. + // + // A file is admitted only when it is the NEAREST declarer (to the document + // being linted) of a class it defines. That keeps each FQN's ancestry + // single-sourced even when the workspace has duplicate FQNs across + // packages / fixture trees (`pathFor` resolves the proximity winner; + // open buffers already in $parsedFiles win and exclude their on-disk + // copies). Files that declare no class — pure usage sites — contribute + // nothing to the ancestry and are skipped. $hierarchyAsts = []; foreach ($this->fqnIndex->indexedFilesystemPaths() as $path) { $uri = 'file://' . $path; @@ -117,17 +162,82 @@ public function analyzeSync(TextDocumentItem $textDocument): array if ($peek === null || $peek->ast === null) { continue; } - $hierarchyAsts[$uri] = $peek->ast; + foreach (WorkspaceAnalyzer::classFqnsIn($peek->ast) as $fqn) { + if ($this->fqnIndex->pathFor($fqn, $currentUri) === $path) { + $hierarchyAsts[$uri] = $peek->ast; + break; + } + } } - $workspaceByUri = $this->workspaceAnalyzer->analyze($parsedFiles, $hierarchyAsts); - $currentWorkspaceDiagnostics = $workspaceByUri[$currentUri] ?? []; + $workspaceByUri = $parsedFiles === [] + ? [] + : $this->workspaceAnalyzer->analyze($parsedFiles, $hierarchyAsts); - $lspWorkspaceDiagnostics = array_map( - static fn ($d) => DiagnosticTranslator::toLsp($d), - $currentWorkspaceDiagnostics, - ); + $result = []; + foreach ($perFileByUri as $uri => $perFile) { + $workspaceDiagnostics = array_map( + static fn ($d): LspDiagnostic => DiagnosticTranslator::toLsp($d), + $workspaceByUri[$uri] ?? [], + ); + $result[$uri] = array_merge($perFile, $workspaceDiagnostics); + } + + return $result; + } - return array_merge($perFileDiagnostics, $lspWorkspaceDiagnostics); + /** + * Push `textDocument/publishDiagnostics` for every open document OTHER than + * the one being linted whose diagnostics changed since we last published + * them. No-op when no client handle is wired (pull-mode / unit contexts). + * + * @param array> $byUri + */ + private function broadcastDependents(string $currentUri, array $byUri): void + { + if ($this->clientApi === null) { + return; + } + foreach ($byUri as $uri => $diagnostics) { + if ($uri === $currentUri) { + // The engine publishes the linted document itself. + continue; + } + if (!$this->workspace->has($uri)) { + continue; + } + $signature = self::signature($diagnostics); + if (($this->lastBroadcast[$uri] ?? null) === $signature) { + continue; + } + $this->lastBroadcast[$uri] = $signature; + $this->clientApi->diagnostics()->publishDiagnostics( + $uri, + $this->workspace->get($uri)->version, + $diagnostics, + ); + } + } + + /** + * Order-sensitive fingerprint of a diagnostic list, used to skip redundant + * re-publishes. + * + * @param list $diagnostics + */ + private static function signature(array $diagnostics): string + { + $parts = []; + foreach ($diagnostics as $d) { + $parts[] = implode(':', [ + $d->range->start->line, + $d->range->start->character, + $d->range->end->line, + $d->range->end->character, + (string) ($d->code ?? ''), + $d->message, + ]); + } + return implode('|', $parts); } } diff --git a/src/Dispatcher/OriginTrackingMiddleware.php b/src/Dispatcher/OriginTrackingMiddleware.php new file mode 100644 index 0000000..92a169c --- /dev/null +++ b/src/Dispatcher/OriginTrackingMiddleware.php @@ -0,0 +1,60 @@ +fqnIndex->withOrigin(self::originUri($message)); + + return $handler->handle($message); + } + + private static function originUri(Message $message): ?string + { + if (!$message instanceof RequestMessage && !$message instanceof NotificationMessage) { + return null; + } + $params = $message->params; + if (!is_array($params)) { + return null; + } + $textDocument = $params['textDocument'] ?? null; + if (is_array($textDocument) && isset($textDocument['uri']) && is_string($textDocument['uri'])) { + return $textDocument['uri']; + } + return null; + } +} diff --git a/src/Handler/SemanticTokens/AstVisitor.php b/src/Handler/SemanticTokens/AstVisitor.php index 79bbb13..2dd8064 100644 --- a/src/Handler/SemanticTokens/AstVisitor.php +++ b/src/Handler/SemanticTokens/AstVisitor.php @@ -36,9 +36,14 @@ * 1. **Token scan** via PHP's built-in {@see PhpToken::tokenize}. The * tokens are byte-indexed into the ORIGINAL source (not the stripped * buffer), so positions feed directly into {@see PositionMap}. - * Emits keywords, variables, numbers, single-quoted strings, - * double-quoted strings (as a single span -- finer-grained - * interpolation classification is not yet handled), and comments. Deliberately + * Emits keywords, variables, numbers, and comments. Strings are + * classified at token granularity: a non-interpolated string is one + * `string` span; an interpolated `"… $x …"` is decomposed by the + * tokenizer into its literal slabs (`T_ENCAPSED_AND_WHITESPACE` -> + * `string`) and inner `T_VARIABLE` (-> `variable`), each emitted + * separately so the variable highlights inside the string. Token + * lengths are reported in UTF-16 code units (LSP's unit), so a token + * spanning a multi-byte character stays spec-correct. Deliberately * skips T_STRING (identifiers) so the AST pass can classify each * identifier into its semantic role without overlap. * @@ -477,10 +482,12 @@ public function emit(array &$out, int $originalOffset, int $length, string $type return; } [$line, $startChar] = $this->positionMap->offsetToPosition($originalOffset); - // Length stays in BYTES at this point -- correct for ASCII-only - // identifiers (the vast majority of PHP source). LSP wants - // UTF-16 code units; for ASCII the two are equal. Non-ASCII - // tokens (e.g. UTF-8 strings) are an edge case not yet handled. + // LSP measures token lengths in UTF-16 code units, not bytes. For + // ASCII the two are equal (the common case), but a token covering a + // multi-byte character (e.g. a `"café"` string literal) would be + // over-long by the extra UTF-8 bytes if we reported the byte count. + // Convert the covered byte span to its UTF-16 length. + $length = PositionMap::lengthInUtf16(substr($this->source, $originalOffset, $length)); $out[] = new TokenSpec( line: $line, startChar: $startChar, diff --git a/src/Handler/XphpCodeActionHandler.php b/src/Handler/XphpCodeActionHandler.php index 995ae14..772e12e 100644 --- a/src/Handler/XphpCodeActionHandler.php +++ b/src/Handler/XphpCodeActionHandler.php @@ -15,6 +15,7 @@ use Phpactor\LanguageServerProtocol\CodeActionParams; use Phpactor\LanguageServerProtocol\ServerCapabilities; use XPHP\Lsp\PositionMap; +use XPHP\Lsp\Resolver\BoundErrorCodeActionProvider; use XPHP\Lsp\Resolver\DiagnosticCodeActionProvider; use XPHP\Lsp\Resolver\ImportCodeActionProvider; use XPHP\Lsp\Resolver\OptimizeImportsCodeActionProvider; @@ -48,6 +49,7 @@ public function __construct( private readonly ImportCodeActionProvider $importProvider, private readonly DiagnosticCodeActionProvider $diagnosticProvider, private readonly OptimizeImportsCodeActionProvider $optimizeImportsProvider, + private readonly BoundErrorCodeActionProvider $boundErrorProvider, ) { } @@ -97,6 +99,12 @@ public function codeAction(CodeActionParams $params, ?CancellationToken $cancel $params->context->diagnostics ?? [], ); $optimizeActions = $this->optimizeImportsProvider->actionsFor($uri, $item->version, $item->text); - return new Success(array_merge($importActions, $diagnosticActions, $optimizeActions)); + $boundActions = $this->boundErrorProvider->actionsFor( + $uri, + $item->version, + $item->text, + $params->context->diagnostics ?? [], + ); + return new Success(array_merge($importActions, $diagnosticActions, $optimizeActions, $boundActions)); } } diff --git a/src/Handler/XphpDefinitionHandler.php b/src/Handler/XphpDefinitionHandler.php index 924e069..651153f 100644 --- a/src/Handler/XphpDefinitionHandler.php +++ b/src/Handler/XphpDefinitionHandler.php @@ -21,6 +21,7 @@ use XPHP\Lsp\Analyzer\ParsedDocumentCache; use XPHP\Lsp\PositionMap; use XPHP\Lsp\Reflection\FqnIndex; +use XPHP\Lsp\Resolver\GenericResolver; use XPHP\Lsp\Resolver\PhpDefinitionResolver; use XPHP\Lsp\Resolver\ReferenceFinder; use XPHP\Transpiler\Monomorphize\XphpSourceParser; @@ -55,6 +56,7 @@ public function __construct( private readonly FqnIndex $fqnIndex, private readonly ReferenceFinder $referenceFinder, private readonly ?PhpDefinitionResolver $phpResolver = null, + private readonly ?GenericResolver $genericResolver = null, ) { } @@ -154,6 +156,24 @@ public function definition(DefinitionParams $params, ?CancellationToken $cancel } } + // Path 2.5: cursor on a generic method-call name (`first` in + // `$users->first()` where `$users = new Collection()`). The + // receiver's class carries xphp generic syntax (`T[]`, reified `T`) that + // worse-reflection can't reflect, so Path 3 below misses it. Resolve the + // method declaration xphp-natively: GenericResolver infers the receiver + // class (the same inference inlay hints use) and FqnIndex locates the + // method member. Plain (non-generic) receivers fall through to Path 3, + // which already handles them. + if ($this->genericResolver !== null) { + $location = self::locationToLsp($this->genericResolver->resolveMethodDeclarationAt( + $params->textDocument->uri, + $offset, + )); + if ($location !== null) { + return new Success($location); + } + } + // Path 3: PHP-semantic GTD via worse-reflection. Handles everything // the xphp-specific paths above don't: `use App\Models\User;`, // `new User(...)`, `$obj->method()`, `Cls::method()`, `strlen(...)` diff --git a/src/Handler/XphpDocumentHighlightHandler.php b/src/Handler/XphpDocumentHighlightHandler.php index 049bc26..505a587 100644 --- a/src/Handler/XphpDocumentHighlightHandler.php +++ b/src/Handler/XphpDocumentHighlightHandler.php @@ -14,7 +14,9 @@ use Phpactor\LanguageServerProtocol\DocumentHighlightKind; use Phpactor\LanguageServerProtocol\DocumentHighlightParams; use Phpactor\LanguageServerProtocol\ServerCapabilities; +use XPHP\Lsp\Analyzer\ParsedDocumentCache; use XPHP\Lsp\PositionMap; +use XPHP\Lsp\Resolver\DocumentHighlightKindResolver; use XPHP\Lsp\Resolver\ReferenceFinder; /** @@ -26,12 +28,10 @@ * `textDocument/references` -- we run the same resolver and filter to * the requesting document only. * - * We don't distinguish read / write / text kind today; everything is - * reported as `DocumentHighlightKind::TEXT`, which clients render with - * the default highlight colour. Read/write classification needs nikic - * AST parent-walk (assignment LHS vs RHS); not implemented here - * because the LSP spec marks kind as optional and PhpStorm renders - * TEXT identically to the other kinds anyway. + * Each occurrence is classified as `WRITE` (the symbol's declaration or + * an assignment / lvalue) or `READ` (a use site) via + * {@see DocumentHighlightKindResolver}, so clients that colour read vs. + * write (e.g. VS Code) paint them distinctly. * * Available since IntelliJ Platform 2025.3. */ @@ -40,6 +40,8 @@ final class XphpDocumentHighlightHandler implements Handler, CanRegisterCapabili public function __construct( private readonly PhpactorWorkspace $workspace, private readonly ReferenceFinder $finder, + private readonly ParsedDocumentCache $cache, + private readonly DocumentHighlightKindResolver $kindResolver, ) { } @@ -68,7 +70,8 @@ public function documentHighlight(DocumentHighlightParams $params, ?Cancellation return new Success([]); } $item = $this->workspace->get($uri); - $offset = (new PositionMap($item->text))->positionToOffset( + $positionMap = new PositionMap($item->text); + $offset = $positionMap->positionToOffset( $params->position->line, $params->position->character, ); @@ -86,11 +89,28 @@ public function documentHighlight(DocumentHighlightParams $params, ?Cancellation // matches stay available through textDocument/references. $locations = $this->finder->findReferences($uri, $offset, true, $cancel, $uri); - $highlights = []; + // Classify each occurrence as read/write. The location ranges are in + // ORIGINAL-source coordinates; the resolver maps the AST's stripped + // offsets back to original, so the keys line up. + $targetOffsets = []; foreach ($locations as $location) { + $targetOffsets[] = $positionMap->positionToOffset( + $location->range->start->line, + $location->range->start->character, + ); + } + $parsed = $this->cache->getOrParse($uri, $item->version, $item->text); + $kindByOffset = $parsed->ast !== null + ? $this->kindResolver->resolve($parsed->ast, $parsed->byteOffsetMap, $targetOffsets) + : []; + + $highlights = []; + foreach ($locations as $i => $location) { $highlights[] = new DocumentHighlight( range: $location->range, - kind: DocumentHighlightKind::TEXT, + // Default to READ for any occurrence the classifier didn't + // resolve (it's a use site) -- never silently drop a highlight. + kind: $kindByOffset[$targetOffsets[$i]] ?? DocumentHighlightKind::READ, ); } return new Success($highlights); diff --git a/src/LspDispatcherFactory.php b/src/LspDispatcherFactory.php index d54eb90..e5f94ae 100644 --- a/src/LspDispatcherFactory.php +++ b/src/LspDispatcherFactory.php @@ -182,6 +182,9 @@ public function create(MessageTransmitter $transmitter, InitializeParams $initia new WorkspaceAnalyzer(), $workspace, $fqnIndex, + // Push path: lets a workspace pass re-publish diagnostics for the + // dependents of the edited file (cross-file broadcast). + $clientApi, ); $diagnosticsEngine = new DiagnosticsEngine( @@ -279,6 +282,7 @@ public function create(MessageTransmitter $transmitter, InitializeParams $initia $fqnIndex, new ReferenceFinder($workspace, $cache, $fqnIndex, $xphpParser, $reflector, $genericResolver), $phpDefinitionResolver, + $genericResolver, ), new XphpTypeDefinitionHandler($phpDefinitionResolver), new XphpCompletionHandler($workspace, $workspaceSymbols, $phpCompletionResolver, $fqnIndex, $reflector), @@ -290,6 +294,7 @@ public function create(MessageTransmitter $transmitter, InitializeParams $initia new ImportCodeActionProvider($fqnIndex, $cache), new DiagnosticCodeActionProvider(), new OptimizeImportsCodeActionProvider($cache), + new \XPHP\Lsp\Resolver\BoundErrorCodeActionProvider(), ), new XphpCodeActionResolveHandler(), new XphpDocumentSymbolHandler($workspace, $cache), @@ -309,6 +314,8 @@ public function create(MessageTransmitter $transmitter, InitializeParams $initia new XphpDocumentHighlightHandler( $workspace, new ReferenceFinder($workspace, $cache, $fqnIndex, $xphpParser, $reflector, $genericResolver), + $cache, + new \XPHP\Lsp\Resolver\DocumentHighlightKindResolver(), ), new XphpRenameHandler( $workspace, @@ -366,6 +373,9 @@ public function create(MessageTransmitter $transmitter, InitializeParams $initia ]), new ShutdownMiddleware($eventDispatcher), new ResponseHandlingMiddleware($responseWatcher), + // Anchor FQN resolution to each request's textDocument so duplicate + // FQNs resolve to the declaration nearest the file being worked on. + new \XPHP\Lsp\Dispatcher\OriginTrackingMiddleware($fqnIndex), new CancellationMiddleware($runner), new HandlerMiddleware($runner), ); diff --git a/src/PositionMap.php b/src/PositionMap.php index 8e138e3..1be36a2 100644 --- a/src/PositionMap.php +++ b/src/PositionMap.php @@ -169,6 +169,18 @@ public function fullLineRangeFromNikic(int $nikicLine): array return [$line, 0, $line, self::toLspCharacter($lineText)]; } + /** + * Length of a byte string in UTF-16 code units -- the unit LSP uses for + * both column offsets AND semantic-token lengths. Exposes the same + * conversion {@see offsetToPosition} uses internally so callers that hold a + * byte span (e.g. the semantic-token emitter) can report a spec-correct + * length instead of a byte count. + */ + public static function lengthInUtf16(string $text): int + { + return self::toLspCharacter($text); + } + /** * UTF-16 code-unit count -- the LSP wire encoding for column offsets. * BMP characters (UTF-8 1/2/3-byte) contribute 1 unit each; diff --git a/src/Reflection/FqnIndex.php b/src/Reflection/FqnIndex.php index d8ed1c3..a32f203 100644 --- a/src/Reflection/FqnIndex.php +++ b/src/Reflection/FqnIndex.php @@ -40,7 +40,11 @@ * * Open-doc URIs win over filesystem paths when they collide on the same * FQN: a generic class with unsaved edits in the editor reflects the - * editor state, not the on-disk state. + * editor state, not the on-disk state. Among filesystem declarations of + * the SAME FQN (common in monorepo / fixture trees), resolution picks the + * one nearest the requesting document -- see {@see selectDecl} / + * {@see nearestDecl} -- so duplicate FQNs no longer require excluding whole + * subtrees from the walk (the prior `test/fixture` skip is gone). * * Filesystem walk lifecycle: built lazily on first call, retained for the * LSP session. A file watcher (`workspace/didChangeWatchedFiles`) is the @@ -58,23 +62,21 @@ final class FqnIndex private const SKIP_DIRS = ['.git', 'vendor', 'node_modules', 'var', 'build']; /** - * Path components skipped when their immediate parent matches. Captures - * test-fixture trees that declare classes with the same FQN as real - * source -- a common Composer-style layout where running the LSP over - * the xphp repo itself surfaced wrong-file GTD targets in prod traces - * (Phase 2.2 / 2.3). - * - * Format: `['parentBasename' => ['skippedChild', ...]]` + * @var array|null FQN -> absolute filesystem path; null until first build. */ - private const SKIP_NESTED = [ - 'test' => ['fixture', 'fixtures'], - 'tests' => ['fixture', 'fixtures'], - ]; + private ?array $filesystemMap = null; /** - * @var array|null FQN -> absolute filesystem path; null until first build. + * @var array, bounds: list}>>|null + * FQN -> EVERY filesystem declaration of that FQN, one record per + * declaring file. The authoritative multi-path map that powers + * proximity-aware resolution: when several files declare the same FQN + * (common in monorepo fixture trees), resolution picks the record + * nearest the requesting document. The legacy single-value maps above + * are derived views (global shortest-path winner) for origin-less + * consumers. Null until first build. */ - private ?array $filesystemMap = null; + private ?array $filesystemDecls = null; /** * @var array|null FQN -> "class" or "function" kind for the filesystem entries. @@ -145,6 +147,16 @@ final class FqnIndex */ private ?array $typeParamFqns = null; + /** + * The document a resolution is being performed "on behalf of", used as the + * proximity anchor when several files declare the same FQN. Set once per + * request by {@see \XPHP\Lsp\Dispatcher\OriginTrackingMiddleware} from the + * request's `textDocument.uri`. The deep resolver chains and the + * worse-reflection locator carry no document context of their own, so this + * holder is how proximity reaches them. Null = no anchor (global tiebreak). + */ + private ?string $currentOrigin = null; + public function __construct( private readonly PhpactorWorkspace $workspace, private readonly ParsedDocumentCache $cache, @@ -153,11 +165,25 @@ public function __construct( ) { } + /** + * Set the proximity anchor for subsequent resolutions (the document the + * current request is for), or null to clear it. Cheap; called per request. + */ + public function withOrigin(?string $uri): void + { + $this->currentOrigin = $uri; + } + + public function currentOrigin(): ?string + { + return $this->currentOrigin; + } + /** * Path to the declaration site for `$fqn`, or null if no declaration * is known. Open-doc URIs win over filesystem paths. */ - public function pathFor(string $fqn): ?string + public function pathFor(string $fqn, ?string $origin = null): ?string { $needle = ltrim($fqn, '\\'); if ($needle === '') { @@ -167,8 +193,7 @@ public function pathFor(string $fqn): ?string if ($uri !== null) { return $uri; } - $filesystemMap = $this->filesystemMap(); - return $filesystemMap[$needle] ?? null; + return $this->selectDecl($needle, $origin)['path'] ?? null; } /** @@ -181,7 +206,7 @@ public function pathFor(string $fqn): ?string * function call sites (`identity(...)`). Methods reuse * `classLikeFor()` -> `findMethod` instead. */ - public function functionFor(string $fqn): ?Function_ + public function functionFor(string $fqn, ?string $origin = null): ?Function_ { $needle = ltrim($fqn, '\\'); if ($needle === '') { @@ -191,11 +216,11 @@ public function functionFor(string $fqn): ?Function_ if ($hit !== null) { return $hit; } - $filesystemMap = $this->filesystemMap(); - if (!isset($filesystemMap[$needle])) { + $path = $this->selectDecl($needle, $origin)['path'] ?? null; + if ($path === null) { return null; } - return $this->functionFromFile($filesystemMap[$needle], $needle); + return $this->functionFromFile($path, $needle); } /** @@ -206,7 +231,7 @@ public function functionFor(string $fqn): ?Function_ * (tolerant) so attributes are still attached even on partially * malformed sources. */ - public function classLikeFor(string $fqn): ?ClassLike + public function classLikeFor(string $fqn, ?string $origin = null): ?ClassLike { $needle = ltrim($fqn, '\\'); if ($needle === '') { @@ -219,11 +244,11 @@ public function classLikeFor(string $fqn): ?ClassLike return $hit; } - $filesystemMap = $this->filesystemMap(); - if (!isset($filesystemMap[$needle])) { + $path = $this->selectDecl($needle, $origin)['path'] ?? null; + if ($path === null) { return null; } - return $this->classLikeFromFile($filesystemMap[$needle], $needle); + return $this->classLikeFromFile($path, $needle); } /** @@ -346,6 +371,7 @@ public function iterGenericFunctionsAndMethods(): iterable public function invalidateFilesystem(): void { $this->filesystemMap = null; + $this->filesystemDecls = null; $this->filesystemKinds = null; $this->filesystemGenericParams = null; $this->filesystemFuncMethodGenericParams = null; @@ -496,7 +522,7 @@ private static function namespaceOf(string $fqn): string * * @return list|null */ - public function boundsForGenericClass(string $fqn): ?array + public function boundsForGenericClass(string $fqn, ?string $origin = null): ?array { $needle = ltrim($fqn, '\\'); if ($needle === '') { @@ -513,7 +539,10 @@ public function boundsForGenericClass(string $fqn): ?array } } } - return $this->filesystemGenericBoundsMap()[$needle] ?? null; + $decl = $this->selectDecl($needle, $origin); + // Empty bounds == not a generic class (the per-param bound list always + // has >=1 slot for a real generic); keep the null contract for those. + return ($decl === null || $decl['bounds'] === []) ? null : $decl['bounds']; } /** @@ -598,7 +627,7 @@ public function allDeclarations(): iterable * * @return array{uri: string, line: int, char: int, short: string}|null */ - public function locationForFqn(string $fqn): ?array + public function locationForFqn(string $fqn, ?string $origin = null): ?array { $needle = ltrim($fqn, '\\'); if ($needle === '') { @@ -624,22 +653,98 @@ public function locationForFqn(string $fqn): ?array ]; } } - $symbols = $this->filesystemSymbols(); - if (!isset($symbols[$needle])) { - return null; - } - $path = $this->filesystemMap()[$needle] ?? null; - if ($path === null) { + $decl = $this->selectDecl($needle, $origin); + if ($decl === null) { return null; } return [ - 'uri' => 'file://' . $path, - 'line' => $symbols[$needle]['line'], - 'char' => $symbols[$needle]['char'], + 'uri' => 'file://' . $decl['path'], + 'line' => $decl['line'], + 'char' => $decl['char'], 'short' => self::shortOf($needle), ]; } + /** + * Locate a method declaration's name span by (class FQN, method name). + * Mirrors {@see locationForFqn} but resolves a member: finds the class's + * ClassLike (open doc first, then filesystem), the ClassMethod by name, and + * maps its name-node offset through the document's ByteOffsetMap. This is the + * xphp-native path go-to-definition uses for generic method calls -- the + * receiver class (e.g. `Collection::first`) carries xphp syntax + * (`T[]`, reified `T`) that worse-reflection can't reliably reflect. + * + * Returns null when the class isn't found or declares no such method + * directly (inherited methods are not yet resolved). + * + * @return array{uri: string, line: int, char: int, short: string}|null + */ + public function methodLocation(string $classFqn, string $methodName, ?string $origin = null): ?array + { + $needle = ltrim($classFqn, '\\'); + if ($needle === '' || $methodName === '') { + return null; + } + + // Open-doc first: live view of unsaved edits beats the on-disk copy. + foreach ($this->workspace as $uri => $item) { + $result = $this->cache->getOrParse($uri, $item->version, $item->text); + if ($result->ast === null) { + continue; + } + $classLike = self::findClassLikeInAst($result->ast, $needle); + if ($classLike === null) { + continue; + } + $startByte = self::methodNameStartByte($classLike, $methodName); + if ($startByte === null) { + return null; + } + $origByte = $result->byteOffsetMap->toOriginal($startByte); + [$line, $char] = self::byteToLineChar($item->text, $origByte); + return ['uri' => (string) $uri, 'line' => $line, 'char' => $char, 'short' => $methodName]; + } + + $path = $this->selectDecl($needle, $origin)['path'] ?? null; + if ($path === null) { + return null; + } + $source = @file_get_contents($path); + if ($source === false) { + return null; + } + try { + $parsed = $this->parser->parseTolerantWithMap($source); + } catch (Throwable) { + return null; + } + if ($parsed === null || $parsed->ast === null) { + return null; + } + $classLike = self::findClassLikeInAst($parsed->ast, $needle); + if ($classLike === null) { + return null; + } + $startByte = self::methodNameStartByte($classLike, $methodName); + if ($startByte === null) { + return null; + } + $origByte = $parsed->byteOffsetMap->toOriginal($startByte); + [$line, $char] = self::byteToLineChar($source, $origByte); + return ['uri' => 'file://' . $path, 'line' => $line, 'char' => $char, 'short' => $methodName]; + } + + private static function methodNameStartByte(ClassLike $classLike, string $methodName): ?int + { + foreach ($classLike->getMethods() as $method) { + if (strcasecmp($method->name->toString(), $methodName) === 0) { + $start = $method->name->getStartFilePos(); + return $start >= 0 ? $start : null; + } + } + return null; + } + /** * Locate ANY declaration whose short name matches `$shortName`. Used * by the definition handler's Path 2 (type-arg identifier inside a @@ -694,46 +799,53 @@ public function fqnsByShortName(string $shortName): array return $sorted; } - public function locationByShortName(string $shortName): ?array + public function locationByShortName(string $shortName, ?string $origin = null): ?array { if ($shortName === '') { return null; } $tailSuffix = '\\' . $shortName; $tailLen = strlen($tailSuffix); - $openDocHit = null; - $fsCandidates = []; + + // Open-doc wins outright. allDeclarations yields open docs first, then + // filesystem ('file://' prefix); stop scanning once we reach the + // filesystem section (open docs are exhausted). foreach ($this->allDeclarations() as $hit) { - $fqn = $hit['fqn']; - $matches = $fqn === $shortName - || (strlen($fqn) > $tailLen && substr($fqn, -$tailLen) === $tailSuffix); - if (!$matches) { - continue; - } if (str_starts_with($hit['uri'], 'file://')) { - $fsCandidates[] = $hit; - } elseif ($openDocHit === null) { - $openDocHit = $hit; + break; + } + $fqn = $hit['fqn']; + if ($fqn === $shortName + || (strlen($fqn) > $tailLen && substr($fqn, -$tailLen) === $tailSuffix) + ) { + return [ + 'uri' => $hit['uri'], + 'line' => $hit['line'], + 'char' => $hit['char'], + 'short' => $shortName, + ]; } } - if ($openDocHit !== null) { - return [ - 'uri' => $openDocHit['uri'], - 'line' => $openDocHit['line'], - 'char' => $openDocHit['char'], - 'short' => $shortName, - ]; + + // Filesystem candidates: EVERY declaring path across matching FQNs, + // so a duplicated short name resolves to the declaration nearest the + // requesting document (or shortest path when origin is null). + $candidates = []; + foreach ($this->filesystemDeclsMap() as $fqn => $records) { + if ($fqn === $shortName + || (strlen($fqn) > $tailLen && substr($fqn, -$tailLen) === $tailSuffix) + ) { + foreach ($records as $record) { + $candidates[] = $record; + } + } } - if ($fsCandidates === []) { + $best = self::nearestDecl($candidates, $origin); + if ($best === null) { return null; } - usort($fsCandidates, static function (array $a, array $b): int { - $byLength = strlen($a['uri']) <=> strlen($b['uri']); - return $byLength !== 0 ? $byLength : strcmp($a['uri'], $b['uri']); - }); - $best = $fsCandidates[0]; return [ - 'uri' => $best['uri'], + 'uri' => 'file://' . $best['path'], 'line' => $best['line'], 'char' => $best['char'], 'short' => $shortName, @@ -941,6 +1053,97 @@ private function filesystemMap(): array return $this->filesystemMap ?? []; } + /** + * @return array, bounds: list}>> + */ + private function filesystemDeclsMap(): array + { + if ($this->filesystemDecls === null) { + $this->buildFilesystemIndex(); + } + return $this->filesystemDecls ?? []; + } + + /** + * Pick the filesystem declaration of `$fqn` nearest the requesting + * document. When `$origin` is null (no requesting context), this reduces + * to the global tiebreak (shortest path, then alphabetical) -- preserving + * the index's pre-proximity behavior for origin-less callers. + * + * @return array{path: string, kind: string, line: int, char: int, genericParams: list, bounds: list}|null + */ + private function selectDecl(string $fqn, ?string $origin): ?array + { + // An explicit origin (passed by a caller that has it) wins; otherwise + // fall back to the per-request anchor set by the middleware. + $effective = $origin ?? $this->currentOrigin; + return self::nearestDecl($this->filesystemDeclsMap()[$fqn] ?? [], $effective); + } + + /** + * Among candidate declaration records, return the one nearest `$origin` + * (the requesting document URI/path). Proximity = number of leading path + * components shared; ties (and a null origin) fall back to shortest path, + * then alphabetical, for deterministic resolution. + * + * @param list, bounds: list}> $records + * @return array{path: string, kind: string, line: int, char: int, genericParams: list, bounds: list}|null + */ + private static function nearestDecl(array $records, ?string $origin): ?array + { + if ($records === []) { + return null; + } + $originPath = $origin === null ? null : self::stripScheme($origin); + $best = null; + $bestShared = -1; + foreach ($records as $record) { + $shared = $originPath === null ? 0 : self::sharedPrefixComponents($originPath, $record['path']); + if ($best === null + || $shared > $bestShared + || ($shared === $bestShared && self::pathTiebreak($record['path'], $best['path']) < 0) + ) { + $best = $record; + $bestShared = $shared; + } + } + return $best; + } + + /** + * Count of leading directory components two absolute paths share. Higher + * means "in the same subtree" -- the proximity signal for resolution. + */ + private static function sharedPrefixComponents(string $a, string $b): int + { + $aParts = explode('/', $a); + $bParts = explode('/', $b); + // Compare directory components only (drop the filename on each side). + array_pop($aParts); + array_pop($bParts); + $n = min(count($aParts), count($bParts)); + $shared = 0; + for ($i = 0; $i < $n; $i++) { + if ($aParts[$i] !== $bParts[$i]) { + break; + } + $shared++; + } + return $shared; + } + + /** Deterministic tiebreak: shorter path first, then alphabetical. */ + private static function pathTiebreak(string $a, string $b): int + { + $byLength = strlen($a) <=> strlen($b); + return $byLength !== 0 ? $byLength : strcmp($a, $b); + } + + private static function stripScheme(string $uri): string + { + return str_starts_with($uri, 'file://') ? substr($uri, strlen('file://')) : $uri; + } + /** * @return array FQN -> "class" or "function" */ @@ -988,6 +1191,7 @@ private function filesystemSymbols(): array private function buildFilesystemIndex(): void { $map = []; + $decls = []; $kinds = []; $genericParams = []; $funcMethodGenericParams = []; @@ -1000,6 +1204,7 @@ private function buildFilesystemIndex(): void $this->rootPath, )); $this->filesystemMap = $map; + $this->filesystemDecls = $decls; $this->filesystemKinds = $kinds; $this->filesystemGenericParams = $genericParams; $this->filesystemFuncMethodGenericParams = $funcMethodGenericParams; @@ -1035,18 +1240,37 @@ private function buildFilesystemIndex(): void $ast = $parsed->ast; $offsets = $parsed->byteOffsetMap; + // Per-file declaration records, joined by FQN across the + // individual collector walks. Seeded from collectDeclarations + // (classes + free functions) and enriched with the finer symbol + // kind/position + generic params/bounds where available. + $fileDecls = []; foreach (self::collectDeclarations($ast) as $fqn => $kind) { $map[$fqn] = $file->getPathname(); $kinds[$fqn] = $kind; + $fileDecls[$fqn] = [ + 'path' => $file->getPathname(), + 'kind' => $kind, + 'line' => 0, + 'char' => 0, + 'genericParams' => [], + 'bounds' => [], + ]; } foreach (self::collectGenericClasses($ast) as $fqn => $paramNames) { $genericParams[$fqn] = $paramNames; + if (isset($fileDecls[$fqn])) { + $fileDecls[$fqn]['genericParams'] = $paramNames; + } } foreach (self::collectGenericFunctionsAndMethods($ast) as $fqn => $paramNames) { $funcMethodGenericParams[$fqn] = $paramNames; } foreach (self::collectGenericClassBounds($ast) as $fqn => $bounds) { $genericBounds[$fqn] = $bounds; + if (isset($fileDecls[$fqn])) { + $fileDecls[$fqn]['bounds'] = $bounds; + } } foreach (self::collectSymbolHits($ast) as $hit) { $origByte = $offsets->toOriginal($hit['startByte']); @@ -1056,6 +1280,14 @@ private function buildFilesystemIndex(): void 'line' => $line, 'char' => $char, ]; + if (isset($fileDecls[$hit['fqn']])) { + $fileDecls[$hit['fqn']]['kind'] = $hit['kind']; + $fileDecls[$hit['fqn']]['line'] = $line; + $fileDecls[$hit['fqn']]['char'] = $char; + } + } + foreach ($fileDecls as $fqn => $record) { + $decls[$fqn][] = $record; } } @@ -1068,6 +1300,7 @@ private function buildFilesystemIndex(): void )); $this->filesystemMap = $map; + $this->filesystemDecls = $decls; $this->filesystemKinds = $kinds; $this->filesystemGenericParams = $genericParams; $this->filesystemFuncMethodGenericParams = $funcMethodGenericParams; @@ -1110,12 +1343,6 @@ static function (SplFileInfo $file): bool { if (in_array($name, self::SKIP_DIRS, true)) { return false; } - $parent = basename($file->getPath()); - if (isset(self::SKIP_NESTED[$parent]) - && in_array($name, self::SKIP_NESTED[$parent], true) - ) { - return false; - } return true; }, ); diff --git a/src/Resolver/BoundErrorCodeActionProvider.php b/src/Resolver/BoundErrorCodeActionProvider.php new file mode 100644 index 0000000..cf0eabd --- /dev/null +++ b/src/Resolver/BoundErrorCodeActionProvider.php @@ -0,0 +1,161 @@ +" -- one per bound-satisfying + * workspace type, replacing the offending type-argument. Works even when + * the concrete type is a scalar. + * - "Add implements \Bound to " -- a cross-file edit on the + * offending concrete class (only when it's an editable open class). + */ +final class BoundErrorCodeActionProvider +{ + /** + * @param list $diagnostics + * @return list + */ + public function actionsFor(string $uri, int $version, string $source, array $diagnostics): array + { + $actions = []; + foreach ($diagnostics as $diagnostic) { + if (self::diagnosticCode($diagnostic) !== DiagnosticCode::BoundViolation->value) { + continue; + } + $data = $diagnostic->data ?? null; + if (!is_array($data) || ($data['kind'] ?? null) !== 'bound') { + continue; + } + $actions = array_merge( + $actions, + $this->swapActions($uri, $version, $diagnostic, $data), + $this->implementActions($diagnostic, $data), + ); + } + return $actions; + } + + /** + * @param array $data + * @return list + */ + private function swapActions(string $uri, int $version, Diagnostic $diagnostic, array $data): array + { + $range = self::rangeFrom($data['typeArgRange'] ?? null); + $candidates = $data['candidates'] ?? []; + if ($range === null || !is_array($candidates)) { + return []; + } + $actions = []; + foreach ($candidates as $candidate) { + if (!is_string($candidate) || $candidate === '') { + continue; + } + $actions[] = new CodeAction( + title: sprintf('Change type argument to %s', $candidate), + kind: CodeActionKind::QUICK_FIX, + diagnostics: [$diagnostic], + edit: new WorkspaceEdit(null, [ + new TextDocumentEdit( + new OptionalVersionedTextDocumentIdentifier($uri, $version), + [new TextEdit($range, $candidate)], + ), + ]), + ); + } + return $actions; + } + + /** + * @param array $data + * @return list + */ + private function implementActions(Diagnostic $diagnostic, array $data): array + { + $insert = $data['implementsInsert'] ?? null; + $bound = $data['bound'] ?? null; + $concrete = $data['concrete'] ?? null; + if (!is_array($insert) || !is_string($bound) || !is_string($concrete)) { + return []; + } + $uri = $insert['uri'] ?? null; + $line = $insert['line'] ?? null; + $character = $insert['character'] ?? null; + $newText = $insert['newText'] ?? null; + if (!is_string($uri) || !is_int($line) || !is_int($character) || !is_string($newText)) { + return []; + } + $point = new Position($line, $character); + $concreteShort = strrpos($concrete, '\\') !== false + ? substr($concrete, strrpos($concrete, '\\') + 1) + : $concrete; + return [ + new CodeAction( + title: sprintf('Add implements \\%s to %s', $bound, $concreteShort), + kind: CodeActionKind::QUICK_FIX, + diagnostics: [$diagnostic], + edit: new WorkspaceEdit(null, [ + new TextDocumentEdit( + new OptionalVersionedTextDocumentIdentifier($uri, null), + [new TextEdit(new Range($point, $point), $newText)], + ), + ]), + ), + ]; + } + + /** + * @param mixed $range + */ + private static function rangeFrom($range): ?Range + { + if (!is_array($range)) { + return null; + } + foreach (['startLine', 'startCharacter', 'endLine', 'endCharacter'] as $key) { + if (!is_int($range[$key] ?? null)) { + return null; + } + } + return new Range( + new Position($range['startLine'], $range['startCharacter']), + new Position($range['endLine'], $range['endCharacter']), + ); + } + + /** + * LSP carries diagnostic codes as either string OR int; our analyzer emits + * the string form. + */ + private static function diagnosticCode(Diagnostic $diagnostic): string + { + return is_string($diagnostic->code) || is_int($diagnostic->code) + ? (string) $diagnostic->code + : ''; + } +} diff --git a/src/Resolver/DocumentHighlightKindResolver.php b/src/Resolver/DocumentHighlightKindResolver.php new file mode 100644 index 0000000..114a40a --- /dev/null +++ b/src/Resolver/DocumentHighlightKindResolver.php @@ -0,0 +1,191 @@ + $ast + * @param list $targetOriginalOffsets start byte offset (original source) of each occurrence + * @return array original-offset -> DocumentHighlightKind + */ + public function resolve(array $ast, ByteOffsetMap $map, array $targetOriginalOffsets): array + { + if ($ast === [] || $targetOriginalOffsets === []) { + return []; + } + $result = []; + $visitor = new class($map, array_fill_keys($targetOriginalOffsets, true), $result) extends NodeVisitorAbstract { + /** @var list */ + private array $stack = []; + + /** + * @param array $wanted + * @param array $result + */ + public function __construct( + private readonly ByteOffsetMap $map, + private readonly array $wanted, + public array &$result, + ) { + } + + public function enterNode(Node $node): null + { + $parent = $this->stack === [] ? null : $this->stack[count($this->stack) - 1]; + $this->classify($node, $parent); + $this->stack[] = $node; + return null; + } + + public function leaveNode(Node $node): null + { + array_pop($this->stack); + return null; + } + + private function mark(int $strippedStart, int $kind): void + { + if ($strippedStart < 0) { + return; + } + $original = $this->map->toOriginal($strippedStart); + if (isset($this->wanted[$original])) { + $this->result[$original] = $kind; + } + } + + private function classify(Node $node, ?Node $parent): void + { + // Declaration names -- the symbol is defined ("written") here. + if ($node instanceof ClassLike && $node->name !== null) { + $this->mark($node->name->getStartFilePos(), DocumentHighlightKind::WRITE); + return; + } + if ($node instanceof ClassMethod || $node instanceof Function_) { + $this->mark($node->name->getStartFilePos(), DocumentHighlightKind::WRITE); + return; + } + if ($node instanceof PropertyItem) { + $this->mark($node->name->getStartFilePos(), DocumentHighlightKind::WRITE); + return; + } + + // Variables: lvalue -> write, otherwise read. + if ($node instanceof Variable) { + $this->mark( + $node->getStartFilePos(), + self::isVariableWrite($node, $parent) ? DocumentHighlightKind::WRITE : DocumentHighlightKind::READ, + ); + return; + } + + // Property / static-property fetches: write when on an + // assignment's left-hand side. + if ($node instanceof PropertyFetch && $node->name instanceof Identifier) { + $this->mark( + $node->name->getStartFilePos(), + self::isAssignTarget($node, $parent) ? DocumentHighlightKind::WRITE : DocumentHighlightKind::READ, + ); + return; + } + if ($node instanceof StaticPropertyFetch && $node->name instanceof VarLikeIdentifier) { + $this->mark( + $node->name->getStartFilePos(), + self::isAssignTarget($node, $parent) ? DocumentHighlightKind::WRITE : DocumentHighlightKind::READ, + ); + return; + } + + // Bare class references (`new User`, type names, `use App\User`). + if ($node instanceof Name) { + $this->mark($node->getStartFilePos(), DocumentHighlightKind::READ); + return; + } + + // Method-call names (`$x->foo()`, `Foo::bar()`). + if ($node instanceof Identifier + && ($parent instanceof MethodCall || $parent instanceof NullsafeMethodCall || $parent instanceof StaticCall) + && $parent->name === $node + ) { + $this->mark($node->getStartFilePos(), DocumentHighlightKind::READ); + } + } + + private static function isVariableWrite(Variable $node, ?Node $parent): bool + { + if ($parent instanceof Assign || $parent instanceof AssignRef || $parent instanceof AssignOp) { + return $parent->var === $node; + } + if ($parent instanceof Foreach_) { + return $parent->valueVar === $node || $parent->keyVar === $node; + } + if ($parent instanceof PreInc || $parent instanceof PostInc + || $parent instanceof PreDec || $parent instanceof PostDec + ) { + return $parent->var === $node; + } + return false; + } + + private static function isAssignTarget(Node $node, ?Node $parent): bool + { + return ($parent instanceof Assign || $parent instanceof AssignRef || $parent instanceof AssignOp) + && $parent->var === $node; + } + }; + + $traverser = new NodeTraverser(); + $traverser->addVisitor($visitor); + $traverser->traverse($ast); + + return $visitor->result; + } +} diff --git a/src/Resolver/GenericResolver.php b/src/Resolver/GenericResolver.php index 7e24847..4f66200 100644 --- a/src/Resolver/GenericResolver.php +++ b/src/Resolver/GenericResolver.php @@ -344,6 +344,48 @@ public function resolveMethodCallSubstitutionAt(string $uri, int $byteOffset): ? return new MethodCallSubstitution($returnTypeRendered, $paramTypes); } + /** + * Locate the declaration of the method called at `$byteOffset`: infer the + * receiver's class (reusing {@see resolveMethodCallSubstitutionAt()}'s + * inference) and look the method up xphp-natively via + * {@see FqnIndex::methodLocation()}. This is the go-to-definition path for + * generic method calls (`$users->first()` with `$users: Collection`), + * where the receiver class carries generic syntax (`T[]`, reified `T`) that + * worse-reflection can't reliably reflect. + * + * Returns the location array ({uri, line, char, short}) or null when the + * cursor isn't on a resolvable method call. + * + * @return array{uri: string, line: int, char: int, short: string}|null + */ + public function resolveMethodDeclarationAt(string $uri, int $byteOffset): ?array + { + if (!$this->workspace->has($uri)) { + return null; + } + $item = $this->workspace->get($uri); + $scopes = $this->scopesFor($uri, $item->version, $item->text); + $bindings = self::bindingsAt($scopes, $byteOffset); + + $result = $this->documents->getOrParse($uri, $item->version, $item->text); + if ($result->ast === null) { + return null; + } + $call = self::findEnclosingMethodCallNameAt($result->ast, $byteOffset); + if ($call === null || !$call->name instanceof Identifier) { + return null; + } + $receiverType = self::inferType($call->var, $bindings, $this->classes, $this->fqnIndex, [], ''); + if ($receiverType === null || $receiverType->ref->isScalar || $receiverType->ref->isTypeParam) { + return null; + } + $fqn = $receiverType->ref->name; + if ($fqn === '') { + return null; + } + return $this->fqnIndex->methodLocation($fqn, $call->name->toString()); + } + /** * Resolve the FQN of the class that should host a member-access * completion at `$byteOffset`. Generalises the existing diff --git a/src/Resolver/ReferenceFinder.php b/src/Resolver/ReferenceFinder.php index 1f0cad5..410f59d 100644 --- a/src/Resolver/ReferenceFinder.php +++ b/src/Resolver/ReferenceFinder.php @@ -570,7 +570,13 @@ private static function shortNameNeedles(array $target): array return match ($target['kind']) { 'alias' => [$target['aliasName']], 'class', 'function' => [self::shortSegment($target['fqn'])], - 'method', 'property' => [$target['memberName']], + // A `__construct` target is reached through `new ClassName(...)` + // sites, whose text is the class name, not `__construct` -- so the + // class short name must also count as a textual hit or the + // filesystem pre-filter would skip those files. + 'method', 'property' => ($target['memberName'] ?? null) === '__construct' && isset($target['className']) + ? [$target['memberName'], self::shortSegment((string) $target['className'])] + : [$target['memberName']], default => [], }; } @@ -771,6 +777,31 @@ private function collectReferences( return; } if ($target['kind'] === 'method') { + // `new X(...)` is a call to `X::__construct`. Count every + // instantiation as a reference to the constructor so "find + // usages", the code-lens count, and document-highlight see + // them -- the source text says `new X`, never `__construct`, + // so the plain member-name match below would miss it. + // Rename is unaffected: RenameProvider skips any reference + // whose covered text (here the class name) != the member name. + if ($targetName === '__construct' + && $node instanceof New_ + && $node->class instanceof Name + ) { + $resolved = $node->class->getAttribute('resolvedName'); + $instantiated = $resolved instanceof Name + ? $resolved->toString() + : ltrim($node->class->toString(), '\\'); + foreach ($targetClasses as $candidate) { + if ($instantiated === $candidate + || $this->inheritsMemberFromTarget($instantiated, $targetName, $candidate, true) + ) { + yield ['node' => $node->class, 'kind' => 'method']; + break; + } + } + continue; + } if (($node instanceof MethodCall || $node instanceof NullsafeMethodCall) && $node->name instanceof Identifier && $node->name->toString() === $targetName diff --git a/test/Analyzer/CallArgumentCheckerTest.php b/test/Analyzer/CallArgumentCheckerTest.php new file mode 100644 index 0000000..a12a993 --- /dev/null +++ b/test/Analyzer/CallArgumentCheckerTest.php @@ -0,0 +1,333 @@ +checkWorkspace([ + '/User.xphp' => <<<'PHP' + <<<'PHP' + rename(42); + PHP, + ]); + + $diags = self::filterByCode($diagnostics['/Use.xphp'], DiagnosticCode::ArgumentMismatch); + self::assertCount(1, $diags); + self::assertStringContainsString('App\\User::rename()', $diags[0]->message); + self::assertStringContainsString('expects string, got int', $diags[0]->message); + } + + public function testAcceptsCorrectInstanceMethodArgument(): void + { + $diagnostics = $this->checkWorkspace([ + '/User.xphp' => <<<'PHP' + <<<'PHP' + rename('alice'); + PHP, + ]); + + self::assertSame([], self::filterByCode($diagnostics['/Use.xphp'], DiagnosticCode::ArgumentMismatch)); + } + + public function testFlagsMismatchOnGenericInstanceMethod(): void + { + // Collection::add(T $item) -> with T=User, passing a Tag + // (an unrelated class) is a TypeError. + $diagnostics = $this->checkWorkspace([ + '/Collection.xphp' => <<<'PHP' + { + public function add(T $item): void {} + } + PHP, + '/User.xphp' => <<<'PHP' + <<<'PHP' + <<<'PHP' + (); + $c->add(new Tag()); + PHP, + ]); + + $diags = self::filterByCode($diagnostics['/Use.xphp'], DiagnosticCode::ArgumentMismatch); + self::assertCount(1, $diags); + self::assertStringContainsString('App\\User', $diags[0]->message); + self::assertStringContainsString('App\\Tag', $diags[0]->message); + } + + public function testAcceptsCorrectGenericInstanceMethodArgument(): void + { + $diagnostics = $this->checkWorkspace([ + '/Collection.xphp' => <<<'PHP' + { + public function add(T $item): void {} + } + PHP, + '/User.xphp' => <<<'PHP' + <<<'PHP' + (); + $c->add(new User()); + PHP, + ]); + + self::assertSame([], self::filterByCode($diagnostics['/Use.xphp'], DiagnosticCode::ArgumentMismatch)); + } + + public function testFlagsScalarLiteralPassedToStaticMethod(): void + { + $diagnostics = $this->checkWorkspace([ + '/Factory.xphp' => <<<'PHP' + <<<'PHP' + message); + self::assertStringContainsString('expects string, got int', $diags[0]->message); + } + + public function testFlagsScalarLiteralPassedToFreeFunction(): void + { + $diagnostics = $this->checkWorkspace([ + '/greet.xphp' => <<<'PHP' + <<<'PHP' + message); + self::assertStringContainsString('expects string, got int', $diags[0]->message); + } + + public function testFlagsLocallyAssignedScalarVariable(): void + { + // Simple-locals: `$n = 42;` makes `$n` an int; passing it to a + // `string` param is a mismatch. + $diagnostics = $this->checkWorkspace([ + '/User.xphp' => <<<'PHP' + <<<'PHP' + rename($n); + PHP, + ]); + + $diags = self::filterByCode($diagnostics['/Use.xphp'], DiagnosticCode::ArgumentMismatch); + self::assertCount(1, $diags); + self::assertStringContainsString('expects string, got int', $diags[0]->message); + } + + public function testSkipsVariableAssignedFromNonInferableExpression(): void + { + // `$n = compute();` -> not inferable -> the arg is skipped. + $diagnostics = $this->checkWorkspace([ + '/User.xphp' => <<<'PHP' + <<<'PHP' + rename($n); + PHP, + ]); + + self::assertSame([], self::filterByCode($diagnostics['/Use.xphp'], DiagnosticCode::ArgumentMismatch)); + } + + public function testSkipsReassignedVariable(): void + { + // `$n` assigned twice -> ambiguous -> dropped from the binding + // map -> the arg is skipped (no false positive). + $diagnostics = $this->checkWorkspace([ + '/User.xphp' => <<<'PHP' + <<<'PHP' + rename($n); + PHP, + ]); + + self::assertSame([], self::filterByCode($diagnostics['/Use.xphp'], DiagnosticCode::ArgumentMismatch)); + } + + public function testSkipsMethodCallOnUnknownReceiver(): void + { + // Receiver type can't be inferred (param, not a local `new`) -> + // the whole call is skipped. + $diagnostics = $this->checkWorkspace([ + '/User.xphp' => <<<'PHP' + rename(42); + } + } + PHP, + ]); + + self::assertSame([], self::filterByCode($diagnostics['/User.xphp'], DiagnosticCode::ArgumentMismatch)); + } + + public function testPositionsDiagnosticOnTheBadArgument(): void + { + $diagnostics = $this->checkWorkspace([ + '/User.xphp' => <<<'PHP' + <<<'PHP' + rename(42); + PHP, + ]); + + $diag = self::filterByCode($diagnostics['/Use.xphp'], DiagnosticCode::ArgumentMismatch)[0]; + // `$u->rename(42);` is on line 3 (0-indexed): startLine); + // `42` is 2 chars wide. + self::assertSame(2, $diag->endCharacter - $diag->startCharacter); + } + + /** + * @return list<\XPHP\Lsp\Analyzer\Diagnostic> + */ + private static function filterByCode(array $diagnostics, DiagnosticCode $code): array + { + return array_values(array_filter($diagnostics, static fn ($d): bool => $d->code === $code)); + } + + /** + * @param array $sources + * @return array> + */ + private function checkWorkspace(array $sources): array + { + $files = $this->parseFiles($sources); + return (new WorkspaceAnalyzer())->analyze($files); + } + + /** + * Mirror the prod path: the per-file Analyzer does NOT run nikic's + * NameResolver before handing ASTs to the WorkspaceAnalyzer. + * + * @param array $sources + * @return array, source: string}> + */ + private function parseFiles(array $sources): array + { + $parser = new XphpSourceParser((new ParserFactory())->createForHostVersion()); + $analyzer = new Analyzer($parser); + $out = []; + foreach ($sources as $path => $source) { + $result = $analyzer->analyzeFile($source); + self::assertNotNull($result->ast, "fixture {$path} should parse"); + $out[$path] = ['ast' => $result->ast, 'source' => $source]; + } + return $out; + } +} diff --git a/test/Analyzer/WorkspaceAnalyzerTest.php b/test/Analyzer/WorkspaceAnalyzerTest.php index db6f9f0..87cb3c2 100644 --- a/test/Analyzer/WorkspaceAnalyzerTest.php +++ b/test/Analyzer/WorkspaceAnalyzerTest.php @@ -191,17 +191,25 @@ class Box { public T $item; } $diagnostics = (new WorkspaceAnalyzer())->analyze($files); - // The second declaration encountered carries the duplicate-declaration diagnostic. - // Iteration order over $files is insertion order, so BoxOne wins, BoxTwo gets the error. - self::assertSame([], $diagnostics['/BoxOne.xphp']); + // A duplicate is a property of ALL colliding declarations, not of + // iteration order: BOTH files carry the diagnostic (each naming the + // other). This is what lets the pull provider surface the duplicate on + // whichever file the editor is looking at. + self::assertCount(1, $diagnostics['/BoxOne.xphp']); self::assertCount(1, $diagnostics['/BoxTwo.xphp']); - $d = $diagnostics['/BoxTwo.xphp'][0]; - self::assertStringContainsString('already declared', $d->message); - self::assertSame(DiagnosticCode::Definition, $d->code); + $one = $diagnostics['/BoxOne.xphp'][0]; + $two = $diagnostics['/BoxTwo.xphp'][0]; + self::assertStringContainsString('already declared', $one->message); + self::assertStringContainsString('already declared', $two->message); + // Each diagnostic names the OTHER file as the collision site. + self::assertStringContainsString('/BoxTwo.xphp', $one->message); + self::assertStringContainsString('/BoxOne.xphp', $two->message); + self::assertSame(DiagnosticCode::Definition, $one->code); + self::assertSame(DiagnosticCode::Definition, $two->code); // Column accuracy: the diagnostic must point at the duplicate class // identifier (`Box`, 3 chars), not span the whole line. Locks the // `getEndFilePos() + 1` arithmetic against off-by-one regressions. - self::assertSame(3, $d->endCharacter - $d->startCharacter, 'range must span just the `Box` identifier'); + self::assertSame(3, $two->endCharacter - $two->startCharacter, 'range must span just the `Box` identifier'); } public function testHierarchyAstsEnrichBoundCheckWithoutBeingWalked(): void diff --git a/test/Behat/EditContext.php b/test/Behat/EditContext.php index bba9226..724c34d 100644 --- a/test/Behat/EditContext.php +++ b/test/Behat/EditContext.php @@ -169,6 +169,36 @@ public function iRequestCodeActionsForADiagnosticOnAtLineOf(string $needle, int $this->world->request('textDocument/codeAction', $params); } + /** + * Pull the real diagnostics for a file, then request code actions with the + * matching diagnostic in context -- so its structured `data` payload flows + * through to the providers exactly as a client would round-trip it. + * + * @When I request code actions for the :code diagnostic in :path + */ + public function iRequestCodeActionsForTheDiagnosticIn(string $code, string $path): void + { + $this->world->request('textDocument/diagnostic', ['textDocument' => ['uri' => $path]]); + $report = $this->world->last(); + $items = is_array($report) ? ($report['items'] ?? $report) : []; + + $match = null; + foreach ((array) $items as $diagnostic) { + if (is_object($diagnostic) && ($diagnostic->code ?? null) === $code) { + $match = $diagnostic; + break; + } + } + $this->world->assert($match !== null, sprintf('no "%s" diagnostic found in %s', $code, $path)); + + $params = new CodeActionParams( + new TextDocumentIdentifier($path), + $match->range, + new CodeActionContext([$match]), + ); + $this->world->request('textDocument/codeAction', $params); + } + /** * @Then a code action titled :title is offered */ @@ -295,6 +325,20 @@ public function iResolveTheFirstCodeLens(): void $this->world->request('codeLens/resolve', $lenses[0]); } + /** + * @When I resolve the code lens on line :line + */ + public function iResolveTheCodeLensOnLine(int $line): void + { + foreach ((array) $this->world->last() as $lens) { + if ($lens instanceof CodeLens && $lens->range->start->line === $line) { + $this->world->request('codeLens/resolve', $lens); + return; + } + } + $this->world->fail(sprintf('no code lens on line %d', $line)); + } + /** * @Then a code lens titled :title is offered */ diff --git a/test/Behat/NavigateContext.php b/test/Behat/NavigateContext.php index ec73d95..a813ff9 100644 --- a/test/Behat/NavigateContext.php +++ b/test/Behat/NavigateContext.php @@ -247,6 +247,37 @@ public function eachHighlightCoversIn(string $text, string $path): void } } + /** + * @Then a :kind highlight covers :text in :path + */ + public function aKindHighlightCoversIn(string $kind, string $text, string $path): void + { + $kinds = ['text' => 1, 'read' => 2, 'write' => 3]; + $this->world->assert(isset($kinds[$kind]), sprintf('unknown highlight kind: %s', $kind)); + $wantKind = $kinds[$kind]; + + $highlights = $this->world->last(); + $this->world->assert(is_array($highlights) && $highlights !== [], 'expected a non-empty highlight list'); + $seen = []; + foreach ($highlights as $highlight) { + $covered = $this->world->textForRange($path, $highlight->range); + if ($covered !== $text) { + continue; + } + $seen[] = (int) $highlight->kind; + if ((int) $highlight->kind === $wantKind) { + return; + } + } + $this->world->fail(sprintf( + 'expected a "%s" (%d) highlight covering "%s"; kinds seen for that text: [%s]', + $kind, + $wantKind, + $text, + implode(', ', $seen) ?: '', + )); + } + private function matchesPath(string $uri, string $path): bool { return $uri === $path diff --git a/test/Behat/ValidateContext.php b/test/Behat/ValidateContext.php index d355234..a4f8435 100644 --- a/test/Behat/ValidateContext.php +++ b/test/Behat/ValidateContext.php @@ -5,12 +5,17 @@ namespace XPHP\Lsp\Test\Behat; use Behat\Behat\Context\Context; +use Behat\Gherkin\Node\PyStringNode; /** * Steps for the Validate theme: diagnostics (parse errors, generic bound - * violations, duplicate templates, undefined barewords, constructor-arg + * violations, duplicate templates, undefined barewords, argument-type * mismatches). Diagnostics are pulled through the real XphpPullDiagnosticsHandler * over the open workspace -- cross-file checks see every open document. + * + * The cross-file broadcast steps additionally drive the PUSH path: the real + * diagnostics engine re-publishes a dependent's diagnostics when an unrelated + * file it depends on is edited. */ final class ValidateContext implements Context { @@ -20,6 +25,46 @@ public function __construct(private readonly World $world) { } + /** + * @Given the diagnostics service is running + */ + public function theDiagnosticsServiceIsRunning(): void + { + $this->world->startDiagnosticsService(); + } + + /** + * @When I change the file at :path to contain the following lines: + */ + public function iChangeTheFileToContain(string $path, PyStringNode $lines): void + { + $this->world->changeFile($path, $lines->getRaw()); + } + + /** + * @Then a :code diagnostic is published for :path without re-requesting it + */ + public function aDiagnosticIsPublishedFor(string $code, string $path): void + { + // Pump the cooperative loop until the broadcast publishes a diagnostic + // with this code for the (untouched) dependent, or give up. + for ($try = 0; $try < 100; $try++) { + foreach ($this->world->publishedDiagnostics($path) as $params) { + foreach ($params['diagnostics'] as $diagnostic) { + if ((string) ($diagnostic->code ?? '') === $code) { + return; + } + } + } + $this->world->pumpLoop(); + } + $this->world->fail(sprintf( + 'expected a "%s" diagnostic to be broadcast for "%s"; none was published', + $code, + $path, + )); + } + /** * @When I analyze :path for diagnostics */ diff --git a/test/Behat/World.php b/test/Behat/World.php index 8d33f61..72d173b 100644 --- a/test/Behat/World.php +++ b/test/Behat/World.php @@ -79,7 +79,11 @@ private function server(): LanguageServerTester { if ($this->tester === null) { $this->tester = new LanguageServerTester( - new LspDispatcherFactory(), + // debounce 0: the push-mode diagnostics engine publishes as soon + // as the cooperative loop is pumped (see pumpLoop / published + // diagnostics), so push scenarios stay deterministic. Pull-mode + // scenarios don't touch the engine, so this is a no-op for them. + new LspDispatcherFactory(diagnosticsDebounceMs: 0), new InitializeParams(new ClientCapabilities()), ); $this->tester->initialize(); @@ -87,6 +91,55 @@ private function server(): LanguageServerTester return $this->tester; } + // ---- push-mode diagnostics (cross-file broadcast) ---------------------- + + /** Start the push-mode diagnostics engine service if it isn't already running. */ + public function startDiagnosticsService(): void + { + $services = $this->server()->services(); + if (!in_array('diagnostics', $services->listRunning(), true)) { + $services->start('diagnostics'); + } + } + + /** Send a real textDocument/didChange for an already-open fixture. */ + public function changeFile(string $uri, string $source): void + { + $this->sources[$uri] = $source; + $this->server()->textDocument()->update($uri, $source); + } + + /** + * Advance the Amp event loop a bounded number of ticks so queued engine + * work (debounced lint + publish + broadcast) runs to completion. + */ + public function pumpLoop(int $ticks = 1): void + { + for ($i = 0; $i < $ticks; $i++) { + \Amp\Promise\wait(\Amp\delay(1)); + } + } + + /** + * Non-destructive snapshot of the `textDocument/publishDiagnostics` + * notifications transmitted for a URI, newest last. Each entry is the + * notification's params (`{uri, version, diagnostics}`). + * + * @return list + */ + public function publishedDiagnostics(string $uri): array + { + $out = []; + $filtered = $this->server()->transmitter()->filterByMethod('textDocument/publishDiagnostics'); + while (($message = $filtered->shift()) !== null) { + $params = $message->params ?? null; + if (is_array($params) && ($params['uri'] ?? null) === $uri) { + $out[] = $params; + } + } + return $out; + } + // ---- position / fixture helpers --------------------------------------- /** diff --git a/test/Diagnostics/XphpDiagnosticsProviderTest.php b/test/Diagnostics/XphpDiagnosticsProviderTest.php index d4a37a4..6880b30 100644 --- a/test/Diagnostics/XphpDiagnosticsProviderTest.php +++ b/test/Diagnostics/XphpDiagnosticsProviderTest.php @@ -6,6 +6,11 @@ use Amp\CancellationTokenSource; use PhpParser\ParserFactory; +use Phpactor\LanguageServer\Core\Rpc\NotificationMessage; +use Phpactor\LanguageServer\Core\Server\ClientApi; +use Phpactor\LanguageServer\Core\Server\ResponseWatcher\DeferredResponseWatcher; +use Phpactor\LanguageServer\Core\Server\RpcClient\JsonRpcClient; +use Phpactor\LanguageServer\Core\Server\Transmitter\TestMessageTransmitter; use Phpactor\LanguageServer\Core\Workspace\Workspace as PhpactorWorkspace; use Phpactor\LanguageServerProtocol\Diagnostic as LspDiagnostic; use Phpactor\LanguageServerProtocol\TextDocumentItem; @@ -106,6 +111,34 @@ class Box { public T $item; } self::assertSame([], $diagnostics, 'no duplicate-declaration must surface when the only file holding Box is the one being linted'); } + public function testDuplicateTemplateSurfacesOnWhicheverFileIsPulled(): void + { + // Two open files both declare `App\Box`. The pull provider forces the + // current file first in the workspace pass, which used to make it the + // canonical (clean) one -- so the duplicate only ever landed on the OTHER + // file and was never returned for the file the editor was looking at. + // Now a duplicate is flagged on ALL colliding declarations, so pulling + // diagnostics for EITHER file returns it. + $workspace = new PhpactorWorkspace(); + $one = $this->openDoc($workspace, '/BoxOne.xphp', <<<'XPHP' + { public T $item; } + XPHP); + $two = $this->openDoc($workspace, '/BoxTwo.xphp', <<<'XPHP' + { public T $item; } + XPHP); + + foreach ([$one, $two] as $doc) { + $diagnostics = $this->lint($workspace, $doc); + self::assertCount(1, $diagnostics, "duplicate must surface when pulling {$doc->uri}"); + self::assertSame('xphp.definition', $diagnostics[0]->code); + self::assertStringContainsString('already declared', $diagnostics[0]->message); + } + } + public function testWorkspaceDiagnosticsTranslateToLspWireFormatRanges(): void { // Locks the array_map translation on line 96. Without it, the @@ -443,6 +476,165 @@ class Box { public T $item; } self::assertSame('xphp.bound', $diagnostics[0]->code); } + public function testEditingADependencyBroadcastsDiagnosticsForOpenDependents(): void + { + // Box.xphp declares a bounded template; Use.xphp instantiates it with a + // type that violates the bound. Linting Box.xphp (e.g. the user is + // editing it) must re-publish Use.xphp's diagnostics WITHOUT the user + // touching Use.xphp -- that's the cross-file broadcast. + $workspace = new PhpactorWorkspace(); + $transmitter = new TestMessageTransmitter(); + $provider = $this->newBroadcastProvider($workspace, $transmitter); + + $boxDoc = $this->openDoc($workspace, '/Box.xphp', <<<'XPHP' + { public T $item; } + XPHP); + $this->openDoc($workspace, '/Use.xphp', <<<'XPHP' + (); + XPHP); + + wait($provider->provideDiagnostics($boxDoc, (new CancellationTokenSource())->getToken())); + + $published = self::publishedDiagnosticsFor($transmitter, '/Use.xphp'); + self::assertCount(1, $published, 'expected exactly one publish for the dependent'); + $diagnostics = array_values($published[0]['diagnostics']); + self::assertCount(1, $diagnostics); + self::assertSame('xphp.bound', $diagnostics[0]->code); + } + + public function testUnchangedDependentIsNotRebroadcast(): void + { + // Linting the dependency twice with no change must publish the + // dependent's diagnostics only once (the signature guard). + $workspace = new PhpactorWorkspace(); + $transmitter = new TestMessageTransmitter(); + $provider = $this->newBroadcastProvider($workspace, $transmitter); + + $boxDoc = $this->openDoc($workspace, '/Box.xphp', <<<'XPHP' + { public T $item; } + XPHP); + $this->openDoc($workspace, '/Use.xphp', <<<'XPHP' + (); + XPHP); + + $token = (new CancellationTokenSource())->getToken(); + wait($provider->provideDiagnostics($boxDoc, $token)); + wait($provider->provideDiagnostics($boxDoc, $token)); + + self::assertCount( + 1, + self::publishedDiagnosticsFor($transmitter, '/Use.xphp'), + 'an unchanged dependent must not be re-published', + ); + } + + public function testTheLintedDocumentIsNotBroadcastByTheProvider(): void + { + // The engine publishes the document being linted; the broadcast must + // NOT also publish it (that would double-publish the current file). + $workspace = new PhpactorWorkspace(); + $transmitter = new TestMessageTransmitter(); + $provider = $this->newBroadcastProvider($workspace, $transmitter); + + $useDoc = $this->openDoc($workspace, '/Use.xphp', <<<'XPHP' + (); + XPHP); + $this->openDoc($workspace, '/Box.xphp', <<<'XPHP' + { public T $item; } + XPHP); + + wait($provider->provideDiagnostics($useDoc, (new CancellationTokenSource())->getToken())); + + self::assertSame( + [], + self::publishedDiagnosticsFor($transmitter, '/Use.xphp'), + 'the linted document must not be broadcast by the provider', + ); + } + + public function testBoundCheckUsesTheTypeArgDeclarationNearestTheLintedFile(): void + { + // Two packages declare the SAME FQN `App\Shared\Tag`, but only pkgA's + // implements \Stringable. The bound check on a file in pkgA must use + // pkgA's Tag (bound satisfied -> no diagnostic); the same instantiation + // in pkgB must use pkgB's Tag (bound violated -> diagnostic). This + // exercises the proximity-scoped hierarchy: each FQN's ancestry comes + // from its nearest declarer, not whichever copy was walked last. + $root = sys_get_temp_dir() . '/xphp-diag-proximity-' . bin2hex(random_bytes(6)); + mkdir($root . '/pkgA', 0o755, true); + mkdir($root . '/pkgB', 0o755, true); + try { + $box = <<<'PHP' + + { + public function __construct(public T $item) {} + } + PHP; + file_put_contents($root . '/pkgA/Box.xphp', $box); + file_put_contents($root . '/pkgB/Box.xphp', $box); + file_put_contents($root . '/pkgA/Tag.xphp', <<<'PHP' + createForHostVersion()); + $cache = new ParsedDocumentCache(new Analyzer($parser)); + $workspace = new PhpactorWorkspace(); + $fqnIndex = new FqnIndex($workspace, $cache, $parser, $root); + (new \XPHP\Lsp\Analyzer\ParsedDocumentCacheWarmer($fqnIndex, $cache, $workspace))->warmNow(); + + $useSource = "(new Tag());\n"; + $provider = new XphpDiagnosticsProvider($cache, new WorkspaceAnalyzer(), $workspace, $fqnIndex); + + $useA = $this->openDoc($workspace, 'file://' . $root . '/pkgA/Use.xphp', $useSource); + $diagsA = $this->codes(wait($provider->provideDiagnostics($useA, (new CancellationTokenSource())->getToken()))); + self::assertNotContains('xphp.bound', $diagsA, 'pkgA Tag implements \Stringable -> no violation'); + + $useB = $this->openDoc($workspace, 'file://' . $root . '/pkgB/Use.xphp', $useSource); + $diagsB = $this->codes(wait($provider->provideDiagnostics($useB, (new CancellationTokenSource())->getToken()))); + self::assertContains('xphp.bound', $diagsB, 'pkgB Tag does not implement \Stringable -> violation'); + } finally { + foreach (['pkgA', 'pkgB'] as $pkg) { + array_map('unlink', glob($root . '/' . $pkg . '/*') ?: []); + @rmdir($root . '/' . $pkg); + } + @rmdir($root); + } + } + + /** + * @param list $diagnostics + * @return list + */ + private function codes(mixed $diagnostics): array + { + $diagnostics = is_array($diagnostics) ? $diagnostics : []; + return array_values(array_map(static fn ($d): string => (string) $d->code, $diagnostics)); + } + /** * @return list */ @@ -473,4 +665,42 @@ private function openDoc(PhpactorWorkspace $workspace, string $uri, string $text $workspace->open($item); return $item; } + + private function newBroadcastProvider( + PhpactorWorkspace $workspace, + TestMessageTransmitter $transmitter, + ): XphpDiagnosticsProvider { + $parser = new XphpSourceParser((new ParserFactory())->createForHostVersion()); + $cache = new ParsedDocumentCache(new Analyzer($parser)); + $clientApi = new ClientApi(new JsonRpcClient($transmitter, new DeferredResponseWatcher())); + return new XphpDiagnosticsProvider( + $cache, + new WorkspaceAnalyzer(), + $workspace, + new FqnIndex($workspace, $cache, $parser, ''), + $clientApi, + ); + } + + /** + * Collect the `textDocument/publishDiagnostics` notifications transmitted + * for a given URI, decoded into `{uri, version, diagnostics}` arrays. + * + * @return list}> + */ + private static function publishedDiagnosticsFor(TestMessageTransmitter $transmitter, string $uri): array + { + $out = []; + $filtered = $transmitter->filterByMethod('textDocument/publishDiagnostics'); + while (($message = $filtered->shift()) !== null) { + if (!$message instanceof NotificationMessage) { + continue; + } + $params = $message->params; + if (($params['uri'] ?? null) === $uri) { + $out[] = $params; + } + } + return $out; + } } diff --git a/test/Dispatcher/OriginTrackingMiddlewareTest.php b/test/Dispatcher/OriginTrackingMiddlewareTest.php new file mode 100644 index 0000000..bdec2b9 --- /dev/null +++ b/test/Dispatcher/OriginTrackingMiddlewareTest.php @@ -0,0 +1,109 @@ +fqnIndex(); + $middleware = new OriginTrackingMiddleware($index); + + wait($middleware->process( + new RequestMessage('1', 'textDocument/definition', [ + 'textDocument' => ['uri' => 'file:///proj/src/Use.xphp'], + 'position' => ['line' => 1, 'character' => 2], + ]), + $this->terminal(), + )); + + self::assertSame('file:///proj/src/Use.xphp', $index->currentOrigin()); + } + + public function testClearsOriginForRequestsWithoutTextDocument(): void + { + $index = $this->fqnIndex(); + $index->withOrigin('file:///stale.xphp'); + $middleware = new OriginTrackingMiddleware($index); + + wait($middleware->process( + new RequestMessage('2', 'workspace/symbol', ['query' => 'Foo']), + $this->terminal(), + )); + + self::assertNull($index->currentOrigin(), 'workspace-wide requests must not inherit a stale anchor'); + } + + public function testSetsOriginFromNotification(): void + { + $index = $this->fqnIndex(); + $middleware = new OriginTrackingMiddleware($index); + + wait($middleware->process( + new NotificationMessage('textDocument/didOpen', [ + 'textDocument' => ['uri' => 'file:///proj/A.xphp', 'text' => 'terminal(), + )); + + self::assertSame('file:///proj/A.xphp', $index->currentOrigin()); + } + + public function testDelegatesToTheRestOfTheChain(): void + { + $index = $this->fqnIndex(); + $middleware = new OriginTrackingMiddleware($index); + + $result = wait($middleware->process( + new RequestMessage('3', 'textDocument/hover', ['textDocument' => ['uri' => 'file:///x.xphp']]), + $this->terminal('handled'), + )); + + self::assertSame('handled', $result); + } + + private function fqnIndex(): FqnIndex + { + $parser = new XphpSourceParser((new ParserFactory())->createForHostVersion()); + $cache = new ParsedDocumentCache(new Analyzer($parser)); + return new FqnIndex(new PhpactorWorkspace(), $cache, $parser, ''); + } + + /** + * A RequestHandler whose single (terminal) middleware returns $value. + */ + private function terminal(mixed $value = null): RequestHandler + { + $terminal = new class($value) implements Middleware { + public function __construct(private readonly mixed $value) + { + } + + public function process(Message $request, RequestHandler $handler): Promise + { + return new Success($this->value); + } + }; + return new RequestHandler([$terminal]); + } +} diff --git a/test/Handler/SemanticTokens/AstVisitorTest.php b/test/Handler/SemanticTokens/AstVisitorTest.php index 9a35b44..f3a3ea9 100644 --- a/test/Handler/SemanticTokens/AstVisitorTest.php +++ b/test/Handler/SemanticTokens/AstVisitorTest.php @@ -379,8 +379,59 @@ public function go(): void { $x = new Foo(); } self::assertEmpty($fooSpecs, '`Foo` outside a generic body must not be classified as typeParameter'); } + public function testInterpolatedStringDecomposesIntoLiteralSlabsAndVariable(): void + { + $source = <<<'XPHP' + collect($source); + + // The interpolated variable highlights as a variable INSIDE the string. + $this->assertTokenSubstring($specs, $source, '$name', 'variable'); + // The literal slabs on either side stay `string`. + $this->assertTokenSubstring($specs, $source, 'Hello ', 'string'); + $this->assertTokenSubstring($specs, $source, ' world', 'string'); + } + + public function testNonAsciiStringLengthIsUtf16NotBytes(): void + { + // `"café"` is 6 UTF-16 code units (quotes + c, a, f, é) but 7 bytes + // (é is 2 bytes in UTF-8). The emitted length must be the UTF-16 count. + $source = "firstSpecOfType($this->collect($source), 'string'); + + self::assertNotNull($token, 'expected a string token'); + self::assertSame(6, $token->length, 'token length must be UTF-16 code units, not bytes'); + } + + public function testFourByteCharacterCountsAsTwoUtf16Units(): void + { + // A supplementary-plane char (😀, 4 UTF-8 bytes) is a UTF-16 surrogate + // pair (2 units). `"😀"` => quote + 2 + quote = 4 UTF-16 units. + $source = "firstSpecOfType($this->collect($source), 'string'); + + self::assertNotNull($token, 'expected a string token'); + self::assertSame(4, $token->length); + } + // --- helpers ----------------------------------------------------------- + /** + * @param list $specs + */ + private function firstSpecOfType(array $specs, string $type): ?TokenSpec + { + foreach ($specs as $spec) { + if ($spec->type === $type) { + return $spec; + } + } + return null; + } + /** * @return list */ diff --git a/test/Handler/XphpCodeActionHandlerTest.php b/test/Handler/XphpCodeActionHandlerTest.php index 3e3973f..f2d0e4f 100644 --- a/test/Handler/XphpCodeActionHandlerTest.php +++ b/test/Handler/XphpCodeActionHandlerTest.php @@ -169,6 +169,7 @@ private function newHandler(PhpactorWorkspace $workspace): XphpCodeActionHandler new ImportCodeActionProvider($fqnIndex, $cache), new DiagnosticCodeActionProvider(), new OptimizeImportsCodeActionProvider($cache), + new \XPHP\Lsp\Resolver\BoundErrorCodeActionProvider(), ); } diff --git a/test/Handler/XphpDefinitionHandlerTest.php b/test/Handler/XphpDefinitionHandlerTest.php index 4f2af92..4fe6f3c 100644 --- a/test/Handler/XphpDefinitionHandlerTest.php +++ b/test/Handler/XphpDefinitionHandlerTest.php @@ -433,6 +433,33 @@ private function definitionAt( return wait($handler->definition($params)); } + public function testJumpsFromGenericMethodCallToMethodDeclaration(): void + { + // worse-reflection can't reflect `Collection`'s generic body, so this + // resolves xphp-natively: GenericResolver infers the receiver class and + // FqnIndex locates the `first` method. + $workspace = new PhpactorWorkspace(); + $workspace->open(new TextDocumentItem('/Collection.xphp', 'xphp', 1, <<<'XPHP' + + { + public function first(): ?T { return null; } + } + XPHP)); + $useSource = "();\n\$first = \$users->first();\n"; + $workspace->open(new TextDocumentItem('/Use.xphp', 'xphp', 1, $useSource)); + + $byte = strpos($useSource, '->first') + strlen('->'); // cursor on `first` + self::assertNotFalse($byte); + $location = $this->definitionAtOffset($this->newHandler($workspace), '/Use.xphp', $useSource, $byte); + + self::assertNotNull($location); + self::assertSame('/Collection.xphp', $location->uri); + // Range squiggles the `first` method name (5 chars), not the whole line. + self::assertSame(5, $location->range->end->character - $location->range->start->character); + } + private function newHandler(PhpactorWorkspace $workspace, ?string $rootPath = null): XphpDefinitionHandler { $parser = new XphpSourceParser((new ParserFactory())->createForHostVersion()); @@ -459,6 +486,8 @@ private function newHandler(PhpactorWorkspace $workspace, ?string $rootPath = nu new WorkspaceSymbols($workspace, $cache), $fqnIndex, $referenceFinder, + null, + $genericResolver, ); } diff --git a/test/Handler/XphpDocumentHighlightHandlerTest.php b/test/Handler/XphpDocumentHighlightHandlerTest.php index 8062e5b..203bdba 100644 --- a/test/Handler/XphpDocumentHighlightHandlerTest.php +++ b/test/Handler/XphpDocumentHighlightHandlerTest.php @@ -21,6 +21,7 @@ use XPHP\Lsp\Reflection\FqnIndex; use XPHP\Lsp\Reflection\ReflectorFactory; use XPHP\Lsp\Resolver\CompositeClassLikeLookup; +use XPHP\Lsp\Resolver\DocumentHighlightKindResolver; use XPHP\Lsp\Resolver\FilesystemClassLikeLookup; use XPHP\Lsp\Resolver\GenericResolver; use XPHP\Lsp\Resolver\ReferenceFinder; @@ -44,8 +45,11 @@ public function testHighlightsAllReferencesInCurrentFile(): void self::assertCount(3, $highlights); foreach ($highlights as $h) { self::assertInstanceOf(DocumentHighlight::class, $h); - self::assertSame(DocumentHighlightKind::TEXT, $h->kind); } + // The declaration is a WRITE; the two use sites are READs. + $kinds = array_map(static fn (DocumentHighlight $h): int => $h->kind, $highlights); + self::assertCount(1, array_filter($kinds, static fn (int $k): bool => $k === DocumentHighlightKind::WRITE)); + self::assertCount(2, array_filter($kinds, static fn (int $k): bool => $k === DocumentHighlightKind::READ)); } public function testFiltersOutCrossFileMatches(): void @@ -121,7 +125,7 @@ public function testSkipsFilesystemScanForOpenDocOnlyRequest(): void ); $generic = new GenericResolver($workspace, $cache, $classLikeLookup, $parser, $fqnIndex); $finder = new ReferenceFinder($workspace, $cache, $fqnIndex, $parser, $reflector, $generic); - $handler = new XphpDocumentHighlightHandler($workspace, $finder); + $handler = new XphpDocumentHighlightHandler($workspace, $finder, $cache, new DocumentHighlightKindResolver()); // Cursor on `class User` -- two in-file matches (decl + new). $byte = strpos($source, 'class User') + strlen('class '); @@ -264,6 +268,6 @@ private function handler(PhpactorWorkspace $workspace): XphpDocumentHighlightHan ); $generic = new GenericResolver($workspace, $cache, $classLikeLookup, $parser, $fqnIndex); $finder = new ReferenceFinder($workspace, $cache, $fqnIndex, $parser, $reflector, $generic); - return new XphpDocumentHighlightHandler($workspace, $finder); + return new XphpDocumentHighlightHandler($workspace, $finder, $cache, new DocumentHighlightKindResolver()); } } diff --git a/test/Handler/XphpReferencesHandlerTest.php b/test/Handler/XphpReferencesHandlerTest.php index 6151d63..df56388 100644 --- a/test/Handler/XphpReferencesHandlerTest.php +++ b/test/Handler/XphpReferencesHandlerTest.php @@ -1058,6 +1058,50 @@ private function referencesAtPosition( return $result; } + public function testNewIsTrackedAsConstructorUsage(): void + { + // `new User(...)` calls User::__construct. Find Usages (and the + // code-lens count / highlight) on the constructor must include every + // instantiation, even though the source says `new User`, not + // `__construct`. + $workspace = new PhpactorWorkspace(); + $workspace->open(new TextDocumentItem('/User.xphp', 'xphp', 1, <<<'XPHP' + open(new TextDocumentItem('/Use1.xphp', 'xphp', 1, "open(new TextDocumentItem('/Use2.xphp', 'xphp', 1, "references($workspace, '/User.xphp', '__construct', 2); + $uris = array_map(fn (Location $l): string => $l->uri, $locations); + sort($uris); + + // Declaration + both instantiation sites. + self::assertCount(3, $locations); + self::assertSame(['/Use1.xphp', '/Use2.xphp', '/User.xphp'], $uris); + } + + public function testConstructorUsageCoversTheClassNameToken(): void + { + // The instantiation reference points at the class-name token of + // `new User()`, so highlight / usages land on `User`. + $workspace = new PhpactorWorkspace(); + $workspace->open(new TextDocumentItem('/User.xphp', 'xphp', 1, "open(new TextDocumentItem('/Use.xphp', 'xphp', 1, "references($workspace, '/User.xphp', '__construct', 2, includeDeclaration: false); + self::assertCount(1, $locations); + + $useText = $workspace->get('/Use.xphp')->text; + $map = new PositionMap($useText); + $start = $map->positionToOffset($locations[0]->range->start->line, $locations[0]->range->start->character); + $end = $map->positionToOffset($locations[0]->range->end->line, $locations[0]->range->end->character); + self::assertSame('User', substr($useText, $start, $end - $start)); + } + private function references( PhpactorWorkspace $workspace, string $uri, diff --git a/test/Reflection/FqnIndexTest.php b/test/Reflection/FqnIndexTest.php index 120f54e..eeff324 100644 --- a/test/Reflection/FqnIndexTest.php +++ b/test/Reflection/FqnIndexTest.php @@ -256,23 +256,30 @@ public function testLocationForFqnReturnsNullForEmptyOrUnknown(): void self::assertNull($index->locationForFqn('Nope\\Mystery')); } - public function testFilesystemWalkSkipsNestedTestFixturesDir(): void + public function testFilesystemWalkIndexesNestedTestFixturesAndResolvesByProximity(): void { - // Phase 3 polish: `test/fixture/...` declarations were polluting - // workspace symbol search + closed-file GTD in the xphp repo - // itself (confirmed in prod traces 2.2 + 2.3). The walk now - // skips `fixture` / `fixtures` when nested under a `test` / - // `tests` directory. + // Previously `test/fixture/...` was excluded from the walk -- a blunt + // guard against duplicate-FQN pollution. Fixture trees are now indexed; + // the same FQN declared in several places is disambiguated by proximity + // to the requesting document instead of being hidden. $this->writeFile('src/User.xphp', "writeFile('test/fixture/source/User.xphp', "writeFile('test/fixtures/another/User.xphp', "writeFile('tests/fixture/yetmore/User.xphp', "writeFile('test/fixture/demoA/User.xphp', "index(new PhpactorWorkspace()); - $location = $index->locationForFqn('App\\Models\\User'); - self::assertNotNull($location); - self::assertSame('file://' . $this->root . '/src/User.xphp', $location['uri']); + // The fixture-tree declaration is indexed and reachable when a file in + // its own subtree asks for it. + self::assertSame( + $this->root . '/test/fixture/demoA/User.xphp', + $index->pathFor('App\\Models\\User', 'file://' . $this->root . '/test/fixture/demoA/Use.xphp'), + ); + // A request from src resolves to the src copy (nearest to it). + self::assertSame( + $this->root . '/src/User.xphp', + $index->pathFor('App\\Models\\User', 'file://' . $this->root . '/src/Use.xphp'), + ); + // With no requesting context, the deterministic shortest-path tiebreak + // still applies. + self::assertSame($this->root . '/src/User.xphp', $index->pathFor('App\\Models\\User')); } public function testFilesystemWalkDoesNotSkipNonTestFixtureDirs(): void @@ -577,18 +584,11 @@ public function testGlobalNamespaceBlockBoundIsCollectedUnderBareName(): void self::assertSame(['Stringable'], $index->boundsForGenericClass('Bare')); } - public function testFilesystemWalkSkipsVendorTestFixtureSubdirs(): void + public function testFilesystemWalkIndexesTestFixtureSubdirs(): void { - // Pins the `iterator()` SKIP_NESTED check (line ~1031). - // Without a fixture that PLACES files inside the - // skip-listed nested dirs, `FalseValue` (return true instead - // of false) and `ReturnRemoval` (drop the early `return`) - // survive because the walker never reaches the branch. - // - // SKIP_NESTED currently includes `test/fixture/source` and - // `test/fixtures`. Both should be invisible to the FQN - // index even when they contain .xphp files declaring real - // classes. + // The walk no longer prunes `test/fixture` / `test/fixtures`: those + // declarations are indexed like any other (duplicate FQNs across them + // are disambiguated by proximity at resolution time, not by exclusion). $this->writeFile('src/Real.xphp', "writeFile('test/fixture/source/Shadow.xphp', "writeFile('test/fixtures/Phantom.xphp', "allClassFqns(); self::assertContains('App\\Real', $fqns); - self::assertNotContains( - 'App\\Shadow', - $fqns, - 'test/fixture/source nested dir must be skipped by iterator()', - ); - self::assertNotContains( - 'App\\Phantom', - $fqns, - 'test/fixtures nested dir must be skipped by iterator()', - ); + self::assertContains('App\\Shadow', $fqns, 'test/fixture/source declarations are now indexed'); + self::assertContains('App\\Phantom', $fqns, 'test/fixtures declarations are now indexed'); } public function testPublicLookupApisAcceptLeadingBackslashForm(): void @@ -828,6 +820,70 @@ public function testIsBareBuiltinFunctionFqnRejectsUserDefinedFunctions(): void )); } + public function testResolvesDuplicateFqnByProximityToOrigin(): void + { + // Two packages declare the SAME FQN. Resolution must pick the copy in + // the requesting file's own package (longest shared path prefix). + $this->writeFile('pkgA/src/User.xphp', "writeFile('pkgB/src/User.xphp', "index(new PhpactorWorkspace()); + + $fromA = $index->pathFor('App\\Models\\User', 'file://' . $this->root . '/pkgA/Demo.xphp'); + $fromB = $index->pathFor('App\\Models\\User', 'file://' . $this->root . '/pkgB/Demo.xphp'); + + self::assertSame($this->root . '/pkgA/src/User.xphp', $fromA); + self::assertSame($this->root . '/pkgB/src/User.xphp', $fromB); + } + + public function testNullOriginFallsBackToShortestPathTiebreak(): void + { + // With no requesting context, the deterministic global tiebreak + // (shortest path) applies -- preserving pre-proximity behavior. + $this->writeFile('a/User.xphp', "writeFile('deeper/path/User.xphp', "index(new PhpactorWorkspace()); + + self::assertSame($this->root . '/a/User.xphp', $index->pathFor('App\\Models\\User')); + } + + public function testLocationForFqnReturnsNearestDeclarationsPosition(): void + { + // The two copies put the class name on different lines; proximity must + // return BOTH the near file's URI AND its identifier position. + $this->writeFile('pkgA/User.xphp', "writeFile('pkgB/User.xphp', "index(new PhpactorWorkspace()); + + $locA = $index->locationForFqn('App\\Models\\User', 'file://' . $this->root . '/pkgA/Use.xphp'); + self::assertNotNull($locA); + self::assertSame('file://' . $this->root . '/pkgA/User.xphp', $locA['uri']); + self::assertSame(2, $locA['line']); + + $locB = $index->locationForFqn('App\\Models\\User', 'file://' . $this->root . '/pkgB/Use.xphp'); + self::assertNotNull($locB); + self::assertSame('file://' . $this->root . '/pkgB/User.xphp', $locB['uri']); + self::assertSame(4, $locB['line']); + } + + public function testOpenDocStillWinsRegardlessOfProximity(): void + { + // An open buffer beats any on-disk copy, even a nearer one. + $this->writeFile('pkgA/User.xphp', "open(new TextDocumentItem( + 'file:///elsewhere/User.xphp', + 'xphp', + 1, + "index($workspace); + + self::assertSame( + 'file:///elsewhere/User.xphp', + $index->pathFor('App\\Models\\User', 'file://' . $this->root . '/pkgA/Use.xphp'), + ); + } + private function index(PhpactorWorkspace $workspace): FqnIndex { $parser = new XphpSourceParser((new ParserFactory())->createForHostVersion()); diff --git a/test/Resolver/BoundErrorCodeActionProviderTest.php b/test/Resolver/BoundErrorCodeActionProviderTest.php new file mode 100644 index 0000000..fae6bfd --- /dev/null +++ b/test/Resolver/BoundErrorCodeActionProviderTest.php @@ -0,0 +1,160 @@ + { public T $item; } + PHP; + + public function testScalarConcreteOffersSwapButNotImplementInterface(): void + { + $actions = $this->actionsForUse([ + '/Stringy.xphp' => self::STRINGY, + '/Box.xphp' => self::BOX, + '/Use.xphp' => "();\n", + ]); + + $titles = self::titles($actions); + self::assertContains('Change type argument to Stringy', $titles); + foreach ($titles as $title) { + self::assertStringStartsNotWith('Add implements', $title, 'no implement-interface fix for a scalar concrete'); + } + + // The swap edit replaces the offending `int` type-argument. + $swap = self::actionTitled($actions, 'Change type argument to Stringy'); + $edit = $swap->edit->documentChanges[0]->edits[0]; + self::assertSame('Stringy', $edit->newText); + self::assertSame(2, $edit->range->start->line, 'the `int` is on line 2 of Use.xphp'); + } + + public function testWorkspaceClassConcreteOffersBothFixes(): void + { + $actions = $this->actionsForUse([ + '/Stringy.xphp' => self::STRINGY, + '/Box.xphp' => self::BOX, + '/Money.xphp' => " "();\n", + ]); + + $titles = self::titles($actions); + self::assertContains('Change type argument to Stringy', $titles); + self::assertContains('Add implements \\Stringable to Money', $titles); + + // The implement-interface fix is a cross-file edit on Money.xphp. + $implement = self::actionTitled($actions, 'Add implements \\Stringable to Money'); + $change = $implement->edit->documentChanges[0]; + self::assertSame('/Money.xphp', $change->textDocument->uri); + self::assertStringContainsString('implements \\Stringable', $change->edits[0]->newText); + } + + public function testNoFixesWhenConcreteAlreadyImplementsViaAnotherViolation(): void + { + // Two type params; only the second is violated. The fix must target the + // SECOND type-argument, not the first. + $actions = $this->actionsForUse([ + '/Stringy.xphp' => self::STRINGY, + '/Pair.xphp' => " { public A \$a; public B \$b; }\n", + '/Use.xphp' => "();\n", + ]); + + $swap = self::actionTitled($actions, 'Change type argument to Stringy'); + $covered = $swap->edit->documentChanges[0]->edits[0]->range; + // `int` is the second arg; assert the edit lands on it (not on Stringy). + self::assertSame(2, $covered->start->line); + // The replaced span should be 3 chars wide (`int`). + self::assertSame(3, $covered->end->character - $covered->start->character); + } + + /** + * @param array $sources + * @return list + */ + private function actionsForUse(array $sources): array + { + $files = $this->parseFiles($sources); + $diagnostics = (new WorkspaceAnalyzer())->analyze($files); + $lspDiagnostics = []; + foreach ($diagnostics['/Use.xphp'] as $diagnostic) { + if ($diagnostic->code === DiagnosticCode::BoundViolation) { + $lspDiagnostics[] = DiagnosticTranslator::toLsp($diagnostic); + } + } + self::assertNotSame([], $lspDiagnostics, 'expected a bound violation on /Use.xphp'); + + return (new BoundErrorCodeActionProvider())->actionsFor( + '/Use.xphp', + 1, + $sources['/Use.xphp'], + $lspDiagnostics, + ); + } + + /** + * @param list $actions + * @return list + */ + private static function titles(array $actions): array + { + return array_map(static fn (CodeAction $a): string => $a->title, $actions); + } + + /** + * @param list $actions + */ + private static function actionTitled(array $actions, string $title): CodeAction + { + foreach ($actions as $action) { + if ($action->title === $title) { + return $action; + } + } + self::fail(sprintf('no action titled "%s"; got [%s]', $title, implode(', ', self::titles($actions)))); + } + + /** + * @param array $sources + * @return array, source: string}> + */ + private function parseFiles(array $sources): array + { + $parser = new XphpSourceParser((new ParserFactory())->createForHostVersion()); + $analyzer = new Analyzer($parser); + $out = []; + foreach ($sources as $path => $source) { + $result = $analyzer->analyzeFile($source); + self::assertNotNull($result->ast, "fixture {$path} should parse"); + $out[$path] = ['ast' => $result->ast, 'source' => $source]; + } + return $out; + } +} diff --git a/test/Resolver/DocumentHighlightKindResolverTest.php b/test/Resolver/DocumentHighlightKindResolverTest.php new file mode 100644 index 0000000..80acd45 --- /dev/null +++ b/test/Resolver/DocumentHighlightKindResolverTest.php @@ -0,0 +1,95 @@ +kindAt($source, $this->offsetOf($source, '$w = 1', '$w'))); + self::assertSame(DocumentHighlightKind::READ, $this->kindAt($source, $this->offsetOf($source, 'echo $w', '$w'))); + self::assertSame(DocumentHighlightKind::WRITE, $this->kindAt($source, $this->offsetOf($source, '$w++', '$w'))); + self::assertSame(DocumentHighlightKind::WRITE, $this->kindAt($source, $this->offsetOf($source, 'as $v', '$v'))); + } + + public function testClassifiesDeclarationsAsWriteAndUsesAsRead(): void + { + $source = <<<'PHP' + kindAt($source, $this->offsetOf($source, 'class Foo', 'Foo'))); + self::assertSame(DocumentHighlightKind::READ, $this->kindAt($source, $this->offsetOf($source, 'new Foo', 'Foo'))); + } + + public function testClassifiesPropertyWriteVersusRead(): void + { + $source = <<<'PHP' + n = 1; + } + public function value(): int { + return $this->n; + } + } + PHP; + + self::assertSame(DocumentHighlightKind::WRITE, $this->kindAt($source, $this->offsetOf($source, '$this->n = 1', 'n'))); + self::assertSame(DocumentHighlightKind::READ, $this->kindAt($source, $this->offsetOf($source, '$this->n;', 'n'))); + } + + /** + * Resolve the highlight kind at a single original-source byte offset. + */ + private function kindAt(string $source, int $offset): int + { + $parser = new XphpSourceParser((new ParserFactory())->createForHostVersion()); + [$ast, $map] = $parser->parseWithMap($source); + $kinds = (new DocumentHighlightKindResolver())->resolve($ast ?? [], $map, [$offset]); + self::assertArrayHasKey($offset, $kinds, "no kind resolved at offset {$offset}"); + return $kinds[$offset]; + } + + /** + * Byte offset of `$needle` within the first occurrence of `$context`. + */ + private function offsetOf(string $source, string $context, string $needle): int + { + $ctxAt = strpos($source, $context); + self::assertNotFalse($ctxAt, "context not found: {$context}"); + $needleAt = strpos($source, $needle, $ctxAt); + self::assertNotFalse($needleAt, "needle not found: {$needle}"); + return $needleAt; + } +}