-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: optionally do not evaluate under binders (except lets) #1055
Conversation
d0e2486
to
e0e54d2
Compare
Status: Ready for review, but
|
5c45766
to
6be7b00
Compare
476ba69
to
7e29c15
Compare
This is a stepping-stone to making an option to not evaluate under binders for EvalFull. BREAKING CHANGE: This adds a new field to `EvalFullReq`, which is exposed in the APIs. Since it is an extra optional argument in the OpenAPI, old clients of the OpenAPI will still continue to work (this is not true of the richly-typed API). Signed-off-by: Ben Price <[email protected]>
This change means our EvalFull is "closed evaluation", rather than "open evaluation". The main reason to do this is so that evaluations of programs-in-construction (especially recursive ones) do not pointlessly blow up in size: if `even` and `odd` are defined recursively, then evaluating `even` would evaluate under the lambda and inside case branches, expanding `odd` and recursing; when it eventually times out one would have a big tree with many unrolled copies of `even` and `odd`, which is not very illuminating. Note that the reduction of "pushing a let down", e.g. `let x = t in f s ~> (let x = t in f) (let x = t in s)` happens at the `let` node, and thus is not "under" the `let x` binder. Note that we consider a "stack" of lets `let x=s in let y=t in r` to be one binder, so that we can (with `groupedLets = False`) reduce this by pushing the `let y` into the `r` (i.e. we don't count this as "under" the `let x` binder). We will not evaluate inside the `r`. Note that we do not evaluate in the RHS of pattern match branches which bind variables, for the same reason as we do not evaluate under lambdas; for consistency we also do not evaluate in the RHS of branches which do not bind variables. Signed-off-by: Ben Price <[email protected]>
Signed-off-by: Ben Price <[email protected]>
Signed-off-by: Ben Price <[email protected]>
b93a4f5
to
8a6a846
Compare
This has been rebased on top of the push-down-lets work, and is ready for a review. Reviewer note: please give extra scrutiny to comments and code that may have not been updated for push-down-lets (this branch originated before #736) |
@brprice Can you mark this as ready for review so that it shows up in the UI? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, I'm still not fully convinced that this is the right long-term solution to the UX issue of showing unproductive eval steps for unapplied functions etc. But since we're making this optional that doesn't matter.
Any better ideas? |
I'm sure we discussed this at length at some point but I can't find any notes. But I remember a suggestion that we stop with an "are you sure you want to continue?"-style message when the size of the eval tree reaches a certain multiple of the original. Anyway, I don't feel too strongly about it and think further explorations should at least wait for #1119. |
This change means our EvalFull is "closed evaluation", rather than "open evaluation". The main reason to do this is so that evaluations of programs-in-construction (especially recursive ones) do not pointlessly blow up in size: if
even
andodd
are defined recursively, then evaluatingeven
would evaluate under the lambda and inside case branches, expandingodd
and recursing; when it eventually times out one would have a big tree with many unrolled copies ofeven
andodd
, which is not very illuminating.Note that we do not evaluate in the RHS of pattern match branches which bind variables, for the same reason as we do not evaluate under lambdas; for consistency we also do not evaluate in the RHS of branches which do not bind variables.