Skip to content

Commit a79f2af

Browse files
committed
feat: support leading dynamic section when sourcing
This does the same thing that ShellCheck does ([here](https://github.com/koalaman/shellcheck/blob/ba86c6363c30a5dbefd0b8b9a7c5f4ab0478dc91/src/ShellCheck/Parser.hs#L2277-L2283)). `To support the common pattern of . "$CONFIGDIR/mylib.sh", ShellCheck strips one leading, dynamic section before trying to locate the rest.` fixes #926, fixes #659 fix: handle leading dynamic source paths when concatenating This fixes `${var}/path` and `"${var}"/path` as well as countless variations of these patterns Note: this turned out to be rather complex task, so it's broken out into a seperate function, which allows for early returns refactor: move resolveStaticString to util/tree-sitter.ts This seemed the most appropriate place for it. tests: fix existing snapshot and add test for dynamic source paths Adds a simple test for dynamic source paths and updates the snapshot to match new tests / behaivor. Note: all the line numbers in the snapshot increased by 2 because I added two new lines (the test and a blank one) tests: add new test in fixtures/sourcing.sh I added this to the bottom of the file to minimize snapshot diffs. they're large enough as it is.
1 parent 7080c9a commit a79f2af

File tree

6 files changed

+165
-27
lines changed

6 files changed

+165
-27
lines changed

Diff for: server/src/__tests__/analyzer.test.ts

+23
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,28 @@ describe('analyze', () => {
123123
"severity": 3,
124124
"source": "bash-language-server",
125125
},
126+
{
127+
"message": "Source command could not be analyzed: failed to resolve path.
128+
129+
Consider adding a ShellCheck directive above this line to fix or ignore this:
130+
# shellcheck source=/my-file.sh # specify the file to source
131+
# shellcheck source-path=my_script_folder # specify the folder to search in
132+
# shellcheck source=/dev/null # to ignore the error
133+
134+
Disable this message by changing the configuration option "enableSourceErrorDiagnostics"",
135+
"range": {
136+
"end": {
137+
"character": 49,
138+
"line": 26,
139+
},
140+
"start": {
141+
"character": 0,
142+
"line": 26,
143+
},
144+
},
145+
"severity": 3,
146+
"source": "bash-language-server",
147+
},
126148
]
127149
`)
128150

@@ -847,6 +869,7 @@ describe('initiateBackgroundAnalysis', () => {
847869
[expect.stringContaining('parse-problems.sh: syntax error')],
848870
[expect.stringContaining('sourcing.sh line 16: failed to resolve path')],
849871
[expect.stringContaining('sourcing.sh line 21: non-constant source not supported')],
872+
[expect.stringContaining('sourcing.sh line 26: failed to resolve path')],
850873
])
851874

852875
// Intro, stats on glob, one file skipped due to shebang, and outro

Diff for: server/src/util/__tests__/__snapshots__/sourcing.test.ts.snap

+32-18
Original file line numberDiff line numberDiff line change
@@ -86,16 +86,30 @@ exports[`getSourcedUris returns a set of sourced files (but ignores some unhandl
8686
},
8787
"uri": null,
8888
},
89+
{
90+
"error": null,
91+
"range": {
92+
"end": {
93+
"character": 39,
94+
"line": 15,
95+
},
96+
"start": {
97+
"character": 6,
98+
"line": 15,
99+
},
100+
},
101+
"uri": "file:///Users/bash/issue-926.sh",
102+
},
89103
{
90104
"error": "non-constant source not supported",
91105
"range": {
92106
"end": {
93107
"character": 66,
94-
"line": 16,
108+
"line": 18,
95109
},
96110
"start": {
97111
"character": 49,
98-
"line": 16,
112+
"line": 18,
99113
},
100114
},
101115
"uri": null,
@@ -105,11 +119,11 @@ exports[`getSourcedUris returns a set of sourced files (but ignores some unhandl
105119
"range": {
106120
"end": {
107121
"character": 66,
108-
"line": 18,
122+
"line": 20,
109123
},
110124
"start": {
111125
"character": 6,
112-
"line": 18,
126+
"line": 20,
113127
},
114128
},
115129
"uri": null,
@@ -119,11 +133,11 @@ exports[`getSourcedUris returns a set of sourced files (but ignores some unhandl
119133
"range": {
120134
"end": {
121135
"character": 30,
122-
"line": 25,
136+
"line": 27,
123137
},
124138
"start": {
125139
"character": 8,
126-
"line": 25,
140+
"line": 27,
127141
},
128142
},
129143
"uri": "file:///Users/bash/issue206.sh",
@@ -133,53 +147,53 @@ exports[`getSourcedUris returns a set of sourced files (but ignores some unhandl
133147
"range": {
134148
"end": {
135149
"character": 22,
136-
"line": 47,
150+
"line": 49,
137151
},
138152
"start": {
139153
"character": 8,
140-
"line": 47,
154+
"line": 49,
141155
},
142156
},
143157
"uri": null,
144158
},
145159
{
146-
"error": "non-constant source not supported",
160+
"error": null,
147161
"range": {
148162
"end": {
149163
"character": 39,
150-
"line": 59,
164+
"line": 61,
151165
},
152166
"start": {
153167
"character": 8,
154-
"line": 59,
168+
"line": 61,
155169
},
156170
},
157-
"uri": null,
171+
"uri": "file:///Users/bash/staging.sh",
158172
},
159173
{
160-
"error": "non-constant source not supported",
174+
"error": null,
161175
"range": {
162176
"end": {
163177
"character": 42,
164-
"line": 62,
178+
"line": 64,
165179
},
166180
"start": {
167181
"character": 8,
168-
"line": 62,
182+
"line": 64,
169183
},
170184
},
171-
"uri": null,
185+
"uri": "file:///Users/bash/production.sh",
172186
},
173187
{
174188
"error": "non-constant source not supported",
175189
"range": {
176190
"end": {
177191
"character": 67,
178-
"line": 89,
192+
"line": 91,
179193
},
180194
"start": {
181195
"character": 10,
182-
"line": 89,
196+
"line": 91,
183197
},
184198
},
185199
"uri": null,

Diff for: server/src/util/__tests__/sourcing.test.ts

+5
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ describe('getSourcedUris', () => {
4646
4747
source "$LIBPATH" # dynamic imports not supported
4848
49+
source "$SCRIPT_DIR"/issue-926.sh # remove leading dynamic segment
50+
4951
# conditional is currently not supported
5052
if [[ -z $__COMPLETION_LIB_LOADED ]]; then source "$LIBPATH" ; fi
5153
@@ -144,7 +146,10 @@ describe('getSourcedUris', () => {
144146
"file:///Users/bash/x",
145147
"file:///Users/scripts/release-client.sh",
146148
"file:///Users/bash-user/myscript",
149+
"file:///Users/bash/issue-926.sh",
147150
"file:///Users/bash/issue206.sh",
151+
"file:///Users/bash/staging.sh",
152+
"file:///Users/bash/production.sh",
148153
}
149154
`)
150155

Diff for: server/src/util/sourcing.ts

+67-9
Original file line numberDiff line numberDiff line change
@@ -102,20 +102,32 @@ function getSourcedPathInfoFromNode({
102102
}
103103
}
104104

105-
if (argumentNode.type === 'word') {
105+
const strValue = TreeSitterUtil.resolveStaticString(argumentNode)
106+
if (strValue !== null) {
106107
return {
107-
sourcedPath: argumentNode.text,
108+
sourcedPath: strValue,
108109
}
109110
}
110111

111-
if (argumentNode.type === 'string' || argumentNode.type === 'raw_string') {
112-
const children = argumentNode.namedChildren
113-
if (
114-
children.length === 0 ||
115-
(children.length === 1 && children[0].type === 'string_content')
116-
) {
112+
// Strip one leading dynamic section.
113+
if (argumentNode.type === 'string' && argumentNode.namedChildren.length === 1) {
114+
const [variableNode] = argumentNode.namedChildren
115+
if (TreeSitterUtil.isExpansion(variableNode)) {
116+
const stringContents = argumentNode.text.slice(1, -1)
117+
if (stringContents.startsWith(`${variableNode.text}/`)) {
118+
return {
119+
sourcedPath: `.${stringContents.slice(variableNode.text.length)}`,
120+
}
121+
}
122+
}
123+
}
124+
125+
if (argumentNode.type === 'concatenation') {
126+
// Strip one leading dynamic section from a concatenation node.
127+
const sourcedPath = resolveSourceFromConcatenation(argumentNode)
128+
if (sourcedPath) {
117129
return {
118-
sourcedPath: argumentNode.text.slice(1, -1),
130+
sourcedPath,
119131
}
120132
}
121133
}
@@ -171,3 +183,49 @@ function resolveSourcedUri({
171183

172184
return null
173185
}
186+
187+
/*
188+
* Resolves the source path from a concatenation node, stripping a leading dynamic directory segment.
189+
* Returns null if the source path can't be statically determined after stripping a segment.
190+
* Note: If a non-concatenation node is passed, null will be returned. This is likely a programmer error.
191+
*/
192+
function resolveSourceFromConcatenation(node: Parser.SyntaxNode): string | null {
193+
if (node.type !== 'concatenation') return null
194+
const stringValue = TreeSitterUtil.resolveStaticString(node)
195+
if (stringValue !== null) return stringValue // This string is fully static.
196+
197+
const values: string[] = []
198+
// Since the string must begin with the variable, the variable must be in the first child.
199+
const [firstNode, ...rest] = node.namedChildren
200+
// The first child is static, this means one of the other children is not!
201+
if (TreeSitterUtil.resolveStaticString(firstNode) !== null) return null
202+
203+
// if the string is unquoted, the first child is the variable, so there's no more text in it.
204+
if (!TreeSitterUtil.isExpansion(firstNode)) {
205+
if (firstNode.namedChildCount > 1) return null // Only one variable is allowed.
206+
// Since the string must begin with the variable, the variable must be first child.
207+
const variableNode = firstNode.namedChildren[0] // Get the variable (quoted case)
208+
// This is command substitution!
209+
if (!TreeSitterUtil.isExpansion(variableNode)) return null
210+
const stringContents = firstNode.text.slice(1, -1)
211+
// The string doesn't start with the variable!
212+
if (!stringContents.startsWith(variableNode.text)) return null
213+
// Get the remaining static portion the string
214+
values.push(stringContents.slice(variableNode.text.length))
215+
}
216+
217+
for (const child of rest) {
218+
const value = TreeSitterUtil.resolveStaticString(child)
219+
// The other values weren't statically determinable!
220+
if (value === null) return null
221+
values.push(value)
222+
}
223+
224+
// Join all our found static values together.
225+
const staticResult = values.join('')
226+
// The path starts with slash, so trim the leading variable and replace with a dot
227+
if (staticResult.startsWith('/')) return `.${staticResult}`
228+
// The path doesn't start with a slash, so it's invalid
229+
// PERF: can we fail earlier than this?
230+
return null
231+
}

Diff for: server/src/util/tree-sitter.ts

+36
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,16 @@ export function isVariableInReadCommand(n: SyntaxNode): boolean {
5757
return false
5858
}
5959

60+
export function isExpansion(n: SyntaxNode): boolean {
61+
switch (n.type) {
62+
case 'expansion':
63+
case 'simple_expansion':
64+
return true
65+
default:
66+
return false
67+
}
68+
}
69+
6070
export function findParent(
6171
start: SyntaxNode,
6272
predicate: (n: SyntaxNode) => boolean,
@@ -78,3 +88,29 @@ export function findParentOfType(start: SyntaxNode, type: string | string[]) {
7888

7989
return findParent(start, (n) => type.includes(n.type))
8090
}
91+
92+
/**
93+
* Resolves the full string value of a node
94+
* Returns null if the value can't be statically determined (ie, it contains a variable or command substition).
95+
* Supports: word, string, raw_string, and concatenation
96+
*/
97+
export function resolveStaticString(node: SyntaxNode): string | null {
98+
if (node.type === 'concatenation') {
99+
const values = []
100+
for (const child of node.namedChildren) {
101+
const value = resolveStaticString(child)
102+
if (value === null) return null
103+
values.push(value)
104+
}
105+
return values.join('')
106+
}
107+
if (node.type === 'word') return node.text
108+
if (node.type === 'string' || node.type === 'raw_string') {
109+
if (node.namedChildCount === 0) return node.text.slice(1, -1)
110+
const children = node.namedChildren
111+
if (children.length === 1 && children[0].type === 'string_content')
112+
return children[0].text
113+
return null
114+
}
115+
return null
116+
}

Diff for: testing/fixtures/sourcing.sh

+2
Original file line numberDiff line numberDiff line change
@@ -23,3 +23,5 @@ loadlib () {
2323
}
2424

2525
loadlib "issue206"
26+
27+
source $SCRIPT_DIR/tag-release.inc with arguments

0 commit comments

Comments
 (0)