shellcheck icon indicating copy to clipboard operation
shellcheck copied to clipboard

Suppress SC2120, SC2119 on optional arguments

Open emsaks opened this issue 5 years ago • 14 comments

For bugs

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

#! /bin/bash
function test () {
        echo ${1-hi}
}

test

Here's what shellcheck currently says:

Line 2: function test () { ^-- SC2120: test references arguments, but none are ever passed.

Line 6: test ^-- SC2119: Use test "$@" if function's $1 should mean script's $1.

Here's what I wanted or expected to see:

Since a fallback is specified for all references to the argument ($1), the argument is obviously optional and the warning can be suppressed.

(Technically, if the code explicitly tests for a value that could also be used for suppression, but that might be too complex to implement?)

emsaks avatar Jul 29 '20 10:07 emsaks

@emsaks did you find a clean workaround yet? I am about to write test hi which seems redundant tho.

ChillerDragon avatar Dec 24 '20 13:12 ChillerDragon

shellcheck does not run the script, instead it interprets it.

You called the function without parameters and then expected a parameter, even though that parameter has a fallback. shellcheck is highlighting the function call without expected parameters.

TinCanTech avatar Dec 25 '20 00:12 TinCanTech

@TinCanTech: if the parameter has a fallback it obviously doesn't expect a parameter: it is optional.

emsaks avatar Dec 25 '20 00:12 emsaks

@TinCanTech: the truth is, I see that shellcheck only complains if there is never a function call with a referenced parameter but if you make one call with and one without it doesn't complain (which is really implied by the wording of the warning). I find that odd; to me the proper behavior would be to always complain if the is no fallback, and never complain if there is one.

emsaks avatar Dec 25 '20 00:12 emsaks

How you call the function, with or without a parameter, is exactly what shellcheck is letting you know.

TinCanTech avatar Dec 25 '20 01:12 TinCanTech

@TinCanTech: shellcheck should be warning me when there seems to be a problem/bug. If I've explicitly shown that it is OK to call the function without a parameter, why should shellcheck let me know that I did so?

emsaks avatar Dec 25 '20 01:12 emsaks

If I've explicitly shown that it is OK to call the function without a parameter, why should shellcheck let me know that I did so?

How did you make this explicit to shellcheck ?

I recommend:

#!/bin/bash

# shellcheck disable=SC2120
function test () {
        echo ${1-hi}
}

# shellcheck disable=SC2119
test

TinCanTech avatar Dec 25 '20 01:12 TinCanTech

@TinCanTech: it seems you keep missing the point. It is explicit because if the code provides a fallback, it obviously expects to be (optionally) called without a parameter - that's what the 'fallback' is for: for when the function is called without a parameter

emsaks avatar Dec 25 '20 10:12 emsaks

I understand your point and I understand shellcheck's warnings. I do not know if shellcheck should be able to detect your proposition ..

TinCanTech avatar Dec 25 '20 17:12 TinCanTech

Same issue here. It'd be great to suppress these.

example

#!/usr/bin/env bash

foo() {
	local str
	str="${1:-bar}"
	printf 'foo%s\n' "$str"
}

foobar() {
	foo
}

cornfeedhobo avatar Jan 28 '21 18:01 cornfeedhobo

I just ran into this in pycharm trying to build out a

#!/bin/bash

main() {
	 [call a bunch of other functions above]
}
main()

Pycharm offers this over the function to disable the warning:

# shellcheck disable=SC2120

banagale avatar Jan 29 '21 22:01 banagale

I understand your point and I understand shellcheck's warnings. I do not know if shellcheck should be able to detect your proposition ..

I now think the Notice which shellcheck emits is appropriate.

Silence however you see fit ..

TinCanTech avatar Jan 29 '21 23:01 TinCanTech

I think we all know how to disable it, the question is whether we all agree on if this is a "bug" or not.

I personally think this is a bug.

cornfeedhobo avatar Jan 30 '21 02:01 cornfeedhobo

The question is, do we want to be able to support functions that optionally take parameters? It seems like a pretty useful thing to me.

sourcedelica avatar Feb 03 '24 01:02 sourcedelica