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

revisit exit code of `while` builtin #4982

Closed
mqudsi opened this Issue May 14, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@mqudsi
Copy link
Contributor

mqudsi commented May 14, 2018

I'm not sure the current exit code for while when the loop evaluation condition returns non-zero makes sense. I understand that it passes through the value returned by the condition executable, but I'm not sure that a non-zero exit code accurately reflects the intent of the code, nor is particularly useful.

while foo
    if bar
        break
    end
end

This loop can terminate with either a zero or non-zero exit code. But it's equal to the following, which can only terminate with a zero exit code:

while true
    if not bar
        break
    end
end

Now if the condition failed to run (instead of ran and exited with a non-zero exit code), it would make sense for while to return non-zero:

while does-not-exist
end

Here, it would make sense for $status to evaluate to non-zero after the loop.

My problem is with a script or function that executes in a loop, an explicit return/exit 0 is needed to "override" the misleading nonzero exit code after the loop, which shouldn't (imho) ever be necessary unless a failure was detected and is being explicitly ignored.

@faho

This comment has been minimized.

Copy link
Member

faho commented May 17, 2018

How would you like it to behave?

Return 0 when the loop has been run at least once, 1 otherwise?

As best as I can tell, in bash it just always returns 0.

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented May 17, 2018

I like the general principle of "if it's run at least once" but I feel like "if it never runs this is unexpected behavior/an error" is too big of an assumption to make and we'll just get complaints about it later (if anyone actually cares about these things). I think it should just return 1 in case there was an error evaluating the target (i.e. target does not exist).

@faho

This comment has been minimized.

Copy link
Member

faho commented May 17, 2018

if it never runs this is unexpected behavior/an error

But it's not?

A return of 1 isn't "unexpected behavior". It's just a false return. It's something you have to deal with all the time, and which happens for quite trivial reasons, and which has no huge effect.

It's another way to convey information, not a big issue.

it should just return 1 in case there was an error evaluating the target (i.e. target does not exist).

If the "target" does not exist, that would warrant an error message, not just a return value of 1.

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented May 17, 2018

If the "target" does not exist, that would warrant an error message, not just a return value of 1.

Well, yes and no. An error message is for the user, but the return code is for programmatic evaluation (which I understand you already cover with the "not just" in your statement). But my point is that nothing else explicitly warrants a non-zero return code for while.

I understand that a non-zero return code is not code for "error occurred," but as you mention, it is intended to evaluate to false, which in the context of a shell expression means something along the lines of "execution didn't come back with the results you sought/intended" or similar. For while, the intended behavior is for it to either loop or to not, and in both cases, it's the intended behavior (as we cannot infer on behalf of the user what their intention was).

Users wishing to explicitly take an action based on the result of a condition would be best served with while true; if foo; ...., imho, which is both easy to write and easy to understand, with very clear behavior.

Additionally, you have to consider the return code in the context of the script or function, as I initially mentioned. Given a function consisting solely of the following:

function foo
    if bar
        # do something
    end
end

It is understandable why the function would return non-zero if the if was not taken (i.e. the type of function that would look like this probably wants a non-zero return code if bar did not succeed). Even if not, it's understandable why it would return a non-zero return code, and the programmer can opt out of it with an explicit return or a true; at the end.

But for a function that looks like this:

function foo
    bar | while read -l baz
        # do something
    end
end

It really doesn't make sense for the default behavior here to be for the function return non-zero. If anything, in this case the return code of bar should be used, but since by design a while loop must at some point (unless it is an infinite loop or via manual flow control) exit, the side effect of that action shouldn't have a non-zero return code.

@faho

This comment has been minimized.

Copy link
Member

faho commented May 17, 2018

For while, the intended behavior is for it to either loop or to not, and in both cases, it's the intended behavior

I'm not sure that line of reasoning works. "For if, the intended behavior is to either be taken or not".

I think it's reasonable to say that the point of if is to decide. But yet we still have it return false if it's not taken.

It really doesn't make sense for the default behavior here to be for the function return non-zero.

I agree.

If anything, in this case the return code of bar should be used

That's more debatable. In this specific case you might want bar to be used, but for general pipes that's of course not true - something | string match -q as an easy example.

#2039 would introduce a $pipestatus variable, which would allow you to pick after the fact.


I don't think "always" returning 0 is awful, by any means. It's better than "basically always" returning non-zero.

But I think adding that extra behavior of "failing" when the loop is never entered adds an extra bit of information that is quite hard to retrieve otherwise.

Alternatively, it'd be possible to introduce while/else.

@mqudsi mqudsi closed this in f5083d7 Sep 27, 2018

@faho faho added this to the fish-3.0 milestone Sep 27, 2018

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Sep 28, 2018

This needs to be in the documentation for while, I think.

zanchey added a commit that referenced this issue Sep 28, 2018

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Sep 28, 2018

Done in 5cc92ff.

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Sep 28, 2018

Thank you @zanchey

mqudsi added a commit that referenced this issue Jan 14, 2019

Add tests for `$status` after `while` in various scenarios
Includes a regression test for #5513 and asserts the behavior defined in
\#4982.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment