Make and/or short-circuit

[refi64]

This basically wraps statements into a function call. It only wraps statements, so (and 1 1) will still be compiled to just 1 and 1.

Closes #233 and #766.

[olasd]

While I agree with the general sentiment that some behaviors around hy's "turn all statements into expressions" stance are weird, I'm a bit uneasy with the approach of wrapping everything into function calls.

See for instance let. The fact that it wraps stuff in a function call confuses people to no end, as we get all the weird scoping issues of Python. I have the feeling this will end up the same, sadly.

[olasd]

(also, I have no clue for a "more proper" way of fixing this :/)

[paultag]

I wonder if we can constrain what we find in the or/and as conceptually part of the same scope and cheat a little.

[paultag]

e.g. actually getting aggressive with nonlocal

[paultag]

Just re-read my comment, it's basically "fix scoping"

[refi64]

@olasd Well, it doesn't wrap everything. Just when one of the sides is a statement.

Of course, if Python 2 had nonlocal, I could just do what @paultag said and aggressively nonlocal everything.

IMO, the issue with things as they stand is when macros get involved. This:

(and 0 (abc))

could be bad if abc is a macro that returns a series of statements, and the results would be even more surprising. Even though this only fixes part of the issue, it's better than the current state.

[gilch]

Wrapping statements into a function call might not be the right approach, because of the aforementioned scoping issues. Why not make Hy use a macro like this instead?

(defmacro -and [&rest args]
  (cond [(empty? args) True]
        [(cdr args) `(if ~(car args)(-and ~@(cdr args)))]
        [True (car args)]))

Some tests with hy --spy:

=> (-and)
True
True
=> (-and 0 (assert 0))
if 0:
    assert 0
    _hy_anon_var_1 = None
else:
    _hy_anon_var_1 = None
=> (for [i (range 9)] (-and (= i 2)(break))(print i))
from hy.core.language import range
for i in range(9):
    if (i == 2):
        break
        _hy_anon_var_1 = None
    else:
        _hy_anon_var_1 = None
    print(i)
0
1

Hy doesn't appear to introduce any unexpected scope issues this way. As with the proposed function wrapping, Hy need only use the macro when the arguments to and contains a statement, since the old behavior works correctly with expressions. It should be possible to implement or in a similar way.

[refi64]

@gilch I can try updating the PR with that.

[gilch]

Shouldn't this be (and 0 (setv a 2)) to short circuit?

[refi64]

@paultag @gilch @olasd All complaints/issues fixed! Short-circuiting operations are now compiled into if-else branches. Example:

ryan@DevPC-LX:~/stuff/hy$ hy --spy
hy 0.11.0 using CPython(default) 2.7.6 on Linux
=> (print (and (setv a 1) (setv a 0) (setv a 1)))
a = 1L
_hy_anon_var_1 = a
if _hy_anon_var_1:
    a = 0L
    _hy_anon_var_1 = a
    if _hy_anon_var_1:
        a = 1L
        _hy_anon_var_1 = a
print(_hy_anon_var_1)
0
=> 

[gilch]

When looking over this commit I noticed and and or have a minimum of 2 arguments, which is pretty strange for a Lisp. I opened a new issue #835 for this. This might be a good time to fix it.

[refi64]

@gilch Done. :) (Note that the apparent presence of 3 commits is just GitHub getting confused by a rebase; I referenced this PR instead of the issue you opened.)

[gilch]

Hm. If I'm reading this right, (or) now returns False? That's better than an exception, and is, in fact, how Scheme does it:

;;; R5RS
> (or)
#f

I realize that interactive Python suppresses printing None while False doesn't have this problem:

>>>None
>>>False
False
>>>

But now that we have a lisp-if/lif that only accepts nil as falsey, I think (or) must return nil instead of False for consistent behavior.

On the other hand, this might be a good argument for making lif accept both False and nil as falsey, (and nothing else) as Clojure does.

;;; Clojure
user=> (if false 'truthy 'falsey)
falsey
user=> (if nil 'truthy 'falsey)
falsey
user=> (if 0 'truthy 'falsey)
truthy

[refi64]

@gilch I prefer using both false and nil as falsey values. Having (and) return true but (or) return false feels weird, IMO.

Of course, if anyone objects, feel free to mention it. :)

[gilch]

Having (and) return true but (or) return false feels weird, IMO.

Then why not change (or) to return nil anyway? Or did you mean "(or) return nil feels weird"? Because this is exactly how both Clojure and Common Lisp do it:

user=> (or)
nil

* (or)

NIL

Common Lisp doesn't even have a false, just NIL.

Another special case to check:

user=> (and nil)
nil
user=> (and false)
false

[refi64]

@Glich You're right; I said it backwards. I meant that having (or) return nil seemed weird.

Then again, every other Lisp does this, so I should probably just use nil.

[refi64]

@gilch Fixed.

[gilch]

@kirbyfan64 Looks pretty good, though that compile function is getting long-winded. One minor nitpick: the hy2py output could be prettier if it compiled using elifs instead of deeply indenting blocks. Even if we're only using it for debugging purposes, readability helps. I don't know how much you guys care about that sort of thing at this point, but anyway it's much better than how it was before. @paultag @olasd thoughts?

I would like to get this merged, but it's not really up to me.

[refi64]

@gilch I produce the same AST as an elif statement produces. That's actually an issue with the prettt printer hy2py uses, not this itself. To Python, this:

if a: b
elif c: d

is the exact same thing as:

if a: b
else:
 if c: d

Then again, I tested it with Python 2. Does Python 3 have an explicit node for elif clauses?

[gilch]

Dunno, lemme check:

Python 3.4.3 (v3.4.3:9b73f1c3e601, Feb 24 2015, 22:44:40) [MSC v.1600 64 bit (AMD64)] on win32
Type "copyright", "credits" or "license()" for more information.
>>> from ast import dump,parse
>>> w_elif = dump(parse("""
if x:
    pass
elif y:
    pass
else:
    pass
"""))
>>> no_elif = dump(parse("""
if x:
    pass
else:
    if y:
        pass
    else:
        pass
"""))
>>> w_elif
"Module(body=[If(test=Name(id='x', ctx=Load()), body=[Pass()], orelse=[If(test=Name(id='y', ctx=Load()), body=[Pass()], orelse=[Pass()])])])"
>>> no_elif
"Module(body=[If(test=Name(id='x', ctx=Load()), body=[Pass()], orelse=[If(test=Name(id='y', ctx=Load()), body=[Pass()], orelse=[Pass()])])])"
>>> w_elif == no_elif
True
>>> 

I think it doesn't.

Sounds like a separate issue then. I wonder if this can be fixed easily in the pretty printer.

I have no further objections. I can haz merged?

[pmaupin]

If you can make astor codegen fail, please add a test, and then we'll add a fix. See berkerpeksag/astor#46

Thanks,
Pat

P.S. I just double-checked, and the code to print 'elif' was in the code from Armin Ronacher that I started with back in late 2012...

[gilch]

Wait, it does print elifs? Or it's already supposed to? Are we talking about the same version here?

hy --spy

=> (if 0 "zero" (if 1 "one" (assert 0)))
if 0:
    _hy_anon_var_2 = 'zero'
else:
    if 1:
        _hy_anon_var_1 = 'one'
    else:
        assert 0
        _hy_anon_var_1 = None
    _hy_anon_var_2 = _hy_anon_var_1

cond is no better

=> (cond [0 "zero"]
...      [1 "one"]
...      [True (assert 0)])
if 0:
    _hy_anon_var_3 = 'zero'
else:
    if 1:
        _hy_anon_var_2 = 'one'
    else:
        if True:
            assert 0
            _hy_anon_var_1 = None
        else:
            _hy_anon_var_1 = None
        _hy_anon_var_2 = _hy_anon_var_1
    _hy_anon_var_3 = _hy_anon_var_2

We just established that the AST should be the same as if I had used an elif, yes? I don't see an elif in the "Python" hy --spy got from the AST. There could have been an elif 1: line there.

@kirbyfan64 can you show us the hy --spy output of your new or form with several arguments, including a statement? Something like (or 0 false nil "" (,) [] (assert true))? Let's see an and form too.

[pmaupin]

How can you print an elif if you have those additional lines?

(e.g. _hy_anon_var_3 = _hy_anon_var_2 in the second example?)

[gilch]

You're right, of course. I see it now. There couldn't have been an elif 1: line as written. I'm still not sure how the or form turns out though.

[gilch]

In the case of the if and cond forms, the problem is indeed in Hy's AST, not the pretty printer.

I think Hy could have done this instead with the same effect:

=> (cond [0 "zero"]
...      [1 "one"]
...      [True (assert 0)])
if 0:
    _hy_anon_var_1 = 'zero'
elif 1:
    _hy_anon_var_1 = 'one'
else:
    assert 0
    _hy_anon_var_1 = None

Presumably, the current version was easier to implement. It does result in some wasted assignments though.

[gilch]

@kirbyfan64 @olasd @paultag Are we good to merge this now, or do you guys want to deal with #842 first? Even if the AST is a little ugly at this point, it does seem to short-circuit statements as expected, which fixes a pretty serious problem. We can clean it up later.

[paultag]

@gilch I don't see #842 as blocking this, let me get a review on this

[paultag]

@hylang/core RFR

[refi64]

If there are no more objections, I can squash this now.

[gilch]

I have one more suggestion, perhaps for later, and more related to #842. Even in the case that and/or contains a statement, it may have several expressions in a row that don't need to be converted to if branches. Hy could avoid redundant assignments by keeping those runs as expressions and only assigning the result to an anonymous variable.

[gilch]

For example:

(print (and 1 2 3 (setv x 1) 4 5 6))
_hy_anon_var_1 = 1 and 2 and 3
if _hy_anon_var_1:
    x = 1
    _hy_anon_var_1 = x and 4 and 5 and 6
print(_hy_anon_var_1)

[algernon]

I'd say that's a different issue @gilch. A worthwhile topic to explore, but indeed, more related to #842, so lets track that separately from this. Can you open a bug?

@kirbyfan64 Squash it! :)

[gilch]

@algernon good enough for me. Opened #854
Let's merge this already.

[refi64]

Uhhh...I tried to squash it...and I think I messed something up...

I love Git. But I hate it. Is this even mergeable now?

[Foxboron]

@kirbyfan64 join IRC tomorrow and i'll help you fix it \o/

[Foxboron]

basically

git checkout master
git checkout -b <new branch>
git cherry-pick #Get the commits you want from the short-circuit branch
git push -f origin:short-circuit <branch name>

or try revert the changes you did with rebase (not likely) and try again

[refi64]

@Foxboron It worked!!!! Thank you!!! I'll remember git cherry-pick next time a freak accident like this happens.

The exact commands I ran were:

git checkout -b short-circuit2
git cherry-pick b131500a53d80514b655258031b263ee8cf7a501
git cherry-pick d6c1b72a54e47e6293015dffb603f6d61014f0a8
git push -f fork short-circuit2:short-circuit

Now it's ready!

[Foxboron]

\o\ \o/ /o/

[Tritlo]

This might be a little late to the party, but:

Mathematically, + and 'or' are similar and * and 'and', i.e.

0 + 0 = 0, 1 + 0 = 1, 0 + 1 = 1 and 1 + 1 != 0 (i.e. 1),

whereas

0*0 = 0, 0*1 = 0, 1*0 = 0 and 1*1 = 1.

In formal boolean algebraic ring, this is even how the + and * operators are defined.

In math, an empty product is usually defined as a 1, and an empty sum is usually a 0. This is why it makes sense for an empty or to return 0 and an empty and to return 1.

[Foxboron]

👍
Merrgginnggg