-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Big interpreter rewrite #5406
base: main
Are you sure you want to change the base?
Big interpreter rewrite #5406
Conversation
21af1ef
to
8a3496c
Compare
4abbe16
to
29faccd
Compare
thoughts on implemented Edit: it worked out (really well!) but might as well keep this around. |
2c4303b
to
6ae7196
Compare
713821d
to
4299937
Compare
Otherwise, what's the point of all of this
4299937
to
2d17efb
Compare
…indUnsafe, and uncomment more tests (2200 passing)
(match Map.tryFind (parentContext, lambdaID) vm.lambdaInstrCache with | ||
| Some l -> l | ||
| None -> | ||
match Map.tryFind (Source, lambdaID) vm.lambdaInstrCache with | ||
| Some l -> l | ||
| None -> raiseRTE (RTE.VariableNotFound "lambda not found")) // TODO better error |
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.
Hm, why do we need this fallback case? (Source
) - When is that code hit?
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 suppose this is for the cases that we were trying to fix for -- but will the solution work if there are multiple nested lambdas? Or if a lambda is returned by a fn and used later on?
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 is hit when running functions that take a lambda as an argument (e.g. List.takeWhile, List.sortByComparator, List.sortBy, List.member, List.map, etc.)
The lambda couldn't be found in the currentFrame.context
which is a PackageFn
, instead it is found in the Source
, I wasn't sure if it was the best solution.
As for "will the solution work if there are multiple nested lambdas" it works for some functions and doesn't for others (I am encountering other errors in these cases that don't seem to be related to this issue)
vm.lambdaInstrCache | ||
|> Map.add (currentFrame.context, impl.exprId) impl | ||
|> Map.add (Source, impl.exprId) impl |
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 suppose this is a similar question - why do we need to add two entries?
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 lambda is created in the Source
context, but we try to apply it in the lambda that is inside a PackageFn
context, here is an example of one of those functions
let map (list: List<'a>) (fn: 'a -> 'b) : List<'b> =
(Stdlib.List.fold list [] (fun acc elem -> Stdlib.List.push_v0 acc (fn elem)))
|> Stdlib.List.reverse
I've added this to make sure that we can find the lambda later regardless of the context in which it's being applied. Again not sure if it is the right thing to do, please let me know if you think there is a better way to do it.
Rewriting RuntimeTypes, the Interpreter, and PT2RT to:
Here are all of the wins (WIP):
OldStringErrorTODO
is goneI might get stuck on SqlCompiler and circle back to that in another PR - not sure yet.
It'll probably be another 2 weeks until this gets merged.
This closes #5222, closes #5130, closes #5071