Skip to content
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

eval should set exit status if no commands are executed #5692

Closed
jcpetruzza opened this issue Feb 23, 2019 · 11 comments
Closed

eval should set exit status if no commands are executed #5692

jcpetruzza opened this issue Feb 23, 2019 · 11 comments

Comments

@jcpetruzza
Copy link

@jcpetruzza jcpetruzza commented Feb 23, 2019

I'm trying to use read -lz FOO to save the multi-line output of a command in FOO and then evaluate it with eval "$FOO". Now, what I see is that whenever the command outputs nothing, the call to eval "$FOO" will fail, but only the first time around, which I find puzzling. Here's a short example:

$ cat /dev/null | read -lz FOO
$ eval "$FOO"; or echo "eval failed"
eval failed
$ eval "$FOO"; or echo "eval failed"
$

I'm seeing this behavior both in 3.0.1 and 2.7.1, under nixos.

@faho faho added the question label Feb 23, 2019
@faho
Copy link
Member

@faho faho commented Feb 23, 2019

The eval does not fail, the read does, and the eval does nothing, because the string is empty.

See e.g.

$ cat /dev/null | read -lz FOO
$ true
$ eval "$FOO"; or echo "eval failed"

@jcpetruzza
Copy link
Author

@jcpetruzza jcpetruzza commented Feb 23, 2019

I expect eval "$FOO" to do nothing since "$FOO" is the empty string. I was also expecting it to exit with status 0, since there is no error in eval "". Or, alternatively, I'd expect the second call to fail as well? (since no command was run that would change the exit status from read, if that is what is going on)

@jcpetruzza
Copy link
Author

@jcpetruzza jcpetruzza commented Feb 23, 2019

Oh, I see, the echo "eval failed" is clearing the exit status of read.

@jcpetruzza
Copy link
Author

@jcpetruzza jcpetruzza commented Feb 23, 2019

So it was my expectation of eval returning a value what was wrong

@faho
Copy link
Member

@faho faho commented Feb 23, 2019

So it was my expectation of eval returning a value what was wrong

Yup. eval returns what the code it is executing returns, and in this case, that is nothing, so it doesn't even touch $status (conceptually).


Also, you probably want source. In this case specifically - if you're readin from a pipe just to then pass it to eval, you could just directly pass it to source. But also in general, source is the first thing you should pick, eval is just a wrapper around it.

@jcpetruzza
Copy link
Author

@jcpetruzza jcpetruzza commented Feb 24, 2019

Thanks for the explanation and the suggestion to use source, which is probably what I want in my case!

The behavior of eval "" can still be surprising when compared to other shells. For example, both in bash and zsh I see:

$ false; eval ''; echo $?
0

bash documents this in its man page as follows:

If there are no args, or only null arguments, eval returns 0.

It would probably make sense to add a comment about this to the entries for eval and source in fish's command reference?

FWIW, my intuition is that eval is a "command" and as such it should have an exit status regardless of whether it's called with arguments, empty or otherwise; in that sense, I tend to prefer the semantics of bash/zsh.

@jcpetruzza jcpetruzza closed this Feb 24, 2019
@zanchey
Copy link
Member

@zanchey zanchey commented Feb 24, 2019

If there are no args, or only null arguments, eval returns 0.

That seems reasonable.

@zanchey zanchey reopened this Feb 24, 2019
@zanchey zanchey added this to the fish-future milestone Feb 24, 2019
@zanchey zanchey changed the title eval failing on null string obtained via read -lz eval should set exit status if no commands are executed Feb 24, 2019
@faho faho removed this from the fish-future milestone Mar 26, 2019
@faho faho added this to the fish 3.1.0 milestone Mar 26, 2019
@mqudsi
Copy link
Contributor

@mqudsi mqudsi commented Apr 11, 2019

I'm pretty sure I broke this again. It should have a regression test added, as well.

@mqudsi mqudsi reopened this Apr 11, 2019
@mqudsi
Copy link
Contributor

@mqudsi mqudsi commented Apr 11, 2019

This plays very dirty with a fish behavior I honestly find confusing.

Current unit tests expect

function empty
end

false
eval empty
echo $status

to return the status of false, namely 1.

I feel that all of

  • false; eval; echo $status
  • false; eval ""; echo $status
  • false; eval empty; echo $status`
  • false; empty; echo $status

should return the same thing, and that thing should be 0, not 1.

@mqudsi
Copy link
Contributor

@mqudsi mqudsi commented Apr 11, 2019

For what it's worth, an empty function still sets a 0 return value in bash and co, and I don't see why we'd want that to be any different. A function isn't a macro (or an abbr) and has defined semantics that shouldn't be predicated on what that function contains, a caller executing empty shouldn't have to know what is inside it to determine whether $status afterwards is from running the function or from what was executed before.

@mqudsi
Copy link
Contributor

@mqudsi mqudsi commented Apr 13, 2019

That unit test was the only place I could find anywhere that had that expectation; I pushed a few commits that bring all the diffferent permutations in line with what's above and close this PR again.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants