Poison status variable when entering `if` block. #1428

Closed
xfix opened this Issue Apr 24, 2014 · 34 comments

Projects

None yet

9 participants

@xfix
Member
xfix commented Apr 24, 2014

oh-my-fish project has the following code in http://git.io/o_hDOA.

  if test -d $argv[1]; and not contains $argv[1] $$path
    set $path $argv[1] $$path
  end

However, this does something completely unexpected by original writer of the code (possibly expecting that it will work like bash). In my opinion, this should be simply disallowed, and probably the easiest way to do so is poisoning $status (so reads would make an error, until the variable will be written to again) when entering conditional block.

Because some prompts may show status, the variable should be unpoisoned when leaving if block (to not make issues when the body of function is composed entirely of set commands, for example).

@xfix xfix referenced this issue in oh-my-fish/oh-my-fish-legacy Apr 24, 2014
Closed

Fixed condition check on item existence in list #146

@kballard
Contributor

Interesting thought, but it would be kind of surprising to have the following fail:

if test -d $argv[1]
    if set -l foo bar
        # yeah that if can't fail, unless a command substitution is used in the value
        # but maybe there used to be a command substitution and it was deleted
        echo this should still run anyway
    end
end

Or, perhaps more importantly, something like the following shouldn't fail:

function foo
    # we don't want to modify the status, for some reason
    set -l old_status $status
    do_stuff
    return $old_status
end

if test -d $argv[1]
    foo
end

Since and and or are actually special parser things, instead of being mere builtins, it would probably be better to simply change the parser to not accept and or or immediately after the conditional of an if/while.

@zanchey
Member
zanchey commented Oct 1, 2014

Changing the parser sounds sane. Perhaps an error along the lines of "Did you mean if begin..."?

@zanchey
Member
zanchey commented Oct 1, 2014

Alternatively, the parser could insert an implicit block around any if condition followed by and or or. I don't know if this is a good idea.

@kballard
Contributor
kballard commented Oct 1, 2014

Implicit block sounds too confusing. Especially if the user actually types a newline, like

if foo
    and bar
    echo "this is confusing"
end
@kballard
Contributor
kballard commented Oct 1, 2014

Actually, it isn't that confusing. Because fish is using english words, it actually still reads ok: "if foo and bar, echo something". Maybe we should just go ahead and do it.

@ridiculousfish ridiculousfish added this to the next-major milestone Oct 8, 2014
@faho
Member
faho commented Sep 15, 2015

if foo
and bar
echo "this is confusing"
end

That and is completely redundant, no? It only executes if the last command succeeded, but because you've entered the body of the if-block, it by definition always did! An or in the same place would be even worse since it couldn't possibly trigger.

Because of that I'm sure that nobody ever wanted to start the body of an if-block (or else-block) with and or or, so I'd be okay to special-case it to imply begin; end.

@ridiculousfish
Member

I think I'm going to tackle this.

Specifically the change is that a sequence of and and or statements at the top of a while, if or else if block will be treated as part of the condition:

if test -d $argv[1]
   and test -f $argv[2]
    loop body
end

fish_indent will do something to make it clear - not sure what yet, maybe by indenting these conditions one more level:

if test -d $argv[1]
        and test -f $argv[2]
    loop body
end
@faho
Member
faho commented Dec 11, 2015

fish_indent will do something to make it clear - not sure what yet, maybe by indenting these conditions one more level:

Or keep them on the same line as the if:

if test -d $argv[1]; and test -f $argv[2]
    loop body
end

That seems to be what people do intuitively.

@ridiculousfish
Member

fish_indent tries not to join lines. Of course if they're on the same line already they'll stay that way, and yeah, good point that's what people will do for short commands..

@krader1961
Member

FWIW, I agree with @ridiculousfish that "a sequence of and and or statements at the top..." should be treated as part of the preceding condition. That is, there should be an implicit begin...end around such a sequence based on the principal of least surprise to the user. The alternative, which I also think is acceptable, is to raise an error if a and or or is seen immediately after a if, for, while, etc. The question is which fish behavior will result in the fewest programming errors and user surprise. I am personally always wary of implicit behavior where something like the formatting of the code, as just one example, can cause confusion. But in this instance I think the implicit begin...end wrapping is reasonably clear and likely to result in fewer problems than the current behavior.

@jakwings
Contributor

I think we should make the role of the semicolon ; more clear before adding special cases. As I see, the semicolon is used to replace a newline, other than this, it has no other special meanings. So the code should be:

if (test -d $argv[1]; and not contains $argv[1] $$path)
  set $path $argv[1] $$path
end

Use parentheses properly. Do I misunderstand the mechanism of and and or?

@krader1961
Member

Do I misunderstand the mechanism of and and or?

I can't tell if you've misunderstood the point of and and or but you seem to have misunderstand the role of parenthesis in the fish shell. Where you have open and close parens there should be the keywords begin and end. A parenthesized expression is broadly analogous to $(some command) in other shells. Also, while a semicolon is equivalent to a newline that has no bearing on this issue.

@jakwings
Contributor

@krader1961 Yes, I just thought of begin; end, and there is no problem if we use begin; end to quote the command:

if begin; test -d $argv[1]; and not contains $argv[1] $$path; end
  set $path $argv[1] $$path
end

equals to

if begin
  test -d $argv[1]; and not contains $argv[1] $$path
end
  set $path $argv[1] $$path
end
@jakwings
Contributor

If and and or are not really special, I don't see any big problems about the syntax, though begin; end looks cumbersome.

@krader1961
Member

@jakwings The question is whether or not we should require someone writing something like

if test -d $argv[1]; and not contains $argv[1] $$path

to explicitly surround that sequence of conditions with begin and end or infer their presence. The former is unambiguous. The latter is potentially ambiguous but more user friendly. Is there in fact any ambiguity? If not then the answer to this question is clear: it should be done. If there is ambiguity then the question is whether the new behavior of wrapping the conditions in an implicit begin...end block results in more benefits than problems.

@jakwings
Contributor

@krader1961 Ok, it is a mindset problem. If that is required, as I think, it is the user not really understanding the simple role of the semicolon. I don't think it is ambiguous, because I'll never try to end the condition with a ; that way, and because it is a end-marker of the condition in my eyes, even if there is an and right after it. I know it well that begin and end are the most useful tokens to divide multiple commands. Now imagine somebody just tell me ; is a special token to connect and/or with other commands, what? Why not just introduce && and || into fish (and many bash users will like them) instead of abusing the semicolon?

@ridiculousfish
Member

This is definitely not semicolon abuse. The thing to understand is that where other shells introduce special purpose syntax, fish uses plain old commands. For example, the set command replaces the special purpose syntax foo=bar. Likewise, the and and or commands replace the special purpose syntax && and ||.

Since and and or are just commands, they obey command rules, and we already know how to use them. Compare to && and ||, which have their own rules (for example, precedence) that we have to learn and remember.

That said, this proposal is basically to add some special behavior to and and or in the name of convenience, so it's arguably abusing something. The response is that it's replacing a nonsense construct, so it's not so bad.

@jakwings
Contributor

@ridiculousfish

This is definitely not semicolon abuse.
...
That said, this proposal is basically to add some special behavior to and and or in the name of convenience, so it's arguably abusing something. The response is that it's replacing a nonsense construct, so it's not so bad.

So my regular mindset is arguably wrong?

Compare to && and ||, which have their own rules (for example, precedence) that we have to learn and remember.

if true; and false; or true is not fun too!

Likewise, the and and or commands replace the special purpose syntax && and ||.

Now we may have a new special purpose semicolon ;.

@jakwings
Contributor

@ridiculousfish You are the leader, but please don't fool me. Although && and || are not implemented in fish, I cannot write them either. (Ok, I should take some pills. The previous post above is all my options. Nothing more.)

@faho
Member
faho commented Dec 13, 2015

Is there in fact any ambiguity?

@krader1961: There's no real ambiguity. Yes, in theory one could write

if a
   and b
end

and want b to belong to the body of the conditional, but in that case the and is completely superfluous since the only way to reach it is to succeed check "a". An or would be even worse as that would never be executed. For an else clause these would be reversed.

What I mean by that is that while it's currently possible to write this, it's absolute nonsense.

@jakwings:

Semicolons don't really matter here - I only brought them up for fish_indent, a purely cosmetic change. And even with special powers for and and or, semicolons remain purely cosmetic. They are nice for e.g. set -l a; set -l b or to quickly write something interactively, or to disable a function by defining an empty substitute, but they can always be replaced by newlines without any change in functionality.

@jakwings
Contributor

@faho This change in my mind is just making ;and ;or new and confusing keywords (only for if and while), not as elegant as && ||. Both semicolon and and/or become more special, not just the semicolon. The official document has stated in the if section:

In order to use the exit status of multiple commands as the condition of an if block, use begin; ...; end and the short circuit commands and and or.

It is also nice to write short commands test COND; and SOMETHING without using if; end. And the semicolon between test and and is not special. Someone may argue it looks like test (COND; and SOMETHING) in the future.

@faho
Member
faho commented Dec 13, 2015

Both semicolon and and/or become more special, not just the semicolon

No - there'd be no semantic difference between

if a; and b
   # body
end

and

if a
  and b
  # body
end
@jakwings
Contributor

@faho Ok, let the documentation tell the implicit begin; end for those and/or conditions. I'm going to change my mindset now. Sorry for my rants (because of someone definitely).

@ridiculousfish ridiculousfish added a commit that referenced this issue Dec 19, 2015
@ridiculousfish ridiculousfish Write tests for new if/and/or behavior (#1428)
They fail for now.
0a6f623
@ridiculousfish ridiculousfish added a commit that closed this issue Dec 19, 2015
@ridiculousfish ridiculousfish Allow and/or statements to attach to the if/while header
For example:

  if false; and true; echo hello; end

will output 'hello' now.

Fixes #1428
5aec7a9
@ridiculousfish ridiculousfish added a commit that referenced this issue Dec 19, 2015
@ridiculousfish ridiculousfish Allow and/or statements to attach to the if/while header
For example:

  if false; or true; echo hello; end

will output 'hello' now.

Fixes #1428
594b460
@ridiculousfish ridiculousfish modified the milestone: next-2.x, next-major Dec 19, 2015
@ridiculousfish
Member

This is implemented in 594b460 (the other commit had a bogus message). Thanks to everyone who participated in the discussion.

@cben
Contributor
cben commented Dec 31, 2015

Indentation is still misleading (just built from master):

$ if false
      or true
      echo Run.
  end
Run.

IMHO it should be at same level as if:

$ if false
   or true
      echo Run.
  end
Run.

Another sensible option is same as the first condition — but the distinction is barely visible (with if, would be better with while):

$ if false
     or true
      echo Run.
  end
Run.
@ridiculousfish
Member

Let's track that in a separate issue. I filed #2646 to discuss indentation improvements.

@jakwings
Contributor

Maybe I ask a question?

Are and and or keywords, so there are actually two commands in this script?

false; or true

false
or true

(EDIT: use or instead of and)

@ridiculousfish
Member

and and or are keywords, yes. They short circuit so that true; or foo will not execute foo, and similarly with false; and foo.

@jakwings
Contributor

@ridiculousfish Thanks. Then if fish ever has a strict mode (exit once a error happens), false; or true won't exit fish?

@ridiculousfish
Member

Correct, false; or true won't die in any reasonable strict mode

@jakwings
Contributor

@ridiculousfish Oh yeah, I understand now. also find that #805 already has a more detailed and interesting discussion.

@andrex-e-schulman
Contributor

The documentation doesn't seem to have been updated yet to show this. The staging documentation for the if command still says:

In order to use the exit status of multiple commands as the condition of an if block, use begin; ...; end and the short circuit commands and and or.

@ridiculousfish
Member

Good catch, I'll update it.

@ridiculousfish ridiculousfish added a commit that referenced this issue May 19, 2016
@ridiculousfish ridiculousfish Update docs to reflect new if/while condtion chaining
Documents new behavior in #1428
30ea7cc
@ridiculousfish
Member

Docs updated as 30ea7cc

@ridiculousfish ridiculousfish added a commit that referenced this issue May 19, 2016
@ridiculousfish ridiculousfish Update docs to reflect new if/while condtion chaining
Documents new behavior in #1428

Cherry picked from 30ea7cc
ade6ebf
@faho faho referenced this issue Jun 14, 2016
Closed

A couple checks for builtin string in scripts #3142

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