A CEK Evaluator #223
A CEK Evaluator #223
Conversation
doyougnu
force-pushed
the
doyougnu/wip/useful-repl
branch
2 times, most recently
from
fa83318
to
ea1aea6
Compare
|
I'm experimenting with an AI-based code review tool called "CodeRabbit", please let me know if its comments are annoying and I'll turn it off. |
|
Warning Rate limit exceeded@doyougnu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 50 minutes and 39 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent updates enhance the evaluation process by introducing a new Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant REPL
participant Evaluator
participant Expander
User->>REPL: Enter expression
REPL->>Evaluator: evaluateIn(expression)
Evaluator->>Expander: evalInCurrentPhase(expression)
Expander-->>Evaluator: Expanded expression
Evaluator-->>REPL: Evaluated result
REPL-->>User: Display result
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
doyougnu
force-pushed
the
doyougnu/wip/useful-repl
branch
from
056bf16
to
aa970da
Compare
|
Should be ready for review now. Its a bit slower than what we had. Why is that? Well: Because it destroys the branch predictor in my laptop! {59.7% bad speculation) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (5)
- repl/Main.hs (2 hunks)
- src/Evaluator.hs (4 hunks)
- src/Expander.hs (13 hunks)
- src/Expander/Monad.hs (2 hunks)
- src/Pretty.hs (1 hunks)
Additional comments: 18
repl/Main.hs (1)
- 149-151: The replacement of
runExceptTandrunReaderTwithevaluateIndirectly affects the error handling mechanism. This change simplifies the control flow by directly invokingevaluateInfor evaluation, which could potentially make the error handling more specific and tailored to the needs of the REPL environment. However, ensure that this new approach does not overlook any errors that were previously caught by the monadic computations. It's also important to verify that the performance impact, if any, is within acceptable limits given the concerns raised about the new evaluator's efficiency.src/Pretty.hs (1)
- 627-627: Adding the case
pp env (EvalErrorIdent v)to handle errors related to binding identifiers to non-values is a valuable enhancement to the error reporting mechanism. This addition ensures that such errors are reported in a user-friendly manner, aiding in debugging and error resolution. It's crucial to ensure that this new error case is adequately tested to confirm that it correctly identifies and reports the intended error scenarios.src/Evaluator.hs (3)
- 87-90: The introduction of the
EvalResultdata type with constructors likeExampleResultandIOResultis a significant change. It's important to ensure that all parts of the codebase that interact with evaluation results are updated to handle these new cases appropriately.- 111-168: The
Kontdata type is central to the CEK interpreter's operation, representing various states of the continuation. The detailed constructors likeInArg,InFun,InLetDef, etc., are well-documented. However, it's crucial to ensure that all pattern matches againstKontare exhaustive to prevent runtime errors.- 450-666: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [441-666]
The helper functions and the top-level API (
evaluate,evaluateIn,evaluateWithExtendedEnv,projectError,erroneousValue) are crucial for the evaluator's usability and integration with the rest of the codebase. It's important to ensure that these functions are correctly implemented and that their interfaces are clear and consistent with the rest of the project.Verification successful
The usage of the top-level API functions (
evaluate,evaluateIn,evaluateWithExtendedEnv,projectError,erroneousValue) across various parts of the project, including testing, the REPL, expansion, and within the evaluator itself, confirms their correct implementation and integration. Their application in different contexts aligns with their intended purposes, indicating that these functions are crucial for the evaluator's usability and integration with the rest of the codebase. Based on this analysis, the review comment is verified as accurate.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of top-level API functions to ensure they are correctly integrated. rg "evaluate|evaluateIn|evaluateWithExtendedEnv|projectError|erroneousValue" --type hsLength of output: 3679
src/Expander/Monad.hs (2)
- 9-9: The addition of the
FlexibleContextslanguage extension is noted. This extension allows for more flexible context specifications in type signatures, which can be necessary for certain function signatures or typeclass instances involving complex type constraints. Given the changes in theexpandEvalfunction, this addition seems justified.- 864-871: The signature of
expandEvalhas been changed fromEval a -> Expand atoCore -> Expand Value, indicating a shift in how evaluation is performed. This change aligns with the PR's objective to introduce a new CEK evaluator. The function now directly takes aCoreexpression and evaluates it within the current environment, handling errors by throwing aMacroEvaluationErrorwith the current phase and the projected error. This approach simplifies the evaluation process by directly working with core expressions and leveraging the existing environment, which is a positive change in terms of maintainability and clarity. However, it's crucial to ensure that all calls toexpandEvalthroughout the codebase have been updated to match this new signature to avoid type mismatches.src/Expander.hs (11)
- 86-86: The import statements for
Util.SetandUtil.StoreasSetandSrespectively have been added. Ensure that these modules are used appropriately in the file and that their functions are called correctly to avoid any runtime errors or unexpected behavior.- 275-275: The call to
expandEvalwithin theevalDeclfunction for theDefinecase indicates a significant change in how expressions are evaluated during the declaration expansion process. Ensure thatexpandEvalis correctly handling the evaluation of expressions and that the error handling mechanisms are robust enough to catch and report errors effectively.- 302-302: Another instance of
expandEvalbeing used, this time within theExamplecase ofevalDecl. It's crucial to verify that the evaluation context and environment are correctly set up before callingexpandEvalto ensure that the expression evaluation proceeds as expected.- 305-305: The use of
expandEvalin theRuncase ofevalDeclhighlights the importance of ensuring that the evaluated expression is of the expected type, especially when dealing with IO actions. Proper type checking and error handling are essential to prevent runtime type errors.- 316-316: In the
DefineMacroscase, the call toexpandEvalsignifies a critical point where macro definitions are evaluated. It's important to ensure that the macro evaluation environment is correctly set up and that macros are expanded and applied correctly to avoid macro expansion errors.- 556-556: The handling of the
bind-IOmacro action involves a complex interaction between IO actions and function application. Ensure that the environment and closure application are correctly managed to avoid issues with variable scoping, especially in the context of IO actions.- 562-563: The use of
evaluateWithExtendedEnvwithin thebind-IOmacro action processing suggests a nuanced handling of environments during the evaluation of expressions in an extended environment. Verify that the environment extension and subsequent evaluation are correctly implemented to maintain the integrity of variable bindings and evaluations.- 908-921: The handling of type cases in the
AwaitingTypeCasetask processing involves branching based on the type of the expression being evaluated. It's crucial to ensure that the type case handling is robust and correctly interprets the types to execute the appropriate branch of code.- 936-936: The evaluation of macro implementations within the
AwaitingMacrotask processing requires careful handling of the macro expansion environment. Verify that the macro implementation is evaluated in the correct phase and that the environment is appropriately extended to support macro expansion.- 952-952: In the
AwaitingDefntask processing, the evaluation of definitions involves expanding and evaluating expressions in the current environment. Ensure that the environment is correctly managed and that definitions are evaluated and bound correctly to support subsequent expansions.- 1314-1335: The processing of user-defined macros involves evaluating the macro implementation and applying it to the syntax object. It's important to ensure that the macro application is correctly handled and that any errors during macro expansion are appropriately reported to aid in debugging macro definitions.
src/Evaluator.hs
Outdated
| {- Note [The CEK interpreter]: | ||
|
|
||
| The Klister interpreter is a straightforward implementation of a CEK | ||
| interpreter. The interpreter keeps three kinds of state: | ||
|
|
||
| -- C: Control ::= The thing that is being evaluated | ||
| -- E: Environment ::= The interpreter environment | ||
| -- K: Kontinuation ::= The syntactic context of the thing that is being interpreted | ||
|
|
||
| Why a CEK? A CEK interpreter allows us to have precise control over the | ||
| evaluation of a klister program. For example, because the interpreter keeps a | ||
| reference to the kontinuation we can provide stack traces. This handle also | ||
| makes a more advanced debugger possible. Upon an evaluation error we could save | ||
| the kontinuation stack, over write a variable in the environment a la common | ||
| lisp or even rewind the evaluation | ||
|
|
||
| See Matthias Felleison's course website for a good reference: | ||
| https://felleisen.org/matthias/4400-s20/lecture23.html | ||
|
|
||
| The bird's eye view: | ||
|
|
||
| The evaluator crawl's the input AST and progresses in three modes: | ||
|
|
||
| -- 'Down': meaning that the evaluator is searching for a redex to evaluate and | ||
| -- therefore moving "down" the AST. | ||
|
|
||
| -- 'Up': meaning that the evaluator has evaluated some redex to a value and is | ||
| -- passing that valua "up" the execution stack. | ||
|
|
||
| -- 'Er': meaning that something has gone wrong, the stack is captured and the Er | ||
| -- will float up to be handled by the caller of the evaluator. | ||
|
|
||
| All interesting things happen by matching on 'Kont', the continuation. This | ||
| allows the evaluator to know exactly what needs to happen in order to continue. | ||
|
|
||
| -- TODO: #108 describe the how the debugger hooks in | ||
|
|
||
| -} |
There was a problem hiding this comment.
The detailed comments explaining the CEK interpreter provide a clear overview of the interpreter's design and operation. This documentation is valuable for both current and future developers working on the project. However, the TODO comment on line 46 about describing how the debugger hooks in suggests that further documentation is needed for complete clarity.
Would you like assistance in documenting the debugger integration?
| (l:ls) -> Down (unCore l) env (InSyntaxErrorLocations msg_syn ls mempty env kont) | ||
| ) | ||
| (\err -> Er err env kont) | ||
| -- done | ||
| (InSyntaxErrorLocations msg_syn [] dones env kont) -> | ||
| Up (ValueMacroAction | ||
| $ MacroActionSyntaxError (SyntaxError { _syntaxErrorMessage = msg_syn | ||
| , _syntaxErrorLocations = dones | ||
| })) env kont | ||
| (InSyntaxErrorLocations msg (l:ls) dones env kont) -> | ||
| evalAsSyntax v | ||
| (\good -> Down (unCore l) env (InSyntaxErrorLocations msg ls (good : dones) env kont)) | ||
| (\err -> Er err env kont) | ||
|
|
||
| -- the downsweep, searching for a redex to evaluate. | ||
| step (Down c env k) = | ||
| case c of | ||
|
|
||
| -- atoms | ||
| (CoreString s) -> Up (ValueString s) env k | ||
| (CoreInteger i) -> Up (ValueInteger i) env k | ||
| (CoreIntegerSyntax (ScopedInteger int scope)) -> Down (unCore int) env (InInteger scope env k) | ||
| (CoreStringSyntax (ScopedString str scope)) -> Down (unCore str) env (InString scope env k) | ||
| (CoreSyntax s) -> Up (ValueSyntax s) env k | ||
| (CoreError what) -> Down (unCore what) env (InError env k) | ||
| (CoreEmpty (ScopedEmpty scope)) -> Down (unCore scope) env (InScope (List mempty) env k) | ||
| CoreMakeIntroducer -> Up (ValueMacroAction MacroActionIntroducer) env k | ||
| CoreWhichProblem -> Up (ValueMacroAction MacroActionWhichProblem) env k | ||
|
|
||
|
|
||
| -- variables and binders | ||
| (CoreVar var) -> | ||
| case lookupVal var env of | ||
| Just val -> Up val env k | ||
| _ -> Er (EvalErrorUnbound var) env k | ||
|
|
||
| (CoreLet ident var def body) -> | ||
| Down (unCore def) env (InLetDef ident var (unCore body) env k) | ||
|
|
||
| (CoreLetFun fIdent fVar argIdent argVar def body) -> | ||
| let vFun = ValueClosure $ FO $ FOClosure | ||
| { _closureEnv = Env.insert fVar fIdent vFun env | ||
| , _closureIdent = argIdent | ||
| , _closureVar = argVar | ||
| , _closureBody = def | ||
| } | ||
| newEnv = Env.insert fVar fIdent vFun env | ||
| in Down (unCore body) newEnv k | ||
|
|
||
| (CoreCtor con args) -> case args of | ||
| -- just a symbol, shortcut out | ||
| [] -> Up (ValueCtor con mempty) env k | ||
| -- process fields left to right | ||
| (f:fs) -> Down (unCore f) env (InCtor con (fmap unCore fs) mempty env k) | ||
|
|
||
|
|
||
| -- lambdas and application | ||
| (CoreLam ident var body) -> | ||
| let lam = ValueClosure $ FO $ FOClosure | ||
| { _closureEnv = env | ||
| , _closureIdent = ident | ||
| , _closureVar = var | ||
| , _closureBody = body | ||
| } | ||
| in Up lam env k | ||
| (CoreApp fun arg) -> Down (unCore arg) env (InArg (unCore fun) env k) | ||
|
|
||
|
|
||
| -- cases | ||
| (CoreCase loc scrutinee cases) -> Down (unCore scrutinee) env (InCaseScrut cases loc env k) | ||
| (CoreDataCase loc scrutinee cases) -> Down (unCore scrutinee) env (InDataCaseScrut cases loc env k) | ||
| (CoreTypeCase loc scrut cases) -> Down (unCore scrut) env (InTypeCaseScrut cases loc env k) | ||
|
|
||
| (CoreIdent (ScopedIdent ident scope)) -> Down (unCore ident) env (InIdent scope env k) | ||
| (CoreIdentEq how l r) -> Down (unCore l) env (InIdentEqL how r env k) | ||
|
|
||
| (CoreCons (ScopedCons hd tl scope)) -> Down (unCore hd) env (InConsHd scope (unCore tl) env k) | ||
| -- empty, short circuit | ||
| (CoreList (ScopedList ls scope)) -> case ls of | ||
| [] -> Down (unCore scope) env (InScope (List []) env k) | ||
| (e:es) -> Down (unCore e) env (InList scope es mempty env k) | ||
| (CoreReplaceLoc loc stx) -> Down (unCore loc) env (InLoc stx env k) | ||
|
|
||
|
|
||
| -- macros | ||
| (CorePureMacro arg) -> Down (unCore arg) env (InPureMacro env k) | ||
| (CoreBindMacro hd tl) -> Down (unCore hd) env (InBindMacroHd tl env k) | ||
|
|
||
|
|
||
| -- others | ||
| (CoreLog msg) -> Down (unCore msg) env (InLog env k) | ||
| (CoreSyntaxError err) -> | ||
| Down (unCore $ _syntaxErrorMessage err) env (InSyntaxErrorMessage (_syntaxErrorLocations err) env k) | ||
|
|
||
|
|
||
| -- ----------------------------------------------------------------------------- | ||
| -- Helper Functions | ||
|
|
||
| evalErrorText :: EvalError -> Text | ||
| evalErrorText (EvalErrorUnbound x) = "Unbound: " <> T.pack (show x) |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [187-534]
The step function, which transitions the CEK state machine, is a critical part of the evaluator. It handles various cases like function application, constructors, cases, macros, and syntax and atomics. It's essential to review this function for correctness, especially in handling edge cases and ensuring that the transitions are logically sound. Additionally, performance considerations should be taken into account, given the concerns raised about the new evaluator's performance impact.
Ensure that all transitions in the step function are logically sound and consider potential performance optimizations, especially given the concerns about increased bad speculation and its impact on branch prediction.
| -- TODO DYG: Move to separate module | ||
| projectError :: EState -> EvalError | ||
| projectError (Er err _env _k) = err | ||
| projectError _ = error "debugger: impossible" |
There was a problem hiding this comment.
I see that repl/Main.hs has
case evaluateIn ... of
Left evalErr -> print $ erroneousValue $ projectError evalErr
Right val -> ...so is this "impossible" in the sense that when evaluateIn returns a Left eState, that EState is always an Er? In that case, wouldn't it make more sense to return Left err or Left (err, _env, _k), to make impossible states unrepresentable?
There was a problem hiding this comment.
Yes that would make more sense, but I specifically left it this way because this state will become important for the debugger. The debugger will be typed debugger :: EState -> DebugM EState or something like that. But the key point is that the debugger should be able to input EState which is actually at Er ... and return an EState that is not in Er. So I left as much information as possible until the very last projection or pretty printing because when evaluateIn returns a Left (bunch of state) the debugger could take that state and then resume the computation. Perhaps I should add a TODO in a comment
There was a problem hiding this comment.
Thanks for the explanation, that makes sense. But the call to error still seems wrong. I can see several possible solutions:
- Add a precondition to
projectError's documentation, e.g. "should only be called on theEStateinside aLeftreturned by one of theevaluateXfunctions. - Have the
evaluateXfunctions return an opaque representation of the error, let's sayErrState, containing(err, env, k). Then, provide a total functionprojectError :: ErrState -> EvalErrorand a total functionErrState -> EState, so that the resultingEStatecan be given todebugger :: EState -> DebugM EState. - Have the
evaluateXfunctions returnLeft (err, estate), so they can both display the error and resume from theestate, without knowing that the two are related under the hood.
| case out of | ||
| Left err -> error (T.unpack (pretty err)) | ||
| case (evaluateWithExtendedEnv env [(ident, var, vx)] body) of | ||
| Left err -> error (T.unpack (pretty $ projectError err)) |
There was a problem hiding this comment.
woah! why do we fail with error here? I think runEval used to only fail if there was a bug in our implementation of the compiler, e.g. if our type checker allowed the construction of an ill-typed Core term. but nowadays evaluating a Core expression can fail with EvalErrorCase or EvalErrorUser if there is a bug in the user's .kl code too, so we should probably use throwError here. this is unrelated to the current PR, so I'll create a separate ticket to track this: #237
| $ MacroEvaluationError p | ||
| $ evalErrorType "macro action" | ||
| $ erroneousValue | ||
| $ projectError err |
There was a problem hiding this comment.
this error-handling logic is repeated several time (sometimes with the evalErrorType "macro action" bug, sometimes without), wasn't it nicer when expandEval was running in the Expand monad and was thus able to throw this error right away?
There was a problem hiding this comment.
I'm hesitant to DRY it out because I still want to workshop the design of the debugger and how it hooks into expandEval.
throw this error right away
This action will be calling the debugger. Immediately exiting the debugger should then be throwing the error and reporting the error back to the user. Perhaps Expand will have to be a monad transformer over DebugM? I'm honestly not sure if anyone has made a CommonLisp style debugger in Haskell yet.
There was a problem hiding this comment.
Now that I understand what erroneousValue does, I don't think this code is correct. It claims that the problem is that there is a value which should be a macro action, but its actual type is something else. But that problem is raised below, on L1001:
throwError $ MacroEvaluationError p $ evalErrorType "macro action" other
The problem on this line is some unknown thing which happened during evaluation, not necessarily a type error. Instead of using err to construct a type error, we should propagate err upwards as-is.
doyougnu
force-pushed
the
doyougnu/wip/useful-repl
branch
2 times, most recently
from
b2de4f2
to
906a85e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- repl/Main.hs (2 hunks)
- src/Evaluator.hs (4 hunks)
- src/Expander.hs (14 hunks)
- src/Expander/Monad.hs (3 hunks)
- src/Pretty.hs (1 hunks)
Additional context used
Path-based instructions (5)
repl/Main.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Pretty.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Evaluator.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Expander/Monad.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Expander.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
Additional comments not posted (17)
repl/Main.hs (1)
149-151: The integration ofevaluateInin the REPL's evaluation flow.
This change leverages the new CEK evaluator'sevaluateInfunction to process expressions. It effectively handles errors by projecting them throughprojectErrorand then extracting the erroneous value for display. This is a crucial part of the REPL's functionality, ensuring that evaluation errors are properly managed and communicated to the user.src/Pretty.hs (1)
627-627: Handling of new error type in pretty-printing.
The addition of theEvalErrorIdentcase allows the system to specifically address and display errors related to attempts to bind identifiers to non-values. This enhances the error reporting capabilities of the system, making it easier for users to understand the nature of the errors encountered.src/Evaluator.hs (4)
11-29: The introductory note provides a clear and concise explanation of the CEK interpreter's design and its components (Control, Environment, Kontinuation). This documentation is crucial for understanding the operational mechanics of the interpreter.
654-658: TheevaluateWithExtendedEnvfunction extends the environment for evaluation. This is a critical operation, and it's important to ensure that the environment extension is handled correctly to avoid any potential issues during evaluation.Verification successful
The
evaluateWithExtendedEnvfunction appears to handle the environment extension correctly and efficiently usingfoldl'to insert elements into the environment. The usages insrc/Expander.hsandsrc/Evaluator.hsalso seem to correctly pass the environment and extensions.
src/Evaluator.hs:
- Line 654: Definition of
evaluateWithExtendedEnv.- Line 656: Uses
foldl'to insert elements into the environment.
src/Expander.hs:
- Line 1: Usage of
evaluateWithExtendedEnvwith correct parameters.
src/Evaluator.hs:
- Line 1: Usage of
evaluateWithExtendedEnvwith correct parameters.- Line 2: Usage of
evaluateWithExtendedEnvwith correct parameters.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct handling of environment extensions in `evaluateWithExtendedEnv`. # Test: Search for incorrect or inefficient environment extensions. Expect: Environment is extended correctly and efficiently. ast-grep --lang haskell --pattern $'evaluateWithExtendedEnv($_, $_, $_) { $$$ }'Length of output: 81
Script:
#!/bin/bash # Search for the definition of `evaluateWithExtendedEnv` in the codebase. ast-grep --lang haskell --pattern 'evaluateWithExtendedEnv($_, $_, $_) = $_' # Search for all usages of `evaluateWithExtendedEnv` in the codebase. rg 'evaluateWithExtendedEnv'Length of output: 601
Script:
#!/bin/bash # Display the definition of `evaluateWithExtendedEnv` in `src/Evaluator.hs`. cat src/Evaluator.hs | sed -n '654,658p' # Display the usages of `evaluateWithExtendedEnv` in `src/Expander.hs`. rg 'evaluateWithExtendedEnv' src/Expander.hs # Display the usages of `evaluateWithExtendedEnv` in `src/Evaluator.hs`. rg 'evaluateWithExtendedEnv' src/Evaluator.hsLength of output: 748
187-358: Thestepfunction, which is central to the CEK machine's operation, handles various states and transitions. It's crucial to ensure that each case is handled correctly to maintain the integrity of the evaluation process. Given the performance concerns mentioned in the PR, consider optimizing the handling of these transitions to reduce complexity and improve efficiency.Consider refactoring complex cases in the
stepfunction to separate helper functions to improve readability and maintainability.
441-442: Theapplyfunction and its helperapplyInEnvare critical for handling function applications. Ensure that these functions are robust against various edge cases, especially given the new evaluator's performance issues.Also applies to: 502-510
src/Expander/Monad.hs (2)
9-9: The addition of theFlexibleContextslanguage extension is appropriate given the changes in function signatures and types.
864-871: The renaming ofexpandEvaltoevalInCurrentPhaseand its implementation are well-suited to its purpose. The function effectively uses the current environment for evaluation and handles errors appropriately.src/Expander.hs (9)
25-25: Review the addition ofevalInCurrentPhaseand related entities.The addition of
evalInCurrentPhaseand related entities in the export list is consistent with the changes described in the PR summary and AI-generated summary. This function seems to be central to the new evaluation strategy.
275-275: Review the use ofevalInCurrentPhaseinevalDecl.The use of
evalInCurrentPhasehere for evaluating expressions within declarations is appropriate and aligns with the new evaluation strategy. This ensures that expressions are evaluated in the correct phase of the language's compilation or interpretation process.
302-302: Review the use ofevalInCurrentPhaseinExamplecase.Using
evalInCurrentPhaseto evaluate the expression in theExamplecase is correct. It ensures that the example expressions are evaluated in the current phase, which is crucial for testing and example code blocks within the language.
305-305: Review the use ofevalInCurrentPhaseinRuncase.The use of
evalInCurrentPhasein theRuncase is appropriate. It ensures that runtime actions are evaluated in the current phase, which is essential for executing IO actions or other runtime tasks.
316-316: Review the use ofevalInCurrentPhaseinDefineMacroscase.The use of
evalInCurrentPhasein theDefineMacroscase is appropriate. It ensures that macro definitions are evaluated in the current phase, which is crucial for macro expansion and execution.
556-556: Review the handling of function application inbind-IO.The handling of function application in the
bind-IOprimitive is correctly implemented. It uses dynamic environment extension and proper error handling, which are essential for functional programming languages that support first-class functions.
908-920: Review the error handling inAwaitingTypeCase.The error handling in the
AwaitingTypeCasetask is robust, correctly distinguishing between different types of macro evaluation errors and providing detailed error messages. This is crucial for debugging and maintaining the language's macro system.
935-935: Review the evaluation of macro implementations inAwaitingMacro.The evaluation of macro implementations in the
AwaitingMacrotask is correctly handled. It usesinEarlierPhaseto ensure that the macro is evaluated in the correct phase, which is essential for maintaining the phase consistency of the language.
1313-1334: Review the handling of user macros inexpandOneForm.The handling of user macros in
expandOneFormis sophisticated and well-implemented. It correctly uses phase adjustments and dynamic scope management to ensure that macros are expanded correctly and efficiently.
| All interesting things happen by matching on 'Kont', the continuation. This | ||
| allows the evaluator to know exactly what needs to happen in order to continue. | ||
|
|
||
| -- TODO: #108 describe the how the debugger hooks in |
There was a problem hiding this comment.
TODO: Document debugger integration.
Would you like assistance in documenting how the debugger integrates with the CEK interpreter? This could enhance the clarity and maintainability of the debugging features.
| projectError :: EState -> EvalError | ||
| projectError (Er err _env _k) = err | ||
| projectError _ = error "debugger: impossible" | ||
|
|
There was a problem hiding this comment.
Tip
Codebase Verification
The EState type has three constructors: Down, Up, and Er. The projectError function only handles the Er constructor explicitly and defaults to an error for other cases. This means it does not handle the Down and Up states, which could lead to runtime errors if these states are passed to projectError.
- Location to address:
src/Evaluator.hslines 660-663: EnsureprojectErrorhandles all possible states ofEState.
Analysis chain
The projectError function is designed to extract errors from the evaluator's state. Given its critical role in error handling, ensure that it robustly handles all possible error states.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the robustness of the `projectError` function.
# Test: Search for potential issues in error extraction by `projectError`. Expect: All errors are correctly extracted.
ast-grep --lang haskell --pattern $'projectError($_) { $$$ }'
Length of output: 62
Script:
#!/bin/bash
# Description: Identify all possible states of `EState` and verify if `projectError` handles them.
# Step 1: Find the definition of `EState` to list all possible states.
ast-grep --lang haskell --pattern $'data EState = $$$'
# Step 2: Check where `projectError` is used to ensure it handles all states.
rg 'projectError'
Length of output: 780
Script:
#!/bin/bash
# Description: Locate the definition of `EState` and list all possible states.
# Step 1: Find the definition of `EState` to list all possible states.
rg 'data EState' --type haskell
Length of output: 68
Script:
#!/bin/bash
# Description: Extract the full definition of `EState` from `src/Evaluator.hs`.
# Step 1: Extract the full definition of `EState`.
sed -n '/data EState/,/where/p' src/Evaluator.hs
Length of output: 14015
- on track for gelisam#108
doyougnu
force-pushed
the
doyougnu/wip/useful-repl
branch
from
906a85e
to
d3cca7a
Compare
| res <- interpretMacroAction prob act | ||
| case res of | ||
| Left err -> throwError | ||
| $ ValueNotMacro |
There was a problem hiding this comment.
I don't think this should be a ValueNotMacro error. In this branch, looking up transformerName did successfully find a macro definition of type (-> Syntax (Macro Syntax)). That function was then applied to a Syntax argument, and the evaluation of that function failed for some reason, perhaps because (error 'msg) was called.
There was a problem hiding this comment.
Ha! The existing code already misuses ValueNotMacro. When expanding the macro call site (my-macro), many things can go wrong:
my-macromight not be in scope, in which caseresolve(line L1201) throwsUnknownand the user seesUnknown: my-macro.my-macroresolves toEUserMacro transformerName, but thattransformerNameis somehow not theMacroVarwhich was added to the transformer environment by(define-macros ([my-macro ...]))at the same time asmy-macrowas bound toEUserMacroin the binding table. For example, maybetransformerNameis not in the binding table at all. This is a bug in Klister, soInternalErroris thrown;errorwould have been fine too.- Same thing, but
transformerNameis bound to something other than a(-> Syntax (Macro Syntax)). This is also a bug in Klister, butValueNotMacrois thrown. This is definitely wrong, since the error message associated withValueNotMacrois "Not a macro monad value", but it's not even a(Macro Syntax)which is expected, it's a(-> Syntax (Macro Syntax)). transformerNameis bound to a value of type(-> Syntax (Macro Syntax)), but applying it to aSyntaxvalue fails before we can get a(Macro Syntax)value out. The current code fails insideexpandEvalwithMacroEvaluationError, whereas this branch fails here withValueNotMacro. I think it should beMacroEvaluationError.- we do get a value out, but it's not a
(Macro Syntax), soValueNotMacrois thrown. If this happens, that means there is a bug in Klister's type-checker, soInternalErrororerrorwould have been fine too. interpretMacroActionencounters an error while executing the macro action. it fails somewhere insideinterpretMacroActionvia code which isn't relevant here.
As a conclusion, I think this should be MacroEvaluationError, not ValueNotMacro, but the existing code isn't very good at using the right error constructor, so this isn't a big deal.
There was a problem hiding this comment.
This is the same mistake as the other erroneousValue call site. Maybe erroneousValue shouldn't exist at all, since it seems so easy to misuse?
| ValueMacroAction act -> interpretMacroAction prob act | ||
| other -> throwError $ ValueNotMacro other | ||
| view (expanderWorld . worldEnvironments . at phase) $ s | ||
| case applyInEnv env closure boundResult of |
There was a problem hiding this comment.
another place where this MR highlights a mistake in the original code: all of
phase <- view (expanderLocal . expanderPhase)
s <- getState
let env = ...
and the withEnv env is dead code, because apply overwrites this env with its own call to withEnv. So there is no need for an applyInEnv separate from apply.
There was a problem hiding this comment.
here's what I would write:
phase <- view (expanderLocal . expanderPhase)
case apply closure boundResult of
Left err -> throwError
$ MacroEvaluationError phase
$ projectError err
| -- 'Er': meaning that something has gone wrong, the stack is captured and the Er | ||
| -- will float up to be handled by the caller of the evaluator. | ||
|
|
||
| All interesting things happen by matching on 'Kont', the continuation. This |
There was a problem hiding this comment.
Not the Control? What's the Control for, then?
| ( EvalError (..) | ||
| , EvalResult (..) | ||
| , TypeError (..) | ||
| , evaluate |
There was a problem hiding this comment.
evaluate has type Core -> Either EState Value, but EState is not exported. That seems needlessly restrictive: the caller can store the result in a variable, return it to its own caller, but if they do so then they cannot write down a type signature. Would it make sense to at least export it as an opaque type?
| ( EvalError (..) | ||
| , EvalResult (..) | ||
| , TypeError (..) | ||
| , evaluate |
There was a problem hiding this comment.
evaluate looks like it is the main API, with evaluateIn and evaluateWithExtendedEnv being a secondary APIs for special cases. But I see that evaluate is not used anywhere! Perhaps it would make more sense to drop evaluate and to make evaluateIn the main API?
| , evaluateIn | ||
| , evaluateWithExtendedEnv | ||
| , evalErrorType | ||
| , evalErrorText |
There was a problem hiding this comment.
evalErrorText is not used anywhere? that's strange, where do the errors get printed? Maybe the Show instance is used instead of evalErrorText by mistake?
| , projectError | ||
| , erroneousValue | ||
| , applyInEnv | ||
| , apply |
There was a problem hiding this comment.
apply behaves very similarly to the evaluateX functions, would it perhaps make more sense to call it something like evaluateFunctionApplication?
There was a problem hiding this comment.
same for doTypeCase. evaluateTypeCase?
| @@ -38,8 +103,343 @@ data EvalError | |||
| | EvalErrorType TypeError | |||
| | EvalErrorCase SrcLoc Value | |||
There was a problem hiding this comment.
I know we discussed this, but I still find it unintuitive that a SrcLoc is needed for case and type-case, but not for anything else. We should document the reasoning somewhere. Let's see, that reasoning is...
That SrcLoc is used to construct the error message in the case in which no cases match. By comparison, other errors such as looking up a Var don't need a SrcLoc because scope errors are caught during expansion time, before evaluation begins. Are case and type-case really the only ways in which evaluation can fail at runtime, assuming expansion already caught scope errors and type errors?
There was a problem hiding this comment.
Actually, the current datatype exhaustively lists all the errors which can happen at evaluation-time, and #237 already classifies which ones should already be caught in an earlier phase and which ones really can happen at evaluation-time. There is EvalErrorCase, and EvalErrorUser. EvalErrorUser does not need a separate SrcLoc because the error message is a Syntax, which is already associated with a SrcLoc.
Since we're separating between Case and TypeCase everywhere else in the codebase, I wonder why there isn't a separate EvalErrorTypeCase here? 🤔
Anyway, that's not something which changed in the current MR, moving right along...
| , applyInEnv | ||
| , apply | ||
| , doTypeCase | ||
| , try |
There was a problem hiding this comment.
why is Control.Exception.try re-exported? you don't even throw exceptions in this file 😅
| makePrisms ''EvalError | ||
| instance Exception EvalError |
There was a problem hiding this comment.
EvalError is never thrown as an exception, did you perhaps initially intend to use it as an exception and later changed your mind to use Er evalError?
|
|
||
| data EvalResult | ||
| = ExampleResult SrcLoc VEnv Core (Scheme Ty) Value | ||
| | IOResult (IO ()) |
There was a problem hiding this comment.
EvalResult is not even used in this file 😅
Perhaps it should be defined next to evalMod instead?
Let's do that in a separate MR, as this MR does not introduce EvalResult, it merely moves it to the top of the file.
| data EvalResult | ||
| = ExampleResult SrcLoc VEnv Core (Scheme Ty) Value | ||
| | IOResult (IO ()) | ||
|
|
||
| -- TODO: more precise representation | ||
| type Type = Text |
There was a problem hiding this comment.
Text is actually just fine since the type checker has (hopefully) already caught all the type errors, so it doesn't make sense to spend extra effort on type errors which slip through to evaluation-time.
| -- ^ Marks the evaluator finishing | ||
|
|
||
| -- functions | ||
| InArg :: !(CoreF TypePattern ConstructorPattern Core) -> !VEnv -> !Kont -> Kont |
There was a problem hiding this comment.
Why use CoreF TypePattern ConstructorPattern Core instead of the shorter-and-isomorphic Core?
| InArg :: !(CoreF TypePattern ConstructorPattern Core) -> !VEnv -> !Kont -> Kont | ||
| -- ^ The argument is being evaluated, so hold onto the function symbol. | ||
| InFun :: !Value -> !VEnv -> !Kont -> Kont | ||
| -- ^ The function is being evaluated, so hold onto the evaluated argument. |
There was a problem hiding this comment.
Hmm, you evaluate the argument first? I of course know that Klister is a strict language, which means that the arguments are evaluated before the call is made, but what about the case in which the function is an expression, e.g. ((id (+)) (* 2 2) (* 3 3))? Isn't it standard to evaluate (id (+)) to (+) before evaluating (* 2 2) to 4 and (* 3 3) to 9?
| -- functions | ||
| InArg :: !(CoreF TypePattern ConstructorPattern Core) -> !VEnv -> !Kont -> Kont | ||
| -- ^ The argument is being evaluated, so hold onto the function symbol. | ||
| InFun :: !Value -> !VEnv -> !Kont -> Kont |
There was a problem hiding this comment.
Why is the VEnv needed? It will not be used to further evaluate the Value, since it is already a Value. Is it perhaps so that the debugger can display the captured environment at this point in the stack trace?
| -- lists | ||
| InConsHd :: !Core -> !(CoreF TypePattern ConstructorPattern Core) -> !VEnv -> !Kont -> Kont | ||
| InConsTl :: !Core -> !Syntax -> !VEnv -> !Kont -> Kont | ||
| InList :: !Core -> ![Core] -> ![Syntax] -> !VEnv -> !Kont -> Kont |
There was a problem hiding this comment.
It's a shame that you had to waste your time reimplementing those outdated direct syntax-manipulating primitives! I only ever use open-syntax and close-syntax now. We should get rid of this old stuff. In a later MR, of course.
| InArg :: !(CoreF TypePattern ConstructorPattern Core) -> !VEnv -> !Kont -> Kont | ||
| -- ^ The argument is being evaluated, so hold onto the function symbol. | ||
| InFun :: !Value -> !VEnv -> !Kont -> Kont | ||
| -- ^ The function is being evaluated, so hold onto the evaluated argument. |
There was a problem hiding this comment.
These comments are really helpful, let's add one for each constructor!
-- functions
InArg :: !(CoreF TypePattern ConstructorPattern Core) -> !VEnv -> !Kont -> Kont
-- ^ The argument is being evaluated, so hold onto the function expression
InFun :: !Value -> !VEnv -> !Kont -> Kont
-- ^ The function is being evaluated, so hold onto the evaluated argument.
InLetDef :: !Ident -> !Var -> !(CoreF TypePattern ConstructorPattern Core) -> !VEnv -> !Kont -> Kont
-- ^ Inside the rhs of (let [x rhs] body), so hold the name x and the body.
-- constructors
InCtor :: !Constructor -> ![CoreF TypePattern ConstructorPattern Core] -> ![Value] -> !VEnv -> !Kont -> Kont
-- ^ Inside arg_i of (Ctor arg_1 ... arg_i ... arg_n), so hold Ctor, the
-- already evaluated values value_1 ... value_{i-1}, and the
-- yet-to-be-evaluated expressions arg_{i+1} ... arg_n.
-- Cases
InCaseScrut :: ![(SyntaxPattern, Core)] -> !SrcLoc -> !VEnv -> !Kont -> Kont
InDataCaseScrut :: ![(ConstructorPattern, Core)] -> !SrcLoc -> !VEnv -> !Kont -> Kont
InTypeCaseScrut :: ![(TypePattern, Core)] -> !SrcLoc -> !VEnv -> !Kont -> Kont
-- ^ Inside the scrutinee of (case scrutinee [pat_1 rhs_1] ...), so hold
-- the branches.
-- lists
InConsHd :: !Core -> !(CoreF TypePattern ConstructorPattern Core) -> !VEnv -> !Kont -> Kont
-- ^ Inside the head of (cons-list-syntax-cons head tail loc), so hold the tail.
InConsTl :: !Core -> !Syntax -> !VEnv -> !Kont -> Kont
-- ^ Inside the tail of (cons-list-syntax-cons head tail loc), so hold the already-evaluated head.
InList :: !Core -> ![Core] -> ![Syntax] -> !VEnv -> !Kont -> Kont
-- ^ Inside arg_i of (list-syntax (arg_1 ... arg_i ... arg_n) loc), so hold
-- the already-evaluated syntax values stx_1 ... stx_{i-1}, and the
-- yet-to-be-evaluated expressions arg_{i+1} ... arg_n.
-- idents
InIdent :: !Core -> !VEnv -> !Kont -> Kont
InIdentEqL :: !HowEq -> !Core -> !VEnv -> !Kont -> Kont
-- ^ Inside the lhs of (<how>-identifier=? lhs rhs), so hold <how> and rhs.
InIdentEqR :: !HowEq -> !Value -> !VEnv -> !Kont -> Kont
-- ^ Inside the rhs of (<how>-identifier=? lhs rhs), so hold <how> and the
-- already-evaluated lhs.
-- Macros
InPureMacro :: !VEnv -> !Kont -> Kont
-- ^ Inside the expr of (pure expr), so nothing to hold.
InBindMacroHd :: !Core -> !VEnv -> !Kont -> Kont
-- ^ Inside the expr1 of (>>= expr1 expr2), so hold expr2.
InBindMacroTl :: !MacroAction -> !VEnv -> !Kont -> Kont
-- ^ Inside the expr2 of (>>= expr1 expr2), so hold the already-evaluated
-- expr1.
-- atomics
InInteger :: !Core -> !VEnv -> !Kont -> Kont
InString :: !Core -> !VEnv -> !Kont -> Kont
InLoc :: !Core -> !VEnv -> !Kont -> Kont
-- ^ Inside the loc of (replace-loc loc stx), so hold the stx.
InLocStx :: !SrcLoc -> !VEnv -> !Kont -> Kont
-- ^ Inside the stx of (replace-loc loc stx), so hold the already-evaluated
-- loc.
-- scope
InScope :: !(ExprF Syntax) -> !VEnv -> !Kont -> Kont
-- ^ Inside the loc of something like (cons-syntax-list head tail loc), so
-- remember the Syntax-Contents which needs a scope-set and location.
-- logs and errors
InLog :: !VEnv -> !Kont -> Kont
-- ^ Inside the expr of (log expr), so nothing to hold.
InError :: !VEnv -> !Kont -> Kont
-- ^ Inside the expr of (error expr), so nothing to hold.
| -- ^ Evaluating the let def | ||
|
|
||
| -- constructors | ||
| InCtor :: !Constructor -> ![CoreF TypePattern ConstructorPattern Core] -> ![Value] -> !VEnv -> !Kont -> Kont |
There was a problem hiding this comment.
According to the naming convention, shouldn't it be InCtorArg? We're inside arg_i of (Ctor arg_1 ... arg_i ... arg_n), not inside Ctor, right?
There was a problem hiding this comment.
And arg_1 through arg_{i-1} have already been evaluated to Values. Shouldn't the [Value] list be before the [Core] list? Seems weird to use the order [arg_{i+1}, ..., arg_n] [arg_1, ..., arg_{i-1}]. Or are both lists reversed? Is it [arg_n, arg_{n-1}, ..., arg_{i+1}] [arg_{i-1}, arg_{i-2}, ..., arg_1]?
| InInteger :: !Core -> !VEnv -> !Kont -> Kont | ||
| InString :: !Core -> !VEnv -> !Kont -> Kont | ||
| InLoc :: !Core -> !VEnv -> !Kont -> Kont | ||
| InLocStx :: !SrcLoc -> !VEnv -> !Kont -> Kont |
There was a problem hiding this comment.
I think InReplaceLocL and InReplaceLocR would be much clearer than InLoc and InLocStx
| (InSyntaxErrorLocations msg_syn [] dones env kont) -> | ||
| Up (ValueMacroAction | ||
| $ MacroActionSyntaxError (SyntaxError { _syntaxErrorMessage = msg_syn | ||
| , _syntaxErrorLocations = dones |
There was a problem hiding this comment.
| , _syntaxErrorLocations = dones | |
| , _syntaxErrorLocations = reverse (v : dones) |
| -- | The state of the evaluator | ||
| data EState where | ||
| Down :: !(CoreF TypePattern ConstructorPattern Core) -> !VEnv -> !Kont -> EState | ||
| -- ^ 'Down', we are searching the AST for a redux and building up the stack of |
There was a problem hiding this comment.
| -- ^ 'Down', we are searching the AST for a redux and building up the stack of | |
| -- ^ 'Down', we are searching the AST for a redex and building up the stack of |
| , _closureBody = body | ||
| } | ||
| in Up lam env k | ||
| (CoreApp fun arg) -> Down (unCore arg) env (InArg (unCore fun) env k) |
There was a problem hiding this comment.
eval was recurring on the function position first, not the argument, we think the CEK version should do the same
(currently pair-reviewing with @david-christiansen)
Still a draft but all but one test is passing.
TODOs: