feat: implement shellcheck lints in command blocks (#191) #264

thatRichman · 2024-12-08T20:05:56Z

This pull request adds a new rule to wdl-lint.

Rule Name: ShellCheckRule

Describe the rules you have implemented and link to any relevant issues.

I have implemented a lint rule that runs shellcheck against command sections and reports the comments as diagnostics. This is to address #191 .

Before submitting this PR, please make sure:

You have added a few sentences describing the PR here.
You have added yourself or the appropriate individual as the assignee.
You have added at least one relevant code reviewer to the PR.
Your code builds clean without any errors or warnings.
You have added an entry to the relevant CHANGELOG.md (see
["keep a changelog"] for more information).
Your commit messages follow the [conventional commit] style.

Rule specific checks:

You have added the rule as an entry within RULES.md.
You have added the rule to the rules() function in wdl-lint/src/lib.rs.
You have added a test case in wdl-lint/tests/lints that covers every
possible diagnostic emitted for the rule within the file where the rule
is implemented.
If you have implemented a new Visitor callback, you have also
overridden that callback method for the special Validator
(wdl-ast/src/validation.rs) and LintVisitor
(wdl-lint/src/visitor.rs) visitors. These are required to ensure the new
visitor callback will execute.
You have run wdl-gauntlet --refresh to ensure that there are no
unintended changes to the baseline configuration file (Gauntlet.toml).
You have run wdl-gauntlet --refresh --arena to ensure that all of the
rules added/removed are now reflected in the baseline configuration file
(Arena.toml).

thatRichman · 2024-12-08T20:06:48Z

There's still a lot of testing and polishing to do, which will undoubtedly reveal some issues, but I wanted to open for comments before I went any further in case I'm way off from what's desired.

thatRichman · 2024-12-08T20:16:28Z

wdl-lint/src/rules/command_shellcheck.rs

+const SHELLCHECK_BIN: &str = "shellcheck";
+
+/// shellcheck lints that we want to suppress
+const SHELLCHECK_SUPPRESS: &[&str] = &[


This is a copy-paste of miniwdl's list. I think we can remove most of these since most were added to work around their use of dummy literals, which we're not (currently) doing.

Yeah I think this is good for now. It would be nice to make these configurable in the future. Perhaps you could add a TODO comment above it and/or an issue when this is merged?

Not perfect, but users can add #shellcheck disable= directives right into the comment of a command section. Or use SHELLCHECK_OPTS to globally disable.

wdl-lint/src/rules/command_shellcheck.rs

a-frantz

Looks like this is on the right track! Didn't go through it with a fine-tooth comb yet, but left some comments/Qs on things that jumped out at me.

wdl-lint/src/rules/command_shellcheck.rs

claymcleod

Looking really good!

wdl-lint/Cargo.toml

wdl-lint/src/lib.rs

claymcleod · 2024-12-08T21:16:35Z

wdl-lint/src/rules/command_shellcheck.rs

+const SHELLCHECK_BIN: &str = "shellcheck";
+
+/// shellcheck lints that we want to suppress
+const SHELLCHECK_SUPPRESS: &[&str] = &[


Yeah I think this is good for now. It would be nice to make these configurable in the future. Perhaps you could add a TODO comment above it and/or an issue when this is merged?

wdl-lint/src/rules/command_shellcheck.rs

wdl-lint/src/util.rs

wdl-lint/tests/lints/command-shellcheck-ok/source.errors

wdl-lint/src/rules/command_shellcheck.rs

wdl-lint/tests/lints/deprecated-placeholder-options-v1.1/source.errors

a-frantz

This is looking great so far. Really excited to get this feature merged in 😎

wdl-lint/src/rules/shellcheck.rs

a-frantz · 2024-12-10T14:06:02Z

wdl-lint/src/rules/shellcheck.rs

+        "ShellCheck (https://shellcheck.net) is a static analysis tool and linter for sh / bash. \
+        The lints provided by ShellCheck help prevent common errors and \
+        pitfalls in your scripts. Following its recommendations will increase \
+        the robustness of your command sections."


Just want to say good job writing this explanation() text. I think it's spot on 👍

wdl-lint/src/rules/shellcheck.rs

wdl-lint/tests/lints/shellcheck-error/source.errors

wdl-lint/src/rules/shellcheck.rs

thatRichman · 2024-12-12T03:00:00Z

Rebased. Gauntlet in arena mode is giving me the following:

thread 'main' panicked at /home/spencer/projects/wdl/gauntlet/src/lib.rs:258:25:
duplicate diagnostic: `ww-fastq-to-cram.wdl:101:20: note[ShellCheck]: `shellcheck` reported the following diagnostic` at /ww-fastq-to-cram.wdl:101

so I'll look into that tomorrow.

a-frantz · 2024-12-12T14:15:23Z

@thatRichman do you think you can figure out how to install shellcheck on the Arena CI runner? I'd rather not merge this until we can see the lints that will show up "in the wild". I won't be surprised if we find another SC rule or two should be disabled as being unhelpful in the context of a WDL command block.

Feel free to ping me on Zulip if that poses any issues! I imagine the windows runner could pose a challenge 🤔

thatRichman · 2024-12-12T14:27:54Z

@thatRichman do you think you can figure out how to install shellcheck on the Arena CI runner? I'd rather not merge this until we can see the lints that will show up "in the wild". I won't be surprised if we find another SC rule or two should be disabled as being unhelpful in the context of a WDL command block.

Feel free to ping me on Zulip if that poses any issues! I imagine the windows runner could pose a challenge 🤔

@a-frantz Sure, shouldn't be too bad. Shellcheck provides a pre-built windows binary that we can pull from their releases. Might just take a little fiddling.

thatRichman · 2024-12-13T21:37:22Z

@a-frantz Here's a link to an example of a new install-shellcheck action running on my fork. One thing to note is Ubuntu being Ubuntu only has 0.8.0 by default. macOS and Windows will always run latest. Maybe a good thing to be testing across versions. But if we want to forego the apt caching we could just pull from GH the same as Windows.

edit: it's also suspicious that Arena passed on Windows when the duplicate diagnostic bug was still present. Makes me think it probably isn't finding the executable.

thatRichman · 2024-12-13T21:48:08Z

wdl-lint/src/util.rs

@@ -24,6 +34,22 @@ pub fn is_inline_comment(token: &Comment) -> bool {
    false
 }

+/// Determines if a string containing embedded quotes is properly quoted.


Suggested change

/// Determines if a string containing embedded quotes is properly quoted.

/// Determines whether or not a string containing embedded quotes is properly quoted.

claymcleod · 2024-12-14T23:19:14Z

@a-frantz Here's a link to an example of a new install-shellcheck action running on my fork. One thing to note is Ubuntu being Ubuntu only has 0.8.0 by default. macOS and Windows will always run latest. Maybe a good thing to be testing across versions. But if we want to forego the apt caching we could just pull from GH the same as Windows.

edit: it's also suspicious that Arena passed on Windows when the duplicate diagnostic bug was still present. Makes me think it probably isn't finding the executable.

My two cents: let's just always pull from latest on everything (including Mac). It makes it a bit more straightforward to maintain the Github action associated with that, and it also ensures that every platform always works with the bleeding edge Shellcheck.

thatRichman · 2024-12-14T23:38:09Z

I'm starting to see why the miniwdl folks went with literals instead of variables / expansions when substituting placeholders.

The common false positives I'm seeing from Arena are:

SC2076

❌ read_group.wdl:200:43: note[ShellCheck]: Remove quotes from right-hand side of =~ to match as a regex rather than literally.

caused by:

| [[ ~{read_group.ID} =~ ~{restrictive_pattern} ]]

because we're replacing both of these placeholders with double-quoted expansions.

and SC2001

❌ util.wdl:132:9: note[ShellCheck]: See if you can use ${variable//search/replace} instead.

caused by:

echo ~{string} | sed 's/~{delimiter}/\n/g' > split_strings.txt

because with a bash variable you can use the suggested syntax but that obviously doesn't apply to WDL placeholders.

as well as SC2066:

❌ samtools.wdl:577:21: note[ShellCheck]: Since you double quoted this, it will not word split, and the loop will only run once.

caused by:

 for file in ~{sep(" ", bams)}

Though, this one is also basically identical to one that miniwdl has to suppress.

I've found (what I think) is an elegant solution to determine whether to use a literal or expansion, which reduces but does not eliminate false positives.

That being said, I think miniwdl's approach of guessing the placeholder's type and using an appropriate literal would be overly complicated here. So I think it comes down to the severity and frequency of the lints we end up needing to suppress. I still think (with the commit I'm about to push) that we're better off than always using literals. We could probably spend weeks working to get the false positives to an absolute minimum without suppressing anything, but that feels a bit bikesheddy to me. Of course I will defer.

a-frantz · 2024-12-15T13:35:47Z

I'm starting to see why the miniwdl folks went with literals instead of variables / expansions when substituting placeholders.

So am I. However with wdl-lint currently being based off of wdl-ast, we don't have a good way of knowing the right type of literal to inject. We've discussed potentially refactoring wdl-lint to be based off of wdl-analysis so that we'd have access to resolved type information. That refactor would certainly be OOS for this PR, but would potentially simplify this problem by a great amount.

I'm thinking we shouldn't expend a large amount of effort eliminating these false positives with the current AST-based implementation.

@thatRichman can you remove this rule from the default set of rules (as Clay originally suggested) so that this is opt-in only?

We should re-architect wdl-lint so that it can have the results of a full static-analysis and revisit this rule at that time. In the meantime, this rule can be in a bit of a beta stage. It's certainly still useful and worth merging into main, but the false-positive lints are too big an issue to include this in the default rule set IMO.

thatRichman · 2024-12-15T16:47:20Z

Sounds good. I'll disable the shellcheck-install action (once I get it working on all OSes) so that it be re-enabled at a later date.

I know the intent is to support non-default rules in sprocket eventually, but should I put any work into supporting non-default rules in the dev CLI in this PR? Seems OOS, so happy to throw in an issue if there isn't one already.

Also, I think I will wait for the resolution of #268 before making any final changes to this PR, as it throws off diagnostic placements.

a-frantz · 2024-12-17T16:17:20Z

@thatRichman

Also, I think I will wait for the resolution of #268 before making any final changes to this PR, as it throws off diagnostic placements.

Fixed! Thanks for logging that issue.

a-frantz · 2024-12-18T02:12:18Z

Sweetness! Will give this a review in the morning. I also added the rest of the team as reviewers in case I miss something.

One question I have is whether or not to run gauntlet with shellcheck enabled (I added this option), since this rule is in 'beta' and thus gauntlet will report missing lints if not run with --shellcheck (if it has previously been blessed while using --shellcheck).

IMO we leave it out of gauntlet/arena for now. I imagine it is going to add a boatload of diagnostics, and we like to review each one. Considering we know upfront some portion will be false positives, I don't think it's worth the effort.

Also, I fully intend to squash these 39 commits into 1. I can do that before or after final review, whatever you prefer.

We squash as part of the merge, so you shouldn't need to do anything to deal with it.

gauntlet/src/lib.rs

wdl-lint/src/rules/shellcheck.rs

wdl/src/bin/wdl.rs

a-frantz · 2024-12-18T14:16:54Z

wdl-lint/src/rules/shellcheck.rs

+    let label = format!(
+        "SC{}[{}]: {}",
+        diagnostic.code, diagnostic.level, diagnostic.message
+    );
+    Diagnostic::note(&diagnostic.message)
+        .with_rule(ID)
+        .with_label(label, span)


This repeats the diagnostic.message twice. Is that desirable? Should we omit it from the label as being redundant, or do we like it appearing twice?

I'm conflicted. Pulling an example from below, I think the initial instance by the note is fine. The second provides the additional info link, which could be helpful. I do notice that the span isn't highlighted correctly. I also think the fix needs to be something else.

note[ShellCheck]: You need a space before the ]. ┌─ tests/lints/shellcheck-error/source.wdl:18:25 │ 18 │ if [ -f "$broken"] │ ^ │ │ SC1020[error]: You need a space before the ]. │ more info: https://www.shellcheck.net/wiki/SC1020 │ = fix: address the diagnostic as recommended in the message

@adthrasher Could you clarify what you mean about the span not being highlighted correctly? Shellcheck outputs this diagnostic as a single position one character ahead of the closing bracket.

#!/usr/bin/env bash # the below line has 11 columns of text [ -f "foo"]

yields:

{ "file": "test.sh", "line": 3, "endLine": 3, "column": 12, "endColumn": 12, "level": "error", "code": 1072, "message": "Missing space before ]. Fix any mentioned problems and try again.", "fix": null }

(note that shellcheck 1-indexes columns)
so I think it's 1:1 with how shellcheck reports it.

Re: the message

@peterhuene and I discussed this some in the Zulip. This initially came about because Gauntlet requires all diagnostic messages to be unique for a given line + position. The solution I came up with was to include the SC###[] in the message, but only the shellcheck message in the label. But happy to rework. Agree the fix could be much more useful.

So it's the underlying shellcheck that returns the column position after ]. That's good to know. It still seems like the highlight should be one position earlier, so that it flags the ].

This thread got a little sidetracked. Bringing up the original Q again, are we ok with the SC message being repeated? Maybe I'm lacking in imagination here, but I don't really see a way to get rid of the duplication that still looks ok.

I like it at the top of the diagnostic and that ensures uniqueness for gauntlet. Removing it from the label leaves that part lacking... I'm stumped.
I'm ok with it appearing twice, but that's mostly because I can't think of an alternative I like. Open to any ideas here.

I'm fine with the duplication. We can always revisit it in the future.

a-frantz · 2024-12-18T14:20:31Z

@thatRichman can you add a test that #shellcheck disable= directives work as expected?

wdl-lint/src/rules/shellcheck.rs

wdl-lint/tests/lints/shellcheck-error/source.errors

thatRichman · 2024-12-18T22:14:55Z

wdl-lint/src/lib.rs

@@ -150,3 +152,32 @@ pub fn rules() -> Vec<Box<dyn Rule>> {

    rules
 }
+
+/// Gets the optional rule set.
+pub fn optional_rules() -> Vec<Box<dyn Rule>> {


this is probably overkill since this will (hopefully) be a temporary home for shellcheck.

gauntlet/src/lib.rs

a-frantz · 2024-12-19T19:57:11Z

We're pretty dang close here @thatRichman . Please get the CI all green, then I think this will have my approval
https://github.com/stjude-rust-labs/wdl/blob/main/CONTRIBUTING.md#the-ci-has-turned-red-how-do-i-make-it-green-again

The general rule we follow is to require approval from two core team members for a merge.

thatRichman · 2024-12-19T21:58:50Z

@a-frantz Sounds good. One thing to note is that I did end up needing to re-enable shellcheck install in the CI. This is because if shellcheck isn't present, the first lint test that is run will end up with the 'shellcheck not found' warning, and the source.errors for the shellcheck-* lints will be empty. I thought about modifying the lints test script to selectively skip the shellcheck lints, but this would not address the 'missing shellcheck' lint.

Here is an example of the modified CI going green on my repo but I don't think there's any way to reflect that here unless we merge the actions changes to main in a separate PR first.

gauntlet/CHANGELOG.md

a-frantz · 2024-12-20T13:35:06Z

wdl-lint/src/rules/shellcheck.rs

+    let label = format!(
+        "SC{}[{}]: {}",
+        diagnostic.code, diagnostic.level, diagnostic.message
+    );
+    Diagnostic::note(&diagnostic.message)
+        .with_rule(ID)
+        .with_label(label, span)


This thread got a little sidetracked. Bringing up the original Q again, are we ok with the SC message being repeated? Maybe I'm lacking in imagination here, but I don't really see a way to get rid of the duplication that still looks ok.

I like it at the top of the diagnostic and that ensures uniqueness for gauntlet. Removing it from the label leaves that part lacking... I'm stumped.
I'm ok with it appearing twice, but that's mostly because I can't think of an alternative I like. Open to any ideas here.

wdl-lint/tests/lints/shellcheck-ok/source.wdl

Co-authored-by: Andrew Frantz <[email protected]>

wdl-lint/tests/lints/shellcheck-ok/source.wdl

a-frantz

LGTM!

thatRichman commented Dec 8, 2024

View reviewed changes

wdl-lint/src/rules/command_shellcheck.rs Outdated Show resolved Hide resolved

a-frantz reviewed Dec 8, 2024

View reviewed changes

claymcleod requested changes Dec 8, 2024

View reviewed changes

thatRichman commented Dec 9, 2024

View reviewed changes

wdl-lint/src/rules/command_shellcheck.rs Outdated Show resolved Hide resolved

thatRichman commented Dec 10, 2024

View reviewed changes

wdl-lint/tests/lints/deprecated-placeholder-options-v1.1/source.errors Outdated Show resolved Hide resolved

thatRichman force-pushed the feat/shellcheck-lint-command-blocks branch from 958e222 to 8474ebd Compare December 10, 2024 13:56

a-frantz reviewed Dec 10, 2024

View reviewed changes

wdl-lint/src/rules/shellcheck.rs Outdated Show resolved Hide resolved

claymcleod self-requested a review December 11, 2024 02:02

thatRichman commented Dec 11, 2024

View reviewed changes

wdl-lint/src/rules/shellcheck.rs Show resolved Hide resolved

thatRichman force-pushed the feat/shellcheck-lint-command-blocks branch from 759c7f4 to 6f792a2 Compare December 11, 2024 23:46

thatRichman commented Dec 11, 2024

View reviewed changes

wdl-lint/src/rules/shellcheck.rs Outdated Show resolved Hide resolved

thatRichman force-pushed the feat/shellcheck-lint-command-blocks branch 2 times, most recently from b76e234 to f2506be Compare December 12, 2024 02:42

thatRichman marked this pull request as ready for review December 12, 2024 03:00

claymcleod assigned thatRichman Dec 12, 2024

thatRichman mentioned this pull request Dec 13, 2024

Possible CommandSection strip_whitespace regression, now removes the first word on the first line #268

Closed

thatRichman commented Dec 13, 2024

View reviewed changes

a-frantz mentioned this pull request Dec 17, 2024

refactor wdl-lint on top of wdl-analysis #273

Open

a-frantz linked an issue Dec 18, 2024 that may be closed by this pull request

feature request: incorporate shellcheck for linting command blocks #191

Closed

fix: always use latest github release of shellcheck

d1bb4ae

chore: lints

58b8d67

a-frantz reviewed Dec 18, 2024

View reviewed changes

adthrasher reviewed Dec 18, 2024

View reviewed changes

wdl-lint/src/rules/shellcheck.rs Show resolved Hide resolved

adthrasher reviewed Dec 18, 2024

View reviewed changes

wdl-lint/tests/lints/shellcheck-error/source.errors Show resolved Hide resolved

thatRichman added 4 commits December 18, 2024 16:33

chore: update wdl and gauntlet changelogs

5e942c5

fix: re-enable missing shellcheck diagnostic

d8dfef1

fix: include optional rules when checking rule validity

984dd70

chore: fmt

9cd70c0

thatRichman commented Dec 18, 2024

View reviewed changes

a-frantz reviewed Dec 19, 2024

View reviewed changes

gauntlet/src/lib.rs Outdated Show resolved Hide resolved

thatRichman added 4 commits December 19, 2024 15:23

chore: lint

7db4246

fix: only use shellcheck rule in arena mode

7b50b5b

chore: lint

3dfa8fd

fix: re-enable shellcheck install for CI

3a8e4ae

a-frantz reviewed Dec 20, 2024

View reviewed changes

fix: gauntlet/CHANGELOG.md description

24d1114

Co-authored-by: Andrew Frantz <[email protected]>

a-frantz reviewed Dec 20, 2024

View reviewed changes

wdl-lint/tests/lints/shellcheck-ok/source.wdl Outdated Show resolved Hide resolved

thatRichman force-pushed the feat/shellcheck-lint-command-blocks branch from 75b3355 to cb4b0fa Compare December 20, 2024 14:28

feat: add test shellcheck disable directives

9040430

thatRichman force-pushed the feat/shellcheck-lint-command-blocks branch from cb4b0fa to 9040430 Compare December 20, 2024 14:28

a-frantz approved these changes Dec 20, 2024

View reviewed changes

adthrasher approved these changes Dec 20, 2024

View reviewed changes

a-frantz mentioned this pull request Dec 20, 2024

improve fix messages #276

Open

a-frantz merged commit a682411 into stjude-rust-labs:main Dec 20, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement shellcheck lints in command blocks (#191) #264

feat: implement shellcheck lints in command blocks (#191) #264

thatRichman commented Dec 8, 2024 •

edited by claymcleod

Loading

thatRichman commented Dec 8, 2024

thatRichman Dec 8, 2024

claymcleod Dec 8, 2024

thatRichman Dec 8, 2024

a-frantz left a comment

claymcleod left a comment

claymcleod Dec 8, 2024

a-frantz left a comment

a-frantz Dec 10, 2024

thatRichman commented Dec 12, 2024

a-frantz commented Dec 12, 2024

thatRichman commented Dec 12, 2024

thatRichman commented Dec 13, 2024 •

edited

Loading

thatRichman Dec 13, 2024

claymcleod commented Dec 14, 2024

thatRichman commented Dec 14, 2024

a-frantz commented Dec 15, 2024

thatRichman commented Dec 15, 2024

a-frantz commented Dec 17, 2024

a-frantz commented Dec 18, 2024

a-frantz Dec 18, 2024

adthrasher Dec 18, 2024

thatRichman Dec 18, 2024 •

edited

Loading

thatRichman Dec 18, 2024 •

edited

Loading

adthrasher Dec 18, 2024

a-frantz Dec 20, 2024

adthrasher Dec 20, 2024

a-frantz commented Dec 18, 2024

thatRichman Dec 18, 2024

a-frantz commented Dec 19, 2024

thatRichman commented Dec 19, 2024

a-frantz Dec 20, 2024

a-frantz left a comment

	/// Determines if a string containing embedded quotes is properly quoted.
	/// Determines whether or not a string containing embedded quotes is properly quoted.

feat: implement shellcheck lints in command blocks (#191) #264

feat: implement shellcheck lints in command blocks (#191) #264

Conversation

thatRichman commented Dec 8, 2024 • edited by claymcleod Loading

thatRichman commented Dec 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-frantz left a comment

Choose a reason for hiding this comment

claymcleod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-frantz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thatRichman commented Dec 12, 2024

a-frantz commented Dec 12, 2024

thatRichman commented Dec 12, 2024

thatRichman commented Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

claymcleod commented Dec 14, 2024

thatRichman commented Dec 14, 2024

a-frantz commented Dec 15, 2024

thatRichman commented Dec 15, 2024

a-frantz commented Dec 17, 2024

a-frantz commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thatRichman Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

thatRichman Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-frantz commented Dec 18, 2024

Choose a reason for hiding this comment

a-frantz commented Dec 19, 2024

thatRichman commented Dec 19, 2024

Choose a reason for hiding this comment

a-frantz left a comment

Choose a reason for hiding this comment

thatRichman commented Dec 8, 2024 •

edited by claymcleod

Loading

thatRichman commented Dec 13, 2024 •

edited

Loading

thatRichman Dec 18, 2024 •

edited

Loading

thatRichman Dec 18, 2024 •

edited

Loading