Skip to content
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

Implement suggestions from issue #587 in stepA of several languages #592

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

asarhaddon
Copy link
Contributor

Related with issue #587 .

  • Merge eval-ast and eval into a single conditional.

  • Print "EVAL: $ast" at the top of EVAL.

  • Expand macros during the apply phase, removing lots of duplicate
    tests, and increasing the overall consistency by allowing the macro
    to be computed instead of referenced by name (((defmacro! cond (...))) is currently illegal for example).

Each change was already applied by some implementations.

With the two last changes, macroexpand and quasiquoteexpand special
forms are not needed anymore.

Not tested locally: common-lisp, julia, io, logo, lua, matlab, vhdl,
vimscript

If macroexpand is kept:

  • an optional test should be added for (macroexpand ()), which
    currently fails on several implementations,

  • it should only apply one macro at a time. The purpose being
    debugging, hiding an iteration does not help. Moreover, the loop is
    currently untested (and probably wrong in nim).

@asarhaddon asarhaddon force-pushed the merge-eval branch 2 times, most recently from 4abbbe9 to 3dad5f7 Compare September 29, 2021 19:53
@kanaka
Copy link
Owner

kanaka commented Dec 11, 2021

It's been a few months since I've been able to do anything mal related. After mulling this change for a while, I think I've decided I now like the idea. It does simplify the flow quite a bit and in particular, I think it aligns well with mal's first class macro functions. And I agree with eval_ast not being a fundamental part of the structure. It's okay if implementations take that approach, but I agree it's an implementation detail and probably not the best structure in most cases anyway.

I think the main change I would like to request is that we keep some form of user invokable macro expansion. I think this is an important tool for learning/pedagogy (in addition to debugging). I do agree that it should only expand one level (so renaming it to "macroexpand-1" is probably good). If you can think of a clean/nice way to use your new mechanism/flow then that would be great, otherwise, I'm fine with keeping the separate macroexpand function renamed to macroexpand-1 (and without the loop).

If you're still willing to push this forward (applying the changes to step8 and 9 and fixing conflicts caused earlier merges), then I think this is nearly ready for merging.

We can create a new low-priority ticket with a list of implementations remaining to convert to the new model (if ever).

@asarhaddon
Copy link
Contributor Author

Please also comment on the changes in process/step*.txt. They are quite intrusive so I do not want them to implicitly pass under your radar.

The macro changes affect steps 8-A, but the eval-ast changes affect steps 2-A.

About macroexpand-1...

In my opinion, it is better that each EVAL outputs its AST, not only macro expansions. The log is noisy, but consistent with the experience in previous steps for new implementors.

A different question is "how should this echo be enabled"?

It is convenient to enable the echo interactively without restarting the interpreter. This is what macroexpand provides, though not for all evaluations. A global state is also possible: (set echo-eval true).
Some implementations already perform macroexpansion inside EVAL.

  • ada.2 implements macroexpand with a mutable boolean in the EVAL loop.
  • haskell and prolog implement macroexpand as a separate function duplicating EVAL.
    The first solution ensures consistency between macroexpand and actual macro expansion, but would require quite intrusive changes in language with immutable data.
    This added complexity in EVAL, especially with language-specific details, is not worth the while, especially if the motivation is educational.

A good compromise would allow one to enable the echo without editing the source code. Since command line arguments are already mandatory, we could for example let step6 replace the print command (optional and commented in previous steps) with a conditional block triggered by an initial --echo-eval single argument.

Most languages already embed a print command in comments, but this is not convenient, especially with compiled languages. Also, it has already happened that the command is obsolete when one actually requires it.

@kanaka
Copy link
Owner

kanaka commented Dec 13, 2021

I reviewed the process/step*.txt changes and they looked fine at first pass. I think my only minor hesitation is that the line lengths have jumped so things don't look as great in some editor configurations (the previous pseudo-code didn't exceed 80 characters). I'll do more thorough review and maybe even work through a few steps based on the update guide and pseudo-code to make sure things are consistent and easy to follow (I haven't done that in a while so it would be a good refresher).

One thing I realized this weekend is that the diagrams will need to be updated because they refer to eval-ast. I might take the opportunity myself to port the diagrams from gliffy to drawio so they are easier to update with this and future changes.

Configurable debug is a great idea. I had an idea for how to do it:

diff --git a/impls/python/step3_env.py b/impls/python/step3_env.py
index 75ec834e..2e0bfe4d 100644
--- a/impls/python/step3_env.py
+++ b/impls/python/step3_env.py
@@ -10,7 +10,9 @@ def READ(str):
 
 # eval
 def EVAL(ast, env):
-    #print("EVAL %s" % printer._pr_str(ast))
+    edbg = env.get(types._symbol('*DEBUG-EVAL*'))
+    if edbg != None and edbg != False:
+        print("EVAL %s" % printer._pr_str(ast))
 
     if types._symbol_Q(ast):
         return env.get(ast)
@@ -50,6 +52,7 @@ repl_env = Env()
 def REP(str):
     return PRINT(EVAL(READ(str), repl_env))
 
+repl_env.set(types._symbol('*DEBUG-EVAL*'), False)
 repl_env.set(types._symbol('+'), lambda a,b: a+b)
 repl_env.set(types._symbol('-'), lambda a,b: a-b)
 repl_env.set(types._symbol('*'), lambda a,b: a*b)

This enables per lexical scope debug but also global debug if desired:

user> (let* [*DEBUG-EVAL* true] (+ 2 (+ 3 4)))
EVAL (+ 2 (+ 3 4))
EVAL +
...
user> (def! *DEBUG-EVAL* true)
true
user> (+ 2 (+ 3 4))
EVAL (+ 2 (+ 3 4))
EVAL +
...

Obviously the same idea would be propagated forwarded from step3 and described in the process as an optional but useful thing to implement at step3 (with an optional test case to make sure it's doing the right thing). And step6 could support a command line parameter for enabling the global setting by default (again, optional).

One thing I really like about it is that it provides a template for more narrower debug (i.e. implementors could create other *DEBUG-FOO* style environment variables to trigger debug prints or other behavior deeper within other contexts).

While I like the dynamically enabled debug flag idea and think it's definitely worthwhile, I'm not completely convinced though that this fully replaces macroexpand/macroexpand-1. The problem with the eval debug prints is that it happens before evaluation and you don't see the actual results of the eval (you can sometimes infer them based on subsequent things that are printed out) but I've found being able to macroexpand important for debugging in some implementations that I've done. We could have macroexpand be an optional feature in the guide and tests (and suggested as a helpful way to debug quasiquote and macro issues).

@asarhaddon
Copy link
Contributor Author

Hello.
I have reduced the width of the lines in the process .txt files.
The symbol in MAL environments is a clever solution. I have implemented it in some languages and adapted the process.
I was previously suggesting a command line option, but (def! DEBUG-EVAL true) is both less difficult to implement and more flexible thanks to let*.
I fail to understand what macroexpand would print that DEBUG-EVAL would not. The result of macro expansion is sent to EVAL (or the TCO loop is restarted), so it will be displayed before anything else happens. Unless I am missing something, DEBUG-EVAL prints strictly more information that {macro,quasiquote}expand. Could you provide an example ?
There is little hope to ever bring these changes to low-level languages like nasm and wasm. For nasm, I have managed to adapt stepA, but the steps differ much from each other and backporting is not a grateful job. This is also true for higher level languages, though less annoying. This is a real obstruction, artificially multiplying the cost of maintenance. Why not suggest to new submitters to minimize the diff between steps, from stepA down to step0 ? Another option is to completely remove steps0-9 as soon as the initial porter agrees, or becomes unreachable.

@asarhaddon asarhaddon force-pushed the merge-eval branch 2 times, most recently from e8245a8 to a1692c1 Compare December 26, 2021 20:26
@asarhaddon
Copy link
Contributor Author

asarhaddon commented Dec 27, 2021

Hello.
A few issues remain before I squash the changes into a single commit.

c.2: the failure on github is unreproducible with gcc 11.12.0.
objpascal: unreproducible with fpc 3.2.2.
ocaml: unreproducible with 4.11.1.
perl6: unreproducible with rakudo 2021.09.
The solution is probably to update the Docker image. I have documented my configuration in #588.

elm: the failure is most probably unrelated because the directory is unchanged.

Some tests were failing with Make 4.3.
See issue kanaka#587.
* Merge eval-ast and eval into a single conditional.
* Expand macros during the apply phase, removing lots of duplicate
  tests, and increasing the overall consistency by allowing the macro
  to be computed instead of referenced by name (`((defmacro! cond
  (...)))` is currently illegal for example).
* Print "EVAL: $ast" at the top of EVAL if DEBUG-EVAL exists in the
  MAL environment.
* Remove macroexpand and quasiquoteexpand special forms.
* Use pattern-matching style in process/step*.txt.

Unresolved issues:
c.2: unable to reproduce with gcc 11.12.0.
elm: the directory is unchanged.
groovy: sometimes fail, but not on each rebuild.
nasm: fails some new soft tests, but the issue is unreproducible when
  running the interpreter manually.
objpascal: unreproducible with fpc 3.2.2.
ocaml: unreproducible with 4.11.1.
perl6: unreproducible with rakudo 2021.09.

Unrelated changes:
Reduce diff betweens steps.
Prevent defmacro! from mutating functions: c forth logo miniMAL vb.
dart: fix recent errors and warnings
ocaml: remove metadata from symbols.

Improve the logo implementation.
Encapsulate all representation in types.lg and env.lg, unwrap numbers.
Replace some manual iterations with logo control structures.
Reduce the diff between steps.
Use native iteration in env_get and env_map
Rewrite the reader with less temporary strings.
Reduce the number of temporary lists (for example, reverse iteration
with butlast requires O(n^2) allocations).
It seems possible to remove a few exceptions: GC settings
(Dockerfile), NO_SELF_HOSTING (IMPLS.yml) and step5_EXCLUDES
(Makefile.impls) .
This is required by the current mal implementation of DBG-EVAL.
asarhaddon added a commit to asarhaddon/mal that referenced this pull request Apr 20, 2023
@asarhaddon
Copy link
Contributor Author

Hello.
It would be nice to at least update the documentation, so that new implementors spend less time on macros.
The purescript implementation is a good example, and has been created while this request was pending.

@asarhaddon asarhaddon mentioned this pull request Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants