-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Quasiquote within quasiquote appears to be broken #103
Comments
@gilch I'm not too surprised that nested quasiquote is broken. For the most part, mal syntax/features are based on Clojure, so this site is probably a good reference: https://blog.8thlight.com/colin-jones/2012/05/22/quoting-without-confusion.html From that page, here is a simpler issue with nested quasiquote
If you want to take a stab at figuring out the issue with the current quasiquote algorithm in one implementation, I could probably port it to most of the other implementations. However, I won't have time to actually figure out the issue any time in the next few months. |
I modified the Ruby impl to include quasiquoting nesting level. Here's the modified def quasiquote(ast, nesting = 0)
if not pair?(ast)
return List.new [:quote, ast]
elsif ast[0] == :quasiquote
return quasiquote(ast[1], nesting + 1)
elsif ast[0] == :unquote
if nesting == 0
return ast[1]
else
return quasiquote(ast[1], nesting - 1)
end
elsif pair?(ast[0]) && ast[0][0] == :"splice-unquote"
expanded_list = nesting == 0 ? ast[0][1] : quasiquote(ast[0][1], nesting - 1)
return List.new [:concat, expanded_list, quasiquote(ast.drop(1), nesting)]
else
return List.new [:cons, quasiquote(ast[0], nesting), quasiquote(ast.drop(1), nesting)]
end
end Here it is in action:
I'm not sure this is the algorithm you had in mind, and maybe I missed some cases. |
It's doesn't quite have the same behavior as some of the examples at the 8thlight link, for example: `{:a 1 :b '~(+ 1 2)} However, quasiquote has never been quite the same as Clojure so these might be reasonable differences. However, the change does seem to be an overall improvement. Let's use the process we are using for native equality and add soft tests for these, and then begin checking off the list of impls. Here is the checklist:
|
Also, it would be good to update process/guide.md and process/step{7,8,9,A}* with the changes. |
@kanaka, I've updated the Swift implementation along the lines of the Ruby change @gilch made, and it passes all current tests (native and self-hosted) and the new examples shown above under "Here it is in action". Please let me know if you'd like a PR, or if you'd like more time to be sure that that's the approach you want to take. |
@kanaka I think that before rushing to fix all impls, we should come up with all the test cases (and expected results) we want to cover, and decide if the algorithm presented in the Ruby code example is the correct one. Also cover a few error cases ( |
@dubek yes, you're right. Spending a bit more time is prudent. I think there are probably multiple "right" paths in some sense. I do want to avoid making quasiquote significantly more complicated. If we can make it behave basically like Clojure (apart from the namespace expansion) without adding too much more complexity, that would definitely be the preferred path. I would probably lean towards the current simplistic non-nested implementation rather than tripling the size of the quasiquote implementation, but if we can get near parity with less than that and it's still easy to explain in the guide, then that would be ideal. That reminds me. We also might want to look at what it would take to implement gensym support while we are looking at this. That's probably more practically useful than nested quasiquoting (which is pretty rare IMO). There likely will be some interaction between gensym and nested quoting. |
I'm not sure if Clojure's behavior is the best target then. Besides the mentioned namespace expansion of symbols, Clojure can also syntax-quote vectors, maps, and sets, which MAL doesn't have either. Once you take all that away, the behavior is probably more like Scheme quasiquote or Common Lisp backquote. There are implementations of these lisps with licenses even more permissive than Clojure's EPL (CMUCL, for example, is public domain.), such that we could possibly "borrow" the implementation outright, or at least adapt their test cases. |
I found a fairly simple-looking Scheme quasiquote implementation (in Scheme) in Alan Bawden's "Quasiquotation in Lisp", which the author asserts is correct. It shouldn't be too difficult to port it to the Racket mal implementation (or mal itself for that matter). We would still need test cases, of course, but the examples from the paper might be a good start. |
Chicken Scheme's quasiquote test cases. |
Hi.
I would even suggest to stop supporting |
@asarhaddon we should definitely continue to support unquote directly under quasiquote. The tree being quasiquoted might be generated or transformed by other code (not just handwritten) in which case you wouldn't want to arbitrarily exclude unquote right under quasiquote. splice-unquote by definition must happen within a sequence but unquote can be top-level. I agree that |
Test on: Ada Ada.2 AWK Bash BBC-BASIC C C++ Guile Haskell java MAL Objective-C Objective-Pascal OCaml Perl PostScript Python VHDL
Pull request #401 hopefully improves the situation. It does not allow nesting. |
New soft tests, quasiquote algorithm described at #103 on some impls.
I hate to say this but it is not at all clear what nested quasi quotes are supposed to do. I propose to forbid the use of nesting until such time that there is a specification, and there is an agreement on this. |
As another test, using defmacro, but without quasiquote:
This just causes the system to reboot. Hrmmm... |
Here's a simplified example in Common Lisp of what I'm trying to do.
The equivalent in mal:
The first case gets the wrong value at the wrong time. And the second case just fails altogether. Nested quasiquotes are not unusual in macros, but I can't seem to make them work in mal. Is there a different syntax for this I'm not seeing?
I've tested this in the python implementation, but since the code seems to follow the guide, I have to assume it's broken in the other implementations too.
The text was updated successfully, but these errors were encountered: