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

Rename def to setv; adjust require statements for let macro. #2

Merged
merged 4 commits into from
Jun 27, 2018
Merged

Rename def to setv; adjust require statements for let macro. #2

merged 4 commits into from
Jun 27, 2018

Conversation

brandonwillard
Copy link
Owner

Small adjustments for recent Hy updates.

@brandonwillard
Copy link
Owner Author

I think the current problems are due to a change in the let macro.

Perhaps it has to do with the way in which its bindings are assigned/bound:

=> (let [a 2 a 1] a)
_hyx_XsemicolonXletXvertical_lineX1270 = {}
_hyx_XsemicolonXletXvertical_lineX1270['a'] = 2
_hyx_XsemicolonXletXvertical_lineX1270['a']

2
=> None

@algernon
Copy link
Contributor

The changes look fine to me. Any idea why the tests fail, and how they could be updated to work with newer Hy?

(I don't have the time to dive into it myself, sadly.)

@gilch
Copy link

gilch commented Jun 27, 2018

That's a bug in the let macro, thanks for finding it. It looks like the binding names can only be used once. I'll fix it soon. In the meantime, you can use nested lets as a workaround.

=> (let [a 2] (let [a 1] a))
_hyx_XsemicolonXletXvertical_lineX1235 = {}
_hyx_XsemicolonXletXvertical_lineX1235['a'] = 2
_hyx_XsemicolonXletXvertical_lineX1236 = {}
_hyx_XsemicolonXletXvertical_lineX1236['a'] = 1
_hyx_XsemicolonXletXvertical_lineX1236['a']

1

@brandonwillard
Copy link
Owner Author

@gilch, thanks for the heads-up! I created an issue for it: hylang/hy#1645.

@algernon
Copy link
Contributor

Tests look better now, thanks a lot! I'll have a bit of time this coming Friday, will look into this (and the adderall PR) then.

@brandonwillard
Copy link
Owner Author

brandonwillard commented Jun 27, 2018

All right, I think I found and corrected the remaining issues (the current test failure is something entirely different and ostensibly Python 3.3 related), but I'm also finding some aspects of Hy macros that are quite confusing.

For instance, the following doesn't work without an additional let require (commented out):

=> (import [monaxhyd [monads]])
=> (require [monaxhyd.core [*]])
=> ;; (require [hy.contrib.walk [let]])
=> (domonad monads.identity-m [[a 1] [b (inc a)]] (+ a b))
from hy import HySymbol
from hy.core.language import inc
let([_hyx_XsemicolonXgXvertical_lineX1236, monads.identity_m, m_bind,
    _hyx_XsemicolonXgXvertical_lineX1236[HySymbol('m-bind')], m_result,
    _hyx_XsemicolonXgXvertical_lineX1236[HySymbol('m-result')], m_zero,
    _hyx_XsemicolonXgXvertical_lineX1236[HySymbol('m-zero')], m_plus,
    _hyx_XsemicolonXgXvertical_lineX1236[HySymbol('m-plus')]], m_bind(1, lambda
    a: m_bind(inc(a), lambda b: m_result(a + b))))

Traceback (most recent call last):
  File "/home/bwillard/projects/code/python/hy/hy/importer.py", line 199, in hy_eval
    return eval(ast_compile(expr, "<eval>", "eval"), namespace)
  File "<eval>", line 1, in <module>
NameError: name 'let' is not defined

It seems odd to require macros that are dependencies of imported macros, no? Am I doing the imports incorrectly?
Regardless, I can guess that macro expansion is happening in the given context (which doesn't explicitly provide let), but shouldn't the let requirement be automatically added/inferred by (require [monaxhyd.core [*]])?

@brandonwillard
Copy link
Owner Author

brandonwillard commented Jun 27, 2018

I've created a PR for the remaining issue: hylang/hy#1646 #3

@algernon
Copy link
Contributor

It seems odd to require macros that are dependencies of imported macros, no? Am I doing the imports incorrectly?
Regardless, I can guess that macro expansion is happening in the given context (which doesn't explicitly provide let), but shouldn't the let requirement be automatically added/inferred by (require [monaxhyd.core [*]])?

I think you are doing the imports right, and it is a reasonable expectation that dependencies would get require'd too. But for the time being, the workaround is acceptable.

As I already merged #3, merging this too. Thank you for your work on updating monaxhyd (and adderall) to recent Hy versions!

@algernon algernon merged commit c5d3b34 into brandonwillard:master Jun 27, 2018
@gilch
Copy link

gilch commented Jun 28, 2018

@brandonwillard you should be able to put a (require [hy.contrib.walk [let]]) in the expansion of any macro that needs it. It doesn't have to be at the top of the file.

Alternatively, you could use macroexpand-all to pre-expand any let in the expansion before you return it.

I think the namespacing of macros is one area where Hy needs improvement.

@brandonwillard
Copy link
Owner Author

@gilch, the latter approach, with macroexpand-all, is exactly what I was looking for!

Otherwise, is there an existing issue that tracks this problem/feature for require (if not, I can create one)? I've found a few related issues, but nothing that directly addresses it.
Regardless, I wouldn't mind digging into this particular problem.

@brandonwillard brandonwillard deleted the fix-def-name-change branch June 28, 2018 16:54
@gilch
Copy link

gilch commented Jun 29, 2018

@brandonwillard I don't know that we have exactly that issue, but these three look related.
hylang/hy#1416
hylang/hy#1407
hylang/hy#277

the latter approach, with macroexpand-all, is exactly what I was looking for!

It might not work as well as you'd hope. It's actually not what I'd recommend in this case, now that I think about it. Both of my suggestions have problems. Including require in the expansion will affect which symbols count as macros for the rest of the module, I think, which could potentially break code far away if it was using a different macro or function with the same name.

But using macroexpand-all like that could end up using the wrong macro too. Which symbols count as macros depends on the current expansion context. The macroexpand-all function has another argument that allows you to specify which module's context to use, and the hidden &name parameter available to defmacro is set to the module it's currently expanding in. (This is how let can pre-expand its body in the right context.) Thus pre-expanding with macroexpand-all may use the wrong context for the body, which may treat some symbols as macros which shouldn't be, or vice-versa, or even expand macro symbols with the wrong macro, e.g. what if the client module had (require [hy.contrib.defmulti [defn]])?

A more Clojure-like symbol namespace system could deal with this pretty easily, but for now, if you want to do this right you'd have to use gensyms. So, wrap the return code in a do, like so

(defmacro/g! [...]
  `(do
     (require [hy.contrib.walk [let :as ~g!let]])
     (~g!let [...

This will add a gensym'd let to the client module's expansion context, which should prevent any kind of accidental capture.

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

Successfully merging this pull request may close these issues.

3 participants