Fix unbound shell variable errors on strict mode

[JoaquinTrinanes]

These errors only happen with strict_env or equivalent (set -u), and in the following cases:

• When using use flake incorrectly, for example passing a flag as first and only argument: use flake --impure
• When using use nix with flags that require extra arguments, without the arguments: use nix -A, use nix -o optionName, use nix -o...

[bbenne10]

How can we catch this moving forward? Can we enable this in shellcheck or something?

[JoaquinTrinanes]

I was surprised that shellcheck didn't warn about this, and as far as I could tell there's no way to make it work that way [1, 2]. I can maybe add tests checking for this behaviour, but I don't think it's ideal given that it would only detect the specific bad cases we write.

I'll play around with shellchecks rules and see if there's one that achieves this.

[bbenne10]

Should we simply add set -u to the top of the direnvrc and always invoke with some equivalence of "strict mode"?

[JoaquinTrinanes]

Should we simply add set -u to the top of the direnvrc and always invoke with some equivalence of "strict mode"?

If it was a standalone script I'd say it would make sense, but in this case I think this should be opted-in by the user, for example via direnv's stdlib (strict_env and unstrict_env). And even if we always ran it in strict mode we wouldn't detect unbound variables outside of the happy path (of the errors fixed on this PR, one is constructing an error message and the others are when accessing missing arguments for specific flags).

Even more, the project's own .envrc already enables strict mode, so most contributors are probably already using it without encountering these issues (?)

I couldn't find anything useful for this in the shellcheck rules, so maybe enforcing it during code review is enough. For what I see, most tests run with strict mode on and off, so the happy path should always be correct in that regard.

[bbenne10]

If we're fixing these errors, we should also attempt to avoid introducing these errors in the first place. I have been convinced that set -u is not appropriate (silly bash. Namespaces are good! We should use more of them!), but I don't know what to do going forward. shellcheck has SC2154 which should help here, but it isn't triggering (for instance, I think it should find the value of "$direnv" being referenced without having been assigned, but it is not - presumably because of the check on 57 above?)

I'll keep searching for some way of getting some tooling to complain about this staticallly, but in the meantime - there's no use standing on principle and I'll merge.

[Mic92]

We could in our shellcheck run prepend set -u to direnvrc without doing it in our code otherwise.

[bbenne10]

The problem with that approach is that it only catches runtime errors. I want shellcheck (or something like it) to figure this out statically rather than relying on execution to move through the script branch by branch. We should've caught these already, but because they're in error handling, we didn't (since we already know how to properly invoke nix-direnv).

That might not be a bad approach in general, but I think we maybe also want to add error cases that fail in argument handling? I'm not entirely sure yet and will have to think on this a bit.

[Mic92]

is shellcheck not getting more strict if you add set -u?

[JoaquinTrinanes]

@bbenne10

If we're fixing these errors, we should also attempt to avoid introducing these errors in the first place

100% agree

shellcheck has SC2154 which should help here, but it isn't triggering (for instance, I think it should find the value of "$direnv" being referenced without having been assigned, but it is not - presumably because of the check on 57 above?)

First of all, the rule specifies:
Note: This message only triggers for variables with lowercase characters in their name (foo and kFOO but not FOO) due to the standard convention of using lowercase variable names for unexported, local variables.

So it will never work with $1 or $SOME_UPPERCASE_VAR and similar. Sad.

But $direnv isn't triggering the warning. I dig some digging and found that this standalone file triggers the warning (as expected)

# -*- mode: sh -*-
# shellcheck shell=bash

echo $direnv # triggers the warning

However, this file doesn't trigger any warning:

# -*- mode: sh -*-
# shellcheck shell=bash

echo $direnv # triggers the warning

if [[ -z $direnv ]]; then
  :
fi

On the other hand, executing this file results in an unbound variable error (without warnings, set -u doesn't change the strictness of the checks):

# -*- mode: sh -*-
# shellcheck shell=bash

set -u

if [[ -z $direnv ]]; then
  :
fi

So, can this rule help us avoid these errors? Yes. Just not all of them. And the moment we reference a variable inside a conditional all the checks and warnings for that variable magically disappear.

Potential improvement

I just read about the check-unassigned-uppercase rule. It can be enabled like so:

# -*- mode: sh -*-
# shellcheck shell=bash
# shellcheck enable=check-unassigned-uppercase

# ... rest of the file ...

It still doesn't work for arguments ($1 and friends), and it has the same issues as the lowercase check. Enabling it doesn't show any warning because any potentially unset variable is inside an if condition at some point in the code (for example, DIRENV_LOG_FORMAT).

---

@Mic92

We could in our shellcheck run prepend set -u to direnvrc without doing it in our code otherwise.

The project's .envrc already calls strict_env, so anyone developing using that devshell will be using strict mode 🤔

is shellcheck not getting more strict if you add set -u?

It doesn't look like it. Funnily enough, while looking for it I found a report of the exact same issue I described above koalaman/shellcheck#2779 (comment)