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

Don't treat out-of-bounds differently than undefined variables #826

Closed
ghost opened this Issue May 24, 2013 · 22 comments

Comments

Projects
None yet
@ghost

ghost commented May 24, 2013

set a b; echo $a[1] yields b, while set a b; echo $a[2] yields the error fish: Could not expand string “$a[2]”.

Meanwhile, echo $a[2] without setting anything simply yields an empty string. This seems kind of like an odd behaviour, and there's no reason that there should be an error if you go out of bounds on an array when there isn't one for undefined variables.

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish May 28, 2013

Member

I think I agree.

Member

ridiculousfish commented May 28, 2013

I think I agree.

@dag

This comment has been minimized.

Show comment
Hide comment
@dag

dag May 29, 2013

Contributor

#805 is relevant.

Contributor

dag commented May 29, 2013

#805 is relevant.

@JeanMertz

This comment has been minimized.

Show comment
Hide comment
@JeanMertz

JeanMertz Jun 4, 2013

I am running into this myself. So how can I work around this right now? Say I have a function that accepts multiple arguments, but they are not required.

If I do

test -n "$argv[2]"; set -x MYVAR $argv[2]

this will fail with the fish: Could not expand string “$argv[2]” error. I haven't found a way to only set MYVAR if $argv[2] is set. I'm sure I am missing something obvious here.

JeanMertz commented Jun 4, 2013

I am running into this myself. So how can I work around this right now? Say I have a function that accepts multiple arguments, but they are not required.

If I do

test -n "$argv[2]"; set -x MYVAR $argv[2]

this will fail with the fish: Could not expand string “$argv[2]” error. I haven't found a way to only set MYVAR if $argv[2] is set. I'm sure I am missing something obvious here.

@maxfl

This comment has been minimized.

Show comment
Hide comment
@maxfl

maxfl Jun 4, 2013

Contributor

you can use 'set -q' to test variable

set -q argv[2]
Contributor

maxfl commented Jun 4, 2013

you can use 'set -q' to test variable

set -q argv[2]
@terlar

This comment has been minimized.

Show comment
Hide comment
@terlar

terlar Jun 4, 2013

Contributor

Another way is to use the read function:

echo $argv | read -l first second last
test -n "$second"; and set -x MYVAR $second

This will assign the variables if they exist and if you have more they will all be assigned to the last one. The usefulness of this depends on your use case of course.

Contributor

terlar commented Jun 4, 2013

Another way is to use the read function:

echo $argv | read -l first second last
test -n "$second"; and set -x MYVAR $second

This will assign the variables if they exist and if you have more they will all be assigned to the last one. The usefulness of this depends on your use case of course.

@JeanMertz

This comment has been minimized.

Show comment
Hide comment
@JeanMertz

JeanMertz Jun 4, 2013

Thanks for the help guys. I did try set -q argv[2] before, but it failed to work, not I noticed I forgot the and after the ;, making the test not relevant anymore.

Also, thanks for the read solution @teriar, I ended up using that as well. It looks nice and clean.

JeanMertz commented Jun 4, 2013

Thanks for the help guys. I did try set -q argv[2] before, but it failed to work, not I noticed I forgot the and after the ;, making the test not relevant anymore.

Also, thanks for the read solution @teriar, I ended up using that as well. It looks nice and clean.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 25, 2013

I'm going to bump this to add that invalid slices should also fit under this category. It'd make it easier to make $argv[1] go to one command and $argv[2..-1] go to another even if you only supply one argument.

ghost commented Jul 25, 2013

I'm going to bump this to add that invalid slices should also fit under this category. It'd make it easier to make $argv[1] go to one command and $argv[2..-1] go to another even if you only supply one argument.

@xfix

This comment has been minimized.

Show comment
Hide comment
@xfix

xfix Jul 26, 2013

Member

Invalid slices should return empty array in my opinion. There are some reasons to use invalid slices. For example, if you have some function that takes infinite arguments from second position, $argv[2..-1] (or even $argv[2..], perhaps...) would be very logical, but it doesn't work with argv containing just one element.

It would cause $argv[2] and $argv[2..2] to do different stuff, but in my opinion, it isn't a problem. For prior art, see Python or Perl 6 (but in Perl 6, it only affects ranges with negative ending, like @array[1..*-1] (or even @array[1..*])).

Member

xfix commented Jul 26, 2013

Invalid slices should return empty array in my opinion. There are some reasons to use invalid slices. For example, if you have some function that takes infinite arguments from second position, $argv[2..-1] (or even $argv[2..], perhaps...) would be very logical, but it doesn't work with argv containing just one element.

It would cause $argv[2] and $argv[2..2] to do different stuff, but in my opinion, it isn't a problem. For prior art, see Python or Perl 6 (but in Perl 6, it only affects ranges with negative ending, like @array[1..*-1] (or even @array[1..*])).

@maxfl

This comment has been minimized.

Show comment
Hide comment
@maxfl

maxfl Jul 26, 2013

Contributor

@glitchmr, agree about slices, I will update the code on this.

Contributor

maxfl commented Jul 26, 2013

@glitchmr, agree about slices, I will update the code on this.

@brandonweiss

This comment has been minimized.

Show comment
Hide comment
@brandonweiss

brandonweiss Apr 10, 2014

Any word on this? I just got bit by it.

brandonweiss commented Apr 10, 2014

Any word on this? I just got bit by it.

@davidpelaez

This comment has been minimized.

Show comment
Hide comment
@davidpelaez

davidpelaez Jun 19, 2014

Same for me, the notion of default empty slice should be used, pretty much the same way that echo $undefinedsomething yields an empty string 👍

davidpelaez commented Jun 19, 2014

Same for me, the notion of default empty slice should be used, pretty much the same way that echo $undefinedsomething yields an empty string 👍

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jun 28, 2014

#1474 also goes along with this.

ghost commented Jun 28, 2014

#1474 also goes along with this.

@kballard

This comment has been minimized.

Show comment
Hide comment
@kballard

kballard Sep 5, 2014

Contributor

In current master the set vs unset variable inconsistency is gone. $a[2] is an error when $a doesn't exist just as it is when $a does exist and has 0 or 1 elements. Note that $a[1] is always valid even if $a has zero elements, primarily because it's just too annoying to have that be an error.


I talked about this issue in #1656. I would very much like to get rid of all "out of bounds" errors. I believe that ranges and indexes need to behave the same though, so $foo[2] is identical to $foo[2..2]. The two options for handling this:

  1. Any out of bounds index evaluates to the empty string "". Note that $foo[5..-1] where $foo has fewer than 5 elements will evaluate to an empty range and therefore zero elements.
  2. Out of bounds indexes evaluate to nothing.

At this point I think we should go with approach 2, as I think it makes more sense, and it's consistent with the behavior of $a[1] today.


A third option that extends approach 2 is to add syntax to denote the default value that should be used for any indexes that don't exist. This could always be added later of course. My worry with this is that it adds complexity, but the motivation for doing so is that, while you could write a function for providing a default for a single value (default val $var), you can't do so for ranges or index collections because you wouldn't know where to insert it into the result. This could possibly look like $foo[1..3 or '']. I think it's worth considering this, but at this point I think it should be deferred and re-considered at some point in the future.

Contributor

kballard commented Sep 5, 2014

In current master the set vs unset variable inconsistency is gone. $a[2] is an error when $a doesn't exist just as it is when $a does exist and has 0 or 1 elements. Note that $a[1] is always valid even if $a has zero elements, primarily because it's just too annoying to have that be an error.


I talked about this issue in #1656. I would very much like to get rid of all "out of bounds" errors. I believe that ranges and indexes need to behave the same though, so $foo[2] is identical to $foo[2..2]. The two options for handling this:

  1. Any out of bounds index evaluates to the empty string "". Note that $foo[5..-1] where $foo has fewer than 5 elements will evaluate to an empty range and therefore zero elements.
  2. Out of bounds indexes evaluate to nothing.

At this point I think we should go with approach 2, as I think it makes more sense, and it's consistent with the behavior of $a[1] today.


A third option that extends approach 2 is to add syntax to denote the default value that should be used for any indexes that don't exist. This could always be added later of course. My worry with this is that it adds complexity, but the motivation for doing so is that, while you could write a function for providing a default for a single value (default val $var), you can't do so for ranges or index collections because you wouldn't know where to insert it into the result. This could possibly look like $foo[1..3 or '']. I think it's worth considering this, but at this point I think it should be deferred and re-considered at some point in the future.

@Profpatsch

This comment has been minimized.

Show comment
Hide comment
@Profpatsch

Profpatsch Sep 12, 2015

At the moment I use the following pattern for optional arguments:

function findia --description 'find case insensitive anywhere (globbing)' --argument search
        # prevent out-of bounds if only one argument
        if set -q argv[2]
          set more $argv[2..-1]
        end

        command find -iname "*$search*" $more
end

Profpatsch commented Sep 12, 2015

At the moment I use the following pattern for optional arguments:

function findia --description 'find case insensitive anywhere (globbing)' --argument search
        # prevent out-of bounds if only one argument
        if set -q argv[2]
          set more $argv[2..-1]
        end

        command find -iname "*$search*" $more
end
@Profpatsch

This comment has been minimized.

Show comment
Hide comment
@Profpatsch

Profpatsch Sep 17, 2015

But isn’t that exactly what shift is normally for?

Profpatsch commented Sep 17, 2015

But isn’t that exactly what shift is normally for?

@cben

This comment has been minimized.

Show comment
Hide comment
@cben

cben Dec 22, 2015

Contributor

In current master the set vs unset variable inconsistency is gone.
$a[2] is an error when $a doesn't exist just as it is when $a does exist and has 0 or 1 elements.
$a[1] is always valid even if $a has zero elements, primarily because it's just too annoying to have that be an error.

I just encountered that and was very perplexed.
$argv[1] and $argv[2] behaving differently goes against everything I expected from fish.
Why?
Is the goal to preserve some kind of $a == $a[1] equivalence (which holds for single element vars but here we're talking 0-element)?

IMHO it's fine that $unset == $empty == zero words but $empty[1] and $unset[1] should be errors.

What surprised me even more was that (count $unset[1]) == (count $empty[1]) == 0.
When $anything[k] is not an error, for any k, I expected it to still be 1 (empty) word, not 0 words.
Example where 0 words is risky:

set dist_dir $argv[1]
mv foo.gz bar.sh $dist_dir
# if $dist_dir is not provided, `bar.sh` will get overwritten.

However see (*) below.


As for ranges, +1 for Python-like behavior where [5] raises errors and [3..10] silently clips.
Safely limiting "up to 10 first items" is much more useful than "first 10 or nothing".
And the splitting invariant $a[..9] $a[10..19] $a[20..] == $a is very neat.


If the decision is to get rid of all out-of-bounds errors, I'm OK with that too, as long [1] and [2] are equivalent.

(*) I do love the proposal that $a[3..5] == $a[3 4 5] == $a[(seq 3 5)], which will all silently clip to existing elements iff non-existing $a[5] expands to zero words. That's an extremely consistent mechanic, easy to explain and understand, which is worth a lot.

Contributor

cben commented Dec 22, 2015

In current master the set vs unset variable inconsistency is gone.
$a[2] is an error when $a doesn't exist just as it is when $a does exist and has 0 or 1 elements.
$a[1] is always valid even if $a has zero elements, primarily because it's just too annoying to have that be an error.

I just encountered that and was very perplexed.
$argv[1] and $argv[2] behaving differently goes against everything I expected from fish.
Why?
Is the goal to preserve some kind of $a == $a[1] equivalence (which holds for single element vars but here we're talking 0-element)?

IMHO it's fine that $unset == $empty == zero words but $empty[1] and $unset[1] should be errors.

What surprised me even more was that (count $unset[1]) == (count $empty[1]) == 0.
When $anything[k] is not an error, for any k, I expected it to still be 1 (empty) word, not 0 words.
Example where 0 words is risky:

set dist_dir $argv[1]
mv foo.gz bar.sh $dist_dir
# if $dist_dir is not provided, `bar.sh` will get overwritten.

However see (*) below.


As for ranges, +1 for Python-like behavior where [5] raises errors and [3..10] silently clips.
Safely limiting "up to 10 first items" is much more useful than "first 10 or nothing".
And the splitting invariant $a[..9] $a[10..19] $a[20..] == $a is very neat.


If the decision is to get rid of all out-of-bounds errors, I'm OK with that too, as long [1] and [2] are equivalent.

(*) I do love the proposal that $a[3..5] == $a[3 4 5] == $a[(seq 3 5)], which will all silently clip to existing elements iff non-existing $a[5] expands to zero words. That's an extremely consistent mechanic, easy to explain and understand, which is worth a lot.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Dec 22, 2015

Contributor

I agree with @cben that Python semantics are desirable. Not least because they match most people's expectations and provide more consistent behavior.

Contributor

krader1961 commented Dec 22, 2015

I agree with @cben that Python semantics are desirable. Not least because they match most people's expectations and provide more consistent behavior.

@DavidPerezIngeniero

This comment has been minimized.

Show comment
Hide comment
@DavidPerezIngeniero

DavidPerezIngeniero Jan 11, 2016

+1 empty results for non-existing slices.

DavidPerezIngeniero commented Jan 11, 2016

+1 empty results for non-existing slices.

@artemruts

This comment has been minimized.

Show comment
Hide comment
@artemruts

artemruts Jun 26, 2016

After reading this thread I think I found what I needed.

echo $argv | read -l first second
test -n $fist; or set -l first "first default value"
test -n $second; or set -l second "second default value"

artemruts commented Jun 26, 2016

After reading this thread I think I found what I needed.

echo $argv | read -l first second
test -n $fist; or set -l first "first default value"
test -n $second; or set -l second "second default value"
@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jun 26, 2016

Contributor

@artemruts Due to quirks of the test command mandated by POSIX you should always quote the arguments passed to the -n or -z subcommands; e.g., test -n "$first". Also, FWIW, an alternative that is in some sense more idiomatic for the fish shell is to do set -q first[1]; or set -l first "first default value".

Contributor

krader1961 commented Jun 26, 2016

@artemruts Due to quirks of the test command mandated by POSIX you should always quote the arguments passed to the -n or -z subcommands; e.g., test -n "$first". Also, FWIW, an alternative that is in some sense more idiomatic for the fish shell is to do set -q first[1]; or set -l first "first default value".

@artemruts

This comment has been minimized.

Show comment
Hide comment
@artemruts

artemruts Jun 26, 2016

@krader1961 Thanks for sharing your knowledge! Refactored my functions.

artemruts commented Jun 26, 2016

@krader1961 Thanks for sharing your knowledge! Refactored my functions.

@tmccombs

This comment has been minimized.

Show comment
Hide comment
@tmccombs

tmccombs Feb 13, 2017

I would love to see this, or even a builtin function that returns an array of all but the first elements of an array (or better yet a builtin function that works like array indexing, but returns an empty array if it is out of bounds)

tmccombs commented Feb 13, 2017

I would love to see this, or even a builtin function that returns an array of all but the first elements of an array (or better yet a builtin function that works like array indexing, but returns an empty array if it is out of bounds)

faho added a commit to faho/fish-shell that referenced this issue May 26, 2017

Remove "Array out of bounds" errors
This just removes every invalid index.

That means with `set foo a b c` and the "show" function from tests/expand.in:

- `show $foo[-5..-1]` prints "3 a b c"
- `show $foo[-10..1]` prints "1 a"
- `show $foo[2..5]` prints "2 b c"
- `show $foo[1 3 7 2]` prints "3 a c b"

and similar for command substitutions.

Fixes fish-shell#826.

faho added a commit to faho/fish-shell that referenced this issue Jun 20, 2017

Remove "Array index out of bounds" errors
This just removes every invalid index.

That means with `set foo a b c` and the "show" function from tests/expand.in:

- `show $foo[-5..-1]` prints "3 a b c"
- `show $foo[-10..1]` prints "1 a"
- `show $foo[2..5]` prints "2 b c"
- `show $foo[1 3 7 2]` prints "3 a c b"

and similar for command substitutions.

Fixes fish-shell#826.

@faho faho referenced this issue Jun 20, 2017

Closed

No index out of bounds #4151

3 of 3 tasks complete

@krader1961 krader1961 modified the milestones: fish 2.7.0, next-major Jun 21, 2017

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