-
Notifications
You must be signed in to change notification settings - Fork 372
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
implement let
#1426
implement let
#1426
Conversation
I'm confused by that test result. It's only failing on version Python versions 3.4 and 3.5. I only tested 3.6 before I pushed, but that failing test is not testing a file I changed. Did a bug sneak through to master in a recent change? Is it the |
Yup. That was it. Any bugs you guys find will need a test. Nested functions have a known issue with default values, I need to expand the value, but protect the argument symbol. That's just not implemented yet. I'm also thinking that There might be other special cases of special forms that need special treatment that I haven't thought of yet. Python accepts dict lookups in place of plain identifiers in most places though, even in assignments and deletions, so most special forms just work. |
7d82d7a
to
60992cf
Compare
Functions should be working now. I haven't tested For now, using |
This way madness lies, but if you're brave enough to try, far be it from me to stop you. |
Madness? Bwa ha ha! So many branches in a 100+ line macro makes me uneasy. This is not design. It's organic growth from the vaguest idea of how this could possibly work. Lisp is good at that. But... the tests all pass! Once I round out the test harness a bit, I'll try to factor out the symbol macros. If I can refactor this enough it might not seem so scary to others (or me in three months). And simplifying the compiler would also help a bit. I wouldn't have to check for so many edge cases if our special forms didn't have them. We can move some of the special form functionality into macros. |
When pondering how This means the I also added more tests. That's almost everything I'd planned, but the |
Probably the reason that |
There's the The semantics were undefined, since Now, a |
I'm not sure. It would probably depend on the details of the new semantics. For example, if each top-level form were executed after being compiled, then
¯\_(ツ)_/¯ Unrelatedly, if you're still interested in symbol namespacing (#277), it's probably worth thinking about how that would interact with |
This let already handles the dot-access symbols. Either of the syntax proposals from #1407 could be made to work. The root
It's not obvious how. As mentioned in #1407, we could implement dynamic variables with it, but that's different than the lexical variables implemented here. We'd probably call the dynamic form |
I factored out |
If you haven't already seen it, #1216 has a list of |
I'll look through those. #180 turned up an issue-- I was able to make We might need to change how |
Looking at those other let-related issues from #1216-- #576 was about a dynamic #1212 is still an issue. I'm expanding a binding that doesn't exist yet. |
That should fix #1212. I also added an xfail for the issue about macros expanding in the wrong module context. The only way I can think of to fix that, short of deferring expansion to runtime (not worth it), is to require the user to pass the name of the current module to |
That last commit adds an implicit I'm not entirely sure if that's the best way to handle it, but Clojure does something similar. This makes the error messages for macroexpansions less pretty. But we need to overhaul those anyway #1429. It may be a separate issue best handled in another PR. I've xfailed the relevant test. |
Okay @hylang/core, I missed things before, but I think the new |
You seem to still have a few issues listed under The documentation should explain that the
This is true for the class name, but class member definitions will neither assign to the unmangled name nor the variable introduced by Is it a bug that |
I think I should make I don't understand what scope
If
OK, documenting this doesn't hurt. On Python3,
Hy special forms with anon vars also have this issue, and so do a lot of other macros that create gensyms with
I feel like the gensym implementation should already be doing that. We don't ever want to Using the That doesn't stop you from clobbering a gensym with But a
What do you mean?
That does seem undesirable. The earlier dict implementation of Note that a Potential options
I'm leaning towards option 2, with
No, that is the intended behavior. If you want to assign to the let binding, you have to use
The earlier dict implementation of let bindings were implicitly |
94f30c3
to
01f3c40
Compare
I implemented I changed the I changed the gensym format to start with |
The new changes sound good to me. I'm impressed you managed to make
For
They're pretty much the same as creating a macro and then immediately calling it. So, they're in the compile-time global scope. Your strategy, as I understand, is to expand all macros in the body of
That's a good point. It may be an important difference that no specific lifetime is implied for our other anonymous variables and gensyms, whereas for
Chiefly that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I waded through the code (there's plenty of it) and tried breaking the new implementation without succeeding, so this looks as good as it can get to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is insane. :O
@rcarmo You're going to like this one...
Hmmm. I’m curious.
… On 1 Nov 2017, at 14:37, Ryan Gonzalez ***@***.***> wrote:
@kirbyfan64 approved this pull request.
This is insane. :O
@rcarmo You're going to like this one...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Wait a minute, guys, are you sure this is ready to merge? |
@kirbyfan64 Also, you should rebase rather than leaving a merge commit inside the branch, per CONTRIBUTING.rst. |
Ah, damn it...another repo I work with mandates merge commits, and I keep mixing these up... Also it seems done enough. @gilch had already requested my review to be the second, this isn't in core, it's tagged as experimental, and it'll be easier reviewing two PRs than one mega-sized one. |
The other project wants you to include a merge from master into the PR's branch, rather than the reverse? I wonder what the point of that is. Okay, I'll open issues. |
Hmmm. Merged already? I wonder... Need to take some time and check this out, I miss Hy. |
Very experimental, unpolished, and not quite finished, but this approach really seems viable--all those tests pass, including those with
yield
,return
, andbreak
. I thought I'd put it up at this point so you guys can look at it and offer thoughts.Oh, and this also fixes #1420. I think. I needed it working properly to try this.
This
let
expands its binding symbols in its lexical scope to dict lookups in a hidden (gensymed) dict. The trick is a propermacroexpand-all
with "symbol macros". Since macros are pre-expanded, we just (heh) have to account for every special form properly, to know which symbols to expand. But that means it's a monster compared to the oldlet
.This kind of thing is motivation to keep the special forms as simple as we reasonably can, since it makes this type of thing easier. I sure wished they were simpler when implementing this. Complexity has costs. Some of this is on Python, but the compiler should probably stick as close as reasonably possible to that, without adding features. Most extra features beyond Python belong at the macro level or higher. Any additions or changes to special forms could potentially break macros like this, so the more complex our compiler, the higher the maintenance burden.
This approach could also give us local symbol macros in Hy, since I basically had to implement them to get this far. It might be possible to factor it out of the
let
implementation.