koalaman / shellcheck

ShellCheck, a static analysis tool for shell scripts
https://www.shellcheck.net
GNU General Public License v3.0
36.45k stars 1.78k forks source link

[SC2295] Example itself is problematic. #3055

Closed Europia79 closed 2 months ago

Europia79 commented 2 months ago

https://github.com/koalaman/shellcheck/wiki/SC2295

Problematic code:

some_call() {
  printf '%s\n' "${2#$1}"
}

Problem: No context: Function references parameters/arguments without INTENT. In other languages, you have at least some semblance of intent via the NAMED parameter list. Here, we only have the function name, which might hint at its purpose, but does not declare valid & invalid invocations. Furthermore, the example itself does not give the function invocation, nor does it give the input state of $1 & $2. Making SC2295 impossible to understand (except for the "Choir"/"Echo Camber").

Correct code:

# $1 = <explanation & requirements>
# $2 = <explanation & requirements>
some_call() {
  printf '%s\n' "${2#$1}"
}
# ...OR...
some_call() {
  param1="$1"
  param2="$2"
  printf '%s\n' "${param2#"$param1"}"
}

Rationale:

Eventho, Bash does not have "function overloading", the concept itself still exists where you can have a family of related functions, all performing the same job, but differing in their implementation (and their parameter/argument list). A "named" parameter list (with either a local declaration, OR, with comments) can help convey necessary context.

Furthermore, SC2295 should show the state of the program both before & after the function call. Otherwise, the "explanation" is extremely cryptic to all except the most seasoned veterans.

Exception:

If that's your Target Audience, then you can ignore this suggestion.

Final Thoughts:

I cannot even begin to offer a specific example for SC2295, because when I read the code: To me, it says:

Delete 1st argument from the 2nd argument, then "print" it.

Like, okay ??? ...That's meaningless as to whether I should or should not use quotes here.

Please give the INTENT of the function AND the PURPOSE of the parameters (with the State of the arguments).

Europia79 commented 2 months ago

Sorry, apparently, this is really an example of "Why friends don't let friends code late at night", lol. ...as this is so much easier to read with the syntax highlighting of Discord or Notepad++:

image

So, this is no longer a problem with SC2295: But could potentially be a possible code issue (in general) if a function starts reading arguments by number reference without "documenting" them, either via comments, or via a local named variable (that conveys a little more information than an unnamed parameter).

I can see maybe getting away with referencing $1 since the function name should give a hint. But I think $2 or more, and it's going to need a "signature": Otherwise, it could become cryptic & hard to read, as well as hard to USE (know exactly what needs to be passed in).