shellcheck icon indicating copy to clipboard operation
shellcheck copied to clipboard

SC2115 fires when it is statically verifyable that the variable cannot be empty

Open benblank opened this issue 1 year ago • 4 comments
trafficstars

For bugs

  • Rule Id: SC2115
  • My shellcheck version: 0.10.0
  • [x] The rule's wiki page does not already cover this (e.g. https://shellcheck.net/wiki/SC2115)
  • [x] I tried on https://www.shellcheck.net/ and verified that this is still a problem on the latest commit

I had assumed this rule would work similarly to SC2086, in that it wouldn't trigger if the variable's declaration were available and could not lead to the described problem occurring. In the example below, STEAMROOT is declared within the script being checked and cannot be empty even if HOME is.

Here's a snippet that shows the problem:

#!/bin/sh

STEAMROOT=$HOME/.local/share/Steam

rm -rf "$STEAMROOT/"*

Here's what shellcheck currently says:

Line 5:
rm -rf "$STEAMROOT/"*
       ^-- SC2115 (warning): Use "${var:?}" to ensure this never expands to /* .

Here's what I expected to see:

No issues detected!

benblank avatar Apr 20 '24 18:04 benblank

Personally I think it is always advisable to use "${var:?}".

Put this case:

STEAMROT=$HOME/.local/share/Steam
rm -rf "$STEAMROOT/"*

In this example there is a missing letter in the variable declaration, and this cause the second commands to wipe all your files.

ale5000-git avatar Apr 20 '24 23:04 ale5000-git

There's definitely a case to be made for safe habits. SC2086 has a corresponding optional/"strict" check, SC2248. Potentially, an optional version of this check could be useful, as well? I do think it would be nice to take advantage of the extra information by default.

In the end, I certainly don't feel that the current behavior is bad, simply that it could perhaps be refined. This issue is really just the result of having been surprised by SC2115's behavior compared to SC2086. 🙂

As for the typo, that would still trigger SC2034 for the unused variable and SC2153 for the undefined variable, both of which I feel are more relevant to having made a typo than not knowing whether the misspelled variable is empty.

benblank avatar Apr 21 '24 18:04 benblank

Personally I think it is always advisable to use "${var:?}".

I agree. The results could be catastrophic, so extra precautions to prevent them are fine I think. I would not trust myself to NEVER misspell a variable name.

Jakub-Kapusta avatar Oct 04 '24 04:10 Jakub-Kapusta

It's something that could in theory become a maintenance issue later on, ie, years down the line. If the assignment to ENVVAR was ever commented / deleted / moved / reassigned / promoted / divorced / retired / deceased in such a way that rm could ever possibly operate on '/*'...? At least, as a failsafe "${foo:?}" is twice as hard to mis-spell than "${foo?}". ;-)

Seems relevant: https://news.ycombinator.com/item?id=19626959

On Thu, Oct 3, 2024, 21:29 Jakub Kapusta @.***> wrote:

Personally I think it is always advisable to use "${var:?}".

I agree. The results could be catastrophic, so extra precautions to prevent them are fine I think. I would not trust myself to NEVER misspell a variable name.

— Reply to this email directly, view it on GitHub https://github.com/koalaman/shellcheck/issues/2968#issuecomment-2392783551, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUF2F2Y5N4JU7TCM5FSPK63ZZYKRJAVCNFSM6AAAAABGQWNVXKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJSG44DGNJVGE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

wileyhy avatar Oct 04 '24 04:10 wileyhy