Make micro-tests pass with lang.lisp (:impl).
Closed this issue · 14 comments
From #13 (comment)
:impl tests (lang.lisp) are disabled because of a (previously-known see Unbound variable in micro-tests (impl repl) #10) bug. This should be fixed, then the tests enabled.
I don't have a fix, but I find extend-rec-aux and extend-rec-aux2 fishy because they drop the rest of the env in case of recursive env. WDYT?
https://github.com/lurk-lang/lurk/blob/master/impl/lang.lisp#L390
Yes, that does look fishy. I'm a little crunched right now, but I think the right thing to do is look at eval.rs which doesn't have this problem. Assuming that the implementations differ, we could try porting it back to lang.lisp to see if it solves the problem.
It would be great if we can fix this particular issue without having to do the full reconciliation that may lead to backporting eval.rs to lang.lisp completely. I do think that may be the right step, but I don't want it to block ongoing work which will be facilitated when the tests agree for all implementations.
The code in Rust seems to also drop the rest of the env:
https://github.com/lurk-lang/lurk-rs/blob/master/src/eval.rs#L944
Separately, trying to make this change:
;; It's a binding, so we are extending a recursive env.
- (hlist (cons (cons var val) binding-or-env)))))))
+ (cons (cons (cons var val) binding-or-env) rest))))))
results in an error:
ERROR: The value
NIL
is not of type
LURK.LANG::HLIST
when binding LURK.LANG::ENV
If you can figure out where that's coming from (maybe with debugger), the issue is that some expression is returning NIL instead of (nill), I think.
I was able to apply that change successfully — both here and in lurk-rs. I believe it is a correct change, but it doesn't fix the underlying problem.
Yeah, though with this change, the meta-tests fail I think?
I also played with some scoping tests, like
(letrec*
((f (lambda (x) (+ x 1)))
(g (lambda (x) (+ (f x) 2))))
(let* ((m 1))
(letrec*
((a (lambda () 1))
(b (lambda () 2)))
(let* ((n 2))
(letrec*
((h (lambda (x) (+ (a) (g x))))
(i (lambda () (f (h (b))))))
(current-env))))))
and
(letrec*
((f (lambda (x) (+ x 1)))
(f2 (lambda (x) (let* ((a 1)) (letrec* ((g (lambda (x) (+ a (f x))))) (g (f x)))))))
(f2 1))
But these work fine, which indicates that I don't quite understand why it's OK to drop the rest of the environment when dealing with a recursive env.
Yeah, though with this change, the meta-tests fail I think?
I see, but interestingly not in lurk-rs.
I think it's not fine to drop it. Your tests just may not exercise the problem. I'll try to create a test case that does. If so, let's push this fix through so we can focus on the other problem. It still might end up making most sense just to bite the bullet and make lang.lisp be a very straight backport of eval.rs next. It might not be worth spending a lot of time debugging this only to change it. I'd be okay with accepting that they are out of sync and lang.lisp needs to be excluded from the .lurk tests for now in that case.
I was just wondering if there were extra properties I am not aware of that makes those tests work despite the drop.
In general, I am not getting much out of lang.lisp. Is there a reason why we need it now, instead of just maintaining the api.lisp and the rust implementation? I find it daunting to think of new features and having to update three implementations.
It's weird to me too. I think it should be possible to isolate cases that make it fail, though as above maybe it's not necessary or worth fixing. We could deprecate lang.lisp for now — though I would then still want to find and add the relevant test to eval.rs.
This points somewhat to the value of lang.lisp longer term. Although it feels like dead weight now, especially as it has drifted, I don't think it would have been possible to arrive at eval.rs without having had it. I'm concerned that it will be very difficult to make some kinds of changes in eval.rs without being able to first work out the details in a more tractable environment. That lang.lisp currently doesn't feel like that environment is a symptom of it having been neglected.
I'm not 100% sure this is right, which is why I'm open to leaving it as-is until such time as it can be more directly aligned. It might be that along the way I realize it has served its purpose and we don't need it anymore.
That said, when it is working it will be much easier to do performance evaluation/tuning and anything that requires seeing how the evaluation will be broken into execution frames if that can be done in Lisp. I think that may be important later when we focus on optimization and reducing the iteration count.
I found a test case which fails on the old code (discarding REST) and succeeds with the fix. It behaves this way in both repos, so I will push the test and fix to lurk-rs also.
For future reference in context of this discussion, here is the example:
(let* ((z 9))
(letrec* ((a 1)
(b 2)
(l (lambda (x) (+ z x))))
(l 9)))
This fails with unbound variable Z, when the old environment is discarded.
Cool :)
In general, I am not getting much out of lang.lisp. Is there a reason why we need it now, instead of just maintaining the api.lisp and the rust implementation? I find it daunting to think of new features and having to update three implementations.
I'm almost convinced. I started in on what it might take to get this synchronized, and I'm not sure it's worth it.
I'm still concerned about having the Rust code be the sole source of truth for the continuation-based behavior. However, I see that the original purpose of lang.lisp may have been fulfilled. It might be better to spin off something which stays close to api.lisp but also implements the continuation evaluation model (so we can get iteration counts while developing in Lisp and also so we can check these between the two implementations to ensure execution is as expected).
@namin Let me know if you're interested in taking a shot at this. It might be a useful way to make sure you have a handle on what's going on. And it might shake out any remaining bugs in what eval.rs actually does.
I'm creating repl tests for the ones in lang.lisp in preparation for deprecating it.
lang.lisp has been deprecated.