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

Auto-promote values to HyObjects in the compiler #1314

Merged
merged 6 commits into from
Jul 10, 2017

Conversation

Kodiologist
Copy link
Member

@gilch
Copy link
Member

gilch commented Jun 26, 2017

We've got conflicting files now.

Auto-promotion sure seems like it would make some things easier.

How would this affect #919? Tag macros are still at compile time, but I think Clojure's tagged literals are at read time, like Common Lisp's #.. I'd also like easier access to Python's eval in Hy (we already have exec). Currently you have to import it from the builtins module. We could either rename Hy's eval (to hy-eval) or rename Python's eval (to py-eval). I'm imagining in-line Python--something like

=> (#py"lambda x: x + 1" 41)
42

--would be possible with no performance impact. Currently, even with a macro, the (py)eval has to happen at runtime.

@gilch
Copy link
Member

gilch commented Jun 27, 2017

If we're generally autoboxing unquotes #1174 shouldn't this work?

=> (disassemble `(+ ~(+ 1 1) 40) True)
from hy.core.language import disassemble
from hy import HyExpression, HyInteger, HySymbol
disassemble(HyExpression(((([] + [HySymbol('+')]) + [(1 + 1)]) + [HyInteger(40)])), True)
Traceback (most recent call last):
  File "c:\users\me\documents\github\autobox-hy\hy\importer.py", line 184, in hy_eval
    return eval(ast_compile(expr, "<eval>", "eval"), namespace)
  File "<eval>", line 1, in <module>
  File "C:\Users\ME\Documents\GitHub\autobox-hy/hy/core/language.hy", line 78, in disassemble
    (fake-source-positions tree)
  File "C:\Users\ME\Documents\GitHub\autobox-hy/hy/core/language.hy", line 182, in fake_source_positions
    (fake-source-positions subtree)))
  File "C:\Users\ME\Documents\GitHub\autobox-hy/hy/core/language.hy", line 185, in fake_source_positions
    (setattr tree attr 1))))
AttributeError: 'int' object has no attribute 'start_line'

It works if I box it manually though.

=> (disassemble `(+ ~(HyInteger (+ 1 1)) 40) True)
from hy.core.language import disassemble
from hy import HyExpression, HyInteger, HySymbol
disassemble(HyExpression(((([] + [HySymbol('+')]) + [HyInteger((1 + 1))]) + [HyInteger(40)])), True)
'(2 + 40)'

Is the compiler really the right place to put this?

@Kodiologist
Copy link
Member Author

I think #919 and inline Python (which I, too, would like to see) are orthogonal to this PR. And yeah, I think it would make sense to have builtins.eval exported from hy.core.language as py-eval, although that too would probably be better in a separate PR.

I'll look into the merge conflicts and disassemble. The latter is presumably running into problems because it does its own faking of source positions, which isn't playing nicely with how I do that in the compiler.

Is the compiler really the right place to put this?

I'm pretty sure that the answer is yes. It's the last chance to do it, so doing it then should mean that we only have to implement it in one place in Hy.

@Kodiologist
Copy link
Member Author

@gilch Done.

@Kodiologist
Copy link
Member Author

Aaaand it's broken on Python 2. I'm working on it now.

@Kodiologist
Copy link
Member Author

It was an easy fix; I'd forgotten about the trailing Ls of long literals.

@gilch
Copy link
Member

gilch commented Jul 2, 2017

hy/docs/language/internals.rst may need some clarification because of these changes. That file was not modified though. This PR digs pretty deep into some of Hy's internals. I want more time to review it.

@Kodiologist
Copy link
Member Author

I'm inclined to think that trying to document the implementation was a mistake in the first place. But I don't see anything on that page that's been made obsolete by these changes.

@gilch
Copy link
Member

gilch commented Jul 3, 2017

hy-eval needs a good docstring explaining its use and parameters now that we're using it as a function. (I think special forms and macros need this too, but that's another issue #356)


document the implementation was a mistake

No, the Hy model system is important enough for macro writing that it should be considered part of the public API. See also, this recent stackoverflow question. This PR certainly would have helped.

It needs to be documented. And I don't think this PR goes far enough to change that.

For example, this works in Clojure

user=> (eval `(/ 2))
1/2
user=> (eval `(/ ~2))
1/2
user=> (eval `(~/ 2))
1/2
user=> (eval `(~/ ~2))
1/2

There's no distinction between value and model, like Hy has currently. This PR helps--

=> (eval `(/ 2))  ; works now
from hy.core.language import eval
from hy import HyExpression, HyInteger, HySymbol
eval(HyExpression((([] + [HySymbol('/')]) + [HyInteger(2)])))
0.5
=> (eval `(/ ~2))  ; requires this PR for autobox
from hy.core.language import eval
from hy import HyExpression, HySymbol
eval(HyExpression((([] + [HySymbol('/')]) + [2])))
0.5
=> (eval `(~/ 2))  ; Boom.
from hy.core.language import eval
from hy.core.shadow import /
from hy import HyExpression, HyInteger
eval(HyExpression((([] + [/]) + [HyInteger(2)])))
Traceback (most recent call last):
  File "c:\users\me\documents\github\autobox-hy\hy\importer.py", line 185, in hy_eval
    return eval(ast_compile(expr, "<eval>", "eval"), namespace)
  File "<eval>", line 1, in <module>
  File "c:\users\me\documents\github\autobox-hy\hy\importer.py", line 159, in hy_eval
    replace_hy_obj(hytree, foo)
  File "c:\users\me\documents\github\autobox-hy\hy\models.py", line 50, in replace_hy_obj
    return obj.replace(other)
  File "c:\users\me\documents\github\autobox-hy\hy\models.py", line 178, in replace
    replace_hy_obj(x, other)
  File "c:\users\me\documents\github\autobox-hy\hy\models.py", line 58, in replace_hy_obj
    % type(obj))
TypeError: Don't know how to wrap a <class 'function'> object to a HyObject

--but only certain basic values can be autoboxed. Even understanding the error message requires knowledge of these "internals": There's no Hy model for functions. And custom data types will have the same problem. I don't see an easy way to fix this, so it seems to be beyond the scope of this PR.

But I don't see anything on that page that's been made obsolete by these changes.

I suppose it can wait then.

@Kodiologist
Copy link
Member Author

hy-eval needs a good docstring explaining its use and parameters now that we're using it as a function

Done.

document the implementation was a mistake

No, the Hy model system is important enough for macro writing that it should be considered part of the public API.

I agree. I'm sorry I was imprecise. What I don't think we should be documenting is the stuff under "Hy Internal Theory".

@Kodiologist
Copy link
Member Author

Confusingly, GitHub doesn't seem to display the commits in parentage order, like Git itself does.

@tuturto tuturto merged commit 7c53a07 into hylang:master Jul 10, 2017
@Kodiologist Kodiologist deleted the compiler-autobox branch July 16, 2017 16:11
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.

4 participants