diff --git a/server/src/__tests__/__snapshots__/server.test.ts.snap b/server/src/__tests__/__snapshots__/server.test.ts.snap index ff2550c5c..c19f48334 100644 --- a/server/src/__tests__/__snapshots__/server.test.ts.snap +++ b/server/src/__tests__/__snapshots__/server.test.ts.snap @@ -567,6 +567,441 @@ exports[`server onRenameRequest File-wide rename returns correct WorkspaceEdits } `; +exports[`server onRenameRequest File-wide rename returns correct WorkspaceEdits for variables within read commands 1`] = ` +{ + "changes": { + "file://__REPO_ROOT_FOLDER__/testing/fixtures/renaming-read.sh": [ + { + "newText": "newName", + "range": { + "end": { + "character": 12, + "line": 2, + }, + "start": { + "character": 5, + "line": 2, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 20, + "line": 2, + }, + "start": { + "character": 13, + "line": 2, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 28, + "line": 2, + }, + "start": { + "character": 21, + "line": 2, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 15, + "line": 3, + }, + "start": { + "character": 8, + "line": 3, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 23, + "line": 3, + }, + "start": { + "character": 16, + "line": 3, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 16, + "line": 6, + }, + "start": { + "character": 9, + "line": 6, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 18, + "line": 7, + }, + "start": { + "character": 11, + "line": 7, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 35, + "line": 8, + }, + "start": { + "character": 28, + "line": 8, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 13, + "line": 9, + }, + "start": { + "character": 6, + "line": 9, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 26, + "line": 11, + }, + "start": { + "character": 19, + "line": 11, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 31, + "line": 12, + }, + "start": { + "character": 24, + "line": 12, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 15, + "line": 13, + }, + "start": { + "character": 8, + "line": 13, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 12, + "line": 15, + }, + "start": { + "character": 5, + "line": 15, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 32, + "line": 15, + }, + "start": { + "character": 25, + "line": 15, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 15, + "line": 16, + }, + "start": { + "character": 8, + "line": 16, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 34, + "line": 16, + }, + "start": { + "character": 27, + "line": 16, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 27, + "line": 17, + }, + "start": { + "character": 20, + "line": 17, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 35, + "line": 17, + }, + "start": { + "character": 28, + "line": 17, + }, + }, + }, + ], + }, +} +`; + +exports[`server onRenameRequest File-wide rename returns correct WorkspaceEdits for variables within read commands 2`] = ` +{ + "changes": { + "file://__REPO_ROOT_FOLDER__/testing/fixtures/renaming-read.sh": [ + { + "newText": "newName", + "range": { + "end": { + "character": 22, + "line": 21, + }, + "start": { + "character": 14, + "line": 21, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 16, + "line": 23, + }, + "start": { + "character": 8, + "line": 23, + }, + }, + }, + ], + }, +} +`; + +exports[`server onRenameRequest File-wide rename returns correct WorkspaceEdits for variables within read commands 3`] = ` +{ + "changes": { + "file://__REPO_ROOT_FOLDER__/testing/fixtures/renaming-read.sh": [ + { + "newText": "newName", + "range": { + "end": { + "character": 14, + "line": 28, + }, + "start": { + "character": 5, + "line": 28, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 15, + "line": 30, + }, + "start": { + "character": 6, + "line": 30, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 16, + "line": 31, + }, + "start": { + "character": 7, + "line": 31, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 16, + "line": 38, + }, + "start": { + "character": 7, + "line": 38, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 15, + "line": 43, + }, + "start": { + "character": 6, + "line": 43, + }, + }, + }, + ], + }, +} +`; + +exports[`server onRenameRequest File-wide rename returns correct WorkspaceEdits for variables within read commands 4`] = ` +{ + "changes": { + "file://__REPO_ROOT_FOLDER__/testing/fixtures/renaming-read.sh": [ + { + "newText": "newName", + "range": { + "end": { + "character": 16, + "line": 33, + }, + "start": { + "character": 7, + "line": 33, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 15, + "line": 34, + }, + "start": { + "character": 6, + "line": 34, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 16, + "line": 35, + }, + "start": { + "character": 7, + "line": 35, + }, + }, + }, + ], + }, +} +`; + +exports[`server onRenameRequest File-wide rename returns correct WorkspaceEdits for variables within read commands 5`] = ` +{ + "changes": { + "file://__REPO_ROOT_FOLDER__/testing/fixtures/renaming-read.sh": [ + { + "newText": "newName", + "range": { + "end": { + "character": 15, + "line": 40, + }, + "start": { + "character": 6, + "line": 40, + }, + }, + }, + { + "newText": "newName", + "range": { + "end": { + "character": 16, + "line": 41, + }, + "start": { + "character": 7, + "line": 41, + }, + }, + }, + ], + }, +} +`; + exports[`server onRenameRequest Workspace-wide rename returns correct WorkspaceEdits for sourced symbols 1`] = ` { "changes": { @@ -863,6 +1298,7 @@ exports[`server onRenameRequest Workspace-wide rename returns correct WorkspaceE "file://__REPO_ROOT_FOLDER__/testing/fixtures/options.sh": [], "file://__REPO_ROOT_FOLDER__/testing/fixtures/override-executable-symbol.sh": [], "file://__REPO_ROOT_FOLDER__/testing/fixtures/parse-problems.sh": [], + "file://__REPO_ROOT_FOLDER__/testing/fixtures/renaming-read.sh": [], "file://__REPO_ROOT_FOLDER__/testing/fixtures/renaming.sh": [ { "newText": "newName", @@ -943,6 +1379,7 @@ exports[`server onRenameRequest Workspace-wide rename returns correct WorkspaceE "file://__REPO_ROOT_FOLDER__/testing/fixtures/options.sh": [], "file://__REPO_ROOT_FOLDER__/testing/fixtures/override-executable-symbol.sh": [], "file://__REPO_ROOT_FOLDER__/testing/fixtures/parse-problems.sh": [], + "file://__REPO_ROOT_FOLDER__/testing/fixtures/renaming-read.sh": [], "file://__REPO_ROOT_FOLDER__/testing/fixtures/renaming.sh": [ { "newText": "newName", diff --git a/server/src/__tests__/analyzer.test.ts b/server/src/__tests__/analyzer.test.ts index 8d4aa357e..51c47a7ac 100644 --- a/server/src/__tests__/analyzer.test.ts +++ b/server/src/__tests__/analyzer.test.ts @@ -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() diff --git a/server/src/__tests__/server.test.ts b/server/src/__tests__/server.test.ts index f9cee23df..5979e254c 100644 --- a/server/src/__tests__/server.test.ts +++ b/server/src/__tests__/server.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,23 +1830,19 @@ 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) @@ -1765,23 +1850,23 @@ describe('server', () => { 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) diff --git a/server/src/analyser.ts b/server/src/analyser.ts index 72cecbaf0..af0290f8c 100644 --- a/server/src/analyser.ts +++ b/server/src/analyser.ts @@ -466,7 +466,7 @@ 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 } @@ -474,7 +474,10 @@ export default class Analyzer { 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 } diff --git a/server/src/util/declarations.ts b/server/src/util/declarations.ts index 618bd2152..53b9a61d3 100644 --- a/server/src/util/declarations.ts +++ b/server/src/util/declarations.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' && diff --git a/server/src/util/tree-sitter.ts b/server/src/util/tree-sitter.ts index 1dc3b0341..41fe915ac 100644 --- a/server/src/util/tree-sitter.ts +++ b/server/src/util/tree-sitter.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, diff --git a/testing/fixtures.ts b/testing/fixtures.ts index 2bee6ef8f..c98331f77 100644 --- a/testing/fixtures.ts +++ b/testing/fixtures.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 = ( diff --git a/testing/fixtures/renaming-read.sh b/testing/fixtures/renaming-read.sh new file mode 100644 index 000000000..cb5253f18 --- /dev/null +++ b/testing/fixtures/renaming-read.sh @@ -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 diff --git a/testing/fixtures/renaming.sh b/testing/fixtures/renaming.sh index addc5ca9c..9b92e13a8 100644 --- a/testing/fixtures/renaming.sh +++ b/testing/fixtures/renaming.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() {