bakpakin / Fennel

Lua Lisp Language
https://fennel-lang.org
MIT License
2.44k stars 126 forks source link

Cant clear "global X in compiler scope" warning #381

Closed rktjmp closed 3 years ago

rktjmp commented 3 years ago

See demo repo here: https://github.com/rktjmp/fennel-macro-g

I want to use os (or likely _G.os) in a macro, but the existing solutions (as far as I can tell) seem to:

I can't tell if maybe I am just misunderstanding the docs, misapplying them or if there is a bug.

This line, makes be think that _G is supposed to be a "no warning escape hatch", (https://fennel-lang.org/tutorial#strict-global-checking sort of implies that might be the case too).

https://github.com/bakpakin/Fennel/blob/4358d6628e6b804f39db37045b34d0cd3803e025/src/fennel/specials.fnl#L954-L956

But keys prefixed with _G (i.e _G.os.date) always come in as os because of this line:

https://github.com/bakpakin/Fennel/blob/4358d6628e6b804f39db37045b34d0cd3803e025/src/fennel/specials.fnl#L1063

Since _G exists as a key on env, the metatable __index is never hit (it's only called for missing keys), and so the requested key ends up being os, not _G.

The following patch does disable the warning for _G, but it also adds _G to the list of always allowed global which is maybe undesired.

diff --git a/src/fennel/compiler.fnl b/src/fennel/compiler.fnl
index e3e8c7b..7491e5a 100644
--- a/src/fennel/compiler.fnl
+++ b/src/fennel/compiler.fnl
@@ -93,7 +93,7 @@ Takes a Lua identifier and returns the Fennel symbol string that created it."
   "If there's a provided list of allowed globals, don't let references thru that
 aren't on the list. This list is set at the compiler entry points of compile
 and compile-stream."
-  (or (not allowed-globals) (utils.member? name allowed-globals)))
+  (or (not allowed-globals) (= name "_G") (utils.member? name allowed-globals)))

 (fn unique-mangling [original mangling scope append]
   (if (and (. scope.unmanglings mangling) (not (. scope.gensyms mangling)))
diff --git a/src/fennel/specials.fnl b/src/fennel/specials.fnl
index 68c6754..e123360 100644
--- a/src/fennel/specials.fnl
+++ b/src/fennel/specials.fnl
@@ -962,6 +962,7 @@ Only works in Lua 5.3+ or LuaJIT with the --use-bit-lib flag.")

 (fn compiler-env-warn [_ key]
   "Warn once when allowing a global that the sandbox would normally block."
+  (print :env-warn key) ; demonstrate _G missing/not missing
   (let [v (. _G key)]
     (when (and v io io.stderr (not (. already-warned? key)))
       (tset already-warned? key true)
@@ -1060,7 +1061,7 @@ Only works in Lua 5.3+ or LuaJIT with the --use-bit-lib flag.")
                                              "must call from macro" ast)
                             (compiler.macroexpand form
                                                   compiler.scopes.macro))}]
-    (set env._G env)
+    ; (set env._G env)
     (setmetatable env
                   {:__index provided
                    :__newindex provided

Basically I am wondering:

technomancy commented 3 years ago

I think I have a fix for this which should let the compiler options propagate as necessary in this context; can you try the latest?

rktjmp commented 3 years ago

Yep, using the following options gives no warning, which I think is on spec:

    opts = {
      env = "_COMPILER",
      compilerEnv = _G
    }
    return function()
      return fennel.eval(code, opts), "macro.fnl"
    end
λ lua demo.lua
using custom macro searcher loader
my-var (value: 'icecream') compiled at 1628656907
rktjmp commented 3 years ago

@technomancy The patch part about (set env._G env) and (local already-warned? {:_G true}) still seems like a smell to me, (local already-warned? {:_G true}) is basically a no-op since it will never be checked against from what I can tell.

technomancy commented 3 years ago

Well, the warning system is going away soon anyway, so I'm not too concerned about that.