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

set -l in `if` scope should be consistent with new for scope #4820

Closed
mqudsi opened this Issue Mar 15, 2018 · 8 comments

Comments

Projects
None yet
4 participants
@mqudsi
Copy link
Contributor

mqudsi commented Mar 15, 2018

Since #1935 has been committed, the following behavior changes from "enhancement" to "bug" since there's now an inconsistency in fish's syntax:

~> if not set -l output (somecmd && echo "success")
                      echo "command failed!" 1>&2
      end
      echo "Output is: "$output
Output is: 

@mqudsi mqudsi changed the title set -l in `if` shouldn't use inner scope? set -l in `if` scope should be consistent with new for scope Mar 15, 2018

@mqudsi mqudsi added this to the fish-3.0 milestone Mar 15, 2018

@zanchey zanchey added the bug label Mar 15, 2018

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Mar 17, 2018

@faho @zanchey are there other keywords that would need this change?

@faho

This comment has been minimized.

Copy link
Member

faho commented Mar 17, 2018

I'd imagine while and possibly begin. Of the other keywords, command and exec can't possibly set a local variable, the booleans already work like this, and the others don't take a command.

Logically, either begin is special or

begin set -l foo bar
# do stuff
end

is different from

begin
set -l foo bar
# do stuff
end
@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Mar 17, 2018

Shouldn't the first form just be illegal, like a command after end? It's too syntactically ambiguous, a ; should be required after it if trying to execute on the same line (which is how I've always used it).

@faho

This comment has been minimized.

Copy link
Member

faho commented Mar 17, 2018

In #986 we decided to just keep it as-is, as

it seems relatively harmless

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Mar 17, 2018

You are a treasure trove of fish history, @faho 👍

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Mar 18, 2018

I think #4443 is the opposite of this, a scope is being created for eval.

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Mar 31, 2018

I went ahead and fixed this for if and while but left begin alone, since there's no reason why you would set a variable inside a begin block if you wanted it outside.

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Mar 31, 2018

Awesome, thanks @ridiculousfish

oranja pushed a commit to oranja/plugin-vcs that referenced this issue Dec 29, 2018

Itzik Ephraim
Fix the `vcs.present` function loop in fish 3
In previous versions, somehow setting the same variable with `set -l`
inside a `while` loop changed the value of the variable with the same name
outside of the scope of the `while`, but in fish 3 the `-l` flag made
the inner variable truly local to the inner scope, while keeping the
outer scope variable intact, thus resulting in an infinite loop.
(might be related to changes introduced while fixing
fish-shell/fish-shell#4820)

`vcs.present` functions for both Mercurial (hg) and git were affected by
this change, resulting in infinite loops in fish 3 while initializing
VCS-aware themes (including the "default" theme).

scorphus added a commit to oh-my-fish/plugin-vcs that referenced this issue Dec 30, 2018

Fix the `vcs.present` function loop in fish 3
In previous versions, somehow setting the same variable with `set -l`
inside a `while` loop changed the value of the variable with the same name
outside of the scope of the `while`, but in fish 3 the `-l` flag made
the inner variable truly local to the inner scope, while keeping the
outer scope variable intact, thus resulting in an infinite loop.
(might be related to changes introduced while fixing
fish-shell/fish-shell#4820)

`vcs.present` functions for both Mercurial (hg) and git were affected by
this change, resulting in infinite loops in fish 3 while initializing
VCS-aware themes (including the "default" theme).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment