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

Python statements are executed unconditionally #233

Closed
joehakimrahme opened this issue Jul 7, 2013 · 17 comments
Closed

Python statements are executed unconditionally #233

joehakimrahme opened this issue Jul 7, 2013 · 17 comments

Comments

@joehakimrahme
Copy link
Contributor

When translating a Hy expression into a Python statement (print, yield, ...), it gets executed unconditionally. For instance:

=> (and False (do (print "hello") True))
hello
False

More info can be found in this discussion.

@paultag
Copy link
Member

paultag commented Jul 7, 2013

If you try it through hy2py, it turns into

print u'hello'
(False and True)

(Which, frankly, I don't see as bad)

Due to Python's distinction between expr and stmt. It's just a side-effect, and the fact it stops at the first isn't really that important.

We can wrap it in a function or something if we need to, but I'm not totally convinced it's needed yet.

@joehakimrahme
Copy link
Contributor Author

I don't know how to use hy2py (can you point me to a doc or even show me how to install/run it?) but I'm pretty sure you'll get a separate behavior in Python3 where print is a function.

We could circumvent the issue by importing the function from __future__ (cc @olasd) but it won't solve the issue for yield and/or other statements.

Should we care that we have the same behavior for all versions of Python?

@paultag
Copy link
Member

paultag commented Jul 7, 2013

so print function won't solve the general problem with stmts in an expr place in the Python AST.

hy2py can be run by invoking ./bin/hy2py file.hy in place in the git repo :)

@joehakimrahme
Copy link
Contributor Author

Thanks. It's worth mentioning that hy2py won't work with Py3 because astor isn't Py3-ready :-(

So what's your take on the discrepancies between Py2/Py3? Does it justify confirming this bug?

@paultag
Copy link
Member

paultag commented Jul 7, 2013

There's nothing we can do about changed behavior - it's like saying there's an issue because two bits of Python code act differently on 2.x to 3.x, it's just a fact of Python life :)

We need to take the Pythonic "We're all consenting adults" approach and trust the programmer can make sound decisions that work for their usecase :)

We make small accommodations (future division, etc), but on the whole, I think we can trust the programmer knows what version of Python the code is running under

@jd
Copy link
Member

jd commented Jul 8, 2013

This is a major issue of Hy IMHO. You can't just consider this as a side effect and let it be. This is going to bite a lot of people, and documenting isn't going to be enough, this has to be fixed.

I'd say that writing `and' as a macro might be a way to fix it.

@joehakimrahme
Copy link
Contributor Author

I'd say that writing `and' as a macro might be a way to fix it.

Is it just a problem with and (and or)? Aren't there other ways of deferring evaluation where statements might get "accidentally" executed?

What's the issue with transforming statements into a function?

@olasd
Copy link
Member

olasd commented Jul 8, 2013

First off, I'll note that I'm firmly of the opinion that the current behavior (I implemented) is a stop-gap before a proper solution can be implemented. Leaking statements that wouldn't be executed if they were "expressions" is confusing behavior.

Writing and as a macro would not solve the general problem (and I think the perf hit wouldn't be worth it)

Transforming statements into a function means creating a closure, which means losing the possibility of effecting on the global namespace. Which I think would be confusing too.

One solution for this problem, would be to resort to a hack hooking storeize to collect variable names and using global to enable access to the toplevel names in the closure (making it leak). But that sounds gross.

I'm open to less-gross ideas if you have any.

@Kodiologist
Copy link
Member

I believe these are some other manifestations of the same issue:

(setv x 1)
(setv y (+= x 1))
(print y)

That should print 2, but it prints None.

(setv l ["a" "b" "c"])
(defn f [a]
  (print "hi")
  a)
(setv y (setv (get l (f 0)) "A"))
(print y)

That should print "hi" only once, but it prints "hi" twice.

@refi64
Copy link
Contributor

refi64 commented Jul 31, 2015

How the heck did this get closed? @Kodiologist showed other manifestations of this issue that #824 did NOT solve. :(

@Kodiologist
Copy link
Member

Yep, it should stay open, in my opinion. Or maybe we should make new issues for each operator or function that exhibits this problem, if they're going to have to be dealt with piecemeal.

@refi64
Copy link
Contributor

refi64 commented Jul 31, 2015

@Kodiologist Well, your second example isn't really an issue; you're supposed to use (setv y (assoc l (f 0) "A")).

@refi64
Copy link
Contributor

refi64 commented Jul 31, 2015

So I think the main issue remaining is just that the mutating operators (+=, -=, etc.) need to return a value.

@Foxboron
Copy link
Member

I got no idea how this was closed. I believe github auto-closed this when i merged the PR

@Foxboron Foxboron reopened this Jul 31, 2015
@gilch
Copy link
Member

gilch commented Jul 31, 2015

Even if assoc is a better alternative, double-calling the function is unexpected. I wonder if this can happen in other cases.

refi64 added a commit to refi64/hy that referenced this issue Jul 31, 2015
@gilch
Copy link
Member

gilch commented Jul 31, 2015

@kirbyfan64 the mutating operators returning values seems like a separate issue from unexpected execution of statements in this issue. If you want to open a new issue for that we can all discuss it, but I think implementing this correctly is going to be more trouble than it's worth. (Also don't forget that setv is a mutating operator)

@Kodiologist
Copy link
Member

Now that setv returns None, we probably don't want the mutating operators to be any different, anyway.

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

No branches or pull requests

8 participants