Robustify handling of special cases in completion provider

Notably, we no longer adjust start/end position based on individual
items because that was not predictable; instead we revert the logic
and populate insert text as needed to match start/end. We only change
end to equal start if there are absolutely no prefix matches, which
is required because otherwise everything would be filtered out.

Author: krassowski

atest/05_Features/Completion.robot

@@ -85,7 +85,7 @@ Invalidates On Focus Loss
     Completer Should Not Suggest    test
     Enter Cell Editor    1    line=2
 
-Uses LSP Completions When Kernel Resoponse Times Out
+Uses LSP Completions When Kernel Response Times Out
     [Tags]    requires:busy-indicator
     Configure JupyterLab Plugin    {"kernelResponseTimeout": 1, "waitForBusyKernel": true}
     ...    plugin id=${COMPLETION PLUGIN ID}

packages/jupyterlab-lsp/src/features/completion/model.ts

@@ -256,7 +256,10 @@ export class LSPCompleterModel extends GenericCompleterModel<MaybeCompletionItem
 
   protected harmoniseItem(item: CompletionHandler.ICompletionItem) {
     if ((item as any).self) {
-      return (item as any).self;
+      const self = (item as any).self;
+      // reflect any changes made on copy
+      self.insertText = item.insertText;
+      return self;
     }
     return super.harmoniseItem(item);
   }

packages/jupyterlab-lsp/src/features/completion/provider.ts

@@ -322,53 +322,66 @@ export function transformLSPCompletions<T>(
   createCompletionItem: (kind: string, match: lsProtocol.CompletionItem) => T,
   console: ILSPLogConsole
 ) {
-  let prefix = token.value.slice(0, positionInToken + 1);
-  let allNonPrefixed = true;
+  let prefix = token.value.slice(0, positionInToken);
+  let suffix = token.value.slice(positionInToken, token.value.length);
   let items: T[] = [];
-  let prefixLength = prefix.length;
+  // If there are no prefixes, we will just insert the text without replacing the token,
+  // which is the case for example in R for `stats::<tab>` which returns module members
+  // without `::` prefix.
+  // If there are prefixes, we will replace the token so we may need to prepend/append to,
+  // or otherwise modify the insert text of individual completion items.
+  let anyPrefixed = false;
+
   lspCompletionItems.forEach(match => {
     let kind = match.kind ? CompletionItemKind[match.kind] : '';
 
-    // Update prefix values
     let text = match.insertText ? match.insertText : match.label;
+    let intendedText = match.insertText ? match.insertText : match.label;
 
-    // declare prefix presence if needed and update it
-    if (text.toLowerCase().startsWith(prefix.toLowerCase())) {
-      allNonPrefixed = false;
-      if (prefix !== token.value) {
-        if (text.toLowerCase().startsWith(token.value.toLowerCase())) {
-          // given a completion insert text "display_table" and two test cases:
-          // disp<tab>data →  display_table<cursor>data
-          // disp<tab>lay  →  display_table<cursor>
-          // we have to adjust the prefix for the latter (otherwise we would get display_table<cursor>lay),
-          // as we are constrained NOT to replace after the prefix (which would be "disp" otherwise)
-          prefix = token.value;
-        }
+    if (intendedText.toLowerCase().startsWith(prefix.toLowerCase())) {
+      anyPrefixed = true;
+    }
+
+    // Add overlap with token prefix
+    if (intendedText.toLowerCase().startsWith(token.value.toLowerCase())) {
+      anyPrefixed = true;
+      // remove overlap with prefix before expanding it
+      if (intendedText.toLowerCase().startsWith(prefix.toLowerCase())) {
+        text = text.substring(prefix.length, text.length);
+        match.insertText = text;
       }
+      text = token.value + text;
+      match.insertText = text;
     }
-    // add prefix if needed
-    else if (token.type === 'String' && prefix.includes('/')) {
-      // special case for path completion in strings, ensuring that:
-      //     '/Com<tab> → '/Completion.ipynb
-      // when the returned insert text is `Completion.ipynb` (the token here is `'/Com`)
-      // developed against pyls and pylsp server, may not work well in other cases
 
+    // special handling for paths
+    if (token.type === 'String' && prefix.includes('/')) {
       const parts = stripQuotes(prefix).split('/');
       if (
         text.toLowerCase().startsWith(parts[parts.length - 1].toLowerCase())
       ) {
         let pathPrefix = parts.slice(0, -1).join('/') + '/';
-        const withStartingQuote =
+        text =
           (prefix.startsWith("'") || prefix.startsWith('"') ? prefix[0] : '') +
           pathPrefix +
           text;
-        match.insertText = withStartingQuote;
+        match.insertText = text;
         // for label without quotes
         match.label = pathPrefix + match.label;
-        if (prefix.endsWith("'") || prefix.endsWith('"')) {
-          prefixLength = prefix.length - 1;
+        anyPrefixed = true;
+      }
+    } else {
+      // harmonise end to token
+      if (text.toLowerCase().endsWith(suffix.toLowerCase())) {
+        text = text.substring(0, text.length - suffix.length);
+        match.insertText = text;
+      } else if (token.type === 'String') {
+        // special case for completion in strings to preserve the closing quote;
+        // there is an issue that this gives opposing results in Notebook vs File editor
+        // probably due to reconciliator logic
+        if (suffix.startsWith("'") || suffix.startsWith('"')) {
+          match.insertText = text + suffix[0];
         }
-        allNonPrefixed = false;
       }
     }
 
@@ -377,29 +390,17 @@ export function transformLSPCompletions<T>(
     items.push(completionItem);
   });
   console.debug('Transformed');
-  // required to make the repetitive trigger characters like :: or ::: work for R with R languageserver,
-  // see https://github.com/jupyter-lsp/jupyterlab-lsp/issues/436
-  let prefixOffset = token.value.length;
-  // completion of dictionaries for Python with jedi-language-server was
-  // causing an issue for dic['<tab>'] case; to avoid this let's make
-  // sure that prefix.length >= prefix.offset
-  if (allNonPrefixed && prefixOffset > prefixLength) {
-    prefixOffset = prefixLength;
+
+  let start = token.offset;
+  let end = token.offset + token.value.length;
+  if (!anyPrefixed) {
+    start = end;
   }
 
   let response = {
-    // note in the ContextCompleter it was:
-    // start: token.offset,
-    // end: token.offset + token.value.length,
-    // which does not work with "from statistics import <tab>" as the last token ends at "t" of "import",
-    // so the completer would append "mean" as "from statistics importmean" (without space!);
-    // (in such a case the typedCharacters is undefined as we are out of range)
-    // a different workaround would be to prepend the token.value prefix:
-    // text = token.value + text;
-    // but it did not work for "from statistics <tab>" and lead to "from statisticsimport" (no space)
-    start: token.offset + (allNonPrefixed ? prefixOffset : 0),
-    end: token.offset + prefixLength,
-    items: items,
+    start,
+    end,
+    items,
     source: 'LSP'
   };
   if (response.start > response.end) {