diff --git a/NEWS.md b/NEWS.md index b6a9f91656..90c6fec6b8 100644 --- a/NEWS.md +++ b/NEWS.md @@ -26,6 +26,7 @@ Language Server/Daemon mode: + Catch exception seen when printing debug info about not being able to parse a file. + Warn when Phan's language server dependencies were installed for php 7.2+ but the language server gets run in php 7.1. (phpdocumentor/reflection-docblock 5.0 requires php 7.2) ++ Immediately return cached hover text when the client repeats an identical hover request. (#3252) Miscellaneous: + PHP 8.0-dev compatibility fixes, analysis for some new functions of PHP 8.0-dev. diff --git a/src/Phan/LanguageServer/CachedHoverResponse.php b/src/Phan/LanguageServer/CachedHoverResponse.php new file mode 100644 index 0000000000..dc27139a13 --- /dev/null +++ b/src/Phan/LanguageServer/CachedHoverResponse.php @@ -0,0 +1,50 @@ +uri = $uri; + $this->position = $position; + $this->file_mapping_hash = $file_mapping->getHash(); + $this->hover = $hover; + } + + /** + * Checks if this is effectively the same request as the previous request + */ + public function isSameRequest(string $uri, Position $position, FileMapping $file_mapping): bool + { + return $this->uri === $uri && $this->position->compare($position) === 0 && $this->file_mapping_hash === $file_mapping->getHash(); + } + + /** + * Get the hover response data or null. + * Callers should check that isSameRequest() is true first. + */ + public function getHoverResponse(): ?Hover + { + return $this->hover; + } +} diff --git a/src/Phan/LanguageServer/FileMapping.php b/src/Phan/LanguageServer/FileMapping.php index 13895e6ea9..748305c1d1 100644 --- a/src/Phan/LanguageServer/FileMapping.php +++ b/src/Phan/LanguageServer/FileMapping.php @@ -39,6 +39,12 @@ public function getOverrides(): array return $this->overrides; } + public function getHash(): string + { + \uksort($this->overrides, 'strcmp'); + return \sha1(\var_export($this->overrides, true)); + } + public function addOverrideURI(string $uri, ?string $new_contents): void { $path = Utils::uriToPath($uri); diff --git a/src/Phan/LanguageServer/GoToDefinitionRequest.php b/src/Phan/LanguageServer/GoToDefinitionRequest.php index 2ddd238a9f..c9a300919e 100644 --- a/src/Phan/LanguageServer/GoToDefinitionRequest.php +++ b/src/Phan/LanguageServer/GoToDefinitionRequest.php @@ -362,12 +362,13 @@ public function getHoverResponse(): ?Hover * * @param ?Hover|?array $hover */ - public function setHoverResponse($hover): void + public function setHoverResponse($hover): ?Hover { if (is_array($hover)) { $hover = Hover::fromArray($hover); } $this->hover_response = $hover; + return $hover; } /** diff --git a/src/Phan/LanguageServer/LanguageServer.php b/src/Phan/LanguageServer/LanguageServer.php index df8614b66f..fb7a0e8628 100644 --- a/src/Phan/LanguageServer/LanguageServer.php +++ b/src/Phan/LanguageServer/LanguageServer.php @@ -161,12 +161,18 @@ class LanguageServer extends AdvancedJsonRpc\Dispatcher /** * @var ?NodeInfoRequest * - * Contains the promise for the most recent "Go to definition" request + * Contains the promise for the most recent "Go to definition" request. * If more than one such request exists, the earlier requests will be discarded. * * TODO: Will need to Resolve(null) for the older requests. */ protected $most_recent_node_info_request = null; + /** + * @var ?CachedHoverResponse + * + * Contains the result of the previous hover response and information used to validate the cached information + */ + protected $cached_hover_response = null; /** * Constructs the only instance of the language server @@ -445,6 +451,8 @@ public function analyzeURIAsync(string $uri): void $path_to_analyze = Utils::uriToPath($uri); Logger::logInfo("Called analyzeURIAsync, uri=$uri, path=$path_to_analyze"); $this->analyze_request_set[$path_to_analyze] = $uri; + // Clear the cached hover response - a file was probably opened, modified, or saved/closed. + $this->cached_hover_response = null; // Don't call file_path_lister immediately - // That has to walk the directories in .phan/config.php to see if the requested path is included and not excluded. } @@ -478,19 +486,24 @@ public function awaitDefinition( /** * Asynchronously generates the hover text for a given URL and position. * - * @return Promise + * @return Promise */ public function awaitHover( string $uri, Position $position ): Promise { + MarkupDescription::eagerlyLoadAllDescriptionMaps(); // TODO: Add a way to "go to definition" without emitting analysis results as a side effect $path_to_analyze = Utils::uriToPath($uri); Logger::logInfo("Called LanguageServer->awaitHover, uri=$uri, position=" . StringUtil::jsonEncode($position)); - $this->discardPreviousNodeInfoRequest(); $request = new GoToDefinitionRequest($uri, $position, GoToDefinitionRequest::REQUEST_HOVER); + $this->discardPreviousNodeInfoRequest(); $this->most_recent_node_info_request = $request; - MarkupDescription::eagerlyLoadAllDescriptionMaps(); + $early_response = $this->generateQuickHoverResponse($request); + if ($early_response) { + return $early_response; + } + $this->cached_hover_response = null; // We analyze this url so that Phan is aware enough of the types and namespace maps to trigger "Go to definition" // E.g. going to the definition of `Bar` in `use Foo as Bar; Bar::method();` requires parsing other statements in this file, not just the name in question. @@ -500,6 +513,29 @@ public function awaitHover( return $request->getPromise(); } + /** + * Immediately generates the hover text for a given URL and position. + * + * @return ?Promise + */ + private function generateQuickHoverResponse(GoToDefinitionRequest $request): ?Promise + { + $description = "uri={$request->getUrl()}, position=" . StringUtil::jsonEncode($request->getPosition()); + Logger::logInfo("Looking for quick hover response for $description"); + if (!$this->cached_hover_response) { + return null; + } + if (!$this->cached_hover_response->isSameRequest($request->getUrl(), $request->getPosition(), $this->file_mapping)) { + return null; + } + $promise = new Promise(); + $cached_response = $this->cached_hover_response->getHoverResponse(); + Logger::logInfo("Returning cached hover response for $description"); + + $promise->fulfill($cached_response); + return $promise; + } + /** * Asynchronously generates the definition for a given URL * @return Promise @@ -767,7 +803,16 @@ private function handleJSONResponseFromWorker(array $uris_to_analyze, array $res if ($most_recent_node_info_request instanceof GoToDefinitionRequest) { // @phan-suppress-next-line PhanPartialTypeMismatchArgument, PhanTypeMismatchArgumentNullable $most_recent_node_info_request->recordDefinitionLocationList($response_data['definitions'] ?? null); - $most_recent_node_info_request->setHoverResponse($response_data['hover_response'] ?? null); + if ($most_recent_node_info_request->isHoverRequest()) { + $normalized_hover = $most_recent_node_info_request->setHoverResponse($response_data['hover_response'] ?? null); + // \fwrite(\STDERR, "Creating cached_hover_response\n"); + $this->cached_hover_response = new CachedHoverResponse( + $most_recent_node_info_request->getUrl(), + $most_recent_node_info_request->getPosition(), + $this->file_mapping, + $normalized_hover + ); + } } elseif ($most_recent_node_info_request instanceof CompletionRequest) { $most_recent_node_info_request->recordCompletionList($response_data['completions'] ?? null); } diff --git a/src/Phan/LanguageServer/Protocol/Position.php b/src/Phan/LanguageServer/Protocol/Position.php index b4900a9740..ea6ae6de54 100644 --- a/src/Phan/LanguageServer/Protocol/Position.php +++ b/src/Phan/LanguageServer/Protocol/Position.php @@ -46,10 +46,6 @@ public function __construct(int $line = null, int $character = null) */ public function compare(Position $position): int { - if ($this->line === $position->line && $this->character === $position->character) { - return 0; - } - if ($this->line !== $position->line) { return $this->line - $position->line; } diff --git a/tests/Phan/LanguageServer/LanguageServerIntegrationTest.php b/tests/Phan/LanguageServer/LanguageServerIntegrationTest.php index 6f796f6a65..f04f72152d 100644 --- a/tests/Phan/LanguageServer/LanguageServerIntegrationTest.php +++ b/tests/Phan/LanguageServer/LanguageServerIntegrationTest.php @@ -1349,8 +1349,8 @@ public function runTestHoverInOtherFileWithPcntlSetting( // Request the definition of the class "MyExample" with the cursor in the middle of that word // NOTE: Line numbers are 0-based for Position - $perform_hover_request = /** @return array */ function () use ($proc_in, $proc_out, $position, $requested_uri): array { - return $this->writeHoverRequestAndAwaitResponse($proc_in, $proc_out, $position, $requested_uri); + $perform_hover_request = /** @return array */ function (bool $is_repeated = false) use ($proc_in, $proc_out, $position, $requested_uri): array { + return $this->writeHoverRequestAndAwaitResponse($proc_in, $proc_out, $position, $requested_uri, $is_repeated); }; $hover_response = $perform_hover_request(); @@ -1380,15 +1380,16 @@ public function runTestHoverInOtherFileWithPcntlSetting( (string)\json_encode($cur_line), (string)\substr($cur_line, $position->character, 10) ); - $this->assertSame($expected_hover_response, $hover_response, $message); // slightly better diff view than assertSame + $this->assertEquals($expected_hover_response, $hover_response, $message); // slightly better diff view than assertSame $this->assertSame($expected_hover_response, $hover_response, $message); // This operation should be idempotent. // If it's repeated, it should give the same response // (and it shouldn't crash the server) + // (and it should avoid repeating the analysis step) $expected_hover_response['id'] = 3; - $hover_response = $perform_hover_request(); + $hover_response = $perform_hover_request(true); $this->assertSame($expected_hover_response, $hover_response, $message); // slightly better diff view than assertSame $this->assertSame($expected_hover_response, $hover_response, $message); @@ -1827,7 +1828,7 @@ private function writeTypeDefinitionRequestAndAwaitResponse($proc_in, $proc_out, * @return array the response of the server to the hover request * @throws InvalidArgumentException */ - private function writeHoverRequestAndAwaitResponse($proc_in, $proc_out, Position $position, string $requested_uri = null): array + private function writeHoverRequestAndAwaitResponse($proc_in, $proc_out, Position $position, string $requested_uri = null, bool $is_repeated = false): array { $requested_uri = $requested_uri ?? self::getDefaultFileURI(); // Implementation detail: We simultaneously emit a notification with new diagnostics @@ -1839,7 +1840,7 @@ private function writeHoverRequestAndAwaitResponse($proc_in, $proc_out, Position 'position' => $position, ]; $this->writeMessage($proc_in, 'textDocument/hover', $params); - if (self::shouldExpectDiagnosticNotificationForURI($requested_uri)) { + if (!$is_repeated && self::shouldExpectDiagnosticNotificationForURI($requested_uri)) { $this->assertHasEmptyPublishDiagnosticsNotification($proc_out, $requested_uri); }