koalaman / shellcheck

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

declare -r cannot be declared & assigned separately #900

Open maxhayward opened 7 years ago

maxhayward commented 7 years ago

For bugs

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

#!/bin/bash
# the variable is  readonly and thus must be assigned at declaration, as with 'local'
declare -r SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
echo "$SCRIPT_DIR"

Here's what shellcheck currently says:

Line 2:
declare -r SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
           ^-- SC2155: Declare and assign separately to avoid masking return values.

Here's what I wanted or expected to see:

Nothing

kurahaupo commented 7 years ago

Try assigning before making it read-only.

Of course, that makes it a three-step process if you also want to make it local inside a function.

Or just #disable that warning if you really don't care about the exit status of the sub-shell.

FOOTNOTE: it is misleading to refer to these as "variable declarations", because they do not work the way you would think of them in other languages, as applying "henceforth to the end of the code block".

Rather, they are commands, which modify the variable when they are executed, and the variable remains that way, regardless of which lines in which file are executing. Local variables are simply returned to their previous state by the act of returning from the function; until then they retain their current setting, regardless of which function is executing.

In particular, they can be conditionally executed, so it can depend on some run-time condition whether or not a variable is local, or is exported, or is read-only, or is tagged for integer evaluation, or is an associative array.

nelson6e65 commented 7 years ago

I'm having this issue too:

declare -xr SCRIPT_DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )

@kurahaupo: Try assigning before making it read-only.

Do you meant... something like following code?

declare SCRIPT_DIR
SCRIPT_DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
declare -xr SCRIPT_DIR="${SCRIPT_DIR}"
kurahaupo commented 7 years ago

Only one declare statement is necessary, and it's not necessary to reassign the value to itself. Just write:

name=value
declare -r name

If you also want it exported, that can be done at any time, before or after. But be aware that the read-only attribute only applies to the current shell; any programs that inherit it as an environment variable won't automatically have it marked read-only.

kadogo commented 6 years ago

Not really an error, but I think it's a little related.

I think that make the warning understand that declare is used to define attribute is a good idea but it make 2 questions for me now.

Cheers.

Vdragon commented 6 years ago

Want to leave my two cents, the following codelet:

init(){
    # stripped

    omegat_release_archive_basename="$(
        basename\
            --suffix=.zip\
            "${OMEGAT_RELEASE_ARCHIVE_DIRECT_DOWNLOADABLE_URL}"
    )"; declare -r omegat_release_archive_basename
    declare -r omegat_installation_prefix="${CACHE_BASE_DIRECTORY}/${omegat_release_archive_basename}"

    # stripped
}

The last command will fail(4.3.48(1)-release) with:

script: line 53: omegat_release_archive_basename: unbound variable

Adding local or declare to the assignment command works, though I have no idea why:

init(){
    # stripped

    local omegat_release_archive_basename="$(
        basename\
            --suffix=.zip\
            "${OMEGAT_RELEASE_ARCHIVE_DIRECT_DOWNLOADABLE_URL}"
    )"; declare -r omegat_release_archive_basename
    declare -r omegat_installation_prefix="${CACHE_BASE_DIRECTORY}/${omegat_release_archive_basename}"

    # stripped
}
furkanpham commented 4 years ago

Just want to make it clear that this exception for the readonly flag is already correctly made for local.

Example snippet:

#!/bin/bash
foo() {
    declare -r a="$(pwd)"
    local -r b="$(pwd)"
    echo "$a $b"
}

shellcheck result:

In line 3:
    declare -r a="$(pwd)"
               ^-- SC2155: Declare and assign separately to avoid masking return values.

This is inconsistent considering declare and local do the same thing in this context. There's no reason to having to resort to ignoring this warning or assigning before making it read-only when this warning already does not appear for local.

kurahaupo commented 4 years ago

Agreed that it's inconsistent between local and declare. And yes if you're within a function and localising a variable and making it read-only, that would imply a 3-step process: local var ; var=$( fancy stuff ) ; readonly var (or an equivalent).

Whether or not it's a good idea to both localise and make read-only, that's a whole other debate.

One of the more annoying anti-orthogonalities of the shell is that you can make a localised read-only variable, but you can't localise a variable that's already read-only.

kurahaupo commented 4 years ago

I would go further: this warning should be given if and only if the exit status of "export", "declare", "readonly" or "local" appears to be checked; that is, immediately followed by "&&", "||", ";then" or ";do".

As well as being annoyingly verbose, "assign and declare separately" can have unwanted side effects, and should not be a blanket recommendation.

which leads in some cases necessitating splitting into THREE statements to comply with this recommendation, with the attendant risks of mismatching variable name between the commands.

-- -Martin