Fix incorrect completions when identifier overlaps with keyword (#138)

neo4j · Nov 22, 2023 · 08db455 · 08db455
1 parent a790700
commit 08db455
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 20 deletions.
diff --git a/.changeset/stale-donkeys-grow.md b/.changeset/stale-donkeys-grow.md
@@ -0,0 +1,5 @@
+---
+'@neo4j-cypher/language-support': patch
+---
+
+Fix incorrect completions when identifier overlaps with keyword
diff --git a/packages/language-support/src/autocompletion/completionCoreCompletions.ts b/packages/language-support/src/autocompletion/completionCoreCompletions.ts
@@ -11,7 +11,12 @@ import CypherParser, {
   Expression2Context,
 } from '../generated-parser/CypherParser';
 import { rulesDefiningVariables } from '../helpers';
-import { CypherTokenType, lexerSymbols, tokenNames } from '../lexerSymbols';
+import {
+  CypherTokenType,
+  lexerKeywords,
+  lexerSymbols,
+  tokenNames,
+} from '../lexerSymbols';
 import { EnrichedParsingResult, ParsingResult } from '../parserWrapper';
 
 const uniq = <T>(arr: T[]) => Array.from(new Set(arr));
@@ -282,7 +287,13 @@ export function completionCoreCompletion(
   // The identfier is finished when the last token is a SPACE or dot etc. etc.
   // this allows us to give completions that replace the current text => for example `RET` <- it's parsed as an identifier
   // The need for this caret movement is outlined in the documentation of antlr4-c3 in the section about caret position
-  if (tokens[caretIndex - 1]?.type === CypherLexer.IDENTIFIER) {
+  // When an identifer overlaps with a keyword, it's no longer treates as an identifier (although it's a valid identifier)
+  // So we need to move the caret back for keywords as well
+  const previousToken = tokens[caretIndex - 1]?.type;
+  if (
+    previousToken === CypherLexer.IDENTIFIER ||
+    lexerKeywords.includes(previousToken)
+  ) {
     caretIndex--;
   }
 

diff --git a/packages/language-support/src/tests/autocompletion/databasename-completion.test.ts b/packages/language-support/src/tests/autocompletion/databasename-completion.test.ts
@@ -319,7 +319,7 @@ describe('can complete database names', () => {
 
     test('suggests parameter in options field of create constraint', () => {
       const query =
-        'CREATE CONSTRAINT abc ON (n:person) ASSERT EXISTS n.name OPTIONS';
+        'CREATE CONSTRAINT abc ON (n:person) ASSERT EXISTS n.name OPTIONS ';
       testCompletions({
         query,
         dbSchema,
@@ -359,14 +359,14 @@ describe('can complete database names', () => {
 
     test('suggests parameters for user management', () => {
       const cases = [
-        'CREATE USER',
-        'DROP USER',
-        'ALTER USER',
-        'RENAME USER',
-        'SHOW USER',
-        'ALTER CURRENT USER SET PASSWORD FROM',
+        'CREATE USER ',
+        'DROP USER ',
+        'ALTER USER ',
+        'RENAME USER ',
+        'SHOW USER ',
+        'ALTER CURRENT USER SET PASSWORD FROM ',
         'ALTER CURRENT USER SET PASSWORD FROM $pw to ',
-        'ALTER USER',
+        'ALTER USER ',
         'ALTER USER foo IF EXISTS SET PASSWORD ',
       ];
       cases.forEach((query) => {
@@ -386,11 +386,11 @@ describe('can complete database names', () => {
 
     test('suggests parameters for role management', () => {
       const cases = [
-        'CREATE ROLE',
-        'DROP ROLE',
-        'RENAME ROLE',
-        'GRANT ROLE',
-        'GRANT ROLE abc TO',
+        'CREATE ROLE ',
+        'DROP ROLE ',
+        'RENAME ROLE ',
+        'GRANT ROLE ',
+        'GRANT ROLE abc TO ',
       ];
       cases.forEach((query) => {
         testCompletions({
@@ -418,8 +418,8 @@ describe('can complete database names', () => {
         'DEALLOCATE DATABASES FROM SERVERS "ab", ',
       ];
       const optionsCases = [
-        'ENABLE SERVER "abc" OPTIONS',
-        'ALTER SERVER "abc" SET OPTIONS',
+        'ENABLE SERVER "abc" OPTIONS ',
+        'ALTER SERVER "abc" SET OPTIONS ',
       ];
 
       nameCases.forEach((query) => {

diff --git a/packages/language-support/src/tests/autocompletion/misc-completions.test.ts b/packages/language-support/src/tests/autocompletion/misc-completions.test.ts
@@ -23,6 +23,25 @@ describe('Misc auto-completion', () => {
     });
   });
 
+  test('Requires space to complete next keyword', () => {
+    const query = 'SHOW';
+
+    testCompletions({
+      query,
+      expected: [{ label: 'SHOW', kind: CompletionItemKind.Keyword }],
+      excluded: [{ label: 'DATABASE', kind: CompletionItemKind.Keyword }],
+    });
+  });
+
+  test('Completes keyword, even when current word is also valid keyword', () => {
+    const query = 'SHOW DATA';
+
+    testCompletions({
+      query,
+      expected: [{ label: 'DATABASE', kind: CompletionItemKind.Keyword }],
+    });
+  });
+
   test('Correctly completes DISTINCT', () => {
     const query = 'MATCH (n:Person)-[r:KNOWS]-(m:Person) RETURN ';
 

diff --git a/packages/language-support/src/tests/autocompletion/variable-completion.test.ts b/packages/language-support/src/tests/autocompletion/variable-completion.test.ts
@@ -38,7 +38,7 @@ describe('unscoped variable completions', () => {
   });
 
   test('suggests both variables after renaming variable', () => {
-    const query = 'MATCH (n:Person) WITH n as m RETURN';
+    const query = 'MATCH (n:Person) WITH n as m RETURN ';
     testCompletions({
       query,
       expected: [
@@ -49,7 +49,7 @@ describe('unscoped variable completions', () => {
   });
 
   test('does not suggest variable when renaming variable', () => {
-    const query = 'MATCH (n:Person) WITH n as';
+    const query = 'MATCH (n:Person) WITH n as ';
 
     testCompletions({
       query,
@@ -58,7 +58,7 @@ describe('unscoped variable completions', () => {
   });
 
   test('does not suggest variables when unwinding ', () => {
-    const query = 'MATCH (n:Person) UNWIND [] as';
+    const query = 'MATCH (n:Person) UNWIND [] as ';
 
     testCompletions({
       query,