fennel: Problem: New fennel macros break programs v1 PROPOSED

Ambrose Bonnaire-Sergeant: 1
 Problem: New fennel macros break programs

 5 files changed, 26 insertions(+), 16 deletions(-)
#959590 .build.yml failed
Phil Hagelberg <phil@hagelb.org> writes:
Phil, I cannot agree more.

I, too, was stopped by Fennel on multiple occasions from overriding its
built-ins.  For example, I found that LuaUnit reports wrong locations
due to Fennel's 'fn' macro not always returning 'nil' (side-note: Fennel
compiler tests have this problem too).  I figured, why not override the
'fn' macro in the test file to always return 'nil'?  Of course, the
compiler stopped me at once from such a hack.  I later solved the
problem by wrapping LuaUnit assertions, but still...
Rudolf Adamkovič <salutis@me.com> writes:
I'm glad you brought this up! This is actually something that's
low-key bothered me for some time. One change I've been meaning to
make in the compiler is a section for "opt-in" macros, where they're
built in, but you have to import them from a built-in module namespace
like any other macro module.

With that in place, any new macros we add would live there (e.g.
`fennel.macros-aux`), and we'd only add new ones we want in the root
scope on the next semver-major release.

Another thing we could do to alleviate programs breaking is introduce
new macros (or maybe they'd need to be specials?) `rename-macro` and
`hide-macro`, which would allow you to allow the use of a symbol by
omitting the macro from the current scope and all child scopes. This
would make it a lot easier for people to shim old code broken by newer
fennel releases, even if we *did* go the `fennel.macros-aux` route.

Jesse Wertheim <jaawerth@gmail.com> writes:
Phil Hagelberg <phil@hagelb.org> writes:
Speaking of the *, it would be nice if the Fennel mode indented starred
versions of built-in macros the way it indents the originals.  For
instance, if the user defines fn* with some light runtime type checking,
nil returns, or whatever they need, the Fennel mode should indent fn*
like it indents fn.

Back to the topic...

As for macro shadowing, the ideas upthread sound too complicated to my
ears.  All I would like to see is the ability to re-define built-in
macros, with no new syntax.  In other words, I would expect locally
defined macros inside of a module, as well as, locally imported macro
modules, to take precedence over the original macros in that module.

Rudolf Adamkovič <salutis@me.com> writes:
Accidentally sent this reply off-list a while back; oops!

Phil Hagelberg <phil@hagelb.org> writes:
Export patchset (mbox)
How do I use this?

Copy & paste the following snippet into your terminal to import this patchset into git:

curl -s https://lists.sr.ht/~technomancy/fennel/patches/39850/mbox | git am -3
Learn more about email & git

[PATCH fennel] Problem: New fennel macros break programs Export this patch

This patch is a proof-of-concept to test if this idea is viable
both via CI and to give the community something concrete to try
out. It is mostly intended to stimulate discussion. Please see
the annotation to this patch for context.

# Original commit log:

Locals shadow macros

Currently, macros may shadow other macros and locals, but locals cannot 
shadow macros. This causes a situation where adding new fennel macros
are breaking changes to some programs.

To implement this, we need to disambiguate who 'wins' when both macros
and locals are in scope, now that locals are allowed to win. A new
metatable scope.resolve is used for this purpose: scope.resolve[name]
is :local if the local wins, ditto for :macro.
This seems like an unnecessary feature for the bootstrapping compiler,
so I left it out.
As a newcomer to Fennel's community, I've noticed a category of
seemingly avoidable work being done by users and maintainers that
could be better invested elsewhere. It's related to fennel introducing
new macros. No matter how amazing they are (even the humble 'case' made
a sceptical andreyorst a believer!), they are still breaking changes for
programs that already use those names.

If I'm preaching to the choir, you can stop reading here and let me
know your thoughts :)

Otherwise, here are my observations.

At the recent user group, andreyorst noted that cljlib is not compatible
with the new 'case' macro, because it uses that name in its code.
He might have even mentioned other projects of his needed the same
change. (I can't find the code, so maybe I misheard the details.
Regardless, the time we spent talking about it could have been better
spent!). In the same meeting, a new user had a valuable call to action
for andreyorst regarding tooling. His time could be better spent
addressing that feedback.

IIRC Phil had an anecdote about surveying fennel programs to look for
unused names to use for new macros, because otherwise a new macro might
inadvertently break too many programs. I think the conclusion was a
name would only break 2 games he found, so the solution wasn't backwards
compatible in practice anyway. Again, if we can save Phil that time
(each release!), we should.

Relatedly, each time a new macro is added to fennel, the changelog does
not say this is a (type of) breaking change. It's unclear how many users
have tried to update programs and spent time renaming their locals,
and/or forking (transitive!) dependencies to fix these issues, and
how successful they were.

I know the user guide for Fennel says transitive deps are rare, but my
first real program in Fennel depended on cljlib -> {itable,lazy-seq}
(ignoring the ubiquity of fennel-test for the moment).
IMO dependency hell is plausible with git submodules---say 'lazy-seq'
shadowed a new macro. The fact that 'lazy-seq' is both modular and
useful now works against it, because it's more likely to be a transitive
dependency and introduce dependency hell. OTOH, it might prevent fennel
from introducing a really good name because 'lazy-seq' might be too

 bootstrap/fennel.lua    |  4 +++-
 src/fennel/compiler.fnl | 26 +++++++++++++++-----------
 test/failures.fnl       |  4 ++--
 test/macro.fnl          |  3 ++-
 test/macros.fnl         |  5 ++++-
 5 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/bootstrap/fennel.lua b/bootstrap/fennel.lua
index bb89b0f..2272f13 100644
--- a/bootstrap/fennel.lua
@@ -714,7 +714,8 @@ local compiler = (function()
            parent = parent,
            vararg = parent and parent.vararg,
            depth = parent and ((parent.depth or 0) + 1) or 0,
            hashfn = parent and parent.hashfn
            hashfn = parent and parent.hashfn,
            resolve = parent and parent.resolve

@@ -2695,6 +2696,7 @@ local specials = (function()
        for k,v in pairs(macros) do
            compiler.assert(type(v) == 'function', 'expected each macro to be function', ast)
            scope.macros[k] = v
            scope.resolve = "macro"

diff --git a/src/fennel/compiler.fnl b/src/fennel/compiler.fnl
index 3c0e2c1..d7d825d 100644
--- a/src/fennel/compiler.fnl
+++ b/src/fennel/compiler.fnl
@@ -18,6 +18,7 @@ via the 'eval-compiler' special form (may change). They use metatables to
implement nesting. "
  (let [parent (or ?parent scopes.global)]
    {:includes (setmetatable [] {:__index (and parent parent.includes)})
     :resolve (setmetatable [] {:__index (and parent parent.resolve)})
     :macros (setmetatable [] {:__index (and parent parent.macros)})
     :manglings (setmetatable [] {:__index (and parent parent.manglings)})
     :specials (setmetatable [] {:__index (and parent parent.specials)})
@@ -184,10 +185,11 @@ rather than generating new one."
    ;; we can't block in the parser because & is still ok in symbols like &as
    (assert-compile (not (name:find "&")) "invalid character: &" symbol)
    (assert-compile (not (name:find "^%.")) "invalid character: ." symbol)
    (assert-compile (not (or (. scope.specials name)
                             (and (not macro?) (. scope.macros name))))
                    (: "local %s was overshadowed by a special form or macro"
    (assert-compile (not (. scope.specials name))
                    (: "local %s was overshadowed by a special form"
                       :format name) ast)
    (when (. scope.macros name)
      (tset scope.resolve name (if macro? :macro :local)))
    (assert-compile (not (utils.quoted? symbol))
                    (string.format "macro tried to bind %s without gensym" name)
@@ -220,9 +222,11 @@ if they have already been declared via declare-local"
    (let [parts (or multi-sym-parts [name])
          etype (or (and (< 1 (length parts)) :expression) :sym)
          local? (. scope.manglings (. parts 1))]
      (when (and local? (. scope.symmeta (. parts 1)))
      (when (and local? (. scope.symmeta (. parts 1))
                 (not (= :macro (. scope.resolve (. parts 1)))))
        (tset (. scope.symmeta (. parts 1)) :used true))
      (assert-compile (not (. scope.macros (. parts 1)))
      (assert-compile (or (not (. scope.macros (. parts 1)))
                          (= :local (. scope.resolve (. parts 1))))
                      (.. "tried to reference a macro without calling it") symbol)
      (assert-compile (or (not (. scope.specials (. parts 1)))
                          (= :require (. parts 1)))
@@ -400,12 +404,12 @@ if opts contains the nval option."
  (let [macro* (-?>> (utils.sym? (. ast 1)) (tostring) (. scope.macros))
        multi-sym-parts (utils.multi-sym? (. ast 1))]
    (if (and (not macro*) multi-sym-parts)
        (let [nested-macro (utils.get-in scope.macros multi-sym-parts)]
          (assert-compile (or (not (. scope.macros (. multi-sym-parts 1)))
                              (= (type nested-macro) :function))
                          "macro not found in imported macro module" ast)
      (let [nested-macro (utils.get-in scope.macros multi-sym-parts)]
        (assert-compile (or (not (. scope.macros (. multi-sym-parts 1)))
                            (= (type nested-macro) :function))
                        "macro not found in imported macro module" ast)

(fn propagate-trace-info [{: filename : line : bytestart : byteend} _index node]
  "The stack trace info should be based on the macro caller, not the macro AST."
diff --git a/test/failures.fnl b/test/failures.fnl
index f08d3cc..ada646d 100644
--- a/test/failures.fnl
+++ b/test/failures.fnl
@@ -8,7 +8,7 @@
  `(let [(ok# msg#) (pcall fennel.compile-string (macrodebug ,form true)
                           {:allowedGlobals ["pairs" "next" "ipairs" "_G"]})]
     (l.assertFalse ok# (.. "Expected failure: " ,(tostring form)))
     (l.assertStrContains msg# ,expected)))
     (l.assertStrContains msg# ,expected false ,(tostring form))))

;; use this only when you can't use the above macro
(fn test-failures [failures]
@@ -17,7 +17,7 @@
                           {:allowedGlobals ["pairs" "next" "ipairs" "_G"]
                            :unfriendly true})]
      (l.assertFalse ok? (.. "Expected compiling " code " to fail."))
      (l.assertStrContains msg expected-msg))))
      (l.assertStrContains msg expected-msg false msg))))

(fn test-names []
  (assert-fail (local + 6) "overshadowed by a special form")
diff --git a/test/macro.fnl b/test/macro.fnl
index fbbb4cc..893e6df 100644
--- a/test/macro.fnl
+++ b/test/macro.fnl
@@ -77,7 +77,8 @@
  (== (do (import-macros {: unsandboxed} :test.macros) (unsandboxed))
      "[\"no\" \"sandbox\"]" {:compiler-env _G})
  (let [not-unqualified "(import-macros hi :test.macros) (print (inc 1))"]
    (l.assertFalse (pcall fennel.eval not-unqualified))))
    (l.assertFalse (pcall fennel.eval not-unqualified)))
  (== (do (import-macros {: case} :test.macros) (case 42)) 42))

(fn test-macro-path []
  (== (do (import-macros m :test.other-macros) (m.m)) "testing macro path"))
diff --git a/test/macros.fnl b/test/macros.fnl
index 0d8124e..090c7e2 100644
--- a/test/macros.fnl
+++ b/test/macros.fnl
@@ -2,6 +2,7 @@

(fn def [] (error "oh no") 32)
(fn abc [] (def) 1)
(fn case [a] a) ;; we can define locals that shadow fennel macros

{"->1" (fn [val ...]
        (var x val)
@@ -21,4 +22,6 @@
                       y# {:one 1}]
                   (+ (x#:abc) y#.one)))
 :unsandboxed (fn [] (view [:no :sandbox]))
 :fail-one (fn [x] (when (= x 1) (abc)) true)}
 :fail-one (fn [x] (when (= x 1) (abc)) true)
 ;; we can reference locals that shadow fennel macros
 : case}
2.37.1 (Apple Git-137.1)
fennel/patches/.build.yml: FAILED in 29s

[Problem: New fennel macros break programs][0] from [Ambrose Bonnaire-Sergeant][1]

[0]: https://lists.sr.ht/~technomancy/fennel/patches/39850
[1]: mailto:abonnairesergeant@gmail.com

✗ #959590 FAILED fennel/patches/.build.yml https://builds.sr.ht/~technomancy/job/959590
Ambrose Bonnaire-Sergeant <abonnairesergeant@gmail.com> writes: