Provide a new hy.eval

[Kodiologist]

• Closes Clean up the signature of hy.eval #2414
• Closes hy.eval crashes when evaluating macros when passed a "<...>" string to module arg  #2202

[scauligi]

Why not call hy_eval with import_stdlib=False?

[Kodiologist]

So something like (hy.eval '(hy.repr "foo") …) works. I already had a test of this, but it was insufficiently stringent (it passed even with import_stdlib = False), so I've tightened it. You should now be able to see it fail with import_stdlib = False added and this del statement commented out.

[scauligi]

I think hy being in the namespace should be handled by inserting it into the __builtins__ key rather than adding/deleting from locals.

From Python eval/exec:

If the globals dictionary is present and does not contain a value for the key builtins, a reference to the dictionary of the built-in module builtins is inserted under that key before expression is parsed.

Since we're already using the same spec, we can piggyback off of that:

    if globals is not None and '__builtins__' not in globals:
        globals['__builtins__'] = dict(vars(builtins), hy=hy)

[Kodiologist]

But that would be a needless inconsistency with the way hy is usually brought into scope. A normal Hy program starts with an implicit import hy; why should a form evaluated with hy.eval be any different?

[scauligi]

In my mind the implicit import hy is an implementation detail just for compiling pyc files, because there's no other way for us to get hy into a bytecode compiled file; to me, going via __builtins__ wherever we otherwise can is more consistent with the intention of hy existing as an always-accessible name.

[scauligi]

Hm. Deleting entries off of locals somehow just feels more fraught to me than futzing with __builtins__, but as long as it works and isn't causing problems I don't have any reason to insist we use builtins over local/import.

[Kodiologist]

Okay. Maybe we'll find a bug eventually. If we then need to switch hy.eval to editing __builtins__, we can hopefully change the usual case of compiling Hy code to edit __builtins__ too instead of doing a regular import.

[scauligi]

Ok bit of a contrived example, but it might not be that much of a stretch if instead of string literals, you were reading and executing snippets of intermixed hy and python out of a file or something:

my_globals = {}
exec("import hy", my_globals)

hy.eval(hy.read("""(do (import types [SimpleNamespace])
                       (setv obj (SimpleNamespace)
                             obj.some-name! 5))"""), my_globals)

print(eval("getattr(obj, hy.mangle('some-name!'))", my_globals))

[Kodiologist]

I see, so this suggests to me that if hy already exists in the locals object, I should save it and then assign it back after hy_eval returns instead of deleting it. Do you think that suffices?

[scauligi]

Yeah that should suffice. Technically we'd still have a problem with something like

my_globals = {}
hy.eval(hy.read("(import hy)"), my_globals)
assert "hy" in my_globals

but... eh

[scauligi]

I think this is slightly incorrect; the documentation for both Python's eval and expr state that if locals is not passed but globals is, then globals is also used for the local vars. Compare:

=> (eval "locals().keys()" {})
(dict-keys ["__builtins__"])
=> (hy.eval '(.keys (locals)) {})
(dict-keys [... (all of the local vars) ...])

[Kodiologist]

Oops, good catch. I've changed hy_eval_user and its docstring appropriately and expanded testing a bit.

[scauligi]

Hmm, this condition expression here now looks confusing. Since we have the None-check and assignment for locals at 736-737, at first glance it looks like the if locals is None on 742 will never be False.

In any case, the locals/globals thing will already be handled by eval anyway, so I think the clearer thing is to reassign locals only when all(arg is None for arg in (locals, globals, module)) (or something more elegant), but otherwise pass locals to hy_eval unmodified.

[Kodiologist]

I'm not 100% sure what change you're suggesting. Could you push a commit?

[scauligi]

    value = hy_eval(
        hytree = model,
        globals = globals,
        locals = (inspect.getargvalues(inspect.stack()[1][0]).locals
            if all(arg is None for arg in (locals, globals, module)) else locals),
        module = get_compiler_module(module, None, True),
        import_stdlib=False)

[Kodiologist]

I tried replacing the assignment to value thus, and removing the preceding if locals is None: locals = globals and the following if locals: …, and the result was a test failure NameError: name 'hy' is not defined in tests/native_tests/hy_eval.hy::test_locals, which seems logical enough.

[scauligi]

ah here's the full diff, I forgot that I still had import_stdlib=False in the snippet above so you need the builtins injection as well for it to work:

diff --git a/hy/compiler.py b/hy/compiler.py
index af0cc8dc..f61d807a 100755
--- a/hy/compiler.py
+++ b/hy/compiler.py
@@ -1,4 +1,5 @@
 import ast
+import builtins
 import copy
 import importlib
 import inspect
@@ -733,18 +734,16 @@ def hy_eval_user(model, globals = None, locals = None, module = None):
         (hy.eval '(my-test-mac) :module hyrule)  ; NameError
         (hy.eval '(list-n 3 1) :module hyrule)   ; => [1 1 1]"""
 
-    if locals is None:
-        locals = globals
+    if globals is not None and '__builtins__' not in globals:
+        globals['__builtins__'] = dict(vars(builtins), hy=hy)
     value = hy_eval(
         hytree = model,
         globals = globals,
         locals = (inspect.getargvalues(inspect.stack()[1][0]).locals
-            if locals is None and module is None
-            else locals),
-        module = get_compiler_module(module, None, True))
-    if locals:
-        # Remove the implicitly added `hy`.
-        del locals['hy']
+                  if all(arg is None for arg in (locals, globals, module))
+                  else locals),
+        module = get_compiler_module(module, None, True),
+        import_stdlib=False)
     return value

[Kodiologist]

I remembered that #2202 existed, so I added another commit to the end to obviate it.

[Kodiologist]

Okay, hy.eval now saves and restores the hy element, in a finally just in case. The new test is test-no-extra-hy-removal. @scauligi Good to merge?

[scauligi]

hunh, that triple trick with hy_was is pretty clever.

looks good to me!