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. |
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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?
| -- 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.
| 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
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
| ( 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?
✏️ this is not a blocker, but after this MR is finally merged, I plan to come back and fix this.
| ( 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?
this is not a blocker.
| , 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?
this is not a blocker.
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?
✏️ this is not a blocker, but after this MR is finally merged, I plan to come back and add a comment explaining this.
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 😅
✏️ this is not a blocker, but after this MR is finally merged, I plan to come back and fix this.
| 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?
✏️ this is not a blocker, but after this MR is finally merged, I plan to come back and fix this.
|
|
||
| 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.
This is not a blocker.
| 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.
This is not a blocker.
src/Evaluator.hs
Outdated
| -- ^ 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?
This is not a blocker.
| -- 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.
✏️ this is not a blocker, but after this MR is finally merged, I plan to come back and delete those outdated primitives.
src/Evaluator.hs
Outdated
| 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.
✏️ this is not a blocker, but after this MR is finally merged, I plan to come back and fix this.
There was a problem hiding this comment.
turns out I was wrong about some of those, as some of those hold a reversed list.
There was a problem hiding this comment.
- add comments for each constructor after merge.
| (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) |
There was a problem hiding this comment.
I believe this is a type error:
v : Valuedones : [Syntax]
here is what I believe the original case is in eval:
eval (Core (CoreSyntaxError syntaxErrorExpr)) = do
syntaxErrorValue <- traverse evalAsSyntax syntaxErrorExpr
pure $ ValueMacroAction
$ MacroActionSyntaxError syntaxErrorValue
is there an example that exhibits this bug?
There was a problem hiding this comment.
I believe this is a type error: [...]
You're right, a call to evalAsSyntax is also needed.
is there an example that exhibits this bug?
Here is one:
#lang kernel
(import (shift kernel 1))
(define-macros
([m (lambda (stx)
(syntax-error 'error-message
'location-1 -- line 7
'location-2 -- line 8
'location-3 -- line 9
'location-4 -- line 10
))]))
(example (m))
on main, this program outputs
toy.kl:7.26-7.36:
Syntax error from macro:
#[toy.kl:6.24-6.37]<error-message>Additional locations:
toy.kl:8.26-8.36
toy.kl:9.26-9.36
toy.kl:10.26-10.36
whereas on this branch, this program outputs one fewer location, and outputs them in the opposite order:
toy.kl:9.26-9.36:
Syntax error from macro:
#[toy.kl:6.24-6.37]<error-message>Additional locations:
toy.kl:8.26-8.36
toy.kl:7.26-7.36
The first location is distinguished because it is printed before the error message, so focusing on the line numbers, let's use the shorthand [7], 8, 9, 10 to describe the output on main and [9], 8, 7 to describe the output on this branch.
Line 10 ('location-4) is missing, because step (Up v e k) is supposed to insert the value v into the context k, but here when the context is InSyntaxErrorLocations, the v is not inserted, it is dropped on the floor.
And the locations are listed in the opposite order because as you traverse InSyntaxErrorLocations's [Core] from left to right, you prepend each output to its [Syntax], so the right-most Core ends up being the leftmost Syntax. You have fixed this elsewhere by calling reverse in the InCtor [] and InList [] cases, you just forgot to also do it in the InSyntaxErrorLocations [] case.
| erroneousValue _ = | ||
| error $ mconcat [ "erroneousValue: " | ||
| , "Evaluator concluded in an error that did not return a value" | ||
| ] |
There was a problem hiding this comment.
I don't like this partial function, I think every single call site is incorrect, but I think we should (finally!) merge this and then I'll open a separate MR to replace all the call sites with something more sensible.
✏️ this is not a blocker, but after this MR is finally merged, I plan to come back and fix this.
| other -> Er (evalErrorType "function" other) e k | ||
|
|
||
| where app (FO (FOClosure{..})) = | ||
| let env = Env.insert _closureVar _closureIdent value (_closureEnv <> e) |
There was a problem hiding this comment.
This is a bug. When applying a closure, the body of the closure must be evaluated in the captured environment, not in the union of the captured environment plus the environment at the call site. That is, the following code should not be accepted because y is not in scope where f is defined:
f = \() -> y
g y = f ()
❌ this is a blocker.
There was a problem hiding this comment.
I'm having a hard time writing a test case for this because the patch errors as expected with (I believe) the correct error message. Here is my translation to klister:
-- in examples/non-examples/bad-lexical-env.kl
#lang kernel
(define f (lambda (ff) y))
(define g (lambda (y) (f)))
(example (g 2))
Note that I have to pass ff or else I get and error about no arguments:
❯ cat examples/non-examples/bad-lexical-env.golden
Expected 1 entries between parentheses, but got
#[bad-lexical-env.kl:3.19-3.22]<()>
this is a sidenote but I'm surprised we cannot define a function of type f : () -> <whatever>.
Here is what main shows:
Test suite klister-tests: RUNNING...
All tests
Golden tests
bad-lexical-env: OK (0.01s)
...
❯ cat examples/non-examples/bad-lexical-env.golden
Unknown: #[bad-lexical-env.kl:4.24-4.25]<y>
Notice that this is what we should expect, an error saying unknown y.
and here is the patch:
❯ cabal test --test-show-details=streaming --test-options '-p bad-lexical-env --accept'
...
Test suite klister-tests: RUNNING...
All tests
Golden tests
bad-lexical-env: OK (0.01s)
Accepted the new version
...
❯ cat examples/non-examples/bad-lexical-env.golden
Unknown: #[bad-lexical-env.kl:3.24-3.25]<y>
That is, I accept the new output for the golden test and we get the same error as main from the patch.
There was a problem hiding this comment.
I'm having a hard time writing a test case for this
Makes sense: scope errors are caught at expansion time, precisely so that they cannot occur at evaluation-time, so it should not be possible to write a Klister program which triggers this bug. Okay, this isn't as big of an issue as I thought then.
✏️ this is no longer a blocker, but after this MR is finally merged, I plan to come back and fix this.
There was a problem hiding this comment.
Note that I have to pass ff or else I get and error about no arguments:
❯ cat examples/non-examples/bad-lexical-env.golden
Expected 1 entries between parentheses, but got
#[bad-lexical-env.kl:3.19-3.22]<()>this is a sidenote but I'm surprised we cannot define a function of type
f : () -> <whatever>.
I agree that it is surprising, but it makes sense if you think about it. (lambda () 42) is a function from zero arguments, not a function from the unit type to Integer. Racket supports functions from zero arguments, but Haskell does not. Like Haskell, Klister only supports functions of one argument, so (lambda (x y) 42) desugars to (lambda (x) (lambda (y) 42)), and if it wasn't rejected (lambda () 42) would desugar to 42. The error message could definitely be improved though!
There was a problem hiding this comment.
great explanation! You're right, I guess I see some scheme or lisp and assume that I can define a function with 0-arity.
Makes sense: scope errors are caught at expansion time, precisely so that they cannot occur at evaluation-time,
I wonder if we could define an enhanced debug build that checks for this. Something like building the compiler/interpreter with extra assertions.
doyougnu
force-pushed
the
doyougnu/wip/useful-repl
branch
2 times, most recently
from
ea4086f to
cac10f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
src/Pretty.hs (1)
628-628: Consider adding more context to the IOResult pretty-printingThe implementation for
IOResultis simple and consistent with how other opaque values are printed. However, consider adding a bit more context to make it clearer that this is a result of evaluation.Here's a suggested improvement:
- pp _env (IOResult _) = text "IO action" + pp _env (IOResult _) = text "Evaluation resulted in an IO action"This change provides more clarity about the nature of the result without exposing implementation details.
src/Evaluator.hs (2)
106-106: Add documentation for theEvalErrorIdentconstructorThe addition of
EvalErrorIdent ValuetoEvalErrorintroduces a new error case related to identifier handling. Consider adding Haddock documentation or inline comments to explain its purpose and when it is used. This will improve code maintainability and help other developers understand error handling.
223-224: Consistent error handling inInTypeCaseScrutIn the continuation for
InTypeCaseScrut, the error handling passeserrwithout modification. Ensure that this is consistent with the rest of the codebase's error handling practices and that any necessary context is included in the error.
📜 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 (12 hunks)
- src/Expander/Monad.hs (3 hunks)
- src/Pretty.hs (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
src/Pretty.hs (1)
627-627: LGTM: New error message for EvalErrorIdentThe new pretty-printing implementation for
EvalErrorIdentis clear, informative, and consistent with the style of other error messages in this instance.src/Evaluator.hs (3)
455-457: Ensure allEvalErrorcases are handled inevalErrorTextThe
evalErrorTextfunction now includes a case forEvalErrorIdent, which is good. Verify that all possibleEvalErrorconstructors are accounted for in this function to prevent incomplete pattern matches and ensure comprehensive error messaging.
1-4: Verify compatibility of added language extensionsThe inclusion of
KindSignatures,BangPatterns, andGADTSyntaxextensions enhances type safety and performance. However, ensure that these extensions are compatible with the project's target GHC versions to prevent any build issues.Run the following script to check the minimum required GHC version:
#!/bin/bash # Description: Verify that the GHC version supports the newly added language extensions. # Expected: GHC version 8.0 or higher supports these extensions. ghc --version # Alternatively, check the project's `.cabal` file for the GHC version constraint. grep -i 'build-depends.*base' *.cabal
551-576: Verify all calls todoTypeCaseare updatedWith the
doTypeCasefunction signature changed to includeVEnv, ensure that all invocations of this function pass the correct environment parameter. Missing or incorrect environment arguments may lead to runtime errors or unexpected behavior.Run the following script to locate calls to
doTypeCase:src/Expander/Monad.hs (2)
38-38: ExportingevalInCurrentPhaseAdding
evalInCurrentPhaseto the export list makes it available to other modules as intended.
864-872: Implementation ofevalInCurrentPhaseis correctThe new function
evalInCurrentPhasecorrectly evaluates theCoreexpression within the current environment and handles errors appropriately by throwing aMacroEvaluationErrorwith the current phase.src/Expander.hs (1)
25-25: Consistent Renaming toevalInCurrentPhaseThe renaming of
expandEvaltoevalInCurrentPhasethroughout the module enhances readability and clarity, accurately reflecting the function's role in evaluating expressions within the current phase.Also applies to: 275-316
| applyInEnv old_env (FO (FOClosure {..})) value = | ||
| let env = Env.insert _closureVar | ||
| _closureIdent | ||
| value | ||
| (_closureEnv <> old_env) | ||
| in evaluateIn env _closureBody | ||
| applyInEnv _ (HO prim) value = return $! prim value | ||
|
|
||
| apply :: Closure -> Value -> Either EState Value | ||
| apply (FO (FOClosure {..})) value = |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor applyInEnv and apply to reduce code duplication
The functions applyInEnv and apply share similar logic, with the primary difference being the environment used. To adhere to the DRY (Don't Repeat Yourself) principle, consider refactoring these functions by extracting the common logic into a helper function. This will improve maintainability and readability.
Example refactor:
-- Extract common logic into a helper function
applyClosure :: VEnv -> Closure -> Value -> Either EState Value
applyClosure env (FO (FOClosure {..})) value =
let newEnv = Env.insert _closureVar _closureIdent value env
in evaluateIn newEnv _closureBody
applyClosure _ (HO prim) value = return $! prim value
-- Refactor applyInEnv
applyInEnv :: VEnv -> Closure -> Value -> Either EState Value
applyInEnv oldEnv closure value = applyClosure (_closureEnv closure <> oldEnv) closure value
-- Refactor apply
apply :: Closure -> Value -> Either EState Value
apply closure value = applyClosure (_closureEnv closure) closure value| projectError _ = error "debugger: impossible" | ||
|
|
There was a problem hiding this comment.
Handle all EState cases in projectError
The projectError function currently throws an error if called with an EState that is not Er. Since projectError might be used in contexts where the state isn't guaranteed to be an error state, handling all cases prevents unexpected crashes.
Consider pattern matching all constructors or documenting the function's precondition.
projectError :: EState -> Maybe EvalError
projectError (Er err _env _k) = Just err
projectError _ = Nothing| projectError (Er err _env _k) = err | ||
| projectError _ = error "debugger: impossible" | ||
|
|
There was a problem hiding this comment.
Avoid partial functions to prevent runtime exceptions
The projectError function uses a partial pattern match that calls error when the EState is not an Er state. Relying on partial functions can lead to runtime exceptions. Consider refactoring projectError to return a Maybe EvalError or handle all possible EState cases to improve robustness.
Refactored code suggestion:
-projectError :: EState -> EvalError
+projectError :: EState -> Maybe EvalError
-projectError (Er err _env _k) = err
-projectError _ = error "debugger: impossible"
+projectError (Er err _env _k) = Just err
+projectError _ = Nothing📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| projectError (Er err _env _k) = err | |
| projectError _ = error "debugger: impossible" | |
| projectError :: EState -> Maybe EvalError | |
| projectError (Er err _env _k) = Just err | |
| projectError _ = Nothing |
| let env = Env.insert _closureVar _closureIdent value (_closureEnv <> e) | ||
| in Down (unCore _closureBody) env k | ||
| app (HO prim) = Up (prim value) mempty k |
There was a problem hiding this comment.
Correct environment usage in closure application
In applyAsClosure, the environment combines the closure's environment with the current environment using (_closureEnv <> e). This could lead to scoping issues, where variables from the caller's environment unintentionally affect the closure's execution. To maintain proper lexical scoping, only the closure's environment should be used.
Apply this diff to fix the environment:
-let env = Env.insert _closureVar _closureIdent value (_closureEnv <> e)
+let env = Env.insert _closureVar _closureIdent value _closureEnv📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let env = Env.insert _closureVar _closureIdent value (_closureEnv <> e) | |
| in Down (unCore _closureBody) env k | |
| app (HO prim) = Up (prim value) mempty k | |
| let env = Env.insert _closureVar _closureIdent value _closureEnv | |
| in Down (unCore _closureBody) env k | |
| app (HO prim) = Up (prim value) mempty k |
| @@ -6,6 +6,7 @@ | |||
| {-# LANGUAGE TemplateHaskell #-} | |||
| {-# LANGUAGE ViewPatterns #-} | |||
| {-# LANGUAGE DerivingStrategies #-} | |||
| {-# LANGUAGE FlexibleContexts #-} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Confirm the necessity of FlexibleContexts extension
The addition of {-# LANGUAGE FlexibleContexts #-} enables more flexible type class constraints. Please ensure this extension is required for the current changes. If it's unnecessary, consider removing it to keep the codebase clean from redundant extensions.
No worries, it was a thorough review! |
- on track for gelisam#108
doyougnu
force-pushed
the
doyougnu/wip/useful-repl
branch
from
cac10f2 to
ec6d08c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
src/Evaluator.hs (3)
11-48: Documentation needs clarification about Control (C) in CEK.The note explains Environment (E) and Kontinuation (K) well, but doesn't clearly explain what Control (C) represents in line 16. This is particularly important as line 43 states "All interesting things happen by matching on 'Kont'", which seems to contradict the importance of Control.
Consider expanding the documentation to clarify:
- What Control represents in the CEK machine
- How Control interacts with Kontinuation
- Why most operations focus on Kontinuation despite Control being part of CEK
115-170: Enhance Kont constructor documentation and naming.While the Kont data type is well-structured, it could benefit from:
- Comprehensive documentation for each constructor
- More consistent naming conventions
Consider:
- Adding documentation for each constructor explaining its purpose and invariants
- Renaming
InLocandInLocStxtoInReplaceLocLandInReplaceLocRfor clarity- Using consistent naming patterns (e.g.,
InCtorArginstead ofInCtor)
189-196: Address performance degradation concerns.The CEK interpreter shows significant performance issues (59.7% bad speculation). This could be due to:
- Complex pattern matching in the
stepfunction causing branch mispredictions- Environment combinations creating allocation pressure
- Insufficient strictness annotations
Consider:
- Adding more strictness annotations using BangPatterns
- Simplifying pattern matching by splitting the
stepfunction- Using a more efficient environment representation
- Adding benchmarks to track performance improvements
src/Expander/Monad.hs (2)
864-871: Consider performance optimizations for evaluation.The current implementation might be contributing to the branch predictor issues mentioned in the PR (59.7% bad speculation). Consider these optimizations:
- Add strictness annotations to force early evaluation of
envandout- Consider using continuation-passing style to reduce branching
Apply this diff to add strictness:
evalInCurrentPhase :: Core -> Expand Value evalInCurrentPhase evalAction = do - env <- currentEnv - let out = evaluateIn env evalAction + !env <- currentEnv + let !out = evaluateIn env evalAction case out of Left err -> do p <- currentPhase
Line range hint
873-890: Optimize environment combination for better performance.The current implementation of environment combination could be optimized:
- Use strict pattern matching for environment lookups
- Consider caching the combined environment if it's frequently accessed
Apply these changes to optimize environment handling:
currentEnv :: Expand VEnv currentEnv = do phase <- currentPhase - globalEnv <- fromMaybe mempty . view (expanderWorld . worldEnvironments . at phase) <$> getState - localEnv <- fromMaybe mempty . view (expanderCurrentEnvs . at phase) <$> getState + !globalEnv <- fromMaybe mempty . view (expanderWorld . worldEnvironments . at phase) <$> getState + !localEnv <- fromMaybe mempty . view (expanderCurrentEnvs . at phase) <$> getState return $! globalEnv <> localEnvsrc/Expander.hs (3)
908-920: Enhance error messages for better debugging.While the error handling is correct, consider enhancing the error messages to include:
- The expected and actual types
- The phase context where the error occurred
- The evaluation context
This would make debugging easier, especially given the complex nature of the CEK evaluator.
- throwError - $ MacroEvaluationError p - $ projectError err + throwError + $ MacroEvaluationError p + $ addContext "Type case evaluation" + $ projectError err
Line range hint
935-1001: Standardize error handling in macro evaluation.As noted in past reviews, the error handling here should be consistent with the rest of the codebase. Consider:
- Using
MacroEvaluationErrorinstead ofValueNotMacrofor evaluation errors- Providing more context in error messages
- Ensuring error types align with their semantic meaning
- throwError $ MacroEvaluationError p $ evalErrorType "macro action" other + throwError $ MacroEvaluationError p $ + addContext "Macro evaluation" $ + evalErrorType "macro action" other
After analyzing the evaluation code, I can now provide a specific response about the branching patterns and performance concerns:
Consider restructuring pattern matching evaluation for better branch prediction
The evaluator shows several areas where branch prediction could be impacted:
- Deeply nested pattern matching in
matchfunctions with multiplecaseexpressions that fall through tonexthandlers- Type-based dispatch patterns in
evalAs*functions that could be reorganized- Scattered error handling paths that may interfere with the CPU's ability to predict the common case
Specific suggestions:
- Consider flattening the nested pattern matches in the
matchfunctions by using helper functions- Reorder the cases to put the most common patterns first
- Group similar patterns together to help the branch predictor recognize patterns
- Consider using lookup tables or other techniques for the type-based dispatching
🔗 Analysis chain
Line range hint
275-302: Consider optimizing evaluation for better branch prediction.Given the performance metrics in the PR description (59.7% bad speculation), consider:
- Reducing conditional branching in the evaluation path
- Making the evaluation pattern more predictable
- Using strict evaluation where possible to avoid thunk creation
This could help address the branch predictor issues and improve the overall performance.
Let me analyze the branching patterns in the evaluation code more carefully to verify the performance concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Analyze branching patterns in evaluation code rg -A 5 'case|if|>>=|=<<|>>|<<' | grep -v '^--' | grep -v '^$'Length of output: 337368
Script:
#!/bin/bash # Look for branching patterns specifically in the evaluator code rg -A 5 'case|if|>>=|=<<|>>|<<' src/Evaluator.hsLength of output: 8016
📜 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(12 hunks)src/Expander/Monad.hs(3 hunks)src/Pretty.hs(1 hunks)
🔇 Additional comments (7)
repl/Main.hs (2)
149-151:
Performance degradation needs investigation.
The new CEK evaluator shows significant performance issues, particularly with branch prediction (59.7% bad speculation). Consider the following recommendations:
- Profile the
evaluateInfunction to identify bottlenecks - Consider adding strictness annotations to reduce thunk buildup
- Investigate opportunities for fusion optimization
- Consider adding bang patterns to force evaluation where appropriate
Let's analyze the evaluation patterns:
149-151: Verify the evaluation environment setup.
The phaseEnv runtime currentWorld setup needs verification to ensure it's correctly initialized for the CEK evaluator.
Let's check the environment setup:
✅ Verification successful
Let me gather more information about the evaluation environment setup.
Let me check one more aspect about the World initialization and environment setup.
CEK evaluator environment setup is correctly implemented
The evaluation environment is properly initialized and structured:
- The
phaseEnvcorrectly extracts environment for runtime phase (Phase 0) - The World structure maintains separate environments per phase
- The evaluation environment is properly passed to the CEK evaluator through
evaluateIn - The environment initialization chain (World -> IORef -> currentWorld -> phaseEnv) is complete
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the phase environment initialization
# Test: Look for phaseEnv usage patterns
echo "=== Phase environment initialization ==="
rg 'phaseEnv\b.*runtime' -A 5
# Test: Look for runtime phase definitions
echo "=== Runtime phase definitions ==="
ast-grep --pattern 'runtime = $_'
Length of output: 540
Script:
#!/bin/bash
# Look for evaluateIn implementation and usage
echo "=== evaluateIn implementation ==="
ast-grep --pattern 'evaluateIn $_'
# Look for Phase environment setup
echo "=== Phase environment setup ==="
rg "phaseEnv" -A 3
# Look for currentWorld usage
echo "=== currentWorld usage ==="
rg "currentWorld" -A 3
Length of output: 1540
Script:
#!/bin/bash
# Look for World initialization and environment setup
echo "=== World initialization ==="
rg "theWorld.*<-" -A 3
# Look for World environment structure
echo "=== World environment structure ==="
ast-grep --pattern 'data World'
# Look for Env implementation
echo "=== Env implementation ==="
rg "type Env" -A 3
Length of output: 1611
src/Pretty.hs (1)
627-627: LGTM! Clear and consistent error message formatting.
The implementation follows good practices by:
- Using consistent formatting with other error messages
- Providing clear and descriptive error text
- Properly utilizing the pretty-printing combinators
src/Evaluator.hs (2)
1-10: LGTM! Language extensions are well-chosen.
The added language extensions are appropriate for implementing a CEK interpreter:
KindSignaturesenables explicit kind annotationsBangPatternshelps with performance by enforcing strict evaluationGADTSyntaxprovides a clear and type-safe way to represent interpreter states
662-665:
Make projectError total to prevent runtime errors.
The function is partial and could crash at runtime with non-error states.
Replace with a total function:
-projectError :: EState -> EvalError
-projectError (Er err _env _k) = err
-projectError _ = error "debugger: impossible"
+projectError :: EState -> Maybe EvalError
+projectError (Er err _env _k) = Just err
+projectError _ = NothingLikely invalid or redundant comment.
src/Expander/Monad.hs (1)
Line range hint 902-908: LGTM: Clean implementation of kind inference scheduling.
The function follows good practices:
- Creates a fresh kind variable
- Properly schedules type expansion
- Maintains consistency with the module's patterns
src/Expander.hs (1)
25-25: LGTM: Clear and descriptive function renaming.
The renaming from expandEval to evalInCurrentPhase better reflects the function's purpose and behavior.
| case evaluateIn (phaseEnv runtime currentWorld) expr of | ||
| Left evalErr -> print $ erroneousValue $ projectError evalErr | ||
| Right val -> prettyPrintLn val |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding performance metrics logging.
Given the performance concerns, it would be helpful to add optional performance logging to track evaluation time and memory usage.
Add performance metrics with this diff:
currentWorld <- readIORef theWorld
+ start <- getCurrentTime
case evaluateIn (phaseEnv runtime currentWorld) expr of
- Left evalErr -> print $ erroneousValue $ projectError evalErr
- Right val -> prettyPrintLn val
+ result -> do
+ end <- getCurrentTime
+ let duration = diffUTCTime end start
+ when (duration > 0.1) $ -- Log slow evaluations
+ hPutStrLn stderr $ "Evaluation took: " ++ show duration
+ case result of
+ Left evalErr -> print $ erroneousValue $ projectError evalErr
+ Right val -> prettyPrintLn valCommittable suggestion was skipped due to low confidence.
| where app (FO (FOClosure{..})) = | ||
| let env = Env.insert _closureVar _closureIdent value (_closureEnv <> e) | ||
| in Down (unCore _closureBody) env k | ||
| app (HO prim) = Up (prim value) mempty k |
There was a problem hiding this comment.
Review environment handling in closure application.
The current implementation in applyAsClosure combines environments with (_closureEnv <> e), which could lead to unexpected variable shadowing and potential performance issues.
Consider:
- Using only the closure's environment:
_closureEnvinstead of(_closureEnv <> e) - Adding a comment explaining the environment combination if it's intentional
- Investigating if this contributes to the reported performance issues (59.7% bad speculation)
| val <- evalInCurrentPhase definiens | ||
| modifyState $ over (expanderCurrentEnvs . at p) $ | ||
| \case | ||
| Nothing -> Just $ Env.singleton x n val | ||
| Just env -> Just $ env <> Env.singleton x n val | ||
| \case | ||
| Nothing -> Just $ Env.singleton x n val | ||
| Just env -> Just $ env <> Env.singleton x n val |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using strict environment updates.
The current environment update pattern could be optimized by:
- Using strict pattern matching
- Pre-allocating environment capacity
- Using strict fields in environment data structures
This could help reduce memory churn and improve performance.
- modifyState $ over (expanderCurrentEnvs . at p) $
- \case
- Nothing -> Just $ Env.singleton x n val
- Just env -> Just $ env <> Env.singleton x n val
+ modifyState $ over (expanderCurrentEnvs . at p) $ \case
+ Nothing -> Just $! Env.singleton x n val
+ Just env -> Just $! env <> Env.singleton x n val📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val <- evalInCurrentPhase definiens | |
| modifyState $ over (expanderCurrentEnvs . at p) $ | |
| \case | |
| Nothing -> Just $ Env.singleton x n val | |
| Just env -> Just $ env <> Env.singleton x n val | |
| \case | |
| Nothing -> Just $ Env.singleton x n val | |
| Just env -> Just $ env <> Env.singleton x n val | |
| val <- evalInCurrentPhase definiens | |
| modifyState $ over (expanderCurrentEnvs . at p) $ \case | |
| Nothing -> Just $! Env.singleton x n val | |
| Just env -> Just $! env <> Env.singleton x n val |
| macroVal <- inEarlierPhase | ||
| $ pure | ||
| $ apply macroImpl | ||
| $ ValueSyntax | ||
| $ addScope p stepScope stx | ||
| case macroVal of | ||
| ValueMacroAction act -> do | ||
| res <- interpretMacroAction prob act | ||
| case res of | ||
| Left err -> throwError | ||
| $ ValueNotMacro | ||
| $ erroneousValue | ||
| $ projectError err | ||
| Right mv -> case mv of | ||
| ValueMacroAction act -> | ||
| interpretMacroAction prob act >>= \case | ||
| StuckOnType loc ty env cases kont -> | ||
| forkAwaitingTypeCase loc prob ty env cases kont | ||
| Done expanded -> | ||
| case expanded of | ||
| ValueSyntax expansionResult -> | ||
| forkExpandSyntax prob (flipScope p stepScope expansionResult) | ||
| other -> throwError $ ValueNotSyntax other | ||
| other -> | ||
| throwError $ ValueNotMacro other | ||
| other -> | ||
| throwError $ ValueNotMacro other |
There was a problem hiding this comment.
Improve phase handling in macro expansion.
Based on past review discussions about phase handling:
- The
inEarlierPhaseusage needs to be consistent - Phase transitions should be more explicit
- Consider adding phase assertions for debugging
- macroVal <- inEarlierPhase
- $ pure
- $ apply macroImpl
+ macroVal <- inEarlierPhase $ do
+ assertPhase "Macro expansion"
+ pure $ apply macroImplCommittable suggestion was skipped due to low confidence.
| view (expanderWorld . worldEnvironments . at phase) $ s | ||
| case applyInEnv env closure boundResult of | ||
| -- FIXME DYG: what error to throw here | ||
| Left err -> throwError | ||
| $ ValueNotMacro | ||
| $ erroneousValue | ||
| $ projectError err | ||
| Right v -> | ||
| case v of | ||
| ValueMacroAction act -> interpretMacroAction prob act | ||
| other -> throwError $ ValueNotMacro other |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify environment application logic.
As noted in past reviews, this code contains redundant environment handling. Consider:
- Removing the redundant environment lookup and
withEnvcall - Simplifying the error handling path
- Making the control flow more direct
- phase <- view (expanderLocal . expanderPhase)
- s <- getState
- let env = fromMaybe Env.empty .
- view (expanderWorld . worldEnvironments . at phase) $ s
- case applyInEnv env closure boundResult of
+ case apply closure boundResult ofCommittable suggestion was skipped due to low confidence.
Still a draft but all but one test is passing.
TODOs:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Style