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

feat: optionally do not evaluate under binders (except lets) #1055

Merged
merged 4 commits into from
Sep 18, 2023

Conversation

brprice
Copy link
Contributor

@brprice brprice commented Jun 5, 2023

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 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.

@brprice
Copy link
Contributor Author

brprice commented Jun 5, 2023

Status: Ready for review, but

  • This will need some integration work with feat: push down lets #736 . In particular, we will not need to treat lets specially (currently we eval under them), but will want to push them through binders; and we will want to not eval under ∀ (currently we don't bother to do any "not eval under binders" at the type level, since the only computation is inlining lets, which we need to do under binders anyway (else they could block e.g. beta). Commit messages (and comments?) will also need updating
  • Rebase away a few TODO/REVIEW comments

@brprice brprice force-pushed the brprice/eval-closed branch 2 times, most recently from 5c45766 to 6be7b00 Compare July 20, 2023 17:50
@brprice brprice changed the title WIP: do not evaluate under binders (except lets) feat: optionally do not evaluate under binders (except lets) Jul 20, 2023
primer-api/src/Primer/API.hs Outdated Show resolved Hide resolved
@brprice brprice force-pushed the brprice/eval-closed branch 3 times, most recently from 476ba69 to 7e29c15 Compare July 25, 2023 14:46
@brprice brprice mentioned this pull request Aug 9, 2023
@dhess
Copy link
Member

dhess commented Aug 30, 2023

Once #736 is merged, I would like to get this merged, as well. This will become even more important once we implement #1119.

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]>
@brprice brprice marked this pull request as ready for review September 14, 2023 14:11
@brprice
Copy link
Contributor Author

brprice commented Sep 14, 2023

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)

@dhess
Copy link
Member

dhess commented Sep 14, 2023

@brprice Can you mark this as ready for review so that it shows up in the UI?

Copy link
Contributor

@georgefst georgefst left a 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.

@dhess
Copy link
Member

dhess commented Sep 18, 2023

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?

@brprice brprice added this pull request to the merge queue Sep 18, 2023
Merged via the queue into main with commit 1ab647e Sep 18, 2023
69 of 70 checks passed
@brprice brprice deleted the brprice/eval-closed branch September 18, 2023 17:06
@georgefst
Copy link
Contributor

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.

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.

3 participants