Merge pull request #1221 from mcecode/fix-1183

Support renaming variables within `read` commands

Author: skovhus

server/src/__tests__/__snapshots__/server.test.ts.snap

@@ -16,7 +16,7 @@ import { Logger } from '../util/logger'
 const CURRENT_URI = 'dummy-uri.sh'
 
 // if you add a .sh file to testing/fixtures, update this value
-const FIXTURE_FILES_MATCHING_GLOB = 19
+const FIXTURE_FILES_MATCHING_GLOB = 20
 
 const defaultConfig = getDefaultConfiguration()
 

server/src/__tests__/analyzer.test.ts

@@ -1408,11 +1408,15 @@ describe('server', () => {
   })
 
   describe('onPrepareRename', () => {
-    async function getPrepareRenameResult(line: LSP.uinteger, character: LSP.uinteger) {
+    async function getPrepareRenameResult(
+      line: LSP.uinteger,
+      character: LSP.uinteger,
+      { uri = FIXTURE_URI.RENAMING } = {},
+    ) {
       const { connection } = await initializeServer()
 
       return connection.onPrepareRename.mock.calls[0][0](
-        { textDocument: { uri: FIXTURE_URI.RENAMING }, position: { line, character } },
+        { textDocument: { uri }, position: { line, character } },
         {} as any,
       )
     }
@@ -1480,6 +1484,22 @@ describe('server', () => {
           },
         }
       `)
+
+      const readvar = await getPrepareRenameResult(2, 18, {
+        uri: FIXTURE_URI.RENAMING_READ,
+      })
+      expect(readvar).toMatchInlineSnapshot(`
+        {
+          "end": {
+            "character": 20,
+            "line": 2,
+          },
+          "start": {
+            "character": 13,
+            "line": 2,
+          },
+        }
+      `)
     })
   })
 
@@ -1660,6 +1680,75 @@ describe('server', () => {
           expect(somevarInsideSomefunc).toStrictEqual(s)
         }
       })
+
+      it('returns correct WorkspaceEdits for variables within read commands', async () => {
+        const [readvar, ...readvars] = await getRenameRequestResults(
+          [2, 8, { uri: FIXTURE_URI.RENAMING_READ }],
+          [2, 19, { uri: FIXTURE_URI.RENAMING_READ }],
+          [2, 21, { uri: FIXTURE_URI.RENAMING_READ }],
+          [3, 10, { uri: FIXTURE_URI.RENAMING_READ }],
+          [3, 19, { uri: FIXTURE_URI.RENAMING_READ }],
+          [6, 14, { uri: FIXTURE_URI.RENAMING_READ }],
+          [7, 15, { uri: FIXTURE_URI.RENAMING_READ }],
+          [8, 32, { uri: FIXTURE_URI.RENAMING_READ }],
+          [9, 7, { uri: FIXTURE_URI.RENAMING_READ }],
+          [11, 23, { uri: FIXTURE_URI.RENAMING_READ }],
+          [12, 30, { uri: FIXTURE_URI.RENAMING_READ }],
+          [13, 10, { uri: FIXTURE_URI.RENAMING_READ }],
+          [15, 10, { uri: FIXTURE_URI.RENAMING_READ }],
+          [15, 31, { uri: FIXTURE_URI.RENAMING_READ }],
+          [16, 11, { uri: FIXTURE_URI.RENAMING_READ }],
+          [16, 30, { uri: FIXTURE_URI.RENAMING_READ }],
+          [17, 23, { uri: FIXTURE_URI.RENAMING_READ }],
+          [17, 33, { uri: FIXTURE_URI.RENAMING_READ }],
+        )
+        expect(readvar).toMatchSnapshot()
+        for (const r of readvars) {
+          expect(readvar).toStrictEqual(r)
+        }
+
+        const [readloop, ...readloops] = await getRenameRequestResults(
+          [21, 21, { uri: FIXTURE_URI.RENAMING_READ }],
+          [23, 12, { uri: FIXTURE_URI.RENAMING_READ }],
+        )
+        expect(readloop).toMatchSnapshot()
+        for (const r of readloops) {
+          expect(readloop).toStrictEqual(r)
+        }
+
+        const [readscope, ...readscopes] = await getRenameRequestResults(
+          [28, 8, { uri: FIXTURE_URI.RENAMING_READ }],
+          [30, 11, { uri: FIXTURE_URI.RENAMING_READ }],
+          [31, 12, { uri: FIXTURE_URI.RENAMING_READ }],
+          [38, 15, { uri: FIXTURE_URI.RENAMING_READ }],
+          [43, 9, { uri: FIXTURE_URI.RENAMING_READ }],
+        )
+        expect(readscope).toMatchSnapshot()
+        for (const r of readscopes) {
+          expect(readscope).toStrictEqual(r)
+        }
+
+        const [readscopeInsideFunction, ...readscopesInsideFunction] =
+          await getRenameRequestResults(
+            [33, 11, { uri: FIXTURE_URI.RENAMING_READ }],
+            [34, 14, { uri: FIXTURE_URI.RENAMING_READ }],
+            [35, 8, { uri: FIXTURE_URI.RENAMING_READ }],
+          )
+        expect(readscopeInsideFunction).toMatchSnapshot()
+        for (const r of readscopesInsideFunction) {
+          expect(readscopeInsideFunction).toStrictEqual(r)
+        }
+
+        const [readscopeInsideSubshell, ...readscopesInsideSubshell] =
+          await getRenameRequestResults(
+            [40, 14, { uri: FIXTURE_URI.RENAMING_READ }],
+            [41, 10, { uri: FIXTURE_URI.RENAMING_READ }],
+          )
+        expect(readscopeInsideSubshell).toMatchSnapshot()
+        for (const r of readscopesInsideSubshell) {
+          expect(readscopeInsideSubshell).toStrictEqual(r)
+        }
+      })
     })
 
     describe('Workspace-wide rename', () => {
@@ -1741,47 +1830,43 @@ describe('server', () => {
     // These may fail in the future when tree-sitter-bash's parsing gets better
     // or when the rename symbol implementation is improved.
     describe('Edge or not covered cases', () => {
-      it('only includes variables typed as variable_name', async () => {
+      it('does not include some variables typed as word', async () => {
         const iRanges = await getFirstChangeRanges(getRenameRequestResult(106, 4))
         // This should be 6 if all instances within let, postfix, and binary
         // expressions are included.
         expect(iRanges.length).toBe(3)
-
-        const lineRanges = await getFirstChangeRanges(getRenameRequestResult(118, 10))
-        // This should be 2 if the declaration of `line` is included.
-        expect(lineRanges.length).toBe(1)
       })
 
       it('includes incorrect number of symbols for complex scopes and nesting', async () => {
-        const varRanges = await getFirstChangeRanges(getRenameRequestResult(124, 8))
+        const varRanges = await getFirstChangeRanges(getRenameRequestResult(118, 8))
         // This should only be 2 if `$var` from `3` is not included.
         expect(varRanges.length).toBe(3)
 
-        const localFuncRanges = await getFirstChangeRanges(getRenameRequestResult(144, 5))
+        const localFuncRanges = await getFirstChangeRanges(getRenameRequestResult(138, 5))
         // This should be 2 if the instance of `localFunc` in `callerFunc` is
         // also included.
         expect(localFuncRanges.length).toBe(1)
       })
 
       it('only takes into account subshells created with ( and )', async () => {
         const pipelinevarRanges = await getFirstChangeRanges(
-          getRenameRequestResult(150, 7),
+          getRenameRequestResult(144, 7),
         )
         // This should only be 1 if pipeline subshell scoping is recognized.
         expect(pipelinevarRanges.length).toBe(2)
       })
 
       it('does not take into account sourcing location and scope', async () => {
-        const FOOUris = await getChangeUris(getRenameRequestResult(154, 8))
+        const FOOUris = await getChangeUris(getRenameRequestResult(148, 8))
         // This should only be 1 if sourcing after a symbol does not affect it.
         expect(FOOUris.length).toBe(2)
 
-        const hello_worldUris = await getChangeUris(getRenameRequestResult(160, 6))
+        const hello_worldUris = await getChangeUris(getRenameRequestResult(154, 6))
         // This should only be 1 if sourcing inside an uncalled function does
         // not affect symbols outside of it.
         expect(hello_worldUris.length).toBe(2)
 
-        const PATH_INPUTUris = await getChangeUris(getRenameRequestResult(163, 9))
+        const PATH_INPUTUris = await getChangeUris(getRenameRequestResult(157, 9))
         // This should only be 1 if sourcing inside a subshell does not affect
         // symbols outside of it.
         expect(PATH_INPUTUris.length).toBe(2)

server/src/__tests__/server.test.ts

@@ -466,15 +466,18 @@ export default class Analyzer {
 
     const typeOfDescendants =
       kind === LSP.SymbolKind.Variable
-        ? 'variable_name'
+        ? ['variable_name', 'word']
         : ['function_definition', 'command_name']
     const startPosition = start
       ? { row: start.line, column: start.character }
       : baseNode.startPosition
 
     const ignoredRanges: LSP.Range[] = []
     const filterVariables = (n: Parser.SyntaxNode) => {
-      if (n.text !== word) {
+      if (
+        n.text !== word ||
+        (n.type === 'word' && !TreeSitterUtil.isVariableInReadCommand(n))
+      ) {
         return false
       }
 
@@ -510,11 +513,14 @@ export default class Analyzer {
 
       const declarationCommand = TreeSitterUtil.findParentOfType(n, 'declaration_command')
       const isLocal =
-        (definedVariable?.text === word || !!(!definition && declarationCommand)) &&
-        (parent.type === 'subshell' ||
-          ['local', 'declare', 'typeset'].includes(
-            declarationCommand?.firstChild?.text as any,
-          ))
+        // Local `variable_name`s
+        ((definedVariable?.text === word || !!(!definition && declarationCommand)) &&
+          (parent.type === 'subshell' ||
+            ['local', 'declare', 'typeset'].includes(
+              declarationCommand?.firstChild?.text as any,
+            ))) ||
+        // Local variables within `read` command that are typed as `word`
+        (parent.type === 'subshell' && n.type === 'word')
       if (isLocal) {
         if (includeDeclaration) {
           ignoredRanges.push(TreeSitterUtil.range(parent))
@@ -785,6 +791,14 @@ export default class Analyzer {
       }
     }
 
+    if (TreeSitterUtil.isVariableInReadCommand(node)) {
+      return {
+        word: node.text,
+        range: TreeSitterUtil.range(node),
+        kind: LSP.SymbolKind.Variable,
+      }
+    }
+
     return null
   }
 

server/src/analyser.ts

@@ -361,6 +361,16 @@ export function findDeclarationUsingGlobalSemantics({
       return true
     }
 
+    if (
+      kind === LSP.SymbolKind.Variable &&
+      TreeSitterUtil.isVariableInReadCommand(n) &&
+      n.text === word
+    ) {
+      declaration = n
+      continueSearching = false
+      return false
+    }
+
     if (
       kind === LSP.SymbolKind.Function &&
       n.type === 'function_definition' &&

server/src/util/declarations.ts

@@ -43,6 +43,20 @@ export function isReference(n: SyntaxNode): boolean {
   }
 }
 
+export function isVariableInReadCommand(n: SyntaxNode): boolean {
+  if (
+    n.type === 'word' &&
+    n.parent?.type === 'command' &&
+    n.parent.firstChild?.text === 'read' &&
+    !n.text.startsWith('-') &&
+    !/^-.*[dinNptu]$/.test(n.previousSibling?.text ?? '')
+  ) {
+    return true
+  }
+
+  return false
+}
+
 export function findParent(
   start: SyntaxNode,
   predicate: (n: SyntaxNode) => boolean,

server/src/util/tree-sitter.ts

@@ -38,6 +38,7 @@ export const FIXTURE_URI = {
   SOURCING: `file://${path.join(FIXTURE_FOLDER, 'sourcing.sh')}`,
   SOURCING2: `file://${path.join(FIXTURE_FOLDER, 'sourcing2.sh')}`,
   RENAMING: `file://${path.join(FIXTURE_FOLDER, 'renaming.sh')}`,
+  RENAMING_READ: `file://${path.join(FIXTURE_FOLDER, 'renaming-read.sh')}`,
 }
 
 export const FIXTURE_DOCUMENT: Record<FIXTURE_KEY, TextDocument> = (

testing/fixtures.ts

@@ -0,0 +1,44 @@
+# Varying cases and flag usage
+
+read readvar readvar readvar
+read -r readvar readvar <<< "some output"
+echo readvar
+
+read -n1 readvar
+read -t 10 readvar
+read -rd readvar -p readvar readvar
+echo $readvar
+
+read -p readvar -a readvar -er
+read -sp "Prompt: " -ra readvar
+echo "${readvar[2]}"
+
+read readvar -rp readvar readvar
+read -r readvar -p readvar readvar
+read -p readvar -ir readvar readvar
+
+# While loop
+
+while read -r readloop; do
+	printf readloop
+	echo "$readloop"
+done < somefile.txt
+
+# Different scopes
+
+read readscope
+readfunc() {
+	read readscope
+	echo $readscope
+
+	local readscope
+	read readscope
+	echo $readscope
+}
+(
+	echo $readscope
+
+	read readscope
+	echo $readscope
+)
+echo $readscope

testing/fixtures/renaming-read.sh

@@ -113,12 +113,6 @@ for (( ; i < 10; i++)); do
 	echo "$((2 * i))"
 done
 
-# tree-sitter-bash's parsing limitations with while read loops
-
-while read -r line; do
-	echo "$line"
-done < somefile.txt
-
 # Complex nesting affects self-assignment handling
 
 f1() {