Skip to content

Commit

Permalink
Return None instead of "" for empty hover results
Browse files Browse the repository at this point in the history
Summary:
Empty hover results have historically been returning the empty string. This is ok for the UX and our telemetry metrics, but not ideal for chronicle metrics as they count this as a successful response. By sending `null` instead of the empty string, we fix  these metrics.

See the [LSP spec](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#languageServerProtocol) for more details.

The special casing in pyre_language_server is necessary because my testing of [exclude](lidatong/dataclasses-json#187) seems to not work. It still encodes as {} instead of `None`.

| Why not make an alias for `LspHoverResponse` = `NonEmptyLspHoverResponse | None`?
This isn't ideal for usability since our static method `from_pyre_hover_responses` would be on the `NonEmptyLspHoverResponse`.

Reviewed By: grievejia

Differential Revision: D48253814

fbshipit-source-id: 358ae59e3172dd7566e4806730a0cee72b0746d5
  • Loading branch information
kinto0 authored and facebook-github-bot committed Aug 12, 2023
1 parent 1afca29 commit e21527d
Show file tree
Hide file tree
Showing 6 changed files with 48 additions and 23 deletions.
2 changes: 1 addition & 1 deletion client/commands/daemon_querier.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class GetDefinitionLocationsResponse:
@dataclasses.dataclass(frozen=True)
class GetHoverResponse:
source: DaemonQuerierSource
data: lsp.LspHoverResponse
data: Optional[lsp.LspHoverResponse]


def file_not_typechecked_coverage_result() -> lsp.TypeCoverageResponse:
Expand Down
12 changes: 8 additions & 4 deletions client/commands/pyre_language_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,7 @@ async def process_hover_request(
json_rpc.SuccessResponse(
id=request_id,
activity_key=activity_key,
result=lsp.LspHoverResponse.empty().to_dict(),
result=None,
),
)
daemon_status_before = self.server_state.status_tracker.get_status()
Expand All @@ -636,10 +636,14 @@ async def process_hover_request(
raw_result = None
empty = True
else:
raw_result = lsp.LspHoverResponse.cached_schema().dump(
result.data,
empty = result.data is None
raw_result = (
None
if empty
else lsp.LspHoverResponse.cached_schema().dump(
result.data,
)
)
empty = len(result.data.contents) == 0

await lsp.write_json_rpc(
self.output_channel,
Expand Down
4 changes: 1 addition & 3 deletions client/commands/tests/language_server_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1887,9 +1887,7 @@ async def test_hover__unopened(self) -> None:
output_writer,
[
self._expect_success_message(
result=lsp.LspHoverResponse.cached_schema().dump(
lsp.LspHoverResponse.empty()
),
result=None,
)
],
)
Expand Down
23 changes: 14 additions & 9 deletions client/language_server/protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -563,17 +563,20 @@ class LspHoverResponse(json_mixins.CamlCaseAndExcludeJsonMixin):

contents: str

@staticmethod
def empty() -> "LspHoverResponse":
return LspHoverResponse(contents="")

@staticmethod
def from_pyre_hover_responses(
responses: List["PyreHoverResponse"],
) -> "LspHoverResponse":
return LspHoverResponse(
"\n".join(
[response.to_lsp_hover_response().contents for response in responses]
) -> Optional["LspHoverResponse"]:
lsp_hover_responses = [
lsp_response
for lsp_response in (hover.to_lsp_hover_response() for hover in responses)
if lsp_response is not None
]
return (
None
if len(lsp_hover_responses) == 0
else LspHoverResponse(
"\n".join(response.contents for response in lsp_hover_responses)
)
)

Expand All @@ -584,12 +587,14 @@ class PyreHoverResponse(json_mixins.CamlCaseAndExcludeJsonMixin):
value: Optional[str] = None
docstring: Optional[str] = None

def to_lsp_hover_response(self) -> LspHoverResponse:
def to_lsp_hover_response(self) -> Optional[LspHoverResponse]:
lines = []
if self.value:
lines.append(f"```\n{self.value}\n```" if self.value else "")
if self.docstring:
lines.append(self.docstring)
if len(lines) == 0:
return None
return LspHoverResponse("\n".join(lines))


Expand Down
8 changes: 4 additions & 4 deletions client/language_server/remote_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

import abc
from pathlib import Path
from typing import List
from typing import List, Optional

from . import protocol as lsp

Expand All @@ -30,7 +30,7 @@ async def definition(
@abc.abstractmethod
async def hover(
self, path: Path, position: lsp.PyrePosition
) -> lsp.LspHoverResponse:
) -> Optional[lsp.LspHoverResponse]:
raise NotImplementedError()

@abc.abstractmethod
Expand Down Expand Up @@ -71,8 +71,8 @@ async def references(

async def hover(
self, path: Path, position: lsp.PyrePosition
) -> lsp.LspHoverResponse:
return lsp.LspHoverResponse.empty()
) -> Optional[lsp.LspHoverResponse]:
return None

async def prepare_call_hierarchy(
self,
Expand Down
22 changes: 20 additions & 2 deletions client/language_server/tests/protocol_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
Info,
InitializationOptions,
InitializeParameters,
LspHoverResponse,
PublishDiagnosticsClientCapabilities,
PublishDiagnosticsClientTagSupport,
PyreHoverResponse,
Expand Down Expand Up @@ -185,12 +186,29 @@ class LSPHoverResponseTest(testslide.TestCase):
def test_to_lsp_response(self) -> None:
result = PyreHoverResponse("type", "docstring").to_lsp_hover_response()
expected = "```\ntype\n```\ndocstring"
self.assertIsNotNone(result)
self.assertEqual(result.contents, expected)

def test_to_lsp_response_empty(self) -> None:
result = PyreHoverResponse().to_lsp_hover_response()
expected = ""
self.assertEqual(result.contents, expected)
self.assertIsNone(result)

def test_from_pyre_hover_responses_all_empty(self) -> None:
self.assertIsNone(
LspHoverResponse.from_pyre_hover_responses(
[PyreHoverResponse(), PyreHoverResponse()]
)
)

def test_from_pyre_hover_responses_non_empty(self) -> None:
actual = LspHoverResponse.from_pyre_hover_responses(
[
PyreHoverResponse(),
PyreHoverResponse("type", "docstring"),
]
)
expected = LspHoverResponse("```\ntype\n```\ndocstring")
self.assertEqual(actual, expected)


class LSPParsingTest(testslide.TestCase):
Expand Down

0 comments on commit e21527d

Please sign in to comment.