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

3.1b1: The consumption of whitespace inside braces has changed weirdly since 3.0 #6564

Closed
Phidica opened this issue Feb 1, 2020 · 2 comments · Fixed by #6565
Closed

3.1b1: The consumption of whitespace inside braces has changed weirdly since 3.0 #6564

Phidica opened this issue Feb 1, 2020 · 2 comments · Fixed by #6565
Milestone

Comments

@Phidica
Copy link
Contributor

@Phidica Phidica commented Feb 1, 2020

Tried using tagged release 3.1b1 as well as current master (1101cff) built from source on Fedora 30, running in Gnome Terminal.

I just noticed this while responding to #5880.

In 3.0, extra whitespace inside of braces was consumed from both sides:

$ echo .{  foo  bar  }.
.foo  bar.

In 3.1b1, (perhaps due to the changes from #5869?) we trim whitespace but only from the front:

$ echo .{  foo  bar  }.
.{foo  bar  }.

Honestly, I'm not sure what should be happening. Do we want to preserve everything inside of braces that don't contain a comma/variable? Or, do we still trim spaces from the front and back like we do when there is a comma/variable? Consider that

$ echo .{  foo  ,  bar  }.
.foo. .bar.

still comes out symmetric in 3.1b1.

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Feb 2, 2020

Well spotted, bisects to 967c1d5. IMO this should block 3.1.

@ridiculousfish ridiculousfish added this to the fish 3.1.0 milestone Feb 2, 2020
@mqudsi
Copy link
Contributor

@mqudsi mqudsi commented Feb 2, 2020

Honestly, I'm not sure what should be happening. Do we want to preserve everything inside of braces that don't contain a comma/variable?

If the braces are not being treated as special (and that was what @faho was going for, I believe), then they should be treated the same way as any other whitespace.

There is a bigger problem here: the tokenization is borked.

This is the output with brace expansion as of 3.0 (and even now):

mqudsi@Blitzkrieg /m/d/G/fmath> printf "\"%s\"\n" { line 1, line 2 }
"line 1"
"line 2"

Both leading and trailing unescaped whitespace is removed, unescaped intra-word whitespace is coalesced, and tokens are split by literal , rather than whitespace.

(EDIT: it seems that intraword whitespace is not being coalesced. I'm not sure if this is how 3.0 shipped or if this is a regression?)

In 967c1d5, I believe @faho intended to remove all special treatment of braces containing a single, non-variable item. It's not (just) a matter of whitespace only being trimmed from the front, the tokenization is completely wrong as (based off the original issue that was trying to be fixed and everything that I can see) the output should be as if the brace were escaped, i.e. no special casing whatsoever.

mqudsi@Blitzkrieg /m/d/G/fmath> printf "\"%s\"\n" { hello world }
"{hello world }"
mqudsi@Blitzkrieg /m/d/G/fmath> printf "\"%s\"\n" \{ hello world \}
"{"
"hello"
"world"
"}"

I would expect the two outputs to be identical, otherwise we have introduced considerable complexity into the language (which is one of the reasons I was against special-casing a single- or no-item brace bair, as while it may make the language more friendly, it increases the cognitive cost of understanding the language itself), and instead of having two possible states (brace expansion and no brace expansion) there are now three (brace expansion, no brace expansion, and "brace expansion but printing the output with the braces preserved").

ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Feb 3, 2020
This makes two changes:

1. Remove the 'brace_text_start' idea. The idea of brace_text_start is to
prevent emitting BRACE_SPACE at the beginning or end of an item. But we
later strip these off anyways, so there is no apparent functional change.
If we are not doing brace expansion, this prevented emitting whitespace at
the beginning or end of an item, leading to fish-shell#6564.

2. When performing brace expansion, only stomp the space character with
`BRACE_SPACE`; do not strip newlines and tabs. This is because the fix in
newline or tab literal, then we would have changed the user's input. It is
not easy to place a literal newline or tab inside a brace expansion, so
users who do probably do not mean for it to be stripped.

Fixes fish-shell#6564
ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Feb 3, 2020
This makes two changes:

1. Remove the 'brace_text_start' idea. The idea of 'brace_text_start' was
to prevent emitting `BRACE_SPACE` at the beginning or end of an item. But
we later strip these off anyways, so there is no apparent benefit. If we
are not doing brace expansion, this prevented emitting whitespace at the
beginning or end of an item, leading to fish-shell#6564.

2. When performing brace expansion, only stomp the space character with
`BRACE_SPACE`; do not stomp newlines and tabs. This is because the fix in
came from a newline or tab literal, then we would have effectively
replaced a newline or tab with a space, so this is important for fish-shell#6564 as
well. Moreover, it is not easy to place a literal newline or tab inside a
brace expansion, and users who do probably do not mean for it to be
stripped, so I believe this is a good change in general.

Fixes fish-shell#6564
ridiculousfish added a commit that referenced this issue Feb 4, 2020
This makes two changes:

1. Remove the 'brace_text_start' idea. The idea of 'brace_text_start' was
to prevent emitting `BRACE_SPACE` at the beginning or end of an item. But
we later strip these off anyways, so there is no apparent benefit. If we
are not doing brace expansion, this prevented emitting whitespace at the
beginning or end of an item, leading to #6564.

2. When performing brace expansion, only stomp the space character with
`BRACE_SPACE`; do not stomp newlines and tabs. This is because the fix in
came from a newline or tab literal, then we would have effectively
replaced a newline or tab with a space, so this is important for #6564 as
well. Moreover, it is not easy to place a literal newline or tab inside a
brace expansion, and users who do probably do not mean for it to be
stripped, so I believe this is a good change in general.

Fixes #6564
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants