Permalink
Browse files

Format position of character according to LSP

Summary:
Changes have been made earlier to ensure that Nuclide was sending the correct position according to LSP - this means one position immediately to the right of the character typed.

The Hack server was modified to account for versions of Nuclide that were sending the correct position and versions that weren't, and to send the correct position regardless.

Recently, we found that the check we placed in the Hack server still ended up giving the wrong position for certain formatting cases (T30616028).

However, this also means everyone's version of Nuclide should be up to date now and that as described in D8227541, it is now time to remove the check so that we only return the previous position (AKA the position of the character itself).

Reviewed By: arxanas

Differential Revision: D8580705

fbshipit-source-id: f98ef37561d3fc44294049d0d7cfc680e8a9baab
  • Loading branch information...
ConderS authored and fredemmott committed Jun 25, 2018
1 parent e539f32 commit 15109e45914442fcd28d57b0061724d6f1f2c3c6
@@ -1295,21 +1295,26 @@ let do_documentOnTypeFormatting
: DocumentOnTypeFormatting.result =
let open DocumentOnTypeFormatting in
let open TextDocumentIdentifier in
let fixup_position position =
(* temporary workaround for T29372533: Nuclide points at the trigger character...
Temporarily checks if current position points to trigger character,
otherwise returns the previous position
*)
let uri = params.textDocument.uri in
let lsp_doc = SMap.get uri editor_open_files in
let open Lsp.TextDocumentItem in
let content = Option.value_map ~default:"" ~f:(fun item -> item.text) lsp_doc in
let current_char = Lsp_helpers.get_char_from_lsp_position content position in
let prev_position = {position with character = position.character - 1}
in
if ((current_char = ';') || (current_char = '}')) then position
else prev_position
in
(*
In LSP, positions do not point directly to characters, but to spaces in between characters.
Thus, the LSP position that the cursor points to after typing a character is the space
immediately after the character.
For example:
Character positions: 0 1 2 3 4 5 6
f o o ( ) { }
LSP positions: 0 1 2 3 4 5 6 7
The cursor is at LSP position 7 after typing the "}" of "foo(){}"
But the character position of "}" is 6.
Nuclide currently sends positions according to LSP, but everything else in the server
and in hack formatting assumes that positions point directly to characters.
Thus, to send the position of the character itself for formatting,
we must subtract one.
*)
let fixup_position position = {position with character = position.character - 1} in
let action = ServerFormatTypes.Position
{ Ide_api_types.
filename = lsp_uri_to_path params.textDocument.uri;
@@ -0,0 +1,115 @@
[
{
"jsonrpc": "2.0",
"id": 1,
"result": {
"capabilities": {
"textDocumentSync": {
"openClose": true,
"change": 2,
"willSave": false,
"willSaveWaitUntil": false,
"save": {
"includeText": false
}
},
"hoverProvider": true,
"completionProvider": {
"resolveProvider": true,
"triggerCharacters": [
"$",
">",
"\\",
":",
"<"
]
},
"signatureHelpProvider": {
"triggerCharacters": [
"(",
","
]
},
"definitionProvider": true,
"referencesProvider": true,
"documentHighlightProvider": true,
"documentSymbolProvider": true,
"workspaceSymbolProvider": true,
"codeActionProvider": false,
"documentFormattingProvider": true,
"documentRangeFormattingProvider": true,
"documentOnTypeFormattingProvider": {
"firstTriggerCharacter": ";",
"moreTriggerCharacter": [
"}"
]
},
"renameProvider": true,
"typeCoverageProvider": true,
"rageProvider": true
}
}
},
{
"jsonrpc": "2.0",
"id": 11,
"result": [
{
"range": {
"start": {
"line": 5,
"character": 17
},
"end": {
"line": 9,
"character": 58
}
},
"newText": "{\n test_otf(\n '1234567890',\n '1234567890',\n '1234567890',\n '1234567890',\n '1234567890',\n '1234567890',\n );"
}
]
},
{
"jsonrpc": "2.0",
"id": 12,
"result": [
{
"range": {
"start": {
"line": 13,
"character": 0
},
"end": {
"line": 13,
"character": 1
}
},
"newText": "{"
}
]
},
{
"jsonrpc": "2.0",
"id": 13,
"result": [
{
"range": {
"start": {
"line": 15,
"character": 0
},
"end": {
"line": 15,
"character": 16
}
},
"newText": "function otf() {}"
}
]
},
{
"jsonrpc": "2.0",
"id": 999,
"result": null
}
]
@@ -0,0 +1,90 @@
[
{
"jsonrpc": "2.0",
"id": 1,
"method": "initialize",
"params": {
"initializationOptions": {},
"processId": null,
"rootPath": "${root_path}",
"capabilities": {}
}
},
{
"jsonrpc": "2.0",
"method": "textDocument/didOpen",
"params": {
"textDocument": {
"uri": "${php_file_uri}",
"languageId": "hack",
"version": 1,
"text": "${php_file}"
}
}
},
{
"jsonrpc": "2.0",
"id": 11,
"method": "textDocument/onTypeFormatting",
"params": {
"textDocument": {
"uri": "${php_file_uri}"
},
"position": {
"line": 9,
"character": 58
},
"ch": ";",
"options": {
"tabSize": 2,
"insertSpaces": true
}
}
},
{
"jsonrpc": "2.0",
"id": 12,
"method": "textDocument/onTypeFormatting",
"params": {
"textDocument": {
"uri": "${php_file_uri}"
},
"position": {
"line": 13,
"character": 1
},
"ch": "}",
"comment": "NOTE: This tests bracket-matching",
"comment": "The position describes that of the cursor position after typing '{' (AKA the cursor space in between '{' and '}'), not that of the closing brace '}'",
"options": {
"tabSize": 2,
"insertSpaces": true
}
}
},
{
"jsonrpc": "2.0",
"id": 13,
"method": "textDocument/onTypeFormatting",
"params": {
"textDocument": {
"uri": "${php_file_uri}"
},
"position": {
"line": 15,
"character": 16
},
"ch": "}",
"options": {
"tabSize": 2,
"insertSpaces": true
}
}
},
{
"jsonrpc": "2.0",
"id": 999,
"method": "shutdown",
"params": {}
}
]
@@ -0,0 +1,16 @@
<?hh
function test_otf(mixed ...$_): void {
}
function otf_1() {
test_otf(
'1234567890',
'1234567890',
'1234567890','1234567890', '1234567890','1234567890'); // 11: Format on ";"
}
{} // 12: Format on bracket-matching "{}"
function otf(){} // 13: Format on "}"
@@ -216,6 +216,11 @@ def test_formatting(self):
variables = self.setup_php_file('messy.php')
self.load_and_run('formatting', variables)
def test_ontypeformatting(self):
self.prepare_environment()
variables = self.setup_php_file('ontypeformatting.php')
self.load_and_run('ontypeformatting', variables)
def test_did_change(self):
# Disabling this test because it has a race condition:
# see T27194253 for transcript

0 comments on commit 15109e4

Please sign in to comment.