Format position of character according to LSP

Conder Shou · fredemmott · commit 15109e459144 · 2018-07-12T12:45:28.000-07:00
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
diff --git a/hphp/hack/src/client/clientLsp.ml b/hphp/hack/src/client/clientLsp.ml
@@ -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;
diff --git a/hphp/hack/test/integration/data/lsp_exchanges/ontypeformatting.expected b/hphp/hack/test/integration/data/lsp_exchanges/ontypeformatting.expected
@@ -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
+  }
+]
diff --git a/hphp/hack/test/integration/data/lsp_exchanges/ontypeformatting.json b/hphp/hack/test/integration/data/lsp_exchanges/ontypeformatting.json
@@ -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": {}
+  }
+]
diff --git a/hphp/hack/test/integration/data/lsp_exchanges/ontypeformatting.php b/hphp/hack/test/integration/data/lsp_exchanges/ontypeformatting.php
@@ -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 "}"
diff --git a/hphp/hack/test/integration/test_lsp.py b/hphp/hack/test/integration/test_lsp.py
@@ -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
