Skip to content

Commit 5d6bf02

Browse files
committed
Optimistically match shellcheck errors to source
The yaml ast node exposes if the string nodes are literals or not. If so, we can preserve shellcheck errors to their source in the yaml block rather than just the `runs:` key. Example from sqlite-jdbc: ```yaml - name: Build matrix from Makefile id: set-matrix # parse the Makefile to retrieve the list of targets in 'native-all', without 'native' run: | matrix=$(( echo '{ "target" : [' sed -n "/^native-all *: */ { s///; p }" Makefile | sed "s/^native\s//g" | sed 's/ /, /g' | xargs -n 1 echo | sed -r 's/^([^,]*)(,?)$/"\1"\2/' echo " ]}" ) | jq -c .) echo $matrix | jq . echo "matrix=$matrix" >> $GITHUB_OUTPUT ``` output of ``` ❯ actionlint -version v1.7.7 installed by building from source built with go1.21.6 compiler for darwin/arm64 ``` ``` .github/workflows/build-native.yml:68:9: shellcheck reported issue in this script: SC1102:error:1:8: Shells disambiguate $(( differently or not at all. For $(command substitution), add space after $( . For $((arithmetics)), fix parsing errors [shellcheck] | 68 | run: | | ^~~~ .github/workflows/build-native.yml:68:9: shellcheck reported issue in this script: SC2086:info:6:6: Double quote to prevent globbing and word splitting [shellcheck] | 68 | run: | | ^~~~ .github/workflows/build-native.yml:68:9: shellcheck reported issue in this script: SC2086:info:7:26: Double quote to prevent globbing and word splitting [shellcheck] | 68 | run: | | ^~~~ ``` Output from this branch: ``` .github/workflows/build-native.yml:69:18: shellcheck reported issue in this script: SC1102:error:1:8: Shells disambiguate $(( differently or not at all. For $(command substitution), add space after $( . For $((arithmetics)), fix parsing errors [shellcheck] | 69 | matrix=$(( | ^~~ .github/workflows/build-native.yml:74:16: shellcheck reported issue in this script: SC2086:info:6:6: Double quote to prevent globbing and word splitting [shellcheck] | 74 | echo $matrix | jq . | ^~~~~~~ .github/workflows/build-native.yml:75:36: shellcheck reported issue in this script: SC2086:info:7:26: Double quote to prevent globbing and word splitting [shellcheck] | 75 | echo "matrix=$matrix" >> $GITHUB_OUTPUT | ^~~~~~~~~~~~~~ ``` The difference becomes incredibly stark with greater than 10 errors. The existing output feels empty. i don't beleive this is ready to merge as is. I'm a newcomer to the go language so I expect the code is not idiomatic. This project also has excellent testing coverage and documentation that this changeset does not come close to as is. My first question is if you are open to this change. If so, I am very interseted in your thoughts to get this to the quality you like. Things that might be of interest: - put this feature behind a flag - gather shell scripts from top 1000 github repos and see the results similar to the popular actions - extensive tests in testdata with expected output - update the documentation examples to generate output - extend this to pyflakes mechanism Thank you for the project
1 parent 2ab3a12 commit 5d6bf02

File tree

5 files changed

+23
-9
lines changed

5 files changed

+23
-9
lines changed

ast.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ type String struct {
4040
Quoted bool
4141
// Pos is a position of the string in source.
4242
Pos *Pos
43+
// If string is a literal block which preserves newlines and indentation. Helpful for error messages
44+
Literal bool
4345
}
4446

4547
// ContainsExpression checks if the given string contains a ${{ }} placeholder or not. This function

parse.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,8 @@ func isNull(n *yaml.Node) bool {
3838

3939
func newString(n *yaml.Node) *String {
4040
quoted := n.Style&(yaml.DoubleQuotedStyle|yaml.SingleQuotedStyle) != 0
41-
return &String{n.Value, quoted, posAt(n)}
41+
literal := n.Style == yaml.LiteralStyle
42+
return &String{n.Value, quoted, posAt(n), literal}
4243
}
4344

4445
type workflowKeyVal struct {
@@ -137,7 +138,7 @@ func (p *parser) mayParseExpression(n *yaml.Node) *String {
137138

138139
func (p *parser) parseString(n *yaml.Node, allowEmpty bool) *String {
139140
if !p.checkString(n, allowEmpty) {
140-
return &String{"", false, posAt(n)}
141+
return &String{"", false, posAt(n), false}
141142
}
142143
return newString(n)
143144
}

rule_runner_label.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,7 @@ func (rule *RuleRunnerLabel) tryToGetLabelsInMatrix(label *String, m *Matrix) []
266266
if row, ok := m.Rows[prop]; ok {
267267
for _, v := range row.Values {
268268
if s, ok := v.(*RawYAMLString); ok && !ContainsExpression(s.Value) {
269-
labels = append(labels, &String{s.Value, false, s.Pos()})
269+
labels = append(labels, &String{s.Value, false, s.Pos(), false})
270270
}
271271
}
272272
}
@@ -277,7 +277,7 @@ func (rule *RuleRunnerLabel) tryToGetLabelsInMatrix(label *String, m *Matrix) []
277277
if combi.Assigns != nil {
278278
if assign, ok := combi.Assigns[prop]; ok {
279279
if s, ok := assign.Value.(*RawYAMLString); ok && !ContainsExpression(s.Value) {
280-
labels = append(labels, &String{s.Value, false, s.Pos()})
280+
labels = append(labels, &String{s.Value, false, s.Pos(), false})
281281
}
282282
}
283283
}

rule_runner_label_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,7 @@ func TestRuleRunnerLabelCheckLabels(t *testing.T) {
255255
pos := &Pos{}
256256
labels := make([]*String, 0, len(tc.labels))
257257
for _, l := range tc.labels {
258-
labels = append(labels, &String{l, false, pos})
258+
labels = append(labels, &String{l, false, pos, false})
259259
}
260260
node := &Job{
261261
RunsOn: &Runner{
@@ -264,7 +264,7 @@ func TestRuleRunnerLabelCheckLabels(t *testing.T) {
264264
}
265265

266266
if tc.matrix != nil {
267-
n := &String{"os", false, pos}
267+
n := &String{"os", false, pos, false}
268268
row := make([]RawYAMLValue, 0, len(tc.matrix))
269269
for _, m := range tc.matrix {
270270
row = append(row, &RawYAMLString{m, pos})

rule_shellcheck.go

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package actionlint
33
import (
44
"encoding/json"
55
"fmt"
6+
"os"
67
"strings"
78
"sync"
89
)
@@ -57,7 +58,7 @@ func (rule *RuleShellcheck) VisitStep(n *Step) error {
5758
return nil
5859
}
5960

60-
rule.runShellcheck(run.Run.Value, rule.getShellName(run), run.RunPos)
61+
rule.runShellcheck(run.Run, rule.getShellName(run), run.RunPos)
6162
return nil
6263
}
6364

@@ -160,7 +161,7 @@ func sanitizeExpressionsInScript(src string) string {
160161
}
161162
}
162163

163-
func (rule *RuleShellcheck) runShellcheck(src, shell string, pos *Pos) {
164+
func (rule *RuleShellcheck) runShellcheck(srcAst *String, shell string, pos *Pos) {
164165
var sh string
165166
if shell == "bash" || shell == "sh" {
166167
sh = shell
@@ -172,6 +173,7 @@ func (rule *RuleShellcheck) runShellcheck(src, shell string, pos *Pos) {
172173
return // Skip checking this shell script since shellcheck doesn't support it
173174
}
174175

176+
src := srcAst.Value
175177
src = sanitizeExpressionsInScript(src)
176178
rule.Debug("%s: Run shellcheck for %s script:\n%s", pos, sh, src)
177179

@@ -214,6 +216,8 @@ func (rule *RuleShellcheck) runShellcheck(src, shell string, pos *Pos) {
214216
return nil
215217
}
216218

219+
rule.EnableDebug(os.Stdout)
220+
217221
// Synchronize rule.Errorf calls
218222
rule.mu.Lock()
219223
defer rule.mu.Unlock()
@@ -227,7 +231,14 @@ func (rule *RuleShellcheck) runShellcheck(src, shell string, pos *Pos) {
227231
// Consider the first line is setup for running shell which was implicitly added for better check
228232
line := err.Line - 1
229233
msg := strings.TrimSuffix(err.Message, ".") // Trim period aligning style of error message
230-
rule.Errorf(pos, "shellcheck reported issue in this script: SC%d:%s:%d:%d: %s", err.Code, err.Level, line, err.Column, msg)
234+
235+
var errorLocation Pos
236+
if srcAst.Literal {
237+
errorLocation = Pos{line + srcAst.Pos.Line, err.Column + srcAst.Pos.Col - 4}
238+
} else {
239+
errorLocation = *pos
240+
}
241+
rule.Errorf(&errorLocation, "shellcheck reported issue in this script: SC%d:%s:%d:%d: %s", err.Code, err.Level, line, err.Column, msg)
231242
}
232243

233244
return nil

0 commit comments

Comments
 (0)