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

Dereferencing array[0] should be a tokenizer or parser error #4862

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

Comments

Projects
None yet
4 participants
@mqudsi
Copy link
Contributor

mqudsi commented Mar 29, 2018

I've asked about this in the chat but haven't heard anything regarding if it's ever valid to access array[0]. Presuming the answer is no:

Since fish array indices start at 1 (unlike many other programming/scripting languages), I'm probably not the only one that forgets and starts at 0 from time to time. If [0] has no special meaning, it should be a hard error (instead of returning an empty result) so that it is clear there is something wrong with the script.

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Mar 30, 2018

It used to be:

> fish --version
fish, version 2.4.0
> echo $foo[1]

> echo $foo[0]
Array index out of bounds
fish: echo $foo[0]

This was changed as a result of #4151 (resolving #826), so I think it's fair to say that it is deliberate. Whether $foo[0] should be special-cased is open for discussion, although I don't think so.

@zanchey zanchey added the RFC label Mar 30, 2018

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Mar 30, 2018

Thanks for the links, interesting reads. I agree that if fish has no problem with undefined variables then it shouldn't have a problem with out-of-bound array indices. But I think $array[0] is already a special case, since it's not an undefined variable but rather an impossible variable:

mqudsi@ZBook /m/c/U/Mahmoud> fish --version
fish, version 2.7.1-1016-g04b8b35a
mqudsi@ZBook /m/c/U/Mahmoud> set array[0] hello
set: Array index out of bounds
mqudsi@ZBook /m/c/U/Mahmoud> echo $array[0]

mqudsi@ZBook /m/c/U/Mahmoud> set array[1] hello
mqudsi@ZBook /m/c/U/Mahmoud> echo $array[1]
hello
mqudsi@ZBook /m/c/U/Mahmoud>

I guess the array-out-of-bounds code still lives on here ;)

@faho

This comment has been minimized.

Copy link
Member

faho commented Mar 31, 2018

But I think $array[0] is already a special case, since it's not an undefined variable but rather an impossible variable

Sure. But it's not just used literally, you can also use variables or command substitutions as index - echo $array[$index].

If you make it error out, people will have to add 0-checks everywhere.

Obviously, if the default empty value isn't correct for your code, then the error will show an actual bug.

I guess the array-out-of-bounds code still lives on here ;)

Yeah, that should be consistent.

If we do add an error for 0, then it should either be "Invalid index value" (like it already is for $var[banana]) or a helpful hint ("Fish arrays start with 1, dummy!").

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Mar 31, 2018

So when you say "add an error" do you mean "write to stdout and continue" or do you mean "write to stdout and abort"?

I am suggesting the former (with a special case for literal array[0] that would be a tokenizer error rather than a parser error, complete with the "display error message then bring up the old contents of the line so the user can correct their mistake when at the tty").

@faho

This comment has been minimized.

Copy link
Member

faho commented Apr 4, 2018

Sooo.... we've had some more experiences with signalling errors, and I feel like we should have some more discussion on that in general, and if possible come up with a workable policy.

In the past few cycles, we've done a few changes to tighten errors, and we've done some to loosen them.

We've tightened errors related to test and integers - test banana -eq 5 now complains that "banana" is not a number. Which, in most systems, is true.

We've loosened errors related to array indices - $list[2] used to complain if there was no second element.

This is somewhere between the two.


do you mean "write to stdout and continue"

If anything, stderr.

Though that kind of shows the difference between the test error and this one. You can silence the test errors (by redirecting stderr), but you can't currently silence parser/tokenizer errors (because the redirection is never even executed).

Couple that with something like

echo $somevar[(math "someexpression")]

Do you really want an unsilencable error here just because "someexpression" happened to print 0? That way you just get guard expressions, like

set -l var
set -l result (math "someexpression" 2>/dev/null)
test "$result" -ne 0 2>/dev/null
and set var $somevar[$result]
echo $var

Which is just full of footguns - the quoting, stderr silencing, ...

This could be simplified if set -q var[0] was allowed (and returned false), of course.

a special case for literal array[0]

That would obviously work - fish arrays start at 1, dummy!.

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Apr 12, 2018

I think you're missing the more important point here, in that if you are referencing array index 0, your code already has a bug and won't work, except it will do so silently, making your job so much harder.

There's a difference between the recently discussed:

set -l foo bar
set -el foo
set -el foo

Where the second set -e foo will not return an error even though it failed to erase a non-existent variable. In that case, the operation failed, but the program itself will continue just fine so it makes no sense to force the user to check the return value there.

But if the user is using the result of a math operation to index an array, then the user relies on that value. In particular, the user believes echo $somevar[(math "someexpression")] will return a value that was previously stored to the array at that index. If that operation returns 0, then the user probably forgot to add a + 1 to someexpression, and it would help not hurt to be warned about it.

I can't think of a case where someone is (even indirectly) referencing $array[0] but their program is still correct. I'm sure there is such a case, but certainly the vast majority are the opposite.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 2, 2018

Emit an error message on literal zero indices
Mostly resolves fish-shell#4862, though there remains the lingering question of
whether or not to emit a warning to /dev/tty or stderr when a
non-literal-zero index evaluates to zero.

@mqudsi mqudsi closed this in 264d827 Oct 2, 2018

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Oct 2, 2018

I wrote ".. mostly resolves #4862" in 264d827 and GitHub decided for me that this issue was closed.

sigh I suppose a syntax error on literal zero-based indices takes care of 90% of the problem. If anyone wants to implement the remaining 10% (and fight it out), feel free.

@mqudsi mqudsi added this to the fish-3.0 milestone Oct 2, 2018

mqudsi added a commit that referenced this issue Oct 15, 2018

Correct misdetection of `[001]` as literal zero index
The control flow in expand.cpp is a bit more complicated than it seemed
at first blush. Ref #4862.
@floam

This comment has been minimized.

Copy link
Member

floam commented Nov 7, 2018

It would be cute if, when interactive, $foo[0] immediately highlighted as an error. (Same goes for other invalid index values)

ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Nov 24, 2018

Emit an error message on literal zero indices
Mostly resolves fish-shell#4862, though there remains the lingering question of
whether or not to emit a warning to /dev/tty or stderr when a
non-literal-zero index evaluates to zero.

ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Nov 24, 2018

Correct misdetection of `[001]` as literal zero index
The control flow in expand.cpp is a bit more complicated than it seemed
at first blush. Ref fish-shell#4862.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment