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

Shebang warning should give different advice #1858

Closed
2 of 4 tasks
dseynhae opened this issue Mar 4, 2020 · 7 comments
Closed
2 of 4 tasks

Shebang warning should give different advice #1858

dseynhae opened this issue Mar 4, 2020 · 7 comments

Comments

@dseynhae
Copy link

dseynhae commented Mar 4, 2020

For bugs

  • Rule Id (if any, e.g. SC1000): SC2148
  • My shellcheck version (shellcheck --version or 'online'): online
  • I tried on shellcheck.net and verified that this is still a problem on the latest commit
  • It's not reproducible on shellcheck.net, but I think that's because it's an OS, configuration or encoding issue

For new checks and feature suggestions

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

# shellcheck disable=SC2148
export SHELLCHECK=1

Here's what shellcheck currently says:

Line 1	SC2148: Tips depend on target shell and yours is unknown. Add a shebang.

Here's what I wanted or expected to see:

SC2148: Tips depend on target shell and yours is unknown.
For an executable script add a shebang (#!<full_path>/<shell>).
For a sourced script add a pseudo-shebang (#!/<shell>).

I tried to ignore the tip with

# shellcheck disable SC2148

But that doesn't work (the tip still shows up). Maybe this should turn of the tip after all?

@gromgit
Copy link

gromgit commented Mar 4, 2020

Shellcheck's output tips are one-liners so they can be "stacked" efficiently when a single script line contains multiple issues. Making the tips more verbose clutters the output while adding minimum utility; explanations, detailed hints and alternatives should go in the corresponding wiki page, where they can be tailored/enhanced/expanded without having to re-release shellcheck itself.

I tried to ignore the tip with

# shellcheck disable SC2148

But that doesn't work (the tip still shows up).

Aside from a missing =, SC2148 is red for a reason: It's an error, not a warning or something less serious. Disabling errors kinda defeats the purpose of shellcheck.

@dseynhae
Copy link
Author

dseynhae commented Mar 4, 2020

The actual code that I put in originally does have the '=' sign, sorry for transcribing this wrong in my later comments.
I understand that we should pay attention to the feedback. However I consider my scripts "done", if it doesn't list any issues. And that means I want to locally shut up "false" errors.

For this specific case: I know that spellcheck is not aware of the specific shell... But I consider putting a shebang in a script that is supposed to be sourced an error in itself...

I just found out about the 'shell' directive, and consider the following code more appropriate to solve my dilemma:

# shellcheck shell=bash
export SHELLCHECK=1

I agree that pursuing one-liners keeps the tool readable. But shouldn't the error message assume that a shebang is not needed, and suggest:

Line 1	SC2148: Tips depend on target shell and yours is unknown. Add a 'shell' directive.

@gromgit
Copy link

gromgit commented Mar 4, 2020

But shouldn't the error message assume that a shebang is not needed

Shellcheck has no way of knowing how you intend to use the code it's checking (executable script or sourced file), and the consequences w.r.t. shebangs (or lack thereof) are very different:

  1. If you give shellcheck a sourced file with a shebang, it'll happily check against the specified shell, exactly as if you'd specified a shell directive. No harm done.
  2. If you give shellcheck a script without a shebang, but with a shell directive, shellcheck will check against the shell you specify. But since there's no shebang, your OS will default to /bin/sh for your script, so it'll likely crash at the first bashism. Not good.

Therefore, rewording SC2148 to recommend adding a "shell" directive instead of a shebang is not a good idea.

@dseynhae
Copy link
Author

dseynhae commented Mar 4, 2020

Counterpoints

  1. Your first point is true (a shebang doesn't prevent a script from being sourced), but I think there is harm after all.

If the script has a shebang, the shebang will make the script "executable" (granted, you do need execution privileges as well, but some people are careless with their umask).
It definitely gives the illusion that the intent of the author is to execute the script.

So that is why I never put a shebang in a script that is meant to be sourced; And I also change the permissions to non-executable.

Therefore, if I don't have a shebang in my script, that's not an error:

  • First of all, the shell specifications do not require it!
  • And it is my full intention, to clearly indicate that you should NOT execute this. I do not want the semantics of a shebang to kick in...

Therefore, shellcheck should not assume that I forgot the shebang. I believe it is harmful, for my purposes.

  1. Your second point definitely gives cause to more reflection.
    I always thought that when you source a script, it will be sourced in the context of your active shell. That is not necessarily /bin/sh (it is up to your /etc/passwd configuration). So it can be sh, csh, tcsh, bash, ksh,...
    So, the 'shell' directive is to let shellcheck know which shell the calling shell will be.

Which in my case is bash (we document our scripts that way). And the 'shell' directive is actually the most appropriate reinforcement of this requirement.

Conclusion

  • Not having a shebang is not an error. It is not illegal. It is a desired feature.
  • When the author of a script doesn't have a shebang, assume that they don't want one. And the reminder to put a 'shell' directive is the most appropriate way to solve the conflict.
  • Even if the author forgot the shebang, the directive instruction will trigger them to add the shebang after all.

@austin987
Copy link
Contributor

Or a compromise solution:
SC2148: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

@dseynhae
Copy link
Author

dseynhae commented Mar 4, 2020

Or a compromise solution:
SC2148: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.

Seems like this is not a compromise. It is a superior recommendation!
:-)

@koalaman
Copy link
Owner

A shell directive is obviously the best choice, but for the principle of least astonishment, you're now additionally able to disable the warning with a directive.

The fact that you couldn't was just a bug in the AST traversal: it didn't visit the shebang, and therefore failed to associate the shebang warnings with the script's list of directives.

Kangie pushed a commit to Kangie/shellcheck that referenced this issue Feb 27, 2023
Merge of upstream through 5cf2c00

Conflicts:
	src/ShellCheck/Analytics.hs

Manual changes:
	Minor conflicts in variable lists for portage

Changelog:
----------------------------------------------------------------
Artur Klauser (4):
      Add multi-architecture Docker image build
      Don't try to deploy docker images on PR runs
      Run "deploy" step only for "Build" stages
      Use shellcheck on yourself

Austin English (1):
      src/ShellCheck/Analytics.hs: suggest using a shell directive for SC2148

Benjamin Gordon (1):
      Merge 'upstream/master' into chromeos-0.7

Joseph C. Sible (82):
      Simplify literalEquals
      Use findM instead of filterM
      Use the Identity monad to avoid unnecessary uses of fromJust
      Remove more unnecessary uses of fromJust
      Switch getLiteralStringExt to Identity where it can never be Nothing
      Add getLiteralStringDef and simplify with it
      Replace mapMaybe and concatMap with list comprehensions
      Use mapM instead of implementing a slower version of it
      Use find instead of take 1 and filter
      Use mapM_ and sequence_ instead of reimplementing them
      Simplify check and checkTranslatedStringVariable
      Get rid of potentially
      Use head instead of reimplementing it
      Use void instead of do and return ()
      Simplify matchToken
      Simplify <> for SpaceStatus
      Inline an uncurry
      Avoid unnecessary use of when and unless
      Simplify findFunction
      Use Map.! instead of reimplementing it
      Simplify a double negative
      Do toLower earlier
      Remove unnecessary fromMaybes
      Avoid a zip that breaks fusion
      Use forM_ instead of reimplementing it
      Use isNothing instead of reimplementing it
      Use Map.member instead of isJust and Map.lookup
      Simplify determineShell
      Remove unnecessary uses of head
      Use isJust instead of reimplementing it
      Simplify shellFromFilename
      Simplify mockedSystemInterface
      Remove a partial pattern match equivalent to fromJust from checkFindNameGlob
      Optimize patterns in checkFindNameGlob
      Use headOrDefault instead of fromMaybe and listToMaybe
      Simplify checkForInQuoted
      Simplify checkWhileReadPitfalls
      Mark that base >= 4.8.0.0 is required
      Fix koalaman#1892: Use pattern synonyms to clean up AST
      Simplify nameExpansion
      Use force instead of reimplementing it
      Remove unnecessary Maybe from isQuoteFreeElement
      Implement findFirst in terms of foldr
      Use execState instead of reimplementing it
      Simplify causesSubshell
      Use a list comprehension instead of a concatMap with extra lists
      Remove unnecessary maybeToList
      Remove unnecessary as-patterns
      Remove an unnecessary operator section
      Simplify isArrayFlag
      Use head instead of (!! 0)
      Simplify dropPrefix
      Simplify getSpecial
      Don't bother with asks if you're just immediately binding the result anyway
      Implement supportsArrays with pattern-matching
      Simplify process
      Simplify getCommandNameAndToken
      Simplify getAssociativeArrays
      Remove unnecessary monadicity from wordToPseudoGlob
      Remove unnecessary cases from wordToPseudoGlob
      Only perform the comparisons once
      Use foldr in checkFindNameGlob
      Use pattern matching instead of snd
      Use a guard instead of unless
      Prefer pattern matching in undirected
      Simplify checkArg
      Use MultiWayIf instead of case-matching on ()
      Simplify checkSetAssignment
      Simplify warnRedundant
      Make skipRepeating lazier and faster
      Make it slightly lazier still (and more clear)
      Write getLiteralArgs with foldr and without fromMaybe or monads
      Remove unnecessary fromMaybe and when from bashism
      Use fromRight instead of reimplementing it
      Avoid some awkward parentheses with forM_
      Simplify thenSkip, and use in another location
      Simplify checkVariableBraces
      Combine bracedString into getSingleUnmodifiedVariable
      Get rid of bracedString everywhere it's easy to
      Move bracedString to be local to its last use site
      Clean up and optimize getSuspiciousRegexWildcard
      Revert "Use fromRight instead of reimplementing it"

Marcin Szydelski (1):
      SC2016: disable for mumps -run %XCMD and LOOP%XCMD

Vidar Holen (59):
      Merge pull request koalaman#1828 from josephcsible/cleanups
      Merge pull request koalaman#1827 from josephcsible/nofromjust2
      Merge pull request koalaman#1826 from josephcsible/nofromjust
      Merge pull request koalaman#1825 from josephcsible/nofilterm
      Merge pull request koalaman#1824 from josephcsible/patch-1
      Merge pull request koalaman#1802 from szydell/master
      Merge pull request koalaman#1785 from ArturKlauser/multi-arch-docker
      Merge pull request koalaman#1831 from josephcsible/checkfindnameglob
      Don't try to deploy on PRs
      Rename 'Test' stage
      Parse keywords with case sensitivity (fixes koalaman#1809)
      Bump SC1102/SC1105 about ambiguous `$((` to Error (fixes koalaman#1836)
      Remove unused instance Ord Replacement (fixes koalaman#1829)
      Inspect 'alias' commands for referenced variables (Fixes koalaman#1832)
      SC2257: Warn when changing arithmetic variables in redirections
      Upload to assets to GitHub
      Merge pull request koalaman#1862 from austin987/sc2148-shell-directive
      Fix TravisCI condition
      Refer to GitHub rather than GCS for release builds
      Make SC2095 (ssh in while read loops) more robust and suggest fixes
      Merge pull request koalaman#1865 from josephcsible/patch-1
      Improve detection of for loops with single values
      Try to make TravisCI not fail on deployment of Docker stage
      Include shebang in AST traversal (fixes koalaman#1858)
      Stop deploying artifacts to GCS
      Update distro tests to support newer Cabal
      Merge pull request koalaman#1893 from josephcsible/pattern-synonyms
      Merge pull request koalaman#1872 from josephcsible/checkforinquoted
      Merge pull request koalaman#1873 from josephcsible/checkwhilereadpitfalls
      Merge pull request koalaman#1880 from josephcsible/patch-1
      Merge pull request koalaman#1885 from ArturKlauser/travis-pr-fix
      Merge pull request koalaman#1897 from ArturKlauser/use-shellcheck-on-yourself
      Merge pull request koalaman#1896 from ArturKlauser/travis-deploy-stage-fix
      Merge pull request koalaman#1876 from fork-graveyard/master
      Don't warn about [ 0 -ne $FOO ] || [ 0 -ne $BAR ] (fixes koalaman#1891)
      Stable version v0.7.1
      Update Changelog with new version
      Filter GitHub uploads by tag
      Disable SC2257 about > $((i=42)) for Dash
      Merge pull request koalaman#1898 from josephcsible/nameexpansion
      Merge pull request koalaman#1900 from josephcsible/analyzerlib
      Merge pull request koalaman#1902 from josephcsible/astlib
      Merge pull request koalaman#1904 from josephcsible/commands
      Merge pull request koalaman#1903 from josephcsible/fixer
      Merge pull request koalaman#1901 from josephcsible/bracedstring
      Merge pull request koalaman#1905 from josephcsible/skiprepeating
      Merge pull request koalaman#1906 from josephcsible/shellsupport
      Merge pull request koalaman#1907 from josephcsible/formatters
      Merge pull request koalaman#1917 from josephcsible/thenskip
      Merge pull request koalaman#1918 from josephcsible/getsuspiciousregexwildcard
      Merge pull request koalaman#1925 from josephcsible/nofromright
      Merge pull request koalaman#1927 from scop/sc-prefix
      Merge pull request koalaman#1926 from scop/spelling
      Warn about duplicate uses of stdin/out/err
      Improve SC2259/60/61 messages
      Allow disabling SC1072/SC1073 with annotations (fixes koalaman#1931)
      Merge pull request koalaman#1950 from geeseven/aur-shellcheck-bin
      Count $# as an argument reference in SC2120
      Warn about defining and using an alias in a single command (fixes koalaman#1807)

Ville Skyttä (2):
      Spelling fixes
      Use SC prefix for disable= in man page

geeseven (1):
      update dependency free AUR package

girst (1):
      recognize `: ${parameter=word}` as assignment

 .compile_binaries                     |   6 -
 .github_deploy                        |  58 +++
 .multi_arch_docker                    | 113 +++++
 .travis.yml                           |  34 +-
 CHANGELOG.md                          |  20 +-
 Dockerfile                            |   7 -
 Dockerfile.multi-arch                 |  26 ++
 README.md                             |  17 +-
 ShellCheck.cabal                      |   6 +-
 nextnumber                            |   2 +-
 shellcheck.1.md                       |   6 +-
 shellcheck.hs                         |  12 +-
 src/ShellCheck/AST.hs                 | 542 ++++++++++--------------
 src/ShellCheck/ASTLib.hs              |  83 ++--
 src/ShellCheck/Analytics.hs           | 748 +++++++++++++++++++++++-----------
 src/ShellCheck/AnalyzerLib.hs         | 185 ++++-----
 src/ShellCheck/Checker.hs             |  18 +-
 src/ShellCheck/Checks/Commands.hs     | 162 ++++----
 src/ShellCheck/Checks/ShellSupport.hs |  44 +-
 src/ShellCheck/Fixer.hs               |  16 +-
 src/ShellCheck/Formatter/TTY.hs       |   5 +-
 src/ShellCheck/Interface.hs           |  11 +-
 src/ShellCheck/Parser.hs              |  55 ++-
 test/buildtest                        |  23 +-
 test/check_release                    |   3 +-
 test/distrotest                       |  22 +-
 26 files changed, 1286 insertions(+), 938 deletions(-)
 create mode 100755 .github_deploy
 create mode 100755 .multi_arch_docker
 create mode 100644 Dockerfile.multi-arch

BUG=chromium:1086928
TEST=stack test

Cq-Depend: chromium:2233790
Change-Id: Icaa54e0a17181685f1f8234a9ab811bc24229338
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

4 participants