Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SC2317 findings are confusing and difficult to act on #2613

Open
2 tasks done
dimo414 opened this issue Oct 24, 2022 · 0 comments
Open
2 tasks done

SC2317 findings are confusing and difficult to act on #2613

dimo414 opened this issue Oct 24, 2022 · 0 comments

Comments

@dimo414
Copy link
Contributor

dimo414 commented Oct 24, 2022

For bugs

Here's a snippet or screenshot that shows the problem:

#!/bin/bash

foo() {
  bar() { baz; }
}

Here's what shellcheck currently says:

$ shellcheck myscript

Line 4:
  bar() { baz; }
          ^-- [SC2317](https://www.shellcheck.net/wiki/SC2317) (info): Command appears to be unreachable. Check usage (or ignore if invoked indirectly).

Here's what I wanted or expected to see:

First off, this control flow analysis seems super cool! That said, the findings it emits at present are pretty confusing to work with.

In my specific case, passing bash-cache.sh to ShellCheck at present triggers ~50 SC2317 findings, none of which appear to be accurate/actionable. For example, the first two findings are on this line, on the ( and {.

Now bash-cache does some unusual things like defining dynamic functions, so it's not totally surprising that some of it confuses control-flow analysis, but even so these findings don't seem very actionable to me. In particular, it's not at all clear from the finding how the reachability was determined or why the marked lines are "unreachable", which makes it hard to determine if there's anything to do about it other than suppress.

Furthermore, the fact that the error message itself includes guidance to ignore the finding and the wiki directs users to suppress it on entire functions or files suggests this is generating a lot of false positives. IMHO a check like this should err on the side of false negatives rather than introducing noise with false positives that users then have to suppress.

The-Compiler added a commit to qutebrowser/qutebrowser that referenced this issue Dec 13, 2022
v0.9.0 seems to add SC2317 ("Command appears to be unreachable."):
https://www.shellcheck.net/wiki/SC2317

This adds various false-positives in password_fill, let's just ignore
those...

Also see: koalaman/shellcheck#2613
The-Compiler added a commit to qutebrowser/qutebrowser that referenced this issue Jan 17, 2023
v0.9.0 seems to add SC2317 ("Command appears to be unreachable."):
https://www.shellcheck.net/wiki/SC2317

This adds various false-positives in password_fill, let's just ignore
those...

Also see: koalaman/shellcheck#2613

(cherry picked from commit 5afc8a6)
robn added a commit to robn/zfs that referenced this issue Jul 21, 2023
This new check in 0.9.0 appears to have some issues with various forms
of "early return", like trap, exit and return. This is tripping up (at
least):

  cmd/zed/zed.d/history_event-zfs-list-cacher.sh
  /etc/zfs/zfs-functions

Its not obvious what its complaining about or what the remedy is, so it
seems sensible to disable this check for now.

See also:

  https://www.shellcheck.net/wiki/SC2317
  koalaman/shellcheck#2542
  koalaman/shellcheck#2613

Signed-off-by: Rob Norris <robn@despairlabs.com>
behlendorf pushed a commit to openzfs/zfs that referenced this issue Jul 21, 2023
This new check in 0.9.0 appears to have some issues with various forms
of "early return", like trap, exit and return. This is tripping up (at
least):

  cmd/zed/zed.d/history_event-zfs-list-cacher.sh
  /etc/zfs/zfs-functions

Its not obvious what its complaining about or what the remedy is, so it
seems sensible to disable this check for now.

See also:

  https://www.shellcheck.net/wiki/SC2317
  koalaman/shellcheck#2542
  koalaman/shellcheck#2613

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rob Norris <robn@despairlabs.com>
Closes #15089
behlendorf pushed a commit to behlendorf/zfs that referenced this issue Jul 21, 2023
This new check in 0.9.0 appears to have some issues with various forms
of "early return", like trap, exit and return. This is tripping up (at
least):

  cmd/zed/zed.d/history_event-zfs-list-cacher.sh
  /etc/zfs/zfs-functions

Its not obvious what its complaining about or what the remedy is, so it
seems sensible to disable this check for now.

See also:

  https://www.shellcheck.net/wiki/SC2317
  koalaman/shellcheck#2542
  koalaman/shellcheck#2613

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rob Norris <robn@despairlabs.com>
Closes openzfs#15089
behlendorf pushed a commit to openzfs/zfs that referenced this issue Jul 21, 2023
This new check in 0.9.0 appears to have some issues with various forms
of "early return", like trap, exit and return. This is tripping up (at
least):

  cmd/zed/zed.d/history_event-zfs-list-cacher.sh
  /etc/zfs/zfs-functions

Its not obvious what its complaining about or what the remedy is, so it
seems sensible to disable this check for now.

See also:

  https://www.shellcheck.net/wiki/SC2317
  koalaman/shellcheck#2542
  koalaman/shellcheck#2613

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rob Norris <robn@despairlabs.com>
Closes #15089
lundman pushed a commit to openzfsonwindows/openzfs that referenced this issue Dec 12, 2023
This new check in 0.9.0 appears to have some issues with various forms
of "early return", like trap, exit and return. This is tripping up (at
least):

  cmd/zed/zed.d/history_event-zfs-list-cacher.sh
  /etc/zfs/zfs-functions

Its not obvious what its complaining about or what the remedy is, so it
seems sensible to disable this check for now.

See also:

  https://www.shellcheck.net/wiki/SC2317
  koalaman/shellcheck#2542
  koalaman/shellcheck#2613

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rob Norris <robn@despairlabs.com>
Closes openzfs#15089
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant