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

for loop can't use an existing variable #1935

Closed
ghost opened this Issue Feb 3, 2015 · 11 comments

Comments

Projects
None yet
5 participants
@ghost
Copy link

ghost commented Feb 3, 2015

set index 1; for index in (seq 10)
  echo $index
end; echo $index
1
2
3
4
5
6
7
8
9
10
1

I thought $index would exit the for loop with 10 since I declared $index outside the loop. Why?

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Feb 3, 2015

There's a problem with the scoping here; my reading of the documentation says that it should behave as you expect.

@zanchey zanchey added this to the next-major milestone Feb 3, 2015

@ghost

This comment has been minimized.

Copy link

ghost commented Feb 3, 2015

In the meantime, this is a workaround.

  for _index in (seq $index $count)
    set index $_index
    ...
  end
@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Feb 3, 2015

for loops have historically used local scope like set -l for the iteration variable. The trouble is that they do this after the new scope has been introduced, that is, the for block is pushed on the stack, so the iteration variable is local to that block. It's been that way since fish 1.x, but I think I agree with bucaran and zanchey that the variable should live in the outer scope.

@xfix

This comment has been minimized.

Copy link
Member

xfix commented Feb 6, 2015

for loop declares its iteration variable in its own scope. It works similarly to Perl or Python 3 (but not 2, the behavior has changed in Python 3) in that respect. I wouldn't say it's a bug, because behaviour of reusing scope variable in for loop can be a potential program bug (if you let's say, accidentally reuse identifier in a loop), and loop variables are rarely useful outside of loop.

Not to say that changing this behaviour may be incompatible change, as people may have depended on current behavior by for example reusing identifiers in a loop.

use 5.012;
my $i = 5;
for $i (1..3) {
    say $i;
}
say $i;
>>> i = 5
>>> [i for i in range(1, 4)]
[1, 2, 3]
>>> i
5
>>>

As for workaround if you need last value in the loop, it's quite easy. Just assign it to unscoped variable.

set -l last
for index in (seq 10)
    set last $index
    echo $index
end
echo $last
@ghost

This comment has been minimized.

Copy link

ghost commented Feb 6, 2015

I would love this:

for set -l index 1 in (seq 10)
  echo $index
end

In the same vein of:

if set -l result (get_data)
  # ...
end
@GReagle

This comment has been minimized.

Copy link

GReagle commented Aug 4, 2015

I know that fish is not a Bourne shell and doesn't have to be compatible with anything else. However, I think it is useful to compare it to other shells' behavior. All other things being equal, consistency is good. Is there a good reason for fish to be different in this important way?

fish -c 'set i 1; for i in (seq 5); echo -n $i; end; echo $i'
123451
bash -c 'i=1; for i in $(seq 5); do echo -n $i; done; echo $i'
123455
dash -c 'i=1; for i in $(seq 5); do echo -n $i; done; echo $i'
123455
zsh -c 'i=1; for i in $(seq 5); do echo -n $i; done; echo $i'
123455
mksh -c 'i=1; for i in $(seq 5); do echo -n $i; done; echo $i'
123455

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Oct 15, 2015

No. I think we'd take a fix.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Mar 18, 2017

There is another reason to put the var in the enclosing scope. If you break out of the loop early it makes it possible to easily detect without having to introduce yet another var. However, this could only be changed in a major release and ideally we'd like to do more than one incompatible change if we're going to go to the trouble of making a major release.

@GReagle

This comment has been minimized.

Copy link

GReagle commented Mar 18, 2017

Following up on my earlier comment about being consistent with other shells, this is interesting.

This is rc by Byron Rakitzis via debian package "rc" 1.7.2-1:

/usr/bin/rc -c 'i=1; for (i in `{seq 5}) {echo -n $i}; echo $i'
123455

This is rc by Tom Duff from Plan 9 from User Space:

9 rc -c 'i=1; for (i in `{seq 5}) {echo -n $i}; echo $i'
123451

@krader1961 krader1961 self-assigned this Aug 21, 2017

@krader1961 krader1961 modified the milestones: fish-3.0, next-major Aug 21, 2017

krader1961 added a commit to krader1961/fish-shell that referenced this issue Aug 21, 2017

Don't set loop control var local to for block
This change makes fish `for` loops behave like most other shells.
Specifically, the loop control variable is now visible when the loop
terminates with the last value assigned to it.

Fixes fish-shell#1935

@krader1961 krader1961 removed their assignment Aug 25, 2017

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Aug 25, 2017

You can fix this by cherry-picking commit cc3692fb from https://github.com/krader1961/fish.

@krader1961 krader1961 closed this Aug 25, 2017

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Aug 25, 2017

That's weird. I did not expect that fixing this in my fork would close this issue. I'll be more careful in the future about mentioning issues in this fork.

@krader1961 krader1961 reopened this Aug 25, 2017

krader1961 added a commit to krader1961/fish-shell that referenced this issue Sep 5, 2017

Hoist `for` loop control var to enclosing scope
It should be possible to reference the last value assigned to a `for`
loop control var when the loop terminates. This makes it easier to detect
if we broke out of the loop among other things.  This change makes fish
`for` loops behave like most other shells.

Fixes fish-shell#1935

ridiculousfish added a commit that referenced this issue Sep 9, 2017

Hoist `for` loop control var to enclosing scope (#4376)
* Hoist `for` loop control var to enclosing scope

It should be possible to reference the last value assigned to a `for`
loop control var when the loop terminates. This makes it easier to detect
if we broke out of the loop among other things.  This change makes fish
`for` loops behave like most other shells.

Fixes #1935

* Remove redundant line
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment