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

fix handling of odd strings by `test` builtin #3581

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@krader1961
Contributor

krader1961 commented Nov 25, 2016

The test builtin currently has unexpected behavior with respect to
expressions such as '' -eq 0. That currently evaluates to true with a
return status of zero. This change addresses that oddity while also
ensuring that other unusual strings (e.g., numbers with leading and
trailing whitespace) are handled consistently.

I based this on PR #3579 so it includes commits not already merged to the master branch. The only commit that is relevant for this PR is the final one: 210296b.

krader1961 added some commits Nov 23, 2016

improve converting strings to ints/longs
The existing code is inconsistent, and in a couple of cases wrong, about
dealing with strings that are not valid ints. For example, there are
locations that call wcstol() and check errno without first setting errno
to zero. Normalize the code to a consistent pattern.

This does make some syntax more liberal. For example `echo $PATH[1 .. 3]`
is now valid due to uniformly allowing leading and trailing whitespace
around numbers. Whereas prior to this change you would get a "Invalid
index value" error. Contrast this with `echo $PATH[ 1.. 3 ]` which was
valid and still is.
fix handling of odd strings by `test` builtin
The `test` builtin currently has unexpected behavior with respect to
expressions such as `'' -eq 0`. That currently evaluates to true with a
return status of zero. This change addresses that oddity while also
ensuring that other unusual strings (e.g., numbers with leading and
trailing whitespace) are handled consistently.

Fixes #3346
@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 26, 2016

Contributor

@ridiculousfish: I think the only part of this that needs a quick look from you is my use of parser_t::expand_argument_list() to tokenize the test expressions being tested in src/fish_tests.cpp. The original code to tokenize the statements did not correctly handle quoted strings and that function seemed like the most straightforward solution.

Contributor

krader1961 commented Nov 26, 2016

@ridiculousfish: I think the only part of this that needs a quick look from you is my use of parser_t::expand_argument_list() to tokenize the test expressions being tested in src/fish_tests.cpp. The original code to tokenize the statements did not correctly handle quoted strings and that function seemed like the most straightforward solution.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 26, 2016

Contributor

Also, this fixes issue #3346. I'll update the commit comment to mention that when I merge it.

Contributor

krader1961 commented Nov 26, 2016

Also, this fixes issue #3346. I'll update the commit comment to mention that when I merge it.

@krader1961 krader1961 added the bug label Nov 26, 2016

@krader1961 krader1961 added this to the fish 2.5.0 milestone Nov 26, 2016

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Nov 26, 2016

Why not make a fish_wsctoi/umax instead?

floam commented on 210296b Nov 26, 2016

Why not make a fish_wsctoi/umax instead?

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 27, 2016

Owner

Why not make a fish_wsctoi/umax instead?

Why use it? There is only one place in fish where we use that type, src/builtin_printf.cpp, and I think that should be changed to either long long or int64_t. Regardless, the answer to your question is such a change is outside the scope of this fix. If you want to argue that fish should be using type intmax_t more extensively please open a new issue. Good luck with tilting at that windmill 😄

Owner

krader1961 replied Nov 27, 2016

Why not make a fish_wsctoi/umax instead?

Why use it? There is only one place in fish where we use that type, src/builtin_printf.cpp, and I think that should be changed to either long long or int64_t. Regardless, the answer to your question is such a change is outside the scope of this fix. If you want to argue that fish should be using type intmax_t more extensively please open a new issue. Good luck with tilting at that windmill 😄

This comment has been minimized.

Show comment
Hide comment
@floam

floam Nov 27, 2016

Why use it?

I approach as more of a why-not: What's the advantage of long long or a particular fixed width when intmax_t is defined to be able to store any value of any signed integer type and C++11 comes with it?

We used to use uintmax_t and intmax_t for our raw_string_to_scalar_type, but stopped due to a FreeBSD bug that I can no longer reproduce there.

floam replied Nov 27, 2016

Why use it?

I approach as more of a why-not: What's the advantage of long long or a particular fixed width when intmax_t is defined to be able to store any value of any signed integer type and C++11 comes with it?

We used to use uintmax_t and intmax_t for our raw_string_to_scalar_type, but stopped due to a FreeBSD bug that I can no longer reproduce there.

This comment has been minimized.

Show comment
Hide comment
@floam

floam Nov 27, 2016

We used to use uintmax_t and intmax_t for our raw_string_to_scalar_type, but stopped due to a FreeBSD bug that I can no longer reproduce there.

( with commit https://github.com/maxfl/fish-shell/commit/37140d5e109ef677e011773436ece15d0fdf061b , fixed #626 )

Since you found an instance of us still using the type somewhere else in fish which I didn't know about, I'm going to guess that either we now fail to work around that FreeBSD 8 compiler issue, or more likely the workaround is no longer necessary.

floam replied Nov 27, 2016

We used to use uintmax_t and intmax_t for our raw_string_to_scalar_type, but stopped due to a FreeBSD bug that I can no longer reproduce there.

( with commit https://github.com/maxfl/fish-shell/commit/37140d5e109ef677e011773436ece15d0fdf061b , fixed #626 )

Since you found an instance of us still using the type somewhere else in fish which I didn't know about, I'm going to guess that either we now fail to work around that FreeBSD 8 compiler issue, or more likely the workaround is no longer necessary.

This comment has been minimized.

Show comment
Hide comment
@floam

floam Nov 27, 2016

(What I said above is wrong - it's not the intmax_t type that was problematic originally but the wcstoimax function. Still, our workaround doesn't seem to be necessary on FreeBSD 8.4.)

floam replied Nov 27, 2016

(What I said above is wrong - it's not the intmax_t type that was problematic originally but the wcstoimax function. Still, our workaround doesn't seem to be necessary on FreeBSD 8.4.)

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 27, 2016

Owner

Please take your Don Quixote tilting at windmills discussion about using intmax_t to a new issue. This change is not the place to make such a major change. And, as I said earlier, good luck convincing everyone that using that type where we currently using int, long, long long, etc. makes sense.

Owner

krader1961 replied Nov 27, 2016

Please take your Don Quixote tilting at windmills discussion about using intmax_t to a new issue. This change is not the place to make such a major change. And, as I said earlier, good luck convincing everyone that using that type where we currently using int, long, long long, etc. makes sense.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Nov 26, 2016

Member

I haven't tried this PR yet - there are conflicts.

Does this let builtin test "" -lt 1 and command test "" -lt 1 behave similarly, emitting an error message? Fish was wrong here, test should require numeric arguments for this comparison.

Member

floam commented Nov 26, 2016

I haven't tried this PR yet - there are conflicts.

Does this let builtin test "" -lt 1 and command test "" -lt 1 behave similarly, emitting an error message? Fish was wrong here, test should require numeric arguments for this comparison.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 27, 2016

Contributor

Does this let builtin test "" -lt 1 and command test "" -lt 1 behave similarly, ...

Sort of. Fish now fails the test but does not emit a diagnostic. Note that even without this change doing something nonsensical like test x -lt 1 does not emit a diagnostic. So my change is the minimum needed to provide sane behavior consistent with command test without introducing a new error message. But I'm inclined to think we should take this opportunity to emit a diagnostic.

Contributor

krader1961 commented Nov 27, 2016

Does this let builtin test "" -lt 1 and command test "" -lt 1 behave similarly, ...

Sort of. Fish now fails the test but does not emit a diagnostic. Note that even without this change doing something nonsensical like test x -lt 1 does not emit a diagnostic. So my change is the minimum needed to provide sane behavior consistent with command test without introducing a new error message. But I'm inclined to think we should take this opportunity to emit a diagnostic.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Nov 27, 2016

Member

But I'm inclined to think we should take this opportunity to emit a diagnostic.

👍

Though, beware all - a number of our scripts end up relying on the no-error fish test quirk and it's a bit less convenient when comparing values that might be in a variable that might exist - see fish_vi_cursor.fish for examples of a bunch of stuff that won't work with a system test - the easiest way to "fix" it without adding an additional test is to prepend a 0 before using a var as a sole argument, like is done in __fish_config_interactive.

e.g.

- test "$VTE_VERSION" -lt 4000
+ test "0$VTE_VERSION" -lt 4000
Member

floam commented Nov 27, 2016

But I'm inclined to think we should take this opportunity to emit a diagnostic.

👍

Though, beware all - a number of our scripts end up relying on the no-error fish test quirk and it's a bit less convenient when comparing values that might be in a variable that might exist - see fish_vi_cursor.fish for examples of a bunch of stuff that won't work with a system test - the easiest way to "fix" it without adding an additional test is to prepend a 0 before using a var as a sole argument, like is done in __fish_config_interactive.

e.g.

- test "$VTE_VERSION" -lt 4000
+ test "0$VTE_VERSION" -lt 4000
@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Nov 27, 2016

Member

(Or, you could just redirect stderr to /dev/null, silencing all errors.)

Member

floam commented Nov 27, 2016

(Or, you could just redirect stderr to /dev/null, silencing all errors.)

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 27, 2016

Contributor

Ugh! Adding a diagnostic causes test failures because the unit tests either assume an empty string is equivalent to "0" or don't suppress errors. I'm still going to add the error message because it's the right thing to do, and fix the unit tests. However, this does make me worry that there are a not insignificant number of scripts that depend on the current, silent, behavior.

Contributor

krader1961 commented Nov 27, 2016

Ugh! Adding a diagnostic causes test failures because the unit tests either assume an empty string is equivalent to "0" or don't suppress errors. I'm still going to add the error message because it's the right thing to do, and fix the unit tests. However, this does make me worry that there are a not insignificant number of scripts that depend on the current, silent, behavior.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Nov 27, 2016

Member

However, this does make me worry that there are a not insignificant number of scripts that depend on the current, silent, behavior.

And unfortunately test errors are notoriously cryptic and hard to track down in fish IMO.

What would probably be a nice thing to do given potential pain from new errors in random scripts people may suffer after a change like this, would be to improve our error output to make identifying where a test failure occurred easier - namely some indication of what the hell the arguments were.

I had something along the lines of this happening locally at one point, but can't find the commit.

$ [ $foobar -gt 1 ]
[: Non-numeric argument at index 0: ‘’
[: given args: ‘’, ‘-gt‘, ‘1‘, ‘]‘

Such improvements should be a handled in different issue.

Member

floam commented Nov 27, 2016

However, this does make me worry that there are a not insignificant number of scripts that depend on the current, silent, behavior.

And unfortunately test errors are notoriously cryptic and hard to track down in fish IMO.

What would probably be a nice thing to do given potential pain from new errors in random scripts people may suffer after a change like this, would be to improve our error output to make identifying where a test failure occurred easier - namely some indication of what the hell the arguments were.

I had something along the lines of this happening locally at one point, but can't find the commit.

$ [ $foobar -gt 1 ]
[: Non-numeric argument at index 0: ‘’
[: given args: ‘’, ‘-gt‘, ‘1‘, ‘]‘

Such improvements should be a handled in different issue.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 28, 2016

Contributor

And unfortunately test errors are notoriously cryptic and hard to track down in fish IMO.

See issue #2860 that I opened eight months ago to pointing out that all diagnostic messages need to provide more context. There are too many issues of that type that need to be a high priority to resolve. Not sure how to get attention focused on those issues.

Contributor

krader1961 commented Nov 28, 2016

And unfortunately test errors are notoriously cryptic and hard to track down in fish IMO.

See issue #2860 that I opened eight months ago to pointing out that all diagnostic messages need to provide more context. There are too many issues of that type that need to be a high priority to resolve. Not sure how to get attention focused on those issues.

static bool parse_number(const wcstring &arg, long long *out) {
*out = fish_wcstoll(arg.c_str());
if (errno) {
debug(0, "test: invalid integer '%ls'", arg.c_str());

This comment has been minimized.

@floam

floam Nov 28, 2016

Member

I think in this case it'd be wiser to let an error() bubble up onto the wcstring_list_t errors list to be handled uniformly, rather than call debug here.

@floam

floam Nov 28, 2016

Member

I think in this case it'd be wiser to let an error() bubble up onto the wcstring_list_t errors list to be handled uniformly, rather than call debug here.

This comment has been minimized.

@krader1961

krader1961 Nov 28, 2016

Contributor

Okay, that's probably the right thing to do but I need to think about it. The problem with that approach is it loses context if we ever fix isssue #2860. I need to better understand how the errors list is utilized.

@krader1961

krader1961 Nov 28, 2016

Contributor

Okay, that's probably the right thing to do but I need to think about it. The problem with that approach is it loses context if we ever fix isssue #2860. I need to better understand how the errors list is utilized.

This comment has been minimized.

@krader1961

krader1961 Nov 28, 2016

Contributor

Ahh, I had a brain cramp. Yes, we should definitely append that error to the streams.err stream so it can be redirected. Thanks for catching that, @floam. As part of fixing issue #2860 we'll need to add a generic mechanism for decorating such messages with useful context.

@krader1961

krader1961 Nov 28, 2016

Contributor

Ahh, I had a brain cramp. Yes, we should definitely append that error to the streams.err stream so it can be redirected. Thanks for catching that, @floam. As part of fixing issue #2860 we'll need to add a generic mechanism for decorating such messages with useful context.

This comment has been minimized.

@krader1961

krader1961 Nov 28, 2016

Contributor

J.H.C. Switching to appending to the streams.err stream does not allow redirecting such errors to /dev/null. It's still the right thing to do but clearly doesn't allow such errors to be silently discarded.

@krader1961

krader1961 Nov 28, 2016

Contributor

J.H.C. Switching to appending to the streams.err stream does not allow redirecting such errors to /dev/null. It's still the right thing to do but clearly doesn't allow such errors to be silently discarded.

This comment has been minimized.

@floam

floam Nov 28, 2016

Member

I didn't mean appending the string directly to streams.err, but rather use the test_parser::error() or add_error or any other means to get it onto the errors list on test_parser. It may be necessary to check the state of errnoinstead after using parse_number (because you can't return anything but true or false here, or access errors) and catch that in some caller above (binary_primary_evaluate?), or perhaps check for a sane number at some other stage in evaluation.

@floam

floam Nov 28, 2016

Member

I didn't mean appending the string directly to streams.err, but rather use the test_parser::error() or add_error or any other means to get it onto the errors list on test_parser. It may be necessary to check the state of errnoinstead after using parse_number (because you can't return anything but true or false here, or access errors) and catch that in some caller above (binary_primary_evaluate?), or perhaps check for a sane number at some other stage in evaluation.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Nov 28, 2016

Contributor

Closed with commit 2f33c24 and commit 54a76bb.

Contributor

krader1961 commented Nov 28, 2016

Closed with commit 2f33c24 and commit 54a76bb.

@krader1961 krader1961 closed this Nov 28, 2016

@krader1961 krader1961 deleted the krader1961:test-empty-strings-are-not-zero branch Dec 9, 2016

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