diff --git a/features/edit/code_lens.feature b/features/edit/code_lens.feature index fb58d39..3248b4a 100644 --- a/features/edit/code_lens.feature +++ b/features/edit/code_lens.feature @@ -23,6 +23,7 @@ Feature: Code lens Scenario: Emit a "Show references" lens for a declaration When I request code lenses for "/Foo.xphp" Then a code lens titled "Show references" is offered + And every code lens range is within the bounds of "/Foo.xphp" Scenario: Resolve a lens to a usage count When I request code lenses for "/Foo.xphp" diff --git a/features/navigate/definition.feature b/features/navigate/definition.feature index 902acc8..fa179e4 100644 --- a/features/navigate/definition.feature +++ b/features/navigate/definition.feature @@ -96,3 +96,39 @@ Feature: Go to definition When I request "textDocument/definition" on "Plastic" at line 7 of "SelfUse.xphp" Then the response points to "Models/Plastic.xphp" And the target range covers the "Plastic" class name + + Scenario: Jump through a nullsafe method chain to the final method declaration + # Self-contained fixtures (NOT Background overrides): a warmed FQN index goes + # stale when a Background file is redefined, so the chain's terminal hop would + # miss. Fresh files keep the index consistent. + Given the file at "Chain/Bag.xphp" contains the following lines: + """ + + { + public function first(): ?T { return null; } + } + """ + And the file at "Chain/Widget.xphp" contains the following lines: + """ + (); + $w = $bag->first()?->spin(); + """ + And the FQN index has been warmed on initialize + When I request "textDocument/definition" on "spin" at line 5 of "ChainUse.xphp" + Then the response points to "Chain/Widget.xphp" + And the target range covers the "spin" method declaration diff --git a/features/navigate/document_highlight.feature b/features/navigate/document_highlight.feature index dfb1392..d45a97a 100644 --- a/features/navigate/document_highlight.feature +++ b/features/navigate/document_highlight.feature @@ -17,6 +17,7 @@ Feature: Document highlight When I request "textDocument/documentHighlight" on "User" at line 2 of "/Use.xphp" Then the response contains 3 highlights And each highlight covers "User" in "/Use.xphp" + And every document highlight range is within the bounds of "/Use.xphp" Scenario: Classify the declaration as a write and the uses as reads When I request "textDocument/documentHighlight" on "User" at line 2 of "/Use.xphp" diff --git a/features/navigate/document_symbol.feature b/features/navigate/document_symbol.feature index 9550305..3777219 100644 --- a/features/navigate/document_symbol.feature +++ b/features/navigate/document_symbol.feature @@ -21,6 +21,7 @@ Feature: Document symbol outline When I request "textDocument/documentSymbol" for "/User.xphp" Then the outline contains a class "User" with 5 members And the "User" selection range in "/User.xphp" covers "User" + And every document symbol range is within the bounds of "/User.xphp" Scenario Outline: Each declared member appears nested in the outline When I request "textDocument/documentSymbol" for "/User.xphp" diff --git a/features/understand/folding_range.feature b/features/understand/folding_range.feature index 2e05d4e..e090611 100644 --- a/features/understand/folding_range.feature +++ b/features/understand/folding_range.feature @@ -23,6 +23,7 @@ Feature: Folding ranges When I request "textDocument/foldingRange" for "/Box.xphp" Then the response contains 3 folding ranges And a folding range of kind "region" spans 2 to 12 + And every folding range is within the bounds of "/Box.xphp" Scenario: Single-line declarations are not folded Given the file at "/One.xphp" contains the following lines: diff --git a/features/understand/hover.feature b/features/understand/hover.feature index 53c292f..7b1a513 100644 --- a/features/understand/hover.feature +++ b/features/understand/hover.feature @@ -182,3 +182,133 @@ Feature: Hover And the FQN index has been warmed on initialize When I request "textDocument/hover" on "first" at line 6 of "/Use.xphp" Then the hover contents contain "Pair $first" + + Scenario: A nullsafe property access on a nullable generic result is itself nullable + Given the file at "/Collection.xphp" contains the following lines: + """ + + { + public function first(): ?T { return null; } + } + """ + And the file at "/User.xphp" contains the following lines: + """ + (); + $firstName = $users->first()?->name; + """ + And the FQN index has been warmed on initialize + When I request "textDocument/hover" on "firstName" at line 4 of "/Use.xphp" + Then the hover contents contain "?string $firstName" + + Scenario: A multi-hop nullsafe property chain through a nullable generic result is nullable + Given the file at "/Collection.xphp" contains the following lines: + """ + + { + public function first(): ?T { return null; } + } + """ + And the file at "/User.xphp" contains the following lines: + """ + (); + $friendName = $users->first()?->bestFriend?->name; + """ + And the FQN index has been warmed on initialize + When I request "textDocument/hover" on "friendName" at line 4 of "/Use.xphp" + Then the hover contents contain "?string $friendName" + + Scenario: A chain through a non-generic method resolves + Given the file at "/Collection.xphp" contains the following lines: + """ + + { + public function first(): ?T { return null; } + } + """ + And the file at "/User.xphp" contains the following lines: + """ + (); + $picked = $users->first()?->mirror()?->name; + """ + And the FQN index has been warmed on initialize + When I request "textDocument/hover" on "picked" at line 4 of "/Use.xphp" + Then the hover contents contain "?string $picked" + + Scenario: A chain through a property typed via a cross-namespace use import resolves + Given the file at "/Collection.xphp" contains the following lines: + """ + + { + public function first(): ?T { return null; } + } + """ + And the file at "/Profile.xphp" contains the following lines: + """ + (); + $profileBio = $users->first()?->profile?->bio; + """ + And the FQN index has been warmed on initialize + When I request "textDocument/hover" on "profileBio" at line 4 of "/Use.xphp" + Then the hover contents contain "?string $profileBio" diff --git a/features/understand/inlay_hints.feature b/features/understand/inlay_hints.feature index f83f782..dcb8f38 100644 --- a/features/understand/inlay_hints.feature +++ b/features/understand/inlay_hints.feature @@ -30,6 +30,7 @@ Feature: Inlay hints When I request "textDocument/inlayHint" for the visible range of "/Use.xphp" Then exactly 1 inlay hint is rendered And an inlay hint ": ?App\Models\User" is rendered after "$first" on line 4 of "/Use.xphp" + And every inlay hint position is within the bounds of "/Use.xphp" Scenario: Hint a generic method turbofish called on a local-variable receiver Given the file at "/Util.xphp" contains the following lines: diff --git a/features/understand/semantic_tokens.feature b/features/understand/semantic_tokens.feature index 9a6d838..2c5ceeb 100644 --- a/features/understand/semantic_tokens.feature +++ b/features/understand/semantic_tokens.feature @@ -15,6 +15,7 @@ Feature: Semantic tokens When I request "textDocument/semanticTokens/full" for "/box.xphp" Then the semantic tokens are non-empty And a "typeParameter" token covers "T" in "/box.xphp" + And every semantic token is within the bounds of "/box.xphp" Scenario: Highlight the type parameter of a generic closure Given the file at "/closure.xphp" contains the following lines: diff --git a/features/validate/diagnostics.feature b/features/validate/diagnostics.feature index 5fcc700..04814fe 100644 --- a/features/validate/diagnostics.feature +++ b/features/validate/diagnostics.feature @@ -143,3 +143,59 @@ Feature: Diagnostics When I analyze "/Bounds.xphp" for diagnostics Then a "xphp.ctor-arg-mismatch" diagnostic is reported And the "xphp.ctor-arg-mismatch" diagnostic underlines "new User()" + + Scenario: Warn about a null dereference on a chained nullable receiver + Given the file at "/Collection.xphp" contains the following lines: + """ + + { + public function first(): ?T { return null; } + } + """ + And the file at "/User.xphp" contains the following lines: + """ + (); + $name = $users->first()->name; + """ + And the FQN index has been warmed on initialize + When I analyze "/Use.xphp" for diagnostics + Then a "xphp.null-deref" diagnostic is reported saying "possibly-null" + And the "xphp.null-deref" diagnostic underlines "name" + And every reported diagnostic range is within document bounds + + Scenario: A nullsafe access on the same nullable chain is not flagged + Given the file at "/Collection.xphp" contains the following lines: + """ + + { + public function first(): ?T { return null; } + } + """ + And the file at "/User.xphp" contains the following lines: + """ + (); + $name = $users->first()?->name; + """ + And the FQN index has been warmed on initialize + When I analyze "/Use.xphp" for diagnostics + Then no diagnostics are reported diff --git a/src/Analyzer/DiagnosticCode.php b/src/Analyzer/DiagnosticCode.php index 847b4a5..2abb02b 100644 --- a/src/Analyzer/DiagnosticCode.php +++ b/src/Analyzer/DiagnosticCode.php @@ -91,6 +91,24 @@ enum DiagnosticCode: string */ case ArgumentMismatch = 'xphp.arg-mismatch'; + /** + * A property/method was accessed via plain `->` on a receiver whose + * static type is nullable -- a would-be runtime + * `Error: Attempt to read property "x" on null` (or + * `Call to a member function on null`). + * + * Conservative scope (low false-positive): fires ONLY when the immediate + * receiver is itself a nullable member-access sub-expression -- a chain + * like `$users->first()->name` where `first()` returns `?User` and no + * inline guard is syntactically possible. Bare-variable receivers + * (`$x->y`) are deferred to a future flow-narrowing pass, and a nullsafe + * access (`$users->first()?->name`) is correct so it never fires. + * + * Severity is Warning, not Error: the inference is conservative but the + * editor should keep it dismissable, matching `xphp.undefined-name`. + */ + case NullDeref = 'xphp.null-deref'; + /** * Map a RuntimeException raised by Registry::recordInstantiation to its * diagnostic code. The Registry doesn't (currently) use a typed exception diff --git a/src/Diagnostics/XphpDiagnosticsProvider.php b/src/Diagnostics/XphpDiagnosticsProvider.php index 2a891a4..41870ad 100644 --- a/src/Diagnostics/XphpDiagnosticsProvider.php +++ b/src/Diagnostics/XphpDiagnosticsProvider.php @@ -15,9 +15,13 @@ use Phpactor\LanguageServerProtocol\Range; use Phpactor\LanguageServerProtocol\TextDocumentItem; use XPHP\Lsp\Analyzer\Diagnostic; +use XPHP\Lsp\Analyzer\DiagnosticCode; +use XPHP\Lsp\Analyzer\DiagnosticSeverity; use XPHP\Lsp\Analyzer\ParsedDocumentCache; +use XPHP\Lsp\Analyzer\ParseResult; use XPHP\Lsp\Analyzer\WorkspaceAnalyzer; use XPHP\Lsp\Reflection\FqnIndex; +use XPHP\Lsp\Resolver\GenericResolver; /** * Bridges the xphp analyzer (per-file + cross-file) to phpactor's diagnostics engine. @@ -64,6 +68,12 @@ public function __construct( * is skipped and the provider behaves exactly as the per-document linter. */ private readonly ?ClientApi $clientApi = null, + /** + * Type-inference engine used for the conservative null-dereference + * diagnostic (`xphp.null-deref`). Optional: resolver-less contexts + * (most unit tests) simply skip that diagnostic and behave as before. + */ + private readonly ?GenericResolver $genericResolver = null, ) { } @@ -124,7 +134,10 @@ private function computeAllOpenDiagnostics(TextDocumentItem $textDocument): arra $docMeta = [$currentUri => ['version' => $textDocument->version, 'source' => $textDocument->text]]; $perFileByUri = [ $currentUri => $this->toLspClamped( - $currentResult->diagnostics, + array_merge( + $currentResult->diagnostics, + $this->collectNullDerefDiagnostics($currentUri, $textDocument->version, $textDocument->text, $currentResult), + ), $currentUri, $textDocument->version, $textDocument->text, @@ -141,7 +154,10 @@ private function computeAllOpenDiagnostics(TextDocumentItem $textDocument): arra $otherResult = $this->cache->getOrParse($uri, $item->version, $item->text); $docMeta[$uri] = ['version' => $item->version, 'source' => $item->text]; $perFileByUri[$uri] = $this->toLspClamped( - $otherResult->diagnostics, + array_merge( + $otherResult->diagnostics, + $this->collectNullDerefDiagnostics($uri, $item->version, $item->text, $otherResult), + ), $uri, $item->version, $item->text, @@ -201,6 +217,56 @@ private function computeAllOpenDiagnostics(TextDocumentItem $textDocument): arra return $result; } + /** + * Conservative null-dereference diagnostics (`xphp.null-deref`) for one + * document. Asks the type-inference resolver for plain-`->` accesses on a + * statically nullable chained receiver, then maps each site's STRIPPED-source + * byte offsets back to the original buffer (via the parse's `ByteOffsetMap`) + * and on to an LSP range (via the cached `PositionMap`). + * + * No-op when no resolver is wired (pull-mode unit contexts) or the document + * didn't parse -- the rest of the pipeline is unaffected. + * + * @return list + */ + private function collectNullDerefDiagnostics(string $uri, int $version, string $source, ParseResult $result): array + { + if ($this->genericResolver === null || $result->ast === null) { + return []; + } + $sites = $this->genericResolver->findNullDerefSites($uri, $version, $source); + if ($sites === []) { + return []; + } + $positionMap = $this->cache->positionMap($uri, $version, $source); + $out = []; + foreach ($sites as $site) { + $origStart = $result->byteOffsetMap->toOriginal($site->strippedStart); + // end offset is inclusive in the AST; +1 makes the LSP range + // half-open over the last character of the member name. + $origEnd = $result->byteOffsetMap->toOriginal($site->strippedEnd + 1); + if ($origStart < 0 || $origEnd < $origStart) { + continue; + } + [$startLine, $startChar] = $positionMap->offsetToPosition($origStart); + [$endLine, $endChar] = $positionMap->offsetToPosition($origEnd); + $out[] = new Diagnostic( + startLine: $startLine, + startCharacter: $startChar, + endLine: $endLine, + endCharacter: $endChar, + message: sprintf( + 'Accessing "%s" on a possibly-null value (%s). Use `?->` or guard against null first.', + $site->member, + $site->receiverType, + ), + code: DiagnosticCode::NullDeref, + severity: DiagnosticSeverity::Warning, + ); + } + return $out; + } + /** * Translate internal diagnostics to LSP-wire diagnostics, clamping every * range into the document's bounds first. The server must never emit a range diff --git a/src/LspDispatcherFactory.php b/src/LspDispatcherFactory.php index 71801ea..bf57aa8 100644 --- a/src/LspDispatcherFactory.php +++ b/src/LspDispatcherFactory.php @@ -185,6 +185,9 @@ public function create(MessageTransmitter $transmitter, InitializeParams $initia // Push path: lets a workspace pass re-publish diagnostics for the // dependents of the edited file (cross-file broadcast). $clientApi, + // Type-inference engine for the conservative null-dereference + // diagnostic (xphp.null-deref) on chained nullable receivers. + $genericResolver, ); $diagnosticsEngine = new DiagnosticsEngine( @@ -376,7 +379,7 @@ public function create(MessageTransmitter $transmitter, InitializeParams $initia new ErrorHandlingMiddleware($this->logger), new InitializeMiddleware($handlers, $eventDispatcher, [ 'name' => 'xphp-lsp', - 'version' => '0.2.4', + 'version' => '0.2.5', ]), new ShutdownMiddleware($eventDispatcher), new ResponseHandlingMiddleware($responseWatcher), diff --git a/src/Reflection/FqnIndex.php b/src/Reflection/FqnIndex.php index 83a59d4..48bdf9b 100644 --- a/src/Reflection/FqnIndex.php +++ b/src/Reflection/FqnIndex.php @@ -253,6 +253,51 @@ public function classLikeFor(string $fqn, ?string $origin = null): ?ClassLike return $this->classLikeFromFile($path, $needle); } + /** + * Like {@see classLikeFor} but also return the declaring file's full AST, + * so a caller can derive the file's name-resolution context (use-map + + * namespace). Both branches reuse {@see ParsedDocumentCache} so a deep + * chain doesn't re-parse the declaring file per hop. + * + * @return array{classLike: ClassLike, ast: list<\PhpParser\Node\Stmt>}|null + */ + public function classLikeAstFor(string $fqn, ?string $origin = null): ?array + { + $needle = ltrim($fqn, '\\'); + if ($needle === '') { + return null; + } + + // Open-doc first (live view of unsaved edits). + foreach ($this->workspace as $uri => $item) { + $result = $this->cache->getOrParse($uri, $item->version, $item->text); + if ($result->ast === null) { + continue; + } + $hit = self::findClassLikeInAst($result->ast, $needle); + if ($hit !== null) { + return ['classLike' => $hit, 'ast' => $result->ast]; + } + } + + $path = $this->selectDecl($needle, $origin)['path'] ?? null; + if ($path === null) { + return null; + } + $source = @file_get_contents($path); + if ($source === false) { + return null; + } + // version -1 = "filesystem snapshot" (mirrors boundExprsForGenericClass); + // routes through the cache so repeated chain hops don't re-parse. + $result = $this->cache->getOrParse('file://' . $path, -1, $source); + if ($result->ast === null) { + return null; + } + $hit = self::findClassLikeInAst($result->ast, $needle); + return $hit === null ? null : ['classLike' => $hit, 'ast' => $result->ast]; + } + /** * Every class/interface/trait FQN known to the index, from both open * docs and the filesystem. De-duplicated; ordering is insertion-stable diff --git a/src/Resolver/ClassLikeContext.php b/src/Resolver/ClassLikeContext.php new file mode 100644 index 0000000..b26268d --- /dev/null +++ b/src/Resolver/ClassLikeContext.php @@ -0,0 +1,30 @@ + $useMap alias -> FQN, from the declaring file + */ + public function __construct( + public ClassLike $classLike, + public array $useMap, + public string $namespace, + ) { + } +} diff --git a/src/Resolver/ClassLikeLookup.php b/src/Resolver/ClassLikeLookup.php index 2b5b03b..d9e57ae 100644 --- a/src/Resolver/ClassLikeLookup.php +++ b/src/Resolver/ClassLikeLookup.php @@ -29,4 +29,12 @@ interface ClassLikeLookup * `ATTR_GENERIC_PARAMS` on it without re-parsing. */ public function find(string $fqn): ?ClassLike; + + /** + * Like {@see find}, but also return the DECLARING file's name-resolution + * context (use-map + namespace) so callers can qualify bare class names in + * that class's member types against the file where it was declared. Returns + * null when the FQN isn't found. + */ + public function findWithContext(string $fqn): ?ClassLikeContext; } diff --git a/src/Resolver/CompositeClassLikeLookup.php b/src/Resolver/CompositeClassLikeLookup.php index cd8c987..01b6b2e 100644 --- a/src/Resolver/CompositeClassLikeLookup.php +++ b/src/Resolver/CompositeClassLikeLookup.php @@ -38,4 +38,15 @@ public function find(string $fqn): ?ClassLike } return null; } + + public function findWithContext(string $fqn): ?ClassLikeContext + { + foreach ($this->lookups as $lookup) { + $found = $lookup->findWithContext($fqn); + if ($found !== null) { + return $found; + } + } + return null; + } } diff --git a/src/Resolver/FilesystemClassLikeLookup.php b/src/Resolver/FilesystemClassLikeLookup.php index b76a85f..ae9253b 100644 --- a/src/Resolver/FilesystemClassLikeLookup.php +++ b/src/Resolver/FilesystemClassLikeLookup.php @@ -31,4 +31,14 @@ public function find(string $fqn): ?ClassLike { return $this->index->classLikeFor($fqn); } + + public function findWithContext(string $fqn): ?ClassLikeContext + { + $hit = $this->index->classLikeAstFor($fqn); + if ($hit === null) { + return null; + } + [$useMap, $namespace] = GenericResolver::useMapAndNamespaceFor($hit['ast']); + return new ClassLikeContext($hit['classLike'], $useMap, $namespace); + } } diff --git a/src/Resolver/GenericResolver.php b/src/Resolver/GenericResolver.php index 8293c77..99d84b5 100644 --- a/src/Resolver/GenericResolver.php +++ b/src/Resolver/GenericResolver.php @@ -11,6 +11,7 @@ use PhpParser\Node\Expr\Closure; use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Expr\MethodCall; +use PhpParser\Node\Expr\NullsafeMethodCall; use PhpParser\Node\Expr\New_; use PhpParser\Node\Expr\NullsafePropertyFetch; use PhpParser\Node\Expr\PropertyFetch; @@ -706,10 +707,10 @@ private function bindingAt(string $uri, string $varName, int $byteOffset): VarBi * * @param list $ast */ - private static function findEnclosingMethodCallNameAt(array $ast, int $byteOffset): ?MethodCall + private static function findEnclosingMethodCallNameAt(array $ast, int $byteOffset): MethodCall|NullsafeMethodCall|null { $visitor = new class($byteOffset) extends NodeVisitorAbstract { - public ?MethodCall $hit = null; + public MethodCall|NullsafeMethodCall|null $hit = null; public function __construct(private readonly int $offset) { @@ -720,7 +721,7 @@ public function enterNode(Node $node): null if ($this->hit !== null) { return null; } - if (!$node instanceof MethodCall) { + if (!$node instanceof MethodCall && !$node instanceof NullsafeMethodCall) { return null; } $name = $node->name; @@ -835,10 +836,13 @@ public function enterNode(Node $node): null * The maps are file-scoped: the same use stmt may not be in effect * elsewhere, but for a single-file resolution this is enough. * + * Public so the ClassLikeLookup implementations can compute a declaring + * file's name-resolution context for {@see ClassLikeLookup::findWithContext}. + * * @param list $ast * @return array{0: array, 1: string} */ - private static function useMapAndNamespaceFor(array $ast): array + public static function useMapAndNamespaceFor(array $ast): array { $useMap = []; $namespace = ''; @@ -969,7 +973,7 @@ private static function emptyScopes(): array * @param list}> $scopes * @return array */ - private static function bindingsAt(array $scopes, int $offset): array + public static function bindingsAt(array $scopes, int $offset): array { $best = null; foreach ($scopes as $scope) { @@ -1136,7 +1140,7 @@ private function handleAssign(Assign $node): void } return; } - if ($rhs instanceof MethodCall) { + if ($rhs instanceof MethodCall || $rhs instanceof NullsafeMethodCall) { $resolved = GenericResolver::resolveMethodCall( $rhs, $this->currentBindings(), @@ -1357,9 +1361,17 @@ public static function inferType( $varBinding = self::buildFromNew($expr, $classes); return $varBinding === null ? null : self::resolvedTypeFromBinding($varBinding); } - if ($expr instanceof MethodCall) { + if ($expr instanceof MethodCall || $expr instanceof NullsafeMethodCall) { return self::resolveMethodCall($expr, $bindings, $classes, $fqnIndex, $useMap, $currentNamespace); } + if ($expr instanceof PropertyFetch || $expr instanceof NullsafePropertyFetch) { + // A property-fetch receiver (`$a?->b?->c`): delegate to + // resolvePropertyFetch, which recurses into its own receiver via + // inferType. This terminates at a Variable / New_ / *Call leaf + // (each hop strips one AST node), so deep property chains resolve + // and per-hop nullsafe nullability propagates. + return self::resolvePropertyFetch($expr, $bindings, $classes, $fqnIndex, $useMap, $currentNamespace); + } if ($expr instanceof StaticCall) { return self::resolveStaticCall($expr, $classes, $useMap, $currentNamespace); } @@ -1369,6 +1381,115 @@ public static function inferType( return null; } + /** + * Conservative null-dereference detection over a whole document. + * + * Flags a plain-`->` property/method access whose IMMEDIATE receiver is a + * statically nullable member-access sub-expression -- a chain like + * `$users->first()->name` where `first()` returns `?User`. In that shape no + * inline null-guard is syntactically possible and there is no bare variable + * to flow-narrow, so the inferred nullability is a reliable signal. + * + * Deliberately narrow to keep false positives near zero: + * - only plain `->` accesses (a `?->` access is already null-safe); + * - the receiver must itself be a `->`/`?->` method-call or property-fetch + * (bare-variable receivers like `$x->y` are DEFERRED to a future + * flow-narrowing pass -- guards such as `if ($x !== null)` aren't modelled); + * - when `inferType` can't model the receiver it returns null and nothing + * fires (unresolvable cross-file types degrade safely to no diagnostic). + * + * Returns sites in STRIPPED-source byte offsets; the diagnostics layer maps + * them back to the original buffer. + * + * @return list + */ + public function findNullDerefSites(string $uri, int $version, string $text): array + { + $result = $this->documents->getOrParse($uri, $version, $text); + $ast = $result->ast; + if ($ast === null) { + return []; + } + $scopes = $this->scopesFor($uri, $version, $text); + [$useMap, $namespace] = self::useMapAndNamespaceFor($ast); + + $sites = []; + $traverser = new NodeTraverser(); + $traverser->addVisitor(new class( + $sites, + $scopes, + $this->classes, + $this->fqnIndex, + $useMap, + $namespace, + ) extends NodeVisitorAbstract { + /** + * @param list $sites + * @param list}> $scopes + * @param array $useMap + */ + public function __construct( + private array &$sites, + private array $scopes, + private ClassLikeLookup $classes, + private FqnIndex $fqnIndex, + private array $useMap, + private string $namespace, + ) { + } + + public function enterNode(Node $node): null + { + // Only plain `->` accesses; `?->` is already null-safe. + if (!$node instanceof PropertyFetch && !$node instanceof MethodCall) { + return null; + } + if (!$node->name instanceof Identifier) { + return null; + } + $receiver = $node->var; + // Restrict to member-access receivers: a chained sub-expression + // that can't carry an inline guard and isn't a narrowable bare + // variable. (`$x->y` on a nullable `$x` is deferred.) + if ( + !$receiver instanceof MethodCall + && !$receiver instanceof NullsafeMethodCall + && !$receiver instanceof PropertyFetch + && !$receiver instanceof NullsafePropertyFetch + ) { + return null; + } + $bindings = GenericResolver::bindingsAt($this->scopes, $receiver->getStartFilePos()); + $receiverType = GenericResolver::inferType( + $receiver, + $bindings, + $this->classes, + $this->fqnIndex, + $this->useMap, + $this->namespace, + ); + if ($receiverType === null || !$receiverType->nullable) { + return null; + } + $start = $node->name->getStartFilePos(); + $end = $node->name->getEndFilePos(); + if ($start < 0 || $end < $start) { + return null; + } + $this->sites[] = new NullDerefSite( + $start, + $end, + $node->name->toString(), + $receiverType->render(), + ); + return null; + } + }); + $traverser->traverse($ast); + + return $sites; + } + /** * Convert a `VarBinding` (class + paramMap) into a `ResolvedType` -- a * `TypeRef` whose `args` carry the bound type-args so downstream @@ -1393,7 +1514,7 @@ private static function resolvedTypeFromBinding(VarBinding $binding): ResolvedTy * @internal called from the visitor closure and from inferType. */ public static function resolveMethodCall( - MethodCall $call, + MethodCall|NullsafeMethodCall $call, array $bindings, ClassLikeLookup $classes, FqnIndex $fqnIndex, @@ -1414,10 +1535,13 @@ public static function resolveMethodCall( if ($receiverType === null) { return null; } - $classLike = $classes->find($receiverType->ref->name); - if ($classLike === null) { + // findWithContext (not find) so a bare class name in the return type + // can be qualified against the method's DECLARING file (W1/W2). + $declaring = $classes->findWithContext($receiverType->ref->name); + if ($declaring === null) { return null; } + $classLike = $declaring->classLike; $method = self::findMethod($classLike, $call->name->toString()); if ($method === null) { return null; @@ -1426,29 +1550,44 @@ public static function resolveMethodCall( if ($returnType === null) { return null; } - // Rebuild paramMap from the receiver's TypeRef args (set during - // `resolvedTypeFromBinding` or by a prior chained call's - // substituted output). When the receiver has no args -- e.g. - // an unconstrained or already fully-substituted scalar -- the - // method's own substitution is a no-op, which is correct. + // Rebuild paramMap from the receiver's TypeRef args. Empty for a plain + // method on a non-generic receiver -- substitution is then a no-op. $paramMap = self::paramMapFromReceiver($classLike, $receiverType); $paramMap = self::withMethodTurbofish($paramMap, $call, $method); - // No type params in scope = nothing generic to substitute (a plain - // method on a non-generic receiver). Return null so the result isn't - // recorded as a binding and worse-reflection keeps ownership of it. - if ($paramMap === []) { - return null; - } + $isGeneric = $paramMap !== []; $paramNames = array_keys($paramMap); [$nullable, $ref] = self::returnTypeToRef($returnType, $paramNames) ?? [null, null]; if ($ref === null) { return null; } - // `static`/`self` bind to the receiver's concrete type, not a param. - $substituted = self::relativeTypeToReceiver($ref, $receiverType) - ?? Specializer::substituteTypeRef($ref, $paramMap); - return new ResolvedType($substituted, $nullable); + // `static`/`self`/`parent` bind to the receiver's concrete type. + $relative = self::relativeTypeToReceiver($ref, $receiverType); + $substituted = $relative + ?? ($isGeneric ? Specializer::substituteTypeRef($ref, $paramMap) : $ref); + $substituted = self::qualifyTypeRef($substituted, $declaring, $classes); + + // Gap 2: a non-generic method used to bail to null (defer to + // worse-reflection). Now we OWN it -- so chains continue + // (`$users->first()?->self()?->name`) -- but ONLY when the result is a + // usable receiver: a relative type, or a class the lookup confirms. + // Scalars / unconfirmable names still return null so terminal hovers + // and single non-generic calls keep worse-reflection's richer view. + if (!$isGeneric && $relative === null) { + $confirmable = !$substituted->isScalar + && !$substituted->isTypeParam + && $substituted->name !== '' + && $classes->find(ltrim($substituted->name, '\\')) !== null; + if (!$confirmable) { + return null; + } + } + + // A nullsafe call (`$x?->method()`) short-circuits to null when the + // receiver is null, so the result is nullable on top of the method's + // own return-type nullability. + $resultNullable = $nullable || $call instanceof NullsafeMethodCall; + return new ResolvedType($substituted, $resultNullable); } /** @@ -1489,10 +1628,15 @@ public static function resolvePropertyFetch( if ($receiverType === null) { return null; } - $classLike = $classes->find($receiverType->ref->name); - if ($classLike === null) { + // findWithContext (not find) so we get the DECLARING file's use-map + + // namespace, needed to qualify a bare class name in the property type + // against the file where the property is declared -- incl. cross-namespace + // `use` imports. + $declaring = $classes->findWithContext($receiverType->ref->name); + if ($declaring === null) { return null; } + $classLike = $declaring->classLike; $propertyType = self::findPropertyType($classLike, $fetch->name->toString()); if ($propertyType === null) { return null; @@ -1508,7 +1652,45 @@ public static function resolvePropertyFetch( return null; } $substituted = Specializer::substituteTypeRef($ref, $paramMap); - return new ResolvedType($substituted, $nullable); + // A property typed as a bare class name (`?User`) resolves to an + // UNqualified TypeRef (the resolver runs no NameResolver). That renders + // fine for a terminal hover, but a chained access + // (`$x?->bestFriend?->name`) needs the intermediate's class FQN so the + // next hop can look the class up. Qualify it against the declaring + // file's use-map + namespace. + $substituted = self::qualifyTypeRef($substituted, $declaring, $classes); + // A nullsafe access (`$x?->prop`) short-circuits to null when the + // receiver is null, so the result is `|null` regardless of + // the property's own declared nullability. A regular `->` does NOT + // widen (a null receiver there is a runtime error, not a type). + $resultNullable = $nullable || $fetch instanceof NullsafePropertyFetch; + return new ResolvedType($substituted, $resultNullable); + } + + /** + * A bare (non-scalar, non-type-param) class name in a member type -- + * `?User` -- comes back unqualified because the resolver runs no + * NameResolver. Qualify it against the DECLARING file's name-resolution + * context (use-map + namespace), so a cross-namespace `use App\Models\User` + * resolves correctly and not just same-namespace references. Accept the + * candidate ONLY when the lookup confirms it exists -- an unconfirmable name + * is left bare so a chain degrades to null rather than fabricating a wrong FQN. + */ + private static function qualifyTypeRef(TypeRef $ref, ClassLikeContext $declaring, ClassLikeLookup $classes): TypeRef + { + if ($ref->isScalar || $ref->isTypeParam) { + return $ref; + } + $name = $ref->name; + // Already namespaced / fully-qualified, or resolvable as-is. + if ($name === '' || str_contains($name, '\\') || $classes->find($name) !== null) { + return $ref; + } + $candidate = self::resolveNameWithUseMap(new Name($name), $declaring->useMap, $declaring->namespace); + if ($candidate === null || $candidate === $name || $classes->find($candidate) === null) { + return $ref; // can't confirm -- leave bare (safe degradation) + } + return new TypeRef($candidate, $ref->args, $ref->isScalar, $ref->isTypeParam); } /** diff --git a/src/Resolver/NullDerefSite.php b/src/Resolver/NullDerefSite.php new file mode 100644 index 0000000..2d85044 --- /dev/null +++ b/src/Resolver/NullDerefSite.php @@ -0,0 +1,26 @@ +` member access whose immediate receiver is a statically nullable + * sub-expression -- a potential null dereference the resolver surfaced. + * + * Offsets are in the xphp-STRIPPED source coordinate space (the AST the + * resolver walks); the diagnostics layer translates them back to the original + * buffer via `ByteOffsetMap` before emitting an LSP range. `member` is the + * accessed property/method name (for the message); `receiverType` is the + * inferred nullable type rendered for the user (e.g. `?App\Models\User`). + */ +final readonly class NullDerefSite +{ + public function __construct( + public int $strippedStart, + public int $strippedEnd, + public string $member, + public string $receiverType, + ) { + } +} diff --git a/src/Resolver/WorkspaceClassLikeLookup.php b/src/Resolver/WorkspaceClassLikeLookup.php index 4f33054..93a60ae 100644 --- a/src/Resolver/WorkspaceClassLikeLookup.php +++ b/src/Resolver/WorkspaceClassLikeLookup.php @@ -52,6 +52,26 @@ public function find(string $fqn): ?ClassLike return null; } + public function findWithContext(string $fqn): ?ClassLikeContext + { + $needle = ltrim($fqn, '\\'); + if ($needle === '') { + return null; + } + foreach ($this->workspace as $uri => $item) { + $result = $this->cache->getOrParse($uri, $item->version, $item->text); + if ($result->ast === null) { + continue; + } + $hit = self::findInAst($result->ast, $needle); + if ($hit !== null) { + [$useMap, $namespace] = GenericResolver::useMapAndNamespaceFor($result->ast); + return new ClassLikeContext($hit, $useMap, $namespace); + } + } + return null; + } + /** * @param list $ast */ diff --git a/test/Behat/EditContext.php b/test/Behat/EditContext.php index 3feacfa..822ba34 100644 --- a/test/Behat/EditContext.php +++ b/test/Behat/EditContext.php @@ -351,6 +351,24 @@ public function iResolveTheCodeLensOnLine(int $line): void $this->world->fail(sprintf('no code lens on line %d', $line)); } + /** + * @Then every code lens range is within the bounds of :path + */ + public function everyCodeLensRangeIsWithinTheBoundsOf(string $path): void + { + $lenses = $this->world->last(); + $this->world->assert(is_array($lenses) && $lenses !== [], 'expected a non-empty code-lens list'); + foreach ($lenses as $i => $lens) { + if (!$lens instanceof CodeLens) { + continue; + } + $this->world->assert( + $this->world->rangeWithinDocument($path, $lens->range), + sprintf('code lens #%d out of document bounds', $i), + ); + } + } + /** * @Then a code lens titled :title is offered */ diff --git a/test/Behat/NavigateContext.php b/test/Behat/NavigateContext.php index a813ff9..b50ac4a 100644 --- a/test/Behat/NavigateContext.php +++ b/test/Behat/NavigateContext.php @@ -278,6 +278,52 @@ public function aKindHighlightCoversIn(string $kind, string $text, string $path) )); } + /** + * @Then every document highlight range is within the bounds of :path + */ + public function everyDocumentHighlightRangeIsWithinTheBoundsOf(string $path): void + { + $highlights = $this->world->last(); + $this->world->assert(is_array($highlights) && $highlights !== [], 'expected a non-empty highlight list'); + foreach ($highlights as $i => $highlight) { + $this->world->assert( + $this->world->rangeWithinDocument($path, $highlight->range), + sprintf('document highlight #%d out of document bounds', $i), + ); + } + } + + /** + * @Then every document symbol range is within the bounds of :path + */ + public function everyDocumentSymbolRangeIsWithinTheBoundsOf(string $path): void + { + $symbols = $this->world->last(); + $this->world->assert(is_array($symbols) && $symbols !== [], 'expected a non-empty document-symbol list'); + $this->assertSymbolRangesWithinBounds($symbols, $path); + } + + /** + * Recurse `range` + `selectionRange` of each symbol and its children. + * + * @param list $symbols + */ + private function assertSymbolRangesWithinBounds(array $symbols, string $path): void + { + foreach ($symbols as $symbol) { + if (!$symbol instanceof DocumentSymbol) { + continue; + } + foreach (['range' => $symbol->range, 'selectionRange' => $symbol->selectionRange] as $label => $range) { + $this->world->assert( + $this->world->rangeWithinDocument($path, $range), + sprintf('%s of document symbol "%s" out of document bounds', $label, $symbol->name), + ); + } + $this->assertSymbolRangesWithinBounds(is_array($symbol->children) ? $symbol->children : [], $path); + } + } + private function matchesPath(string $uri, string $path): bool { return $uri === $path diff --git a/test/Behat/UnderstandContext.php b/test/Behat/UnderstandContext.php index a2c644b..5f754ec 100644 --- a/test/Behat/UnderstandContext.php +++ b/test/Behat/UnderstandContext.php @@ -9,11 +9,13 @@ use Phpactor\LanguageServerProtocol\InlayHint; use Phpactor\LanguageServerProtocol\MarkupContent; use Phpactor\LanguageServerProtocol\Position; +use Phpactor\LanguageServerProtocol\Range; use Phpactor\LanguageServerProtocol\SemanticTokens; use Phpactor\LanguageServerProtocol\SignatureHelp; use Phpactor\LanguageServerProtocol\SignatureHelpParams; use Phpactor\LanguageServerProtocol\TextDocumentIdentifier; use XPHP\Lsp\Handler\SemanticTokens\TokenLegend; +use XPHP\Lsp\PositionMap; /** * Steps for the Understand theme: hover, signature help, inlay hints, folding @@ -263,6 +265,65 @@ public function anInlayHintIsRenderedAfterOnLine(string $label, string $var, int )); } + /** + * @Then every semantic token is within the bounds of :path + */ + public function everySemanticTokenIsWithinTheBoundsOf(string $path): void + { + $tokens = $this->world->last(); + $this->world->assert($tokens instanceof SemanticTokens, 'expected a SemanticTokens response, got ' . get_debug_type($tokens)); + foreach ($this->world->decodeSemanticTokens($tokens, $path) as $i => $token) { + // The decoder yields the token's text; its UTF-16 length is what the + // LSP wire `length` encodes, so measure it the same way. + $len = PositionMap::lengthInUtf16($token['text']); + $range = new Range( + new Position($token['line'], $token['char']), + new Position($token['line'], $token['char'] + $len), + ); + $this->world->assert( + $this->world->rangeWithinDocument($path, $range), + sprintf('semantic token #%d out of document bounds at %d:%d (+%d)', $i, $token['line'], $token['char'], $len), + ); + } + } + + /** + * @Then every folding range is within the bounds of :path + */ + public function everyFoldingRangeIsWithinTheBoundsOf(string $path): void + { + $ranges = $this->world->last(); + $this->world->assert(is_array($ranges) && $ranges !== [], 'expected a non-empty folding-range list'); + foreach ($ranges as $i => $range) { + // Folding ranges carry only line numbers; check the line span + // (column 0 is always valid) against the document bounds. + $check = new Range(new Position($range->startLine, 0), new Position($range->endLine, 0)); + $this->world->assert( + $this->world->rangeWithinDocument($path, $check), + sprintf('folding range #%d (%d-%d) out of document bounds', $i, $range->startLine, $range->endLine), + ); + } + } + + /** + * @Then every inlay hint position is within the bounds of :path + */ + public function everyInlayHintPositionIsWithinTheBoundsOf(string $path): void + { + $hints = $this->world->last(); + $this->world->assert(is_array($hints) && $hints !== [], 'expected a non-empty inlay-hint list'); + foreach ($hints as $i => $hint) { + if (!$hint instanceof InlayHint) { + continue; + } + $point = new Range($hint->position, $hint->position); + $this->world->assert( + $this->world->rangeWithinDocument($path, $point), + sprintf('inlay hint #%d position out of document bounds at %d:%d', $i, $hint->position->line, $hint->position->character), + ); + } + } + private function assertHoverContains(string $needle): void { $hover = $this->world->last(); diff --git a/test/Diagnostics/XphpDiagnosticsProviderTest.php b/test/Diagnostics/XphpDiagnosticsProviderTest.php index db7614a..3e0fa26 100644 --- a/test/Diagnostics/XphpDiagnosticsProviderTest.php +++ b/test/Diagnostics/XphpDiagnosticsProviderTest.php @@ -20,6 +20,10 @@ use XPHP\Lsp\Analyzer\WorkspaceAnalyzer; use XPHP\Lsp\Diagnostics\XphpDiagnosticsProvider; use XPHP\Lsp\Reflection\FqnIndex; +use XPHP\Lsp\Resolver\CompositeClassLikeLookup; +use XPHP\Lsp\Resolver\FilesystemClassLikeLookup; +use XPHP\Lsp\Resolver\GenericResolver; +use XPHP\Lsp\Resolver\WorkspaceClassLikeLookup; use XPHP\Lsp\Test\Support\AssertsRangeWithinDocument; use XPHP\Transpiler\Monomorphize\XphpSourceParser; use function Amp\Promise\wait; @@ -46,6 +50,110 @@ class Box { public T $item; } self::assertSame([], $diagnostics); } + public function testNullDerefOnChainedNullableReceiverIsFlagged(): void + { + $workspace = new PhpactorWorkspace(); + $doc = $this->openDoc($workspace, '/Use.xphp', <<<'XPHP' + + { + public function first(): ?T { return null; } + } + $users = new Collection::(); + $name = $users->first()->name; + XPHP); + + $diagnostics = $this->lintWithResolver($workspace, $doc); + + $nullDeref = array_values(array_filter( + $diagnostics, + static fn (LspDiagnostic $d): bool => (string) $d->code === 'xphp.null-deref', + )); + self::assertCount(1, $nullDeref, 'expected exactly one null-deref diagnostic'); + self::assertSame('xphp', $nullDeref[0]->source); + self::assertSame(2, $nullDeref[0]->severity, 'null-deref is a Warning'); + self::assertStringContainsString('name', $nullDeref[0]->message); + self::assertStringContainsString('?App\\User', $nullDeref[0]->message); + // The range must land on the `name` member token, inside the buffer. + self::assertRangeWithinDocument( + $doc->text, + $nullDeref[0]->range->start->line, + $nullDeref[0]->range->start->character, + $nullDeref[0]->range->end->line, + $nullDeref[0]->range->end->character, + ); + self::assertSame(8, $nullDeref[0]->range->start->line); + } + + public function testNullsafeChainIsNotFlagged(): void + { + $workspace = new PhpactorWorkspace(); + $doc = $this->openDoc($workspace, '/Use.xphp', <<<'XPHP' + + { + public function first(): ?T { return null; } + } + $users = new Collection::(); + $name = $users->first()?->name; + XPHP); + + $codes = array_map( + static fn (LspDiagnostic $d): string => (string) $d->code, + $this->lintWithResolver($workspace, $doc), + ); + self::assertNotContains('xphp.null-deref', $codes); + } + + public function testChainThroughNonNullableReceiverIsNotFlagged(): void + { + $workspace = new PhpactorWorkspace(); + $doc = $this->openDoc($workspace, '/Use.xphp', <<<'XPHP' + + { + public function get(): T { return $this->items[0]; } + } + $users = new Collection::(); + $name = $users->get()->name; + XPHP); + + $codes = array_map( + static fn (LspDiagnostic $d): string => (string) $d->code, + $this->lintWithResolver($workspace, $doc), + ); + self::assertNotContains('xphp.null-deref', $codes); + } + + public function testNullDerefIsSkippedWhenNoResolverIsWired(): void + { + $workspace = new PhpactorWorkspace(); + $doc = $this->openDoc($workspace, '/Use.xphp', <<<'XPHP' + + { + public function first(): ?T { return null; } + } + $users = new Collection::(); + $name = $users->first()->name; + XPHP); + + // The default helper builds a provider with NO resolver. + $codes = array_map( + static fn (LspDiagnostic $d): string => (string) $d->code, + $this->lint($workspace, $doc), + ); + self::assertNotContains('xphp.null-deref', $codes); + } + public function testSyntaxErrorYieldsSingleDiagnostic(): void { $workspace = new PhpactorWorkspace(); @@ -705,6 +813,40 @@ private function newProvider(PhpactorWorkspace $workspace, string $rootPath = '' ); } + /** + * Like `lint`, but wires a real `GenericResolver` so the conservative + * null-dereference diagnostic (`xphp.null-deref`) is active. + * + * @return list + */ + private function lintWithResolver(PhpactorWorkspace $workspace, TextDocumentItem $textDocument): array + { + $parser = new XphpSourceParser((new ParserFactory())->createForHostVersion()); + $cache = new ParsedDocumentCache(new Analyzer($parser)); + $fqnIndex = new FqnIndex($workspace, $cache, $parser, ''); + $resolver = new GenericResolver( + $workspace, + $cache, + new CompositeClassLikeLookup( + new WorkspaceClassLikeLookup($workspace, $cache), + new FilesystemClassLikeLookup($fqnIndex), + ), + $parser, + $fqnIndex, + ); + $provider = new XphpDiagnosticsProvider( + $cache, + new WorkspaceAnalyzer(), + $workspace, + $fqnIndex, + null, + $resolver, + ); + $cancel = (new CancellationTokenSource())->getToken(); + $result = wait($provider->provideDiagnostics($textDocument, $cancel)); + return is_array($result) ? array_values($result) : []; + } + private function openDoc(PhpactorWorkspace $workspace, string $uri, string $text): TextDocumentItem { $item = new TextDocumentItem($uri, 'xphp', 1, $text); diff --git a/test/Handler/XphpCodeLensHandlerTest.php b/test/Handler/XphpCodeLensHandlerTest.php index 62a64b1..6195ffb 100644 --- a/test/Handler/XphpCodeLensHandlerTest.php +++ b/test/Handler/XphpCodeLensHandlerTest.php @@ -26,12 +26,15 @@ use XPHP\Lsp\Resolver\GenericResolver; use XPHP\Lsp\Resolver\ReferenceFinder; use XPHP\Lsp\Resolver\WorkspaceClassLikeLookup; +use XPHP\Lsp\Test\Support\AssertsRangeWithinDocument; use XPHP\Transpiler\Monomorphize\XphpSourceParser; use function Amp\Promise\wait; final class XphpCodeLensHandlerTest extends TestCase { + use AssertsRangeWithinDocument; + private string $root; protected function setUp(): void @@ -271,6 +274,50 @@ public function bar(): void {} self::assertSame('2 usages', $resolved->command?->title); } + /** + * W7 invariant: every code-lens `range` must stay within the document. + */ + public function testEmittedLensRangesAreWithinDocumentBounds(): void + { + $source = <<<'PHP' + open(new TextDocumentItem('/Service.xphp', 'xphp', 1, $source)); + + $lenses = wait($this->newHandler($workspace)->codeLens( + new CodeLensParams(new TextDocumentIdentifier('/Service.xphp')), + )); + + self::assertNotEmpty($lenses, 'fixture should produce lenses to check'); + foreach ($lenses as $i => $lens) { + self::assertRangeWithinDocument( + $source, + $lens->range->start->line, + $lens->range->start->character, + $lens->range->end->line, + $lens->range->end->character, + "code lens #{$i}", + ); + } + } + private function newHandler(PhpactorWorkspace $workspace): XphpCodeLensHandler { $parser = new XphpSourceParser((new ParserFactory())->createForHostVersion()); diff --git a/test/Handler/XphpDefinitionHandlerTest.php b/test/Handler/XphpDefinitionHandlerTest.php index d42676f..9715d2f 100644 --- a/test/Handler/XphpDefinitionHandlerTest.php +++ b/test/Handler/XphpDefinitionHandlerTest.php @@ -460,6 +460,44 @@ public function first(): ?T { return null; } self::assertSame(5, $location->range->end->character - $location->range->start->character); } + public function testJumpsThroughAMethodChainToTheFinalMethodDeclaration(): void + { + // Chained-method GTD: the receiver of the clicked `mirror` is itself a + // method call (`$users->first()?->mirror()`), a NON-generic method on + // User -- so this exercises the recursive inferType chain through a + // non-generic mid-hop (W3). + $workspace = new PhpactorWorkspace(); + $workspace->open(new TextDocumentItem('/Collection.xphp', 'xphp', 1, <<<'XPHP' + + { + public function first(): ?T { return null; } + } + XPHP)); + $workspace->open(new TextDocumentItem('/User.xphp', 'xphp', 1, <<<'XPHP' + ();\n\$x = \$users->first()?->mirror()?->mirror();\n"; + $workspace->open(new TextDocumentItem('/Use.xphp', 'xphp', 1, $useSource)); + + // Cursor on the SECOND `mirror` (its receiver is the non-generic + // `$users->first()?->mirror()`). + $byte = strpos($useSource, '?->mirror()?->mirror') + strlen('?->mirror()?->'); + self::assertNotFalse($byte); + $location = $this->definitionAtOffset($this->newHandler($workspace), '/Use.xphp', $useSource, $byte); + + self::assertNotNull($location); + self::assertSame('/User.xphp', $location->uri); + self::assertSame(6, $location->range->end->character - $location->range->start->character); // "mirror" + } + private function newHandler(PhpactorWorkspace $workspace, ?string $rootPath = null): XphpDefinitionHandler { $parser = new XphpSourceParser((new ParserFactory())->createForHostVersion()); diff --git a/test/Handler/XphpDocumentHighlightHandlerTest.php b/test/Handler/XphpDocumentHighlightHandlerTest.php index 203bdba..ba4fcba 100644 --- a/test/Handler/XphpDocumentHighlightHandlerTest.php +++ b/test/Handler/XphpDocumentHighlightHandlerTest.php @@ -26,12 +26,42 @@ use XPHP\Lsp\Resolver\GenericResolver; use XPHP\Lsp\Resolver\ReferenceFinder; use XPHP\Lsp\Resolver\WorkspaceClassLikeLookup; +use XPHP\Lsp\Test\Support\AssertsRangeWithinDocument; use XPHP\Transpiler\Monomorphize\XphpSourceParser; use function Amp\Promise\wait; final class XphpDocumentHighlightHandlerTest extends TestCase { + use AssertsRangeWithinDocument; + + /** + * W7 invariant: every document-highlight `range` must stay within the + * document. The fixture spreads the highlighted symbol across several lines + * so the check exercises ranges beyond line 0. + */ + public function testEmittedHighlightRangesAreWithinDocumentBounds(): void + { + $source = "open(new TextDocumentItem('/Use.xphp', 'xphp', 1, $source)); + $byte = strpos($source, 'class User') + strlen('class '); + + $highlights = $this->highlightsAt($workspace, '/Use.xphp', $source, $byte); + + self::assertNotEmpty($highlights, 'fixture should produce highlights to check'); + foreach ($highlights as $i => $highlight) { + self::assertRangeWithinDocument( + $source, + $highlight->range->start->line, + $highlight->range->start->character, + $highlight->range->end->line, + $highlight->range->end->character, + "document highlight #{$i}", + ); + } + } + public function testHighlightsAllReferencesInCurrentFile(): void { $workspace = new PhpactorWorkspace(); diff --git a/test/Handler/XphpDocumentSymbolHandlerTest.php b/test/Handler/XphpDocumentSymbolHandlerTest.php index 778ceac..2582407 100644 --- a/test/Handler/XphpDocumentSymbolHandlerTest.php +++ b/test/Handler/XphpDocumentSymbolHandlerTest.php @@ -15,12 +15,68 @@ use XPHP\Lsp\Analyzer\Analyzer; use XPHP\Lsp\Analyzer\ParsedDocumentCache; use XPHP\Lsp\Handler\XphpDocumentSymbolHandler; +use XPHP\Lsp\Test\Support\AssertsRangeWithinDocument; use XPHP\Transpiler\Monomorphize\XphpSourceParser; use function Amp\Promise\wait; final class XphpDocumentSymbolHandlerTest extends TestCase { + use AssertsRangeWithinDocument; + + /** + * W7 invariant: every document-symbol `range` AND `selectionRange` (the + * full symbol span and its name token) must stay within the document, at + * every nesting depth (class -> members). + */ + public function testEmittedSymbolRangesAreWithinDocumentBounds(): void + { + $source = <<<'XPHP' + + { + const VERSION = '1'; + + private T[] $items = []; + + public function add(T $item): void + { + $this->items[] = $item; + } + } + + function helper(): void + { + } + XPHP; + + $this->assertSymbolRangesWithinDocument($this->collect($source), $source); + } + + /** + * Recurse `range` + `selectionRange` of each symbol (and its children). + * + * @param list $symbols + */ + private function assertSymbolRangesWithinDocument(array $symbols, string $source): void + { + foreach ($symbols as $symbol) { + foreach (['range' => $symbol->range, 'selectionRange' => $symbol->selectionRange] as $label => $range) { + self::assertRangeWithinDocument( + $source, + $range->start->line, + $range->start->character, + $range->end->line, + $range->end->character, + sprintf('%s of "%s"', $label, $symbol->name), + ); + } + $this->assertSymbolRangesWithinDocument($symbol->children ?? [], $source); + } + } + public function testEmitsClassWithMethodsAndPropertiesAndConstants(): void { $symbols = $this->collect(<<<'XPHP' diff --git a/test/Handler/XphpFoldingRangeHandlerTest.php b/test/Handler/XphpFoldingRangeHandlerTest.php index 2418e92..dfae134 100644 --- a/test/Handler/XphpFoldingRangeHandlerTest.php +++ b/test/Handler/XphpFoldingRangeHandlerTest.php @@ -15,12 +15,53 @@ use XPHP\Lsp\Analyzer\Analyzer; use XPHP\Lsp\Analyzer\ParsedDocumentCache; use XPHP\Lsp\Handler\XphpFoldingRangeHandler; +use XPHP\Lsp\Test\Support\AssertsRangeWithinDocument; use XPHP\Transpiler\Monomorphize\XphpSourceParser; use function Amp\Promise\wait; final class XphpFoldingRangeHandlerTest extends TestCase { + use AssertsRangeWithinDocument; + + /** + * W7 invariant: every folding range must stay within the document. Folding + * ranges carry only line numbers (no characters), so we assert the line + * span against the document bounds (the trait also enforces endLine >= + * startLine). + */ + public function testEmittedFoldingRangesAreWithinDocumentBounds(): void + { + $source = <<<'XPHP' + implements Shape + { + public function area(): float + { + return 0.0; + } + } + XPHP; + + foreach ($this->foldingRangesFor($source) as $i => $range) { + self::assertRangeWithinDocument( + $source, + $range->startLine, + 0, + $range->endLine, + 0, + "folding range #{$i}", + ); + } + } + public function testFoldsMultiLineClassBodyAndMethodBodies(): void { $source = <<<'XPHP' diff --git a/test/Handler/XphpInlayHintHandlerTest.php b/test/Handler/XphpInlayHintHandlerTest.php index 0ec4f3b..5ffc010 100644 --- a/test/Handler/XphpInlayHintHandlerTest.php +++ b/test/Handler/XphpInlayHintHandlerTest.php @@ -23,12 +23,60 @@ use XPHP\Lsp\Resolver\FilesystemClassLikeLookup; use XPHP\Lsp\Resolver\GenericResolver; use XPHP\Lsp\Resolver\WorkspaceClassLikeLookup; +use XPHP\Lsp\Test\Support\AssertsRangeWithinDocument; use XPHP\Transpiler\Monomorphize\XphpSourceParser; use function Amp\Promise\wait; final class XphpInlayHintHandlerTest extends TestCase { + use AssertsRangeWithinDocument; + + /** + * W7 invariant: every inlay-hint anchor `position` must sit within the + * document. A hint carries a point (not a range), so we check it as a + * zero-width range. + */ + public function testEmittedHintPositionsAreWithinDocumentBounds(): void + { + $workspace = new PhpactorWorkspace(); + $workspace->open(new TextDocumentItem( + '/Collection.xphp', + 'xphp', + 1, + <<<'XPHP' + + { + public function first(): ?T { return null; } + } + XPHP, + )); + $workspace->open(new TextDocumentItem( + '/User.xphp', + 'xphp', + 1, + "();\n\$first = \$users->first();\n"; + $workspace->open(new TextDocumentItem('/Use.xphp', 'xphp', 1, $useSource)); + + $hints = $this->hintsFor($workspace, '/Use.xphp'); + + self::assertNotEmpty($hints, 'fixture should produce a hint to check'); + foreach ($hints as $i => $hint) { + self::assertRangeWithinDocument( + $useSource, + $hint->position->line, + $hint->position->character, + $hint->position->line, + $hint->position->character, + "inlay hint #{$i}", + ); + } + } + public function testEmitsSubstitutedReturnTypeForGenericMethodCall(): void { $workspace = new PhpactorWorkspace(); diff --git a/test/Handler/XphpSemanticTokensHandlerTest.php b/test/Handler/XphpSemanticTokensHandlerTest.php index 8a50928..5661509 100644 --- a/test/Handler/XphpSemanticTokensHandlerTest.php +++ b/test/Handler/XphpSemanticTokensHandlerTest.php @@ -14,12 +14,88 @@ use XPHP\Lsp\Analyzer\ParsedDocumentCache; use XPHP\Lsp\Handler\SemanticTokens\TokenLegend; use XPHP\Lsp\Handler\XphpSemanticTokensHandler; +use XPHP\Lsp\Test\Support\AssertsRangeWithinDocument; use XPHP\Transpiler\Monomorphize\XphpSourceParser; use function Amp\Promise\wait; final class XphpSemanticTokensHandlerTest extends TestCase { + use AssertsRangeWithinDocument; + + /** + * W7 invariant: every semantic token the handler emits must sit inside the + * document. A strict client annotator (PhpStorm) throws "Range must be + * inside element being annotated" otherwise. Tokens are delta-encoded and + * single-line (the builder splits multi-line tokens one-per-line), so each + * decodes to `(line, char)..(line, char + length)`. + */ + public function testEmittedTokensAreWithinDocumentBounds(): void + { + $source = <<<'XPHP' + + { + public T $first; + + public function map(callable $fn): Collection + { + // a comment spanning the method body + return $this; + } + } + + $users = new Collection::(); + $mapped = $users->map(fn ($u) => $u); + XPHP; + $workspace = new PhpactorWorkspace(); + $workspace->open(new TextDocumentItem('/bounds.xphp', 'xphp', 1, $source)); + + $result = wait($this->newHandler($workspace)->semanticTokensFull(['uri' => '/bounds.xphp'])); + + self::assertInstanceOf(SemanticTokens::class, $result); + self::assertNotEmpty($result->data, 'fixture should produce tokens to check'); + foreach (self::decodeTokens($result->data) as $i => $token) { + self::assertRangeWithinDocument( + $source, + $token['line'], + $token['char'], + $token['line'], + $token['char'] + $token['length'], + "semantic token #{$i}", + ); + } + } + + /** + * Decode the delta-encoded `data` stream (5 ints per token: deltaLine, + * deltaChar, length, typeIndex, modifierMask) into absolute positions. + * + * @param list $data + * @return list + */ + private static function decodeTokens(array $data): array + { + $out = []; + $line = 0; + $char = 0; + for ($i = 0; $i + 5 <= count($data); $i += 5) { + [$deltaLine, $deltaChar, $length] = array_slice($data, $i, 3); + if ($deltaLine > 0) { + $line += $deltaLine; + $char = $deltaChar; + } else { + $char += $deltaChar; + } + $out[] = ['line' => $line, 'char' => $char, 'length' => $length]; + } + return $out; + } + public function testCapabilityAdvertisesLegendAndFullSupport(): void { $handler = $this->newHandler(new PhpactorWorkspace()); diff --git a/test/Resolver/CompositeClassLikeLookupTest.php b/test/Resolver/CompositeClassLikeLookupTest.php index 9795310..a7860e2 100644 --- a/test/Resolver/CompositeClassLikeLookupTest.php +++ b/test/Resolver/CompositeClassLikeLookupTest.php @@ -8,6 +8,7 @@ use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\ClassLike; use PHPUnit\Framework\TestCase; +use XPHP\Lsp\Resolver\ClassLikeContext; use XPHP\Lsp\Resolver\ClassLikeLookup; use XPHP\Lsp\Resolver\CompositeClassLikeLookup; @@ -42,6 +43,21 @@ public function testEmptyChainReturnsNull(): void $lookup = new CompositeClassLikeLookup(); self::assertNull($lookup->find('Anything')); } + + public function testFindWithContextDelegatesInChainOrder(): void + { + $first = new Class_(new Identifier('First')); + $second = new Class_(new Identifier('Second')); + + $lookup = new CompositeClassLikeLookup( + new FixedLookup(['Foo' => $first]), + new FixedLookup(['Foo' => $second, 'Bar' => $second]), + ); + + self::assertSame($first, $lookup->findWithContext('Foo')?->classLike, 'first lookup wins'); + self::assertSame($second, $lookup->findWithContext('Bar')?->classLike, 'second serves uncovered'); + self::assertNull($lookup->findWithContext('Nothing')); + } } final class FixedLookup implements ClassLikeLookup @@ -55,4 +71,10 @@ public function find(string $fqn): ?ClassLike { return $this->entries[$fqn] ?? null; } + + public function findWithContext(string $fqn): ?ClassLikeContext + { + $hit = $this->entries[$fqn] ?? null; + return $hit === null ? null : new ClassLikeContext($hit, [], ''); + } } diff --git a/test/Resolver/GenericResolverTest.php b/test/Resolver/GenericResolverTest.php index 9297a81..e65acb7 100644 --- a/test/Resolver/GenericResolverTest.php +++ b/test/Resolver/GenericResolverTest.php @@ -40,6 +40,241 @@ public function testSubstitutesNullableTypeParamFromGenericMethodCall(): void self::assertSame('?App\\Models\\User', $resolver->resolveVariable('/Use.xphp', 'user', PHP_INT_MAX)); } + public function testNullsafePropertyFetchOnGenericResultIsNullable(): void + { + // `$users->first()?->name` where `first(): ?T` (=> ?User) and + // `User::$name: string`. The nullsafe `?->` short-circuits to null, + // so the result type is `?string`, NOT `string`. + $workspace = $this->workspace(); + $this->openCollection($workspace, returnType: '?T'); + $this->open($workspace, '/User.xphp', "open($workspace, '/Use.xphp', <<<'XPHP' + (); + $firstName = $users->first()?->name; + XPHP); + + $resolver = $this->resolver($workspace); + + self::assertSame('?string', $resolver->resolveVariable('/Use.xphp', 'firstName', PHP_INT_MAX)); + } + + public function testNonNullsafePropertyFetchKeepsDeclaredNullability(): void + { + // A regular `->` access does NOT widen to nullable just because the + // property's host is reached through a generic: `$pair->first` where + // `first: A` (= Plastic, non-nullable) stays `App\Models\Plastic`. + $workspace = $this->workspace(); + $this->open($workspace, '/Plastic.xphp', "open($workspace, '/User.xphp', "open($workspace, '/Pair.xphp', <<<'XPHP' + { + public function __construct(public A $first, public B $second) {} + } + XPHP); + $this->open($workspace, '/Use.xphp', <<<'XPHP' + (new Plastic(), new User()); + $f = $pair->first; + XPHP); + + $resolver = $this->resolver($workspace); + + self::assertSame('App\\Models\\Plastic', $resolver->resolveVariable('/Use.xphp', 'f', PHP_INT_MAX)); + } + + public function testDeepNullsafePropertyChainPropagatesNullability(): void + { + // Two-hop chain: `$users->first()?->bestFriend?->name`. The receiver of + // the final `?->name` is itself a (nullsafe) property fetch, which + // requires inferType to recurse into property-fetch receivers. + // `bestFriend: ?User` lives on a non-generic User, so this also pins + // the non-generic-intermediate pass-through. Result: `?string`. + $workspace = $this->workspace(); + $this->openCollection($workspace, returnType: '?T'); + $this->open($workspace, '/User.xphp', "open($workspace, '/Use.xphp', <<<'XPHP' + (); + $deep = $users->first()?->bestFriend?->name; + XPHP); + + $resolver = $this->resolver($workspace); + + self::assertSame('?string', $resolver->resolveVariable('/Use.xphp', 'deep', PHP_INT_MAX)); + } + + public function testNullsafeClassPropertyResolvesToQualifiedFqn(): void + { + // A bare class-typed property (`?User`) now resolves to the qualified + // FQN (via qualifyAgainstNamespace), not the unqualified `?User` -- + // this is what lets the next chain hop find the class. + $workspace = $this->workspace(); + $this->openCollection($workspace, returnType: '?T'); + $this->open($workspace, '/User.xphp', "open($workspace, '/Use.xphp', <<<'XPHP' + (); + $bf = $users->first()?->bestFriend; + XPHP); + + $resolver = $this->resolver($workspace); + + self::assertSame('?App\\Models\\User', $resolver->resolveVariable('/Use.xphp', 'bf', PHP_INT_MAX)); + } + + public function testNonGenericMethodMidChainResolves(): void + { + // Gap 2: `$users->first()?->self()?->name` where `self(): ?User` is a + // plain method on the non-generic `User`. Previously resolveMethodCall + // bailed on a non-generic receiver (empty paramMap) and the chain died; + // now it owns a confirmable-class return so the chain continues. + $workspace = $this->workspace(); + $this->openCollection($workspace, returnType: '?T'); + $this->open($workspace, '/User.xphp', "open($workspace, '/Use.xphp', <<<'XPHP' + (); + $n = $users->first()?->self()?->name; + XPHP); + + $resolver = $this->resolver($workspace); + + self::assertSame('?string', $resolver->resolveVariable('/Use.xphp', 'n', PHP_INT_MAX)); + } + + public function testNonGenericScalarMethodCallDefersToWorseReflection(): void + { + // Gate: a non-generic method returning a SCALAR is NOT owned by the + // resolver (returns null) -- worse-reflection keeps its richer view of + // plain method returns. Locks the conservative Gap-2 gate. + $workspace = $this->workspace(); + $this->openCollection($workspace, returnType: '?T'); + $this->open($workspace, '/User.xphp', "open($workspace, '/Use.xphp', <<<'XPHP' + (); + $g = $users->first()?->getName(); + XPHP); + + $resolver = $this->resolver($workspace); + + self::assertNull($resolver->resolveVariable('/Use.xphp', 'g', PHP_INT_MAX)); + } + + public function testCrossNamespaceImportedPropertyTypeResolvesInChain(): void + { + // The intermediate property `profile` is typed with a class imported + // from ANOTHER namespace (`use App\Other\Profile`). Resolving the chain + // requires the DECLARING file's use-map, not just its namespace -- + // same-namespace-only qualification would mis-resolve to + // `App\Models\Profile` (nonexistent) and the chain would collapse. + $workspace = $this->workspace(); + $this->openCollection($workspace, returnType: '?T'); + $this->open($workspace, '/User.xphp', "open($workspace, '/Profile.xphp', "open($workspace, '/Use.xphp', <<<'XPHP' + (); + $bio = $users->first()?->profile?->bio; + XPHP); + + $resolver = $this->resolver($workspace); + + self::assertSame('?string', $resolver->resolveVariable('/Use.xphp', 'bio', PHP_INT_MAX)); + } + + public function testCrossNamespaceImportedPropertyTypeRendersQualifiedFqn(): void + { + // Terminal case: the imported intermediate type itself renders as its + // real cross-namespace FQN (App\Other\Profile), not App\Models\Profile. + $workspace = $this->workspace(); + $this->openCollection($workspace, returnType: '?T'); + $this->open($workspace, '/User.xphp', "open($workspace, '/Profile.xphp', "open($workspace, '/Use.xphp', <<<'XPHP' + (); + $p = $users->first()?->profile; + XPHP); + + $resolver = $this->resolver($workspace); + + self::assertSame('?App\\Other\\Profile', $resolver->resolveVariable('/Use.xphp', 'p', PHP_INT_MAX)); + } + + public function testTripleHopNullsafePropertyChainResolves(): void + { + // Three hops past the method call -- proves the recursion isn't + // limited to a single property-fetch receiver. + $workspace = $this->workspace(); + $this->openCollection($workspace, returnType: '?T'); + $this->open($workspace, '/User.xphp', "open($workspace, '/Use.xphp', <<<'XPHP' + (); + $deep = $users->first()?->bestFriend?->bestFriend?->name; + XPHP); + + $resolver = $this->resolver($workspace); + + self::assertSame('?string', $resolver->resolveVariable('/Use.xphp', 'deep', PHP_INT_MAX)); + } + + public function testDeepPropertyChainWithUnionIntermediateStaysNull(): void + { + // Safe degradation: an intermediate property with a union type isn't + // modelable, so the chain collapses to null (defer to worse-reflection) + // rather than producing a wrong type. + $workspace = $this->workspace(); + $this->openCollection($workspace, returnType: '?T'); + $this->open($workspace, '/User.xphp', "open($workspace, '/Use.xphp', <<<'XPHP' + (); + $x = $users->first()?->thing?->name; + XPHP); + + $resolver = $this->resolver($workspace); + + self::assertNull($resolver->resolveVariable('/Use.xphp', 'x', PHP_INT_MAX)); + } + + public function testThisPropertyChainDegradesToNull(): void + { + // `$this` with no binding (top-level) must not crash or mis-resolve a + // property chain rooted at it. + $workspace = $this->workspace(); + $this->open($workspace, '/Use.xphp', "prop?->name;\n"); + + $resolver = $this->resolver($workspace); + + self::assertNull($resolver->resolveVariable('/Use.xphp', 'x', PHP_INT_MAX)); + } + public function testRendersReceiverVariableWithTypeArgList(): void { // Hovering the receiver itself benefits too: `$users` of type