[refactor]: add resolvers; refactor evaluator #1385
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adds a new abstraction, the *Resolver* which controls the lookup order when resolving symbols. Env, provides mechanisms for storing and retrieving bindings, Context bundles environments together (and provides some retrieval utilities) and Resolver determines the order in which multiple lookups should be executed. The Resolver abstractions allows us to remove much of the gnarly lookup code that was previously directly defined in the evaluator. The abstraction also provides a Semigroup instance for the flexible combination of resolvers, making it significantly easier to try out different orders in the evaluator. This commit changes no behavior -- it just uses Resolvers to emulate the previous lookup order in the evaluator. Future commits will leverage the abstraction to simplify lookups and refine behaviors where needed.
This refactor moves all the form-specific evaluation functions that were previously defined as local functions in the main eval loop into top-level functions in Eval.hs. This should make it easier to tweak and swap out these functions in the main eval loop. As a result of the move, the functions need to take additional parameters to carry state across the evaluator loops, namely, we need to pass: - a resolution mode - a resolver - a root xobj - a context While this may seem tedious, it gives us flexibility. Evaluators that don't need the global eval loop's resolver can refrain from using it, and it's easier to tell what's being passed since we're not referencing values captured much further up in a function's body. Additionally, I've moved out local fold function definitions "successiveEval" and tried to give them more expressive names. I also used shorter definitions (mostly dropping explicit case statements) where possible. I'm hoping this change will also make it easier to try and pinpoint opportunities to improve evaluator performance on large inputs. This commit contains no behavioral differences.
This commit finalizes the migration of local definitions in the main eval loop into top level functions. In addition: - I removed privacy checking. We do this in Expand.hs and it currently covers our test cases. - I consolidated some static form checks. Note that the evaluator is particularly sensitive about return values--I was stuck for a while because I inadvertently returned the head of a list from a static check instead of the list being evaluated itself -- this totally breaks the evaluator. Because of this, I set the return value of this function to Error||Unit. - I've also combined the array evaluation functions. - Finally, I've removed the Fn check from isResolvableObj. After checkStatic consolidation, this introduces a problematic case, and removing it doesn't seem to break anything, so it seems extraneous.
Remove unused functions and data definitions; add some comments.
|
Oh, also, "GlobalDynamic -> DynamicFull -> LocalFull -> GlobalFull -> CurrentModule -> Type -> UseModule ..." etc. so, doing |
hellerve
reviewed
Feb 17, 2022
| isResolvableStaticObj (Instantiate _) = True | ||
| isResolvableStaticObj (Deftemplate _) = True | ||
| isResolvableStaticObj (External _) = True | ||
| isResolvableStaticObj (Match _) = True |
There was a problem hiding this comment.
There's some context in commit msg 5cc5752 -- basically there used to be multiple approaches to checking static calls, two different functions in eval and one direct use of isResolvableObj. I merged the two different definitions of checkStatic by making them rely on isResolvableObj--however, this caused failures in macros when Fns were encountered since they were checked by isResolvable. Removing that check doesn't seem to break anything, so I dropped it.
There was a problem hiding this comment.
Also, the addition of Match is similar -- merged to bring parity with the two old separate definitions.
Co-authored-by: Veit Heller <[email protected]>
Co-authored-by: Veit Heller <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR does two things:
localShadowResolver <> globalDynamicResolverwill return a resolver that first attempts to find shadowed symbols in the local environment and then attempts to find globally defined dynamic symbols. The comments inResolver.hsexplain this in a bit more detail.I'm hoping (2.) makes it a bit easier to start debugging potential performance hotspots in the evaluation loop (see #1215)