Ambrose Bonnaire-Sergeant: 1 Problem: New fennel macros break programs 5 files changed, 26 insertions(+), 16 deletions(-)
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.
I think the use case for this is somewhat limited. If you own the code affected by a new macro, it's already very easy to work around the conflict by slapping a * on the name of the local. If you are pulling the code in as a dependency (possibly indirectly) then editing the source to add a `hide-macro` within the file itself is not always feasible. A fix which helps with the latter case could be one that revolves around the package searcher options table. If you could specify at your application entry point (the point at which the searcher is installed) which macros could be shadowed and which could not, then it might be more helpful at addressing the latter case. Perhaps this could default to including all macros introduced since 1.0.0 in the "allowed to shadow" list. -Phil
Thoughts? -Jesse
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
You can already do this; user macros can replace built-in macros. The solutions in this thread are solving a more difficult problem than that. We are looking for ways to introduce new macros in Fennel that avoid introducing unintentional backwards-incompatibilities, which is necessarily more nuanced, because we can't know which new macro names will cause conflicts with locals in existing codebases. The wrinkle here is that when macros and locals cannot have overlapping names, you never have to worry about which wins in any given symbol resolution. If it's a macro, you know you never have to treat it as a local; this is easy. But once you remove that restriction, there is a great deal of ambiguity, because the lookup for macros vs the lookup for locals involves two distinct scope chains!I have a working implementation of this on a branch here: https://git.sr.ht/~technomancy/fennel/log/macro-shadow Unfortunately it causes a pretty serious performance regression; between 5% and 13% slower depending on the codebase, because every single call has to check many more scope lookups to try to resolve potential conflicts which were not previously possible. While this is the biggest performance regression we've seen recently, going from 1.2.0 to 1.3.0 also had some pretty bad slowdowns that went unnoticed. I haven't spent much time looking for ways to speed things up, so I wouldn't be surprised if there a number of quick wins here. But I don't feel comfortable merging this branch until we can address the slowdown and bring us back to the speeds we had in previous releases. -PhilFor example: (let [t {:b 2}] (import-macros t :test.macros) (t.b :xyz)) (import-macros t :test.macros) (let [t {:b 2}] (t.b :xyz)) These are both is currently illegal. If we do the naive thing and simply stop throwing a compile error, they will be treated the same way. (I *think* they'll both be treated as a macro, but the alternative of having them both be function calls would also be wrong.) We need a new way to walk the scope tree which tries to resolve an identifier without knowing ahead of time whether it's a macro or a local, which currently doesn't exist. -Phil
modules, to take precedence over the original macros in that module. Rud
Rudolf Adamkovič <salutis@me.com> writes:
Accidentally sent this reply off-list a while back; oops! Phil Hagelberg <phil@hagelb.org> writes:
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 -3Learn more about email & git
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.
I think you're right that we might have been too hasty with introducing `case` in a situation where it caused incompatibility with existing code that used that name for a local. I'm not against loosening the restriction of locals shadowing macros on principle. One benefit of starting with restrictive policies is that you can loosen them down the line, whereas you can't go the other way. However, I think I need some more time to think thru the potential pitfalls of this change. It isn't something we should rush into.OK, I think I remembered the rationale here. Consider pattern matching guards: (match (calculate-thing) (where (or {: x} {: y})) (blah-blah x y)) We were able to add `or` here as a keyword specifically because it was reserved; we knew there would be no ambiguity between the `or` in the guard clause vs a local potentially named `or` because the latter is impossible. Similarly we override `=` to accept a single argument and define that to mean it's pinned to the existing value. Removing this restriction makes it so we can't introduce a similar change in the future, because those symbols are no longer reserved. On the other hand, it's admittedly somewhat sketchy to reuse an existing identifier and make it mean something new in a different context. I'm not sure this was a good idea in the context of guards. So maybe it'sRudolf Adamkovič <salutis@me.com>I cannot not agree more!Rudolf Adamkovič <salutis@me.com>LOL. I meant that I not "cannot not" agree more, or in other words, I "cannot" agree more. :) Rud+1 for '&or', and as for '=', which, in the context of pattern matching, looks like a *unary* operator and is highlighted like a macro, that is BANANAS. It quacks like a hack. BTW, would '&or' allow us to use [] instead of () to fit the rest of Fennel, e.g. [&or A B]. With the reserved &-word, the []-form would not be ambiguous, would it? For a different idea, which I mentioned on IRC between lines, how about implementing '&or' in the position of the resulting expression (which would cause a fall-through). Instead of [&or A B], one then writes A &or B ... optionally formatted (for short A and B) as A &or B X Rudfine that we aren't able to do this again in the future. On the other hand, I can't think of a better way to represent an `or` guard clause that doesn't have any ambiguity! Argh. We did reserve symbols with an & in them for ... kind of this purpose. They are currently only used for rest args and &as, both in destructuring. I guess maybe we could have done (&or blah {: blah}) for guards as well. Anyway if we do loosen up the ability to shadow, maybe we can still keep `or` and `=` as reserved due to their special handling here. The important thing is that we understand the tradeoff rather than rushing in with the change. =) -PhilI'd like to hear more from others too. -Phil
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
ubiquitous.
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
}
end
@@ -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"
end
end
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)
symbol)))
@@ -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)
- nested-macro)
- macro*)))
+ (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)
+ nested-macro)
+ macro*)))
(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)
builds.sr.ht <builds@sr.ht>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: