Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should always use e${XXXX:-}, not ${XXXX-} #970

Closed
coolaj86 opened this issue Feb 11, 2023 · 4 comments · Fixed by #974
Closed

Should always use e${XXXX:-}, not ${XXXX-} #970

coolaj86 opened this issue Feb 11, 2023 · 4 comments · Fixed by #974

Comments

@coolaj86
Copy link

coolaj86 commented Feb 11, 2023

I noticed in a recent update (where "recent" means "recent to me" - the update could just as well be 2 years ago) that ${XXXX:-} is auto-corrected to ${XXXX-}.

Although the current change does optimize for slighty faster shell runtimes and 1-byte per instance smaller script size, this is a mistake.

It should optimize for readability and change, not file size or microticks.

If I ever go to refactor ${XXXX:-} to be ${XXXX:-default}, this will work wonderfully.

If I go to refactor ${XXXX-} to ${XXXX-default} and I'm not a shell expert, little do I know that I've just introduced a subtle bug into my program. And even if I am a shell expert, if I don't have my maximum awareness, and my screen isn't squeaky-clean from any dust specs (which is a real and practical problem I've come across on more than one occasion), I'll still introduce a bug into my script.

@mvdan
Copy link
Owner

mvdan commented Feb 12, 2023

You make a good case for reverting this change. And you're not the first either; see #871.

One thing I'll say is that this rewrite doesn't happen by default. You have to opt into it via shfmt --simplify.

You're right that saving one byte is not particularly important. The kinds of transformations that --simplify does are meant to be obviously correct and help make the code a bit easier to understand. They don't happen by default, because they aren't formatting changes, nor should they make scripts potentially harder to maintain, like the example you mention above about future refactors. I had similar worries at first, but couldn't come up with a good example.

Since I want shfmt to lean towards the conservative side in terms of altering scripts, my thinking is to revert that feature.

cc @nemchik @scop for thoughts.

@nemchik
Copy link

nemchik commented Feb 12, 2023

I would imagine that it could confuse users (like myself) who have seen countless stackoverflow articles and other documentation using the ${XXXX:-} syntax, and very few using the ${XXXX-} syntax. From the perspective of mass find/replace in the future, it would make it easier (simpler) to mass search :-} to find anywhere that has an empty default like ${XXXX:-}. Compared to \w\-\} with regex being required to find ${XXXX-}.

It hasn't presented any major challenges for me personally either way, but I don't think saving 1 character is a necessity (and in my opinion it introduces a less-than-common syntax).

@scop
Copy link
Contributor

scop commented Feb 12, 2023

To me, this change has always been about readability. I stick with my opinion that ${foo-} is more readable than ${foo:-}, and disagree that the simplification is objectively a mistake.

With regards to refactoring safety, I think this change is as "unsafe" as some others --simplify does. Some of them, such as this one, are done only if some condition holds. When refactoring after the fact so that it no longer does (such as the example given in the initial comment here), the simplification likely needs to be undone, or the thing rewritten some other way for the code to still work. So they simply aren't refactoring safe in that sense.

I do agree that :- is more commonly found in expansions than plain -. When the replacement is a non-empty string, the former is also more commonly "the right thing to do". And there is some value in being consistent with this within a script that has other :-foo uses that need to stay that way. In this particular case it doesn't make a difference wrt correctness, and I don't personally find the other arguments presented here particularly convincing or the related cases supportable all the way. So to me this basically boils down to opinions. But for sure, nothing beats not modifying in terms of being conservative. Then again, as noted, all simplifications are opt in, for a reason I presume :). Anyway, not going to lose any sleep if this gets reverted.

@mvdan
Copy link
Owner

mvdan commented Feb 26, 2023

There are good arguments both in favor and against reverting. And it's true that the "simplify" rewrites are opt-in. I still lean towards reverting on the basis that I've been using shell/bash for years, and even wrote this parser, and I still regularly forget the distinction between :- and - in parameter expansions. So I don't think we should attempt to be clever and use shorter forms, even as an opt-in feature.

mvdan added a commit that referenced this issue Feb 26, 2023
Per #970, even if the change is technically correct, it can easily lead
to confusion for users or unintentional bugs when they tweak the code
without noticing the subtlety between null (empty) and unset values.

This reverts commit e8ee47f.

Fixes #970.
mvdan added a commit that referenced this issue Mar 11, 2023
Per #970, even if the change is technically correct, it can easily lead
to confusion for users or unintentional bugs when they tweak the code
without noticing the subtlety between null (empty) and unset values.

This reverts commit e8ee47f.

Fixes #970.
jandubois added a commit to jandubois/rancher-desktop that referenced this issue Jul 11, 2023
Changing this to ${FOO-} was done by 3.6.0, but has since been reverted.
See mvdan/sh#970

Signed-off-by: Jan Dubois <jan.dubois@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants