Fix and test onReferences not respecting includeDeclaration

skovhus · skovhus · commit a56061136375 · 2023-01-23T09:35:46.000+01:00
diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts
@@ -699,7 +699,7 @@ describe('server', () => {
             uri: FIXTURE_URI.ISSUE206,
           },
           position: {
-            // readonly cannot be parsed as a word
+            // readonly is a declaration command so not parsed correctly by findOccurrences
             line: 0,
             character: 0,
           },
@@ -1026,7 +1026,7 @@ describe('server', () => {
     })
 
     it.skip('returns documentation from explainshell', async () => {
-      // Skipped as this requires a running explainshell server
+      // Skipped as this requires a running explainshell server (and the code is hard to mock)
       // docker container run --name explainshell --restart always -p 127.0.0.1:6000:5000 -d spaceinvaderone/explainshell
 
       const { connection } = await initializeServer({
@@ -1067,7 +1067,180 @@ describe('server', () => {
   })
 
   describe('onReferences', () => {
-    // FIXME
+    async function getOnReferencesTestCase() {
+      const { connection } = await initializeServer()
+      const onReferences = connection.onReferences.mock.calls[0][0]
+
+      const callOnReferences = ({
+        includeDeclarationOfCurrentSymbol,
+        uri,
+        position,
+      }: {
+        includeDeclarationOfCurrentSymbol: boolean
+        uri: string
+        position: LSP.Position
+      }) =>
+        updateSnapshotUris(
+          onReferences(
+            {
+              textDocument: {
+                uri,
+              },
+              position,
+              context: {
+                includeDeclaration: includeDeclarationOfCurrentSymbol,
+              },
+            },
+            {} as any,
+            {} as any,
+          ),
+        )
+
+      return {
+        callOnReferences,
+      }
+    }
+
+    it('returns null if the word is not found', async () => {
+      const { callOnReferences } = await getOnReferencesTestCase()
+      const result = await callOnReferences({
+        position: { line: 34, character: 1 }, // empty line
+        uri: FIXTURE_URI.INSTALL,
+        includeDeclarationOfCurrentSymbol: true,
+      })
+      expect(result).toBeNull()
+    })
+
+    it('returns references to builtins and executables across the workspace', async () => {
+      const { callOnReferences } = await getOnReferencesTestCase()
+      const result = await callOnReferences({
+        position: { line: 263, character: 5 }, // echo
+        uri: FIXTURE_URI.INSTALL,
+        includeDeclarationOfCurrentSymbol: true,
+      })
+      expect(Array.isArray(result)).toBe(true)
+      if (Array.isArray(result)) {
+        expect(result.length).toBeGreaterThan(50)
+        expect(new Set(result.map((v) => v.uri)).size).toBeGreaterThan(5)
+      }
+    })
+
+    it('returns references depending on the context flag', async () => {
+      const { callOnReferences } = await getOnReferencesTestCase()
+
+      const resultIncludingCurrentSymbol = await callOnReferences({
+        position: { line: 50, character: 10 }, // npm_config_loglevel
+        uri: FIXTURE_URI.INSTALL,
+        includeDeclarationOfCurrentSymbol: true,
+      })
+
+      const resultExcludingCurrentSymbol = await callOnReferences({
+        position: { line: 50, character: 10 }, // npm_config_loglevel
+        uri: FIXTURE_URI.INSTALL,
+        includeDeclarationOfCurrentSymbol: false,
+      })
+
+      expect(resultIncludingCurrentSymbol).toMatchInlineSnapshot(`
+        Array [
+          Object {
+            "range": Object {
+              "end": Object {
+                "character": 19,
+                "line": 40,
+              },
+              "start": Object {
+                "character": 0,
+                "line": 40,
+              },
+            },
+            "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/install.sh",
+          },
+          Object {
+            "range": Object {
+              "end": Object {
+                "character": 21,
+                "line": 48,
+              },
+              "start": Object {
+                "character": 2,
+                "line": 48,
+              },
+            },
+            "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/install.sh",
+          },
+          Object {
+            "range": Object {
+              "end": Object {
+                "character": 26,
+                "line": 50,
+              },
+              "start": Object {
+                "character": 7,
+                "line": 50,
+              },
+            },
+            "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/install.sh",
+          },
+          Object {
+            "range": Object {
+              "end": Object {
+                "character": 26,
+                "line": 42,
+              },
+              "start": Object {
+                "character": 7,
+                "line": 42,
+              },
+            },
+            "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/scope.sh",
+          },
+        ]
+      `)
+
+      expect(resultExcludingCurrentSymbol).toMatchInlineSnapshot(`
+        Array [
+          Object {
+            "range": Object {
+              "end": Object {
+                "character": 19,
+                "line": 40,
+              },
+              "start": Object {
+                "character": 0,
+                "line": 40,
+              },
+            },
+            "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/install.sh",
+          },
+          Object {
+            "range": Object {
+              "end": Object {
+                "character": 21,
+                "line": 48,
+              },
+              "start": Object {
+                "character": 2,
+                "line": 48,
+              },
+            },
+            "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/install.sh",
+          },
+          Object {
+            "range": Object {
+              "end": Object {
+                "character": 26,
+                "line": 42,
+              },
+              "start": Object {
+                "character": 7,
+                "line": 42,
+              },
+            },
+            "uri": "file://__REPO_ROOT_FOLDER__/testing/fixtures/scope.sh",
+          },
+        ]
+      `)
+    })
   })
 
   describe('onWorkspaceSymbol', () => {
diff --git a/server/src/analyser.ts b/server/src/analyser.ts
@@ -264,21 +264,24 @@ export default class Analyzer {
 
   /**
    * Find all the locations where the given word was defined or referenced.
+   * This will include commands, functions, variables, etc.
    *
-   * FIXME: take position into account
-   * FIXME: take file into account
+   * It's currently not scope-aware, see findOccurrences.
    */
   public findReferences(word: string): LSP.Location[] {
     const uris = Object.keys(this.uriToAnalyzedDocument)
     return flattenArray(uris.map((uri) => this.findOccurrences(uri, word)))
   }
 
   /**
-   * Find all occurrences (references or definitions) of a word in the given file.
+   * Find all occurrences of a word in the given file.
    * It's currently not scope-aware.
    *
-   * FIXME: should this take the scope into account? I guess it should
-   * as this is used for highlighting.
+   * This will include commands, functions, variables, etc.
+   *
+   * It's currently not scope-aware, meaning references does include
+   * references to functions and variables that has the same name but
+   * are defined in different files.
    */
   public findOccurrences(uri: string, word: string): LSP.Location[] {
     const analyzedDocument = this.uriToAnalyzedDocument[uri]
@@ -294,6 +297,7 @@ export default class Analyzer {
       let namedNode: Parser.SyntaxNode | null = null
 
       if (TreeSitterUtil.isReference(n)) {
+        // NOTE: a reference can be a command, variable, function, etc.
         namedNode = n.firstNamedChild || n
       } else if (TreeSitterUtil.isDefinition(n)) {
         namedNode = n.firstNamedChild
diff --git a/server/src/server.ts b/server/src/server.ts
@@ -18,6 +18,7 @@ import { SNIPPETS } from './snippets'
 import { BashCompletionItem, CompletionItemDataType } from './types'
 import { uniqueBasedOnHash } from './util/array'
 import { logger, setLogConnection, setLogLevel } from './util/logger'
+import { isPositionIncludedInRange } from './util/lsp'
 import { getShellDocumentation } from './util/sh'
 
 const PARAMETER_EXPANSION_PREFIXES = new Set(['$', '${'])
@@ -708,7 +709,14 @@ export default class BashServer {
     if (!word) {
       return null
     }
-    return this.analyzer.findReferences(word)
+
+    const isCurrentDeclaration = (l: LSP.Location) =>
+      l.uri === params.textDocument.uri &&
+      isPositionIncludedInRange(params.position, l.range)
+
+    return this.analyzer
+      .findReferences(word)
+      .filter((l) => params.context.includeDeclaration || !isCurrentDeclaration(l))
   }
 
   private onWorkspaceSymbol(params: LSP.WorkspaceSymbolParams): LSP.SymbolInformation[] {
diff --git a/testing/fixtures.ts b/testing/fixtures.ts
@@ -40,17 +40,28 @@ export const FIXTURE_DOCUMENT: Record<FIXTURE_KEY, TextDocument> = (
 
 export const REPO_ROOT_FOLDER = path.resolve(path.join(FIXTURE_FOLDER, '../..'))
 
-export function updateSnapshotUris(obj: any) {
-  if (obj.uri) {
-    obj.uri = obj.uri.replace(REPO_ROOT_FOLDER, '__REPO_ROOT_FOLDER__')
-  }
-  Object.values(obj).forEach((child) => {
-    if (Array.isArray(child)) {
-      child.forEach((el) => updateSnapshotUris(el))
-    } else if (typeof child === 'object' && child != null) {
-      updateSnapshotUris(child)
+export function updateSnapshotUris<
+  T extends Record<string, any> | Array<any> | null | undefined,
+>(data: T): T {
+  if (data != null) {
+    if (Array.isArray(data)) {
+      data.forEach((el) => updateSnapshotUris(el))
+      return data
+    }
+
+    if (typeof data === 'object') {
+      if (data.uri) {
+        data.uri = data.uri.replace(REPO_ROOT_FOLDER, '__REPO_ROOT_FOLDER__')
+      }
+      Object.values(data).forEach((child) => {
+        if (Array.isArray(child)) {
+          child.forEach((el) => updateSnapshotUris(el))
+        } else if (typeof child === 'object' && child != null) {
+          updateSnapshotUris(child)
+        }
+      })
     }
-  })
+  }
 
-  return obj
+  return data
 }
diff --git a/testing/fixtures/scope.sh b/testing/fixtures/scope.sh
@@ -39,3 +39,5 @@ for i in 1 2 3 4 5
 do
  echo "$GLOBAL_1 $i"
 done
+
+echo "$npm_config_loglevel" # this is an undefined variable, but defined in install.sh
