kanaka / mal

mal - Make a Lisp
Other
10.1k stars 2.57k forks source link

Jq: update interpreter, merge eval-ast, fix some env errors #685

Closed asarhaddon closed 1 month ago

asarhaddon commented 1 month ago

Hello. This fixes #657 for jq, as well as a few regressions (even step0 fails with the current interpreter version). I have also done some source cleaning, but env.jq and steps 3 to 6 stil share too much similar and partially broken code. I have failed to figure how the stuff is supposed to work. I suggest to first ask help by the original author in removing the code duplications and documenting the design choices, and only then fix REGRESS=1 and/or self hosting (#662), if stil necessary.

kanaka commented 1 month ago

Hi @alimpfard, I know it's been a few years since you did the mal implementation in jq. We are working on refactoring all the implementations with some important simplifications to the process/guide (and often taking the opportunity to upgrade to newer versions of the target language). Any chance you would be willing to refresh your memory on this code base to help us with those changes? @asarhaddon has made a first pass in this PR do do that refactoring and gotten the basic tests to work but there is a failure during regression testing (running tests for earlier steps using stepA). Neither of us are jq experts and could use some assistance if you're willing. I think mostly it would be helping to understand and/or cleanup some of the environment related code in step3 and step6.

alimpfard commented 1 month ago

Sure! I'll take a look later today.

alimpfard commented 1 month ago

I don't quite understand why it's okay for QUASIQUOTE to mention qq-foldr without a definition but not the other way around, maybe something causes eager evaluation that is suppressed by macros, I'll take a look at the jq impl in a bit.

@asarhaddon @kanaka what cleanups are we talking about exactly? I can help out with refactors/cleanups but I'm not familiar with whatever has changed in MAL since I last looked at it 😅

asarhaddon commented 1 month ago

Hello. I have applied your patches, this merge request fixes all regression tests.

Maintenance would be easyer if the obsolete code was removed and the diff between files reduced. The blocking point for me is that step5 differs a lot from step4 (which is normal) but also from step6 (this causes duplicate efforts). It would be nice if you could reduce the diffs between steps and/or document why duplicated code remains in step5 so that we could clean it. Some examples: The difference between env_set and envset is not easy to guess. The diff between step6 and step5 makes me wonder if the evaluation of the "if" condition should use $_menv (as in current step6) instead of $env (as in current step5. Same question for evaluation of hashmaps (env in step5, $_menv in step6).

alimpfard commented 1 month ago

I believe most of the env differences between step 5 & 6 are due to the existence of atoms, and how they're supposed to interact with different scoping rules (env vs. $_menv is almost entirely about this), e.g. let* wouldn't "leak" atoms if we passed the old env through.

I imagine most of the languages that lack global state will have this difference between step 5 & 6?

kanaka commented 1 month ago

Nice work. That was a big lift!

@alimpfard do you think you might be able to add some documentation about the step 4, 5, 6 differences from the typical language pattern? I have an intuitive sense about why that is the case (due to jq's strict immutable nature), but some concrete explanation from the person who wrote it would certainly be appreciated.

alimpfard commented 1 month ago

I'll see if I can draft something up (might take a couple days though, a little busy IRL)