Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Builtins] Drop 'RuntimeScheme' #4778
[Builtins] Drop 'RuntimeScheme' #4778
Changes from 13 commits
2ea0b5c
86a3e24
f5639db
c736ebf
4c001d3
39784e3
eb3467d
9f6211f
2475d51
fbc32ec
dc35696
e16a7d0
f8420c0
404df06
150c935
0975598
b0c515f
5866296
5ada64f
369df92
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
I've never really been sure why this is called
BuiltinRuntime
...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.
'Cause it's the builtin-related stuff that is used at runtime. Name suggestions are always very welcome!
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.
This makes me a bit nervous. Can we not ensure this isn't needed at compile-time? I feel like a bug in the evaluator could make us hit this...
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.
The evaluator doesn't try to get the
ExMemory
of aTerm
as it operates onCekValue
s, for which we have a proper instance.No, because it is needed at compile time. The instance is not ignored, it gets picked up and used, it's the thunk that
memoryUsage
creates gets thrown away, because we don't have a builtin whose costing function would actually use theExMemory
of an argument whose type is a type variable. We could have such a builtin, it was one of the options for the built-inshow
for example.I guess my docs are confusing? Any suggestions?
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.
Why can't we just... not call it if we don't need it?
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.
@michaelpj I don't understand your question. Why can't we express in the types that all polymorphic builtins are sufficiently lazy thus forbidding the possibility of adding builtins that are not lazy, which we may need (it was one of the options for the
show
builtin)? Like how would you even do the first part?Plus,
TypeScheme
works over aterm
, meaning getting the type of a builtin requires providing someterm
type suitable for evaluation even if you don't want to evaluate anything. I had some progress towards fixing that, but you were explicit about builtins not being a priority, so I closed the PR and stopped worrying about polishing builtins.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.
I'm operating too far from the details here so sorry if these are silly questions. I indeed would like it if we could somehow make it explicit that this doesn't get called. But it sounds like we can't do that without doing something fancy with the types about laziness, so I guess we have to live with it.
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.
It's not a big deal if it does get called in the end. It's an instance for
Term
, it's not on a critical path anywhere. We haveerror
there just to signal and test that the logic of the builtins is as expected. On the critical path we have a sensible instance forCekValue
and that one never throws.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.
Right, so the thing that I worry about is being sure that it's not on the critical path. I believe you, it just makes me nervous in case we screw up later...
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.
That has to be a major screwup for us to depend on the
ExMemoryUsage
instance of typed terms on the evaluation path. There's noExMemoryUsage
instance for untyped terms:And thanks to Ziyang we have tests checking that no builtin can throw, which does include costing (because everything on the evaluation path is so strict that there's no way we could not include it).
So much needs to go horribly wrong for us to somehow end up seeing that error triggered on
master
or a release branch.On the other hand unwillingly defining a weird builtin and then supporting it forever is a very real possibility, so I opted for catching that rather than being worried about things causing synergistically weird behavior (particularly since those things are most likely bugs themselves).
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.
There we go, that's what I was missing lol 🤦 I thought this was untyped terms.