basic stack traces #261
basic stack traces #261
Conversation
WalkthroughThis pull request introduces extensive updates across example files and source modules. New error reporting formats and structured stack traces have been added to several golden test files and code examples. A new Debugger module is introduced alongside modifications in the Evaluator, Expander, Pretty, and Value modules to shift error handling from throwing exceptions to logging debug information. Additionally, various closure identifiers have been renamed for greater clarity, and the development environment has been updated via flake.nix and klister.cabal. The REPL options in Main.hs have also been refined. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Expander
participant Debugger
participant Evaluator
Caller->>Expander: Execute code with potential errors
Expander->>Debugger: Log error (debug call)
Debugger-->>Expander: Provide error context
Expander->>Evaluator: Forward updated error state
Evaluator-->>Caller: Return result or detailed error information
Suggested labels
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (57)
examples/non-examples/error.golden (1)
1-5: Great improvement in formatting the error messageThe transition to a multi-line format is much clearer and easier to read. As a further enhancement, consider adding more detail to the stack trace—such as which function or binding triggered the error—to give users deeper insight into the error’s origin.
examples/tiny-types.golden (2)
3-4: Consistent indentation recommended
The formatting change increases clarity by separating the annotation and syntax symbol onto separate lines. However, consider using consistent indentation (e.g., 2 or 4 spaces) rather than a single leading space for<foo>to improve readability across the file.
5-6: Maintain uniform spacing for annotations and syntax symbols
Similar to lines 3-4, this change clarifies the code’s meaning, but line 6 uses additional leading spaces (six spaces) compared with other lines. Adopting a standard indentation convention will help maintain an overall uniform look.examples/primitives-documentation.golden (2)
31-33: Consider aligning or grouping the type variables consistently.
Currently,∀(α : *)is on a separate line from(β : *), which may be inconsistent with the style used elsewhere. Consolidate them if you prefer a uniform approach.
53-55: Maintain consistent line breaks for type variables.
Like forbind-IO, grouping both(α : *)and(β : *)on a single line might keep it uniform with other similar signatures.examples/non-examples/stack-traces/error-in-cons-head.golden (1)
1-2: Consider adding a more descriptive message for clarity.
The current error message shows "Unknown" but doesn't specify what went wrong or how to fix it. Including further context (e.g., the type of error, expected vs. actual) can help users quickly diagnose the issue.examples/non-examples/stack-traces/error-in-bind-head.kl (1)
1-3: Confirm the intended error-handling behavior in monadic context.
This example returns the result of(pure x)after an error occurs. Clarify if this demonstration accurately reflects how>>=should propagate errors or if additional handling is required.examples/non-examples/stack-traces/.#error-in-cons-head.golden (1)
1-1: Potentially sensitive file path.
Revealing an absolute path in error messages can expose user-specific environment details. Consider redacting or abstracting these paths if they are not vital to debugging.examples/non-examples/stack-traces/error-in-cons-head.kl (1)
1-10: Consider utilizing the function argument and addressing the TODO.Here,
(define fail (lambda (thing) (car '(1 2 'something-else))))never uses thethingparameter, which may be confusing. If it’s intentional, consider documenting why. Also, the file has a TODO about testing pairs—ensure these tests are added for thorough coverage.Would you like me to draft a minimal test for pair scenarios?
examples/non-examples/stack-traces/error-in-case-constructor.kl (2)
5-7: Consider clarifying the purpose of this large Alphabet type.
This type enumerates a range of letters, which may be overkill unless you're testing or demonstrating patterns. If this is purely for illustrative error handling, clarify or reduce to only the necessary constructors.
15-15: Provide a test or commentary for the(example ...)invocation.
If(fail (l))is designed to trigger an error, consider explaining how this example is verified (e.g., via a golden file or an automated test).examples/non-examples/stack-traces/error-in-constructor.kl (3)
5-7: Evaluate necessity of Pair type in this demo.
If you only need thepairconstructor for an error demonstration, consider referencing a simpler or pre-existing type to keep this example self-contained. Alternatively, add clarity on why a new type is introduced.
8-11: Add an inline comment explaining how fst is tested.
Given that this example focuses on error handling forfail, clarify thatfstis a utility. Document or reference where its behavior is validated.
20-20: Consider expanded testing for(fail 23).
If this example is meant to produce a deterministic error trace, add explanation or a golden file capturing the exact stack trace. This ensures clarity and consistency in the demonstration.Would you like me to provide a script or snippet that checks for a matching error message in your golden file tests?
examples/non-examples/stack-traces/error-in-bind-tail.kl (2)
4-6: Potential mismatch with thesyntax-errorusage.Within a monadic setting, calling
(syntax-error x)directly in the continuation may not always yield the desired effect. Depending on howsyntax-erroris implemented, it could short-circuit the evaluation or trigger an upstream macro expansion issue.If you’d like, I can investigate how
syntax-errorbehaves under monadic bindings by searching for its definition or usage across the codebase. Let me know if you want me to open a GitHub issue or propose a direct fix.
7-7: Clarify the TODO.It appears the TODO comment is asking why
(syntax-error x)does not propagate as expected. Capturing or clarifying that logic may involve analyzing the macro expansion flow or ensuring the error is thrown in the correct phase.I can assist by walking through the expansion phases or verifying the macro expansion to confirm if
(syntax-error x)is correctly recognized. Let me know if that’s helpful.src/Debugger.hs (2)
1-22: Consider documenting module usage and error-handling model.You've introduced a brand-new debugger module and monad but haven't provided high-level documentation for how end-users, or other contributors, should utilize it. Consider adding a top-level Haddock comment or README references discussing:
• The typical usage pattern ofDebug r e a.
• How errors are meant to be surfaced (viadebug,catch, etc.).
• Basic examples or recommended best practices.
103-112: Consider including stack tracing details indebug.Your
MonadDebuggerimplementation logs errors usingdebug e, but does not appear to attach any additional context like location, call site, or partial state. Including more contextual details can drastically improve debugging experience, especially for advanced use-cases.src/Expander/TC.hs (8)
36-36: Check for additional error context.debug $ InternalError "Dangling type metavar"While this is a valid usage, consider providing more context (e.g., which type metavar is dangling) to make errors more actionable.
82-82: Verify the correctness of occurs-check error.debug $ TypeCheckError $ OccursCheckFailed ptr t'Ensure the
ptrand the normalizedt'are enough to diagnose the exact nature of the failed occurs-check. You might want location data or more descriptive text for better debugging.
113-113: Potential mismatch error detail.debug $ InternalError "Mismatch in number of type vars"If possible, include the expected vs. actual counts for clarity, preventing confusion for future maintainers and users.
198-198: Clarify generalization error.debug $ InternalError "Can't generalize in scheme"It's unclear why the code can't generalize here. Consider specifying the cause or the relevant scheme pointer to enhance debugging.
292-296: Include deeper type mismatch context if possible.debug $ TypeCheckError $ TypeMismatch loc e' r' Nothing ... debug $ TypeCheckError $ TypeMismatch loc outerE' outerR' (Just (e', r'))These error sites are crucial. If there's a possibility to retrieve surrounding type context or call site, it may improve support for diagnosing tricky type mismatches.
305-305: Add pointer reference to "Type variable not found!"debug $ InternalError "Type variable not found!"Consider including the missing pointer or relevant state. This can greatly reduce time to locate issues in large expansions.
327-327: Clarify kind mismatch.debug $ KindMismatch loc k1' k2'If you can, include more context or potential resolution steps in the error message. For instance, mention which type or constructor triggered the mismatch.
381-381: Consider alternative approach.debug $ InternalError "Tried to find kind in open context"If this scenario arises frequently, you may want to unify or infer the kind more gracefully rather than throwing an internal error. Evaluate if there's a legitimate fallback or partial solution.
src/Evaluator.hs (2)
162-169: Enhance documentation for new constructors.InCasePattern InDataCasePatternYou've introduced these pattern-specific constructors to track matched patterns in the stack trace. Ensure test coverage includes scenarios in which these new constructors appear, verifying correct debugging output.
703-706: GuardprojectErrorusage carefully.projectError (Er err _env _kont) = err projectError _ = error "projectError not used on an error!"Accidentally calling
projectErroron a non-error state raises an exception. ConsiderEitheror safer checks to avoid runtime crashes in unforeseen situations.src/Pretty.hs (4)
5-5: EnsureLambdaCaseextension usage is minimal.Using
LambdaCasecan improve conciseness, but if used heavily, it may reduce readability for some developers. Make sure usage is consistent and documented to keep code approachable.
113-114: Refine let-binding layout.hang 2 $ vsep [ text "let" <+> hang 2 ...While
hangandvsepproduce readable blocks, test against deeply nested structures or large let blocks to ensure it doesn't become unwieldy. You might prefer usinggroupor more advanced layout combinators for large expressions.
368-372: Tailor function arrow printing.(TyF TFun [a, b]) -> ...Here, you print the function arrow in a multi-line format. Verify this remains legible in large, curried function types. Some teams prefer single-line for short types, and multiline only when needed. Possibly consider
groupto handle multi-line gracefully.
810-812: Consider partial expansions for macros.(InBindMacroTl action _env k) -> ...Highlight partial expansions or placeholders in logs. If macros are partially expanded, having a marker in the final pretty print might help.
src/Expander/Monad.hs (9)
599-599: Investigate incomplete module.Nothing -> debug $ InternalError "Incomplete module after expansion"You might want a reference to
ptror module name for more clarity, helping identify which module is incomplete.
610-610: Missing decl detail.Nothing -> debug $ InternalError "Missing decl after expansion"Likewise, consider specifying the pointer or the expected declaration name for faster troubleshooting.
619-623: Missing expr or scheme references.Nothing -> debug $ InternalError "Missing expr after expansion" ... Nothing -> debug $ InternalError "Missing scheme after expansion"In each of these checks, incorporate the missing pointers or user-located code snippet references.
632-632: Enhance define macros message.Nothing -> debug $ InternalError "Missing expr after expansion"
DefineMacrosmight be trickier to debug if you have no info about which macro is missing. Consider including the macro’s name or pointer ID.
648-648: Adding pointer detail for missing types.debug $ InternalError "Missing type after expansion"If you have a pointer or constructor name, it would be an excellent reference in the error to help locate which type was lost.
668-668: Missing action detail.debug $ InternalError "Missing action after expansion"Similar to other missing expansions, referencing the pointer or call site helps track down the culprit.
785-785: Likewise for unknown datatype.debug $ InternalError $ "Unknown datatype " ++ show datatypeSame principle: referencing the unknown entity is helpful. Confirm that any place you look up this
datatypeincludes relevant phase or module context if multi-phase expansions are possible.
851-851: Preventing stuck expansions.debug (NoProgress (map (view _3) remainingTasks))Ensure that each
NoProgresscase is properly tested, and the user receives enough info to fix the stuck expansions. Possibly indicate the tasks themselves or current environment state.
867-870: Distinguishing macro evaluation failures.case evaluateIn env evalAction of Left e_state -> debug $ MacroEvaluationError p e_state Right val -> return valDifferentiate macro runtime errors from type or resolution issues if possible. This helps end-users identify macro-level logic errors vs. type mismatch expansions.
examples/non-examples/stack-traces/error-in-list.kl (2)
9-9: Rename the function for clarity or confirm its intended semantics.
failis misleading since it doesn't fail but instead increments its argument. Choose a more descriptive name such asincrementor confirm that this is essentially a "fail" for test scenarios.
11-12: Add tests to validatefailand error usage.A TODO remains regarding testing. Consider verifying that
(fail the-error)indeed produces an appropriate error scenario, and that(fail thing)gives the expected numeric result ifthingis numeric.src/StackTraces.hs (3)
29-32: Add environment context to printed error messages.
printStackprints the error but doesn't show contextual details about the environment or continuation. Consider includingprintEnv envor relevant details fromprintKont kto make debugging more thorough.
33-35: Add descriptive comments for complex continuations.Although
printKontuses alignment and vertical separation, it's not immediately clear how complex continuations are displayed. A comment or docstring explaining the structure of the printed output would be beneficial.
39-40: Consider additional environment details inprintEnv.
printEnvuses default pretty-printing. Evaluate whether you want more context (e.g., variable name-value pairs) in the output for debugging complex scenarios.repl/Main.hs (1)
150-150: Potential Enhancement for Error Reporting
Currently,evalErris printed directly. Consider providing more structured error information (e.g., stack traces or REPL hints) if the user is meant to debug runtime errors in-depth.src/Expander/Syntax.hs (2)
18-18: Debugger Import
ImportingDebuggerindicates an intent to shift from throwing errors to logging or debugging. Ensure consistent usage ofdebugacross this module and that there's no leftoverthrowError.
60-60: Import Error for Modules
Returningdebug (ImportError err)can help track modules not found. Consider logging the actual file path to aid debugging.src/Expander/Error.hs (6)
128-128:NotEmptyPretty Printer
Replacing old code withhang 2 $ vsep [...]. Keep consistent indentation for clarity in the final output.
136-136:NotInteger
Retain example or context in error messages for user clarity if possible.
141-141:NotString
Same comment as above—string literal info might help.
192-192:ValueNotSyntax
Consider logging what the actual value was for deeper insights.
218-218:NotValidType
If there's a partial explanation for why it's invalid, consider adding it to the error.
241-242:CircularImports
Just ensure that any large cycles produce manageable error logs.Would you like me to propose a limit on the depth or size of displayed cycles?
src/Expander.hs (2)
Line range hint
1392-1392: Address TODO for error message in module body handling.There's a TODO comment for implementing a proper error message for malformed module bodies.
Would you like me to help implement a proper error message for malformed module bodies? I can suggest an implementation or create a GitHub issue to track this task.
Line range hint
1-1447: Consider implementing a centralized error handling strategy.The codebase would benefit from a more systematic approach to error handling. Consider:
- Creating an error type hierarchy that covers all possible error cases
- Implementing a centralized error handling mechanism
- Using smart constructors for error creation to ensure consistency
- Adding error context information for better debugging
This would improve the consistency of error handling and make the stack traces more useful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (78)
examples/ambiguous-kind.golden(1 hunks)examples/bound-identifier.golden(1 hunks)examples/contract.golden(1 hunks)examples/datatype-macro.golden(1 hunks)examples/datatypes.golden(1 hunks)examples/defun-test.golden(1 hunks)examples/error.golden(1 hunks)examples/eta-case.golden(1 hunks)examples/failing-examples/bound-vs-free.golden(1 hunks)examples/failing-examples/incorrect-context.golden(1 hunks)examples/failing-examples/keyword-used-incorrectly.golden(1 hunks)examples/failing-examples/wrong-keyword.golden(1 hunks)examples/fix.golden(1 hunks)examples/hygiene.golden(1 hunks)examples/import-scoping.golden(1 hunks)examples/import.golden(1 hunks)examples/lambda-case-test.golden(1 hunks)examples/lang.golden(1 hunks)examples/let.golden(1 hunks)examples/lets.golden(1 hunks)examples/list-syntax.golden(1 hunks)examples/list-test.golden(1 hunks)examples/meta-macro.golden(1 hunks)examples/monad.golden(1 hunks)examples/non-examples/bad-lexical-env.golden(1 hunks)examples/non-examples/bad-lexical-env.kl(1 hunks)examples/non-examples/circular-1.golden(1 hunks)examples/non-examples/circular-2.golden(1 hunks)examples/non-examples/error.golden(1 hunks)examples/non-examples/import-phase.golden(1 hunks)examples/non-examples/missing-import.golden(1 hunks)examples/non-examples/stack-traces/.#error-in-cons-head.golden(1 hunks)examples/non-examples/stack-traces/error-in-arg.golden(1 hunks)examples/non-examples/stack-traces/error-in-arg.kl(1 hunks)examples/non-examples/stack-traces/error-in-bind-head.golden(1 hunks)examples/non-examples/stack-traces/error-in-bind-head.kl(1 hunks)examples/non-examples/stack-traces/error-in-bind-tail.golden(1 hunks)examples/non-examples/stack-traces/error-in-bind-tail.kl(1 hunks)examples/non-examples/stack-traces/error-in-case-constructor.golden(1 hunks)examples/non-examples/stack-traces/error-in-case-constructor.kl(1 hunks)examples/non-examples/stack-traces/error-in-cons-head.golden(1 hunks)examples/non-examples/stack-traces/error-in-cons-head.kl(1 hunks)examples/non-examples/stack-traces/error-in-constructor.golden(1 hunks)examples/non-examples/stack-traces/error-in-constructor.kl(1 hunks)examples/non-examples/stack-traces/error-in-let.golden(1 hunks)examples/non-examples/stack-traces/error-in-let.kl(1 hunks)examples/non-examples/stack-traces/error-in-list.golden(1 hunks)examples/non-examples/stack-traces/error-in-list.kl(1 hunks)examples/non-examples/stack-traces/error-in-pure-macro.golden(1 hunks)examples/non-examples/stack-traces/error-in-pure-macro.kl(1 hunks)examples/non-examples/stack-traces/in-arg-error.golden(1 hunks)examples/non-examples/stack-traces/in-arg-error.kl(1 hunks)examples/non-examples/type-errors.golden(1 hunks)examples/pmatch.golden(1 hunks)examples/prelude-test.golden(1 hunks)examples/primitives-documentation.golden(2 hunks)examples/product-type.golden(1 hunks)examples/quasiquote-syntax-test.golden(1 hunks)examples/syntax-loc.golden(1 hunks)examples/temporaries-test.golden(1 hunks)examples/test-quasiquote.golden(1 hunks)examples/tiny-types.golden(1 hunks)examples/two-defs.golden(1 hunks)examples/unknown-type.golden(1 hunks)examples/which-problem.golden(1 hunks)flake.nix(2 hunks)klister.cabal(2 hunks)repl/Main.hs(3 hunks)src/Debugger.hs(1 hunks)src/Evaluator.hs(10 hunks)src/Expander.hs(26 hunks)src/Expander/Error.hs(7 hunks)src/Expander/Monad.hs(13 hunks)src/Expander/Primitives.hs(5 hunks)src/Expander/Syntax.hs(6 hunks)src/Expander/TC.hs(9 hunks)src/Pretty.hs(22 hunks)src/StackTraces.hs(1 hunks)
✅ Files skipped from review due to trivial changes (40)
- examples/non-examples/circular-2.golden
- examples/failing-examples/wrong-keyword.golden
- examples/bound-identifier.golden
- examples/non-examples/circular-1.golden
- examples/lang.golden
- examples/failing-examples/incorrect-context.golden
- examples/non-examples/import-phase.golden
- examples/temporaries-test.golden
- examples/lets.golden
- examples/non-examples/missing-import.golden
- examples/non-examples/type-errors.golden
- examples/non-examples/bad-lexical-env.golden
- examples/eta-case.golden
- examples/failing-examples/keyword-used-incorrectly.golden
- examples/lambda-case-test.golden
- examples/two-defs.golden
- examples/datatype-macro.golden
- examples/error.golden
- examples/ambiguous-kind.golden
- examples/fix.golden
- examples/contract.golden
- examples/pmatch.golden
- examples/let.golden
- examples/defun-test.golden
- examples/failing-examples/bound-vs-free.golden
- examples/prelude-test.golden
- examples/list-test.golden
- examples/syntax-loc.golden
- examples/import-scoping.golden
- examples/quasiquote-syntax-test.golden
- examples/import.golden
- examples/meta-macro.golden
- examples/product-type.golden
- examples/hygiene.golden
- examples/list-syntax.golden
- examples/test-quasiquote.golden
- examples/datatypes.golden
- examples/monad.golden
- examples/which-problem.golden
- examples/unknown-type.golden
🔇 Additional comments (90)
examples/non-examples/bad-lexical-env.kl (2)
3-3: Potentially undefined variabley.Inside the function
f, the variableyis referenced but never defined in this scope or in a surrounding scope, which may produce a lexical error unless intentionally used to demonstrate the environment error mechanic. Verify that you indeed intend to reference a free variable here for testing or demonstration.
4-4: No argument passed tof.The function
gcallsfwithout passing any arguments, despitefhaving a parameter(ff). If this is intentional for error demonstration, it’s fine, but otherwise consider supplying the needed argument to avoid runtime errors.examples/tiny-types.golden (2)
10-11: Clear separation of annotations and identifiers
Moving<a>to a separate line makes the code structure more readable. This is a valuable improvement.
12-13: Good formatting clarification for free identifier<b>
Separating<b>onto its own line improves readability, matching the adjustments made for<a>. Nicely done!examples/primitives-documentation.golden (5)
29-30: Good separation of type variables to improve readability.
The new multiline format forpure-IO’s signature is clear and accurately expresses the type relationship.
45-46: No issues detected with thenothingsignature.
This multiline formatting is consistent and improves clarity.
47-48:nilsignature is clearly expressed.
No functional or stylistic concerns; this line break pattern matches the rest of the file.
51-52:idsignature is well-formatted.
No change to logic; looks good as is.
56-59:composesignature refactor is clear and readable.
Breaking out each type variable on its own line is understandable, but ensure it’s consistently applied throughout the file for clarity.examples/non-examples/stack-traces/error-in-pure-macro.kl (1)
1-3: Validate that the example effectively demonstrates the error scenario.
The example sets up a pure macro that triggers an error, which is good for ensuring that error handling logic is tested. Verify that any upstream or downstream code referencing this file treats the error as expected.examples/non-examples/stack-traces/in-arg-error.golden (1)
1-6: Good clarity and specificity in the error message.
The mismatch details are clearly itemized by showing the expected and actual types. This approach aids rapid debugging. If needed, consider unifying message formats project-wide for consistency.examples/non-examples/stack-traces/error-in-arg.kl (3)
1-2: Consistent language specification.
The usage of#lang "prelude.kl"is in line with the existing examples, so no changes are needed here.
3-3: Validate numeric input for(fail).
The function assumesthingis numeric. In tests or higher-level logic, ensure that calling(fail)with a non-numeric argument either raises a clear error or is validated beforehand.
5-5: Intentional error example looks good.
Invoking(error 'bad)is a useful demonstration for stack-trace behavior. No changes required.examples/non-examples/stack-traces/error-in-list.golden (1)
1-5: Clear and concise error message format.
This new format provides a straightforward location reference, an error description, and a stack trace. It’s consistent and easy to follow. No issues found.examples/non-examples/stack-traces/in-arg-error.kl (3)
1-2: Language specification confirmed.
The file correctly uses#lang "prelude.kl". No changes needed here.
3-3: Check numeric assumption for(fail).
As with the other example, confirm thatsomethingis numeric or handle invalid input gracefully.
5-5: Error scenario is properly illustrated.
This line shows the function raising an error to demonstrate stack-trace handling. Good for testing.examples/non-examples/stack-traces/error-in-arg.golden (1)
1-6: Structured error format looks consistent.The newly introduced error message and stack trace align with the improved style described in the PR objectives. This should help users quickly locate and diagnose the issue in
error-in-arg.kl.examples/non-examples/stack-traces/error-in-pure-macro.golden (1)
1-6: Good consistency in error framing.This example’s error output provides a clear message and a concise stack trace for a pure macro. Keeping a uniform style across all .golden files will aid debugging and maintain clarity.
examples/non-examples/stack-traces/error-in-case-constructor.golden (1)
1-6: Clear presentation of case-constructor error.This error and stack trace demonstrate a consistent style with the other .golden files. No further issues spotted.
examples/non-examples/stack-traces/error-in-let.kl (1)
1-7: Example snippet successfully demonstrates the error mechanism.The snippet correctly illustrates the behavior of throwing an error and halting the program flow. This is useful for verifying that the new stack trace enhancements are working as intended.
examples/non-examples/stack-traces/error-in-bind-head.golden (1)
1-9: Please verify accuracy of error references.The error message and stack trace are clearly structured. However, ensure that the file reference
error-in-bind-head.kl:3.29-3.30aligns precisely with the location of the failing expression.examples/non-examples/stack-traces/error-in-let.golden (1)
1-8: Maintain consistent error references.This new structured error output is aligned with other error reporting. As a suggestion, verify the file and line references (
error-in-let.kl:4.42-4.53and#[error-in-let.kl:4.22-4.24]) to ensure their accuracy in pointing to the offending code.examples/non-examples/stack-traces/error-in-constructor.golden (1)
1-9: Confirm constructor location and field references.The detailed stack trace is helpful. Double-check that the error location
error-in-constructor.kl:17.45-17.56and field index “2” in the constructorpairare indeed correct. Inconsistencies in trace indexing can cause confusion.examples/non-examples/stack-traces/error-in-case-constructor.kl (1)
9-13: Confirm the handling of(else the-other).
the-otherisn't defined in this snippet, implying it's either an unbound symbol or intentionally a placeholder. If it's a placeholder, consider naming it more clearly or documenting it for clarity.examples/non-examples/stack-traces/error-in-constructor.kl (1)
16-18: Double-check the error strategy infail.
Here,failconstructs a pair whose second element is always an error. If the goal is to illustrate a stack trace at construction, that’s fine; otherwise, consider deferring the error until the pair is accessed.examples/non-examples/stack-traces/error-in-bind-tail.golden (1)
1-5: Good addition to illustrate the error scenario in binding tails.This golden file snippet effectively demonstrates how an error is triggered and traced in the binding tail of a macro scenario. It’s helpful to see the exact points in the code (
#[error-in-bind-tail.kl:4.15-4.28]) where the failure occurs, aiding debugging and clarifying the error context. No issues detected; the changes are aligned with the PR’s goal of improving stack trace outputs.examples/non-examples/stack-traces/error-in-bind-tail.kl (2)
1-3: Solid demonstration setup for error handling tests.The inclusion of a
prelude.kllanguage mode and the(example ...)construct is appropriate for testing macro-related errors in a controlled manner. This ensures coverage for the tail-binding scenario referenced in the.goldenfile.
8-8: Neat final bind.Returning
(pure y)ensures the final result is well-formed if no error is triggered. Coupled with the preceding monadic steps, this lines up with the stated goals of testing stack traces and error handling in a tail-binding scenario.src/Debugger.hs (2)
45-47: Confirm thatdebugRunTis consistent with your monad design.While
debugRunTsimply flipsrunDebug, ensure that all code paths call it appropriately. If other modules expect a function with the signaturer -> Debug r e a -> IO (Either e a), confirm consistency so that you don’t inadvertently cause confusion when wrapping or unwrapping therunDebugfunction.
50-51: Validate themapDebugTtransformation.The function composes
fmapandsecond, suggesting transformation of both left (error) and right (value) sides. Verify that the intent is indeed to allow transformations of the successful result while preserving the error. If, for any reason, you need to transform the error side as well, you might consider a more comprehensive approach to mapping both sides of theEither.src/Expander/TC.hs (1)
20-20: Import check.The added
import Debuggeris consistent with your broader shift toward using theDebugmonad. Ensure all references to the old error-handling approach (if any remain) are removed to avoid confusion.src/Evaluator.hs (2)
256-258: Confirm that mediator logic is correct.(InCasePattern _ kont) -> Up v e kont (InDataCasePattern _ kont) -> Up v e kontYou’re passing the value
vup with no additional transformation or pattern checks. Confirm that skipping further case analysis (beyond capturing the pattern in the continuation) is intended behavior; if so, it makes sense, but you might want explicit docstrings explaining why no additional evaluation is needed here.✅ Verification successful
Let me gather more information about the case pattern handling in the evaluator.
The mediator logic for case patterns is correct and well-documented
The behavior is intentionally designed this way, as explained in the
Note [InCasePattern]comment found in the codebase:In case pattern is strictly not necessary, we could do this evaluation in the host's runtime instead of in the evaluator but doing so would mean that the debugger would not be able to capture the pattern that was matched.
The pattern matching and environment setup is actually done before reaching these continuation handlers, as evidenced by the
matchfunction calls where:
- The environment is properly extended with matched values
- The pattern is stored in the continuation for debugging purposes
- The continuation handlers (
InCasePatternandInDataCasePattern) simply pass the value up after the match is completeThis design ensures that the debugger can track which patterns were matched while keeping the actual pattern handling logic clean and efficient.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for InCasePattern and InDataCasePattern definitions and usages ast-grep --pattern 'data $_ = InCasePattern $$ | InDataCasePattern $$' # Look for comments or documentation about case pattern handling rg -B 2 -A 2 "InCasePattern|InDataCasePattern" # Search for Note about InCasePattern rg "\[InCasePattern\]"Length of output: 4429
707-710: Potential utility in partial states.projectKont (Er _ _ k) = k projectKont (Up _ _ k) = k projectKont (Down _ _ k) = kSimilar to
projectError, ensure that usage in partial states won't lead to misinterpretation. If other code might rely on the presence of a particular value in the continuation, consider clarifying the usage contract in docstrings.✅ Verification successful
Let me gather more information about the usage of
projectKontand related functions to verify the concern.
Let me search for more context about the EState type and its usage.
Based on the gathered information, I can now provide the final response:
projectKontis safely used in partial statesThe codebase shows that
EStateis a well-defined sum type with three constructors (Down,Up, andEr), each carrying a continuation (Kont) as their last parameter. TheprojectKontfunction is implemented correctly for all three constructors, and its usage pattern is consistent with the evaluation model:
- The evaluation step function (
step) handles all three states appropriately- The continuation is consistently used across state transitions
- Error states (
Er) preserve the continuation for proper error handling and stack traces- The function is used in conjunction with
projectErrorfor error reporting🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for projectKont usage and implementation rg "projectKont" -A 3 # Search for EState type definition and related types ast-grep --pattern 'data EState = $_' # Look for similar projection functions rg "project.*\s*::\s*EState" -A 2 # Search for functions that use the continuation (k) rg "k\s*=\s*projectKont" -A 3Length of output: 794
Script:
#!/bin/bash # Search for EState data type definition rg "data EState" -A 10 # Search for Kont type definition rg "type Kont" -A 3 # Search for functions that construct EState values rg "Er\s+|Up\s+|Down\s+" --type haskell -A 2 # Look for functions that pattern match on EState ast-grep --pattern 'case $_ of Er $$ -> $$ Up $$ -> $$ Down $$ -> $$'Length of output: 16311
src/Pretty.hs (2)
697-698: Validate new EState pretty-print approach.instance Pretty VarInfo EState where pp env st = printStack env stMake sure that
printStackprovides enough detail about the CEK machine state for typical debugging. If users often need more detail, you might consider more granular or layered printing options.
74-80: Check for layout configuration consistency.pretty x = renderStrict (layoutSmart defaultLayoutOptions (pp Env.empty x)) prettyPrint x = putDocW 80 (pp Env.empty x) prettyPrintLn x = putDocW 80 (pp Env.empty x) >> putStrLn ""You're using a mix of
layoutSmartandputDocW 80. Verify the chosen layout width aligns with your team’s style guidelines and remains consistent across all pretty-print calls.✅ Verification successful
Let me gather information about the layout configuration usage across the codebase.
Let me search for any configuration files or constants that might define layout settings.
Let me check the entire Pretty.hs file to understand the complete context of pretty printing usage.
Layout configuration is consistently used across pretty-printing functions
After analyzing the codebase, I can confirm that the mix of
layoutSmartandputDocW 80is intentional and correct:
prettyfunction useslayoutSmart defaultLayoutOptionsbecause it's for rendering toText, where smart layout decisions are beneficial for the output formatprettyPrintandprettyPrintLnuseputDocW 80because they're specifically for terminal output where a fixed width of 80 columns is a standard conventionThe layout configuration is consistent with their respective purposes:
- Text rendering: Smart layout for flexible output
- Terminal output: Fixed 80-column width for readability
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for layoutSmart and defaultLayoutOptions usage rg "layoutSmart|defaultLayoutOptions" -A 2 # Search for putDocW usage to check layout width configurations rg "putDocW" -A 2 # Search for any other pretty printing related configuration rg "layoutOptions|pageWidth" -A 2Length of output: 862
Script:
#!/bin/bash # Search for any layout-related constants or configurations rg -g '!*.lock' -g '!dist*' "pageWidth|layoutWidth|lineWidth|columns" -A 2 # Look for any pretty-printing related configuration files fd -e yaml -e toml -e json -e conf -e config . | xargs rg "layout|pretty|print|width" # Search for any module that might contain pretty-printing configurations fd "Pretty|Format|Layout" -t fLength of output: 211
Script:
#!/bin/bash # Get the complete content of Pretty.hs cat src/Pretty.hsLength of output: 28201
src/Expander/Monad.hs (4)
177-177: Confirm removal of legacy error handling.import DebuggerNow that you're pulling in
Debugger, ensure any leftover references toExceptTorthrowErrorare replaced throughout. Mixed usage can lead to confusion during expansions.
775-775: Unknown constructor.debug $ InternalError $ "Unknown constructor " ++ show ctorThis is more descriptive. Good job including the failing constructor's name. Ensure coverage for datatypes as well.
929-929: Handling circular imports.debug $ CircularImports ...You do mention how the cycle is composed. Good job. Ensure it’s easy for a user to see the chain of modules. Possibly advise a fix or an alternate approach to break the cycle.
208-212: Check monad transformer stacking.newtype Expand a = Expand { runExpand :: ReaderT ExpanderContext (Debug DebugContext ExpansionErr) a } deriving (Functor, Applicative, Monad, MonadIO, MonadDebugger ExpansionErr, MonadReader ExpanderContext)This is a significant shift to
Debug. Validate that all places formerly relying onMonadErrorsemantics now handle or interpretExpansionErrviadebug.✅ Verification successful
Let me gather information about the usage of
MonadErrorandDebugin the codebase.
Let me search for more specific error handling patterns to understand how errors are handled in the codebase.
Based on the gathered information, I can now provide the final response:
The shift to
Debugmonad is properly handled withMonadDebuggerinstancesThe codebase has been properly adapted to use the
Debugmonad transformer through theMonadDebuggertypeclass. The key findings show:
- All error handling previously done through
MonadErrorhas been systematically replaced withMonadDebugger'sdebugandcatchoperations.- The
Expandmonad correctly derivesMonadDebugger ExpansionErrinstance, ensuring proper error handling throughout the expander.- Error propagation is consistently handled via the
Debugmonad's semantics, with proper instances for transformer stacks (Reader, State).- The codebase shows systematic use of
debugfor error reporting across all expansion-related modules (Expander.Syntax, Expander.TC, etc.).🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for MonadError usage rg "MonadError" -A 3 # Search for Debug type and its usage rg "Debug\s+\w+\s+\w+" -A 3 # Search for ExpansionErr rg "ExpansionErr" -A 3 # Look for error handling patterns ast-grep --pattern 'throwError $_'Length of output: 4753
Script:
#!/bin/bash # Search for debug error handling patterns rg "debug\s+" -A 3 # Search for catch usage rg "catch\s+" -A 3 # Look for MonadDebugger type class definition ast-grep --pattern 'class MonadDebugger' # Search for error handling in Expand monad rg "Expand.*Error" -A 3Length of output: 24368
examples/non-examples/stack-traces/error-in-list.kl (1)
7-7: Ensure appropriate handling when calling(fail the-error).
the-erroris a symbolic error object, whilefailexpects a numeric argument. This will likely result in an error if these are combined at runtime. Consider adding type checks or clarifying usage to avoid unexpected failures.src/StackTraces.hs (1)
36-37: Ensure allEvalErrorvariants are displayed consistently.
printErrusespretty, but someEvalErrorvariants might need specialized formatting to provide optimal troubleshooting information. Verify that these variants are handled as intended.flake.nix (2)
24-24: Confirm thatcabal-installis necessary in all development scenarios.Including
cabal-installmay be desirable for local builds. Ensure that the rest of the team also installs or uses it consistently to avoid environment disparities.
55-57: Validate the KLISTERPATH environment variable.Setting
KLISTERPATHtoexamples/helps with local testing but may need refinement if code examples or tests exist in other directories. Confirm the correct path to ensure all examples are found.klister.cabal (1)
63-63: AddingDebuggeras an exposed module looks good.This new module is properly declared and consistent with the rest of the code changes aimed at improving error handling and debug support.
repl/Main.hs (4)
36-36: Encapsulation Benefit fromnewtype
SwitchingOptionsto anewtypeis a good practice, providing a more transparent type at runtime while maintaining a clear separation at the type level. This helps avoid accidental misuse of the underlying constructor.
37-38: Addition ofrunOptBindingInfo
The new boolean fieldrunOptBindingInfoshould be tested to verify its impact. If its usage influences output conditionally, ensure there's a test covering that path.
41-41: Consistency in Usingnewtype ReplOptions
UsingnewtypeforReplOptionsimproves type safety and consistency withOptions.
66-67: Subcommand Parser Readability
The parser updates increase readability by removing extra parentheses. Ensure help text and usage messages remain accurate for the "run" and "repl" commands.src/Expander/Syntax.hs (15)
6-6:TypeOperatorsLanguage Extension
EnablingTypeOperatorshere is common for more advanced type-level constructs. Confirm there's a team-wide agreement on using these language extensions for broad consistency.
28-28: Use ofdebug (NotIdentifier other)
Replacing error throws with debug calls avoids immediate failure but might degrade discoverability of certain logic mistakes. Review downstream usage to ensure essential errors still surface properly.
32-32: Empty List Check
debug (NotEmpty other)clarifies an expected shape. Double-check that any code relying on the older thrown exception pattern is updated to handle the new debug approach.
36-36: Cons Check
debug (NotCons other)fits the pattern for logging errors in Syntax checks. Ensure the user or developer receives appropriate feedback when syntax is invalid.
40-40: Cons-Cons Check
debug (NotConsCons other)is consistent with the new debugging approach. Confirm that the build or test suite covers scenarios that trigger this message.
45-45: List Check
debug (NotList other)—similarly be sure this debug message is tested so real syntax mistakes become discoverable.
49-49: Integer Check
debug (NotInteger other)might obscure type-level errors if not tested. Consider a test covering non-integer inputs.
53-53: String Check
debug (NotString other)continues the same logging approach. Keep consistency for user-facing messages.
65-65: Not a Module Name
debug (NotModName other)—the user might need to see the invalid name in context. Double-check that the diagnostic includes a helpful snippet.
78-78: List Length Error
debug (NotRightLength lengths stx)—helpful for understanding mismatched macro patterns. Ensure tests exist for multiple valid lengths.
80-80: Not a List
debug (NotList other)triggered if the shape is not aListat all. Same checks about coverage apply.
145-146:mustHaveShapefor Empty
debug (NotEmpty other)anddebug (NotList other)replaced older error approach. Please confirm call sites that rely on a thrown error are updated.
157-158:mustHaveShapefor Pairs
debug (NotCons other)anddebug (NotList other)—similar note on verifying code that depends on the older approach.
167-167:mustHaveShapefor Lists
debug (NotList other)might cause silent debugging logs if unhandled. Confirm there's an integration test for this scenario.
Line range hint
691-691:UnknownPattern
debug (UnknownPattern other)replaces a thrown error. Ensure there's a mechanism to handle unknown patterns gracefully or fail fast if needed in production.src/Expander/Error.hs (20)
3-3:LambdaCaseExtension
EnablingLambdaCaseis consistent with the rest of the codebase. No issues noted.
54-55:MacroEvaluationError&ValueNotMacro
Switching toEStateties them more closely to the new debugging framework. This can help track evaluation context in logs. Confirm all references to old error types are updated.
130-130:NotConsPretty Printer
Good alignment with the new debugging phrasing.
132-132:NotConsCons
All good. No suggestions beyond verifying final error string.
134-134:NotListPretty Printer
Clearer error messages are beneficial.
146-146:NotModName
Ensuring the expected module name gets clarified in error. Looks good.
151-151:NotRightLength
Thealtsfunction is a neat approach to handle multiple expected lengths. The error text is user-friendly.
166-166:NotVec
Indicates an expectation for square-bracketed syntax. The error message is now consistent with otherNot*style messages.
168-168:NotImportSpec
No concerns. The code is consistent with the new style.
170-170:NotExportSpec
Same note as above regarding consistency.
172-172:UnknownPattern
Ensures that unrecognized patterns become a known debugging scenario.
177-177:MacroRaisedSyntaxError
Displaying additional locations is helpful. Make sure there's no performance overhead if the location list grows large.
186-186:MacroEvaluationError
Now referencesEState. This design fits the new debugging approach well.
196-196:NotExported
It’s good you’re including the phase in the message. Ensure the location is well formatted for large codebases.
205-205:WrongSyntacticCategory
The structure is more streamlined.
207-211: Neat Separation of Expected vs. Supported
The use of hinges (hang) +vsepfor multi-line output is a nice improvement for error readability.
228-228:NotAConstructor
Error text for constructor mismatch is consistent with the new style.
236-238:KindMismatch
Usingmaybe (text "unknown location") ...elegantly handles possible missing source locations.
246-263:TypeMismatch
Good to see the more explicit detail in the error message. The “Specifically” block helps highlight submismatches.
269-270:OccursCheckFailed
The error text is direct, describing the pointer and type. All good.src/Expander/Primitives.hs (4)
76-76: Debugger Import
Consistent with the rest of the codebase’s shift to debugging.
522-522:WrongTypeArityHandling
Usingdebugensures the code logs an error rather than raising an exception. Confirm that calling code can proceed gracefully if the arity is wrong.
534-534: PatternedWrongTypeArity
Again, ensure that shifting from a thrown error to a debug call does not break any expected short-circuit behavior.
592-592:NotValidTypeinmakeLocalType
Previously, an invalid type might raise an exception. Now it's logged viadebug. Double-check any dependent logic that might assume an exception.src/Expander.hs (1)
61-61: LGTM! New import aligns with PR objectives.The addition of the
Debuggerimport supports the transition to improved stack trace functionality.
| Left err -> error (T.unpack (pretty err)) | ||
| Right vioy -> pure vioy |
There was a problem hiding this comment.
Replace direct error call with debug for consistent error handling.
The error handling here is inconsistent with the rest of the changes in the file. Other locations use the new debug functionality for error reporting.
Apply this diff to maintain consistency:
- Left err -> error (T.unpack (pretty err))
+ Left err -> debug $ MacroEvaluationError p errCommittable suggestion skipped: line range outside the PR's diff.
| debug $ InternalError "All modules should be identifier-headed" | ||
| DeclTreeDest {} -> | ||
| throwError $ InternalError "All declarations should be identifier-headed" | ||
| debug $ InternalError "All declarations should be identifier-headed" | ||
| TypeDest {} -> | ||
| throwError $ NotValidType stx | ||
| debug $ NotValidType stx | ||
| ExprDest t dest -> | ||
| case syntaxE stx of | ||
| List xs -> expandOneExpression t dest (addApp stx xs) | ||
| Integer n -> expandOneExpression t dest (addIntegerLiteral stx n) | ||
| String s -> expandOneExpression t dest (addStringLiteral stx s) | ||
| Id _ -> error "Impossible happened - identifiers are identifier-headed!" | ||
| PatternDest {} -> | ||
| throwError $ InternalError "All patterns should be identifier-headed" | ||
| debug $ InternalError "All patterns should be identifier-headed" | ||
| TypePatternDest {} -> | ||
| throwError $ InternalError "All type patterns should be identifier-headed" | ||
| debug $ InternalError "All type patterns should be identifier-headed" |
There was a problem hiding this comment.
Replace internal error calls with debug for consistent error handling.
Multiple internal errors are using direct error calls instead of the new debug functionality.
Apply this diff to maintain consistency:
- debug $ InternalError "All modules should be identifier-headed"
+ debug $ NotValidSyntax "All modules should be identifier-headed" stx
- debug $ InternalError "All declarations should be identifier-headed"
+ debug $ NotValidSyntax "All declarations should be identifier-headed" stx
- debug $ NotValidType stx
+ debug $ NotValidType stx
- Id _ -> error "Impossible happened - identifiers are identifier-headed!"
+ Id _ -> debug $ InternalError "Impossible happened - identifiers are identifier-headed!"
- debug $ InternalError "All patterns should be identifier-headed"
+ debug $ NotValidSyntax "All patterns should be identifier-headed" stx
- debug $ InternalError "All type patterns should be identifier-headed"
+ debug $ NotValidSyntax "All type patterns should be identifier-headed" stx📝 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.
| debug $ InternalError "All modules should be identifier-headed" | |
| DeclTreeDest {} -> | |
| throwError $ InternalError "All declarations should be identifier-headed" | |
| debug $ InternalError "All declarations should be identifier-headed" | |
| TypeDest {} -> | |
| throwError $ NotValidType stx | |
| debug $ NotValidType stx | |
| ExprDest t dest -> | |
| case syntaxE stx of | |
| List xs -> expandOneExpression t dest (addApp stx xs) | |
| Integer n -> expandOneExpression t dest (addIntegerLiteral stx n) | |
| String s -> expandOneExpression t dest (addStringLiteral stx s) | |
| Id _ -> error "Impossible happened - identifiers are identifier-headed!" | |
| PatternDest {} -> | |
| throwError $ InternalError "All patterns should be identifier-headed" | |
| debug $ InternalError "All patterns should be identifier-headed" | |
| TypePatternDest {} -> | |
| throwError $ InternalError "All type patterns should be identifier-headed" | |
| debug $ InternalError "All type patterns should be identifier-headed" | |
| debug $ NotValidSyntax "All modules should be identifier-headed" stx | |
| DeclTreeDest {} -> | |
| debug $ NotValidSyntax "All declarations should be identifier-headed" stx | |
| TypeDest {} -> | |
| debug $ NotValidType stx | |
| ExprDest t dest -> | |
| case syntaxE stx of | |
| List xs -> expandOneExpression t dest (addApp stx xs) | |
| Integer n -> expandOneExpression t dest (addIntegerLiteral stx n) | |
| String s -> expandOneExpression t dest (addStringLiteral stx s) | |
| Id _ -> debug $ InternalError "Impossible happened - identifiers are identifier-headed!" | |
| PatternDest {} -> | |
| debug $ NotValidSyntax "All patterns should be identifier-headed" stx | |
| TypePatternDest {} -> | |
| debug $ NotValidSyntax "All type patterns should be identifier-headed" stx |
There was a problem hiding this comment.
The implementation looks like a great application of the CEK-style evaluator!
Two things:
- The pretty-printer changes seem to be almost entirely regressions, grouping unrelated things and separating related things. They also make the diff harder to follow, because there's tons of noise in tests. Reverting most of the deletions of
groupshould fix that, I think. What problem was it solving? - Why is there a list of
EStates in a debug context? It seems to me that the debugger should get a single evaluator state.
| error-in-arg.kl:5.24-5.27: | ||
| bad | ||
| stack trace: | ||
| ---- with function #<closure> |
There was a problem hiding this comment.
Can we provide more info here? Perhaps closures can contain the source position of their corresponding lambda?
| @@ -0,0 +1,3 @@ | |||
| #lang "prelude.kl" | |||
|
|
|||
| (example (>>= (pure (error 'e)) (lambda (x) (pure x)))) | |||
There was a problem hiding this comment.
Missing newline!
| (>>= (pure 'hello-go-boom) | ||
| (lambda (x) | ||
| (>>= (syntax-error x) | ||
| -- TODO: why doesn't this work? |
There was a problem hiding this comment.
This is a strange error indeed!
src/Pretty.hs
Outdated
|
|
||
| printKont :: Env Var () -> Kont -> Doc VarInfo | ||
| -- the basics | ||
| printKont _ Halt = text "Halt" |
There was a problem hiding this comment.
In empty traces, this was a bit confusing. What about suppressing it, and then special-casing empty continuations at the outside?
src/Pretty.hs
Outdated
| <> pp e k | ||
|
|
||
| -- pairs | ||
| -- TODO: DYG: how to test the cons? |
There was a problem hiding this comment.
What about adding unsafeCoerce as a primitive for testing?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Expander.hs (1)
1333-1347: 🛠️ Refactor suggestionConsider using NotValidSyntax for better error reporting.
Replace
InternalErrorwithNotValidSyntaxfor better error messages when handling invalid syntax.Apply this diff to improve error reporting:
- debug $ InternalError "All modules should be identifier-headed" + debug $ NotValidSyntax "All modules should be identifier-headed" stx - debug $ InternalError "All declarations should be identifier-headed" + debug $ NotValidSyntax "All declarations should be identifier-headed" stx - debug $ InternalError "All patterns should be identifier-headed" + debug $ NotValidSyntax "All patterns should be identifier-headed" stx - debug $ InternalError "All type patterns should be identifier-headed" + debug $ NotValidSyntax "All type patterns should be identifier-headed" stx
🧹 Nitpick comments (3)
src/Evaluator.hs (3)
11-25: Consider enhancing the module documentation.While the module header is well-structured, it would benefit from additional details about:
- How the CEK machine state is converted to stack traces
- The relationship between debugging and stack traces
- Examples of stack trace output format
160-166: Consider documenting performance implications.While the Note [InCasePattern] explains the debugging benefits, it would be helpful to document:
- Performance overhead of evaluating patterns in the interpreter vs. host runtime
- Any benchmarking results if available
- Scenarios where this overhead might be significant
629-667: Consider refactoring pattern matching logic.The pattern matching implementation in
matchfunctions shows similar patterns that could be abstracted:
- Common pattern of extending environment and wrapping with
InCasePattern- Similar error handling paths
Consider introducing a helper function:
matchWithPattern :: SyntaxPattern -> Core -> Value -> VEnv -> Kont -> EState -> (VEnv -> EState) -> EState matchWithPattern pat rhs scrutinee env k next success = case (pat, scrutinee) of (SyntaxPatternIdentifier n x, v@(ValueSyntax (Syntax (Stx _ _ (Id _))))) -> success $ extend n x v env _ -> next
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
klister.cabal(1 hunks)src/Evaluator.hs(10 hunks)src/Expander.hs(26 hunks)src/Expander/Monad.hs(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- klister.cabal
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: macOS-latest / ghc 9.6
- GitHub Check: ubuntu-latest / ghc 9.2.5
- GitHub Check: ubuntu-latest / ghc 9.4
- GitHub Check: ubuntu-latest / ghc 9.6
- GitHub Check: stack / ghc 9.2.8
🔇 Additional comments (18)
src/Evaluator.hs (1)
583-593: Plan to remove stopgap error handling.The
constructErrorTypefunction is marked as a temporary solution. Consider:
- Creating a tracking issue for its removal
- Documenting the proper error handling approach that should replace it
- Adding tests to ensure consistent error handling behavior
Would you like me to help create a GitHub issue to track this technical debt?
src/Expander/Monad.hs (8)
177-177: LGTM!The Debugger module import is correctly placed and aligns with the new error handling approach.
208-212: LGTM! Monad transformation looks good.The change from
ExceptTtoDebugmonad is well-structured and maintains the required type class instances. TheMonadDebuggerinstance is correctly derived.
215-215: Verify handling of partial results in execExpand.While the change to use
runDebugis correct, ensure that partial expansions are properly handled.Run this script to check for potential partial expansion scenarios:
#!/bin/bash # Search for expansion tasks that might be incomplete rg -B 2 -A 2 "forkExpanderTask|stillStuck" src/
599-599: LGTM! Error handling is consistent.The error handling is properly updated to use
debugand provides a clear error message.
Line range hint
610-623: LGTM! Error handling is consistent.The error handling is properly updated to use
debugwith appropriate error messages for missing declarations and expressions.
632-632: LGTM! Error handling is consistent.The error handling for missing expressions in macro definitions is properly updated to use
debug.
657-661: LGTM! Enhanced error messages with location information.The error handling for missing expressions and schemes in examples is properly updated to use
debugand includes helpful source location information.
668-668: LGTM! Error handling is consistent.The error handling for missing actions is properly updated to use
debug.src/Expander.hs (9)
153-156: LGTM! Error handling is consistent.The error handling for file existence and reader errors is properly updated to use
debug.
199-199: LGTM! Error handling is consistent.The error handling for non-exported identifiers is properly updated to use
debug.
851-851: LGTM! Error handling is consistent.The error handling for no progress in tasks is properly updated to use
debug.
Line range hint
867-870: LGTM! Error handling is consistent.The error handling for macro evaluation errors is properly updated to use
debug.
915-919: LGTM! Error handling is consistent.The error handling for type case evaluation errors is properly updated to use
debug.
1324-1329: LGTM! Error handling is consistent.The error handling for macro expansion errors is properly updated to use
debugwith appropriate error messages for various failure cases.
1411-1417: LGTM! Error handling is consistent.The error handling for macro evaluation errors is properly updated to use
debugwith appropriate error messages.
1424-1432: LGTM! Error handling is consistent.The error handling for identifier comparison is properly updated to use
debugwith appropriate error messages and exception handling.
1440-1440: LGTM! Error handling is consistent.The error handling for invalid syntax objects is properly updated to use
debug.
doyougnu
force-pushed
the
doyougnu/stack-traces
branch
from
4de0c16 to
cc73f7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
examples/non-examples/stack-traces/error-in-case-constructor.kl (2)
9-13: Enhance error message for better debugging.While the function correctly demonstrates error handling, consider making the error message more descriptive to aid in debugging and align with the PR's goal of improving stack trace clarity.
Apply this diff to improve the error message:
(define fail (lambda (thing) (case thing - [(l) (error 'Im-an-error)] + [(l) (error 'Error-in-case-constructor-for-letter-l)] [(else the-other) the-other])))
15-15: Add more example cases for comprehensive testing.While the current example tests the error case, consider adding examples for successful cases to demonstrate the complete functionality.
Add these additional examples:
(example (fail (l))) +(example (fail (a))) ; should return (a) +(example (fail (z))) ; should return (z)src/Debugger.hs (3)
1-5: Consider reviewing necessity of advanced language extensions.
These pragmas enable advanced type-level features (e.g.,UndecidableInstancesandFunctionalDependencies). While they are valid for complex designs, please verify that each is essential to reduce potential maintenance overhead.
24-38: Export functions explicitly for better maintainability.
By providing an explicit export list, you help future maintainers see exactly which entities are meant to be public and can reduce namespace pollution.
120-133: Remove or finalize remaining TODO markers.
Lines 121–131 reference upcoming tasks. To keep the codebase clean, consider adding issue trackers or removing stale comments once addressed.src/Evaluator.hs (3)
70-72: Validate exportingEState(..),Kont(..), andVEnv.
Exposing internal constructors (e.g.,EStateandKont) can lead to external misuse. Consider providing intentionally restricted exports if future maintainers only need certain constructors.
160-167: Document new constructors extensively.
InCasePatternandInDataCasePatterntie directly into the debugger's stack trace. Provide doc comments indicating how they should be used and updated in code that manipulates continuations.
629-672: Maintain consistent bind between pattern and environment.
In thesedoCaseanddoDataCaseexpansions, new environment binds are conditionally extended based on pattern matches. It's usually correct, but be sure each fallback path adheres to the same environment assumptions to avoid partial pattern captures.src/Pretty.hs (3)
10-20: Revisit the public exports for clarity.
Multiple new symbols were added (e.g.,ppBind). If they are meant solely for internal usage or testing, consider making them private or documenting them.
216-216: Encourage additional type context inppBind.
Large patterns sometimes require more explicit type contexts to aid maintainers. A short comment clarifying the shape or purpose ofTyFwould be useful.
695-832: Consider user-friendly formatting for large stacks.
printStackandprintKontproduce verbose output. For deeper stacks, consider a condensed view or toggling detailed and brief modes for better user experience.src/Expander/Monad.hs (1)
Line range hint
610-670: Enhance error messages with source location information.The error messages for missing expressions and schemes after expansion now include source location information, which is helpful for debugging.
Consider adding more context to the error messages:
- Nothing -> debug $ InternalError "Missing decl after expansion" + Nothing -> debug $ InternalError $ "Missing declaration after expansion at " <> show locsrc/Expander.hs (1)
Line range hint
1224-1329: Enhance error messages in expandOneForm.The error messages for wrong argument count and syntactic categories could be more descriptive.
Consider adding more context to the error messages:
- then debug $ WrongArgCount stx ctor (length args') (length foundArgs) + then debug $ WrongArgCount stx ctor (length args') (length foundArgs) $ + "Expected " <> show (length args') <> " arguments but got " <> show (length foundArgs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
examples/non-examples/bad-lexical-env.golden(1 hunks)examples/non-examples/bad-lexical-env.kl(1 hunks)examples/non-examples/error.golden(1 hunks)examples/non-examples/stack-traces/.#error-in-cons-head.golden(1 hunks)examples/non-examples/stack-traces/error-in-arg.golden(1 hunks)examples/non-examples/stack-traces/error-in-arg.kl(1 hunks)examples/non-examples/stack-traces/error-in-bind-head.golden(1 hunks)examples/non-examples/stack-traces/error-in-bind-head.kl(1 hunks)examples/non-examples/stack-traces/error-in-bind-tail.golden(1 hunks)examples/non-examples/stack-traces/error-in-bind-tail.kl(1 hunks)examples/non-examples/stack-traces/error-in-case-constructor.golden(1 hunks)examples/non-examples/stack-traces/error-in-case-constructor.kl(1 hunks)examples/non-examples/stack-traces/error-in-cons-head.golden(1 hunks)examples/non-examples/stack-traces/error-in-cons-head.kl(1 hunks)examples/non-examples/stack-traces/error-in-constructor.golden(1 hunks)examples/non-examples/stack-traces/error-in-constructor.kl(1 hunks)examples/non-examples/stack-traces/error-in-let.golden(1 hunks)examples/non-examples/stack-traces/error-in-let.kl(1 hunks)examples/non-examples/stack-traces/error-in-list.golden(1 hunks)examples/non-examples/stack-traces/error-in-list.kl(1 hunks)examples/non-examples/stack-traces/error-in-pure-macro.golden(1 hunks)examples/non-examples/stack-traces/error-in-pure-macro.kl(1 hunks)examples/non-examples/stack-traces/in-arg-error.golden(1 hunks)examples/non-examples/stack-traces/in-arg-error.kl(1 hunks)flake.nix(2 hunks)klister.cabal(1 hunks)src/Debugger.hs(1 hunks)src/Evaluator.hs(11 hunks)src/Expander.hs(26 hunks)src/Expander/Error.hs(2 hunks)src/Expander/Monad.hs(13 hunks)src/Pretty.hs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (26)
- examples/non-examples/error.golden
- examples/non-examples/stack-traces/in-arg-error.kl
- examples/non-examples/stack-traces/error-in-case-constructor.golden
- examples/non-examples/stack-traces/error-in-pure-macro.golden
- examples/non-examples/stack-traces/error-in-cons-head.kl
- examples/non-examples/stack-traces/.#error-in-cons-head.golden
- examples/non-examples/stack-traces/error-in-bind-head.golden
- examples/non-examples/bad-lexical-env.kl
- flake.nix
- klister.cabal
- examples/non-examples/stack-traces/error-in-constructor.golden
- examples/non-examples/stack-traces/in-arg-error.golden
- examples/non-examples/stack-traces/error-in-arg.kl
- examples/non-examples/stack-traces/error-in-pure-macro.kl
- examples/non-examples/stack-traces/error-in-bind-tail.kl
- examples/non-examples/stack-traces/error-in-bind-tail.golden
- src/Expander/Error.hs
- examples/non-examples/stack-traces/error-in-list.golden
- examples/non-examples/stack-traces/error-in-cons-head.golden
- examples/non-examples/stack-traces/error-in-bind-head.kl
- examples/non-examples/stack-traces/error-in-arg.golden
- examples/non-examples/bad-lexical-env.golden
- examples/non-examples/stack-traces/error-in-let.golden
- examples/non-examples/stack-traces/error-in-list.kl
- examples/non-examples/stack-traces/error-in-let.kl
- examples/non-examples/stack-traces/error-in-constructor.kl
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: macOS-latest / ghc 9.6
- GitHub Check: ubuntu-latest / ghc 9.2.5
- GitHub Check: ubuntu-latest / ghc 9.4
- GitHub Check: ubuntu-latest / ghc 9.6
- GitHub Check: stack / ghc 9.2.8
🔇 Additional comments (21)
examples/non-examples/stack-traces/error-in-case-constructor.kl (1)
1-4: LGTM!The language declaration and import of
lambda-case.klare appropriate for implementing case-based error handling.src/Debugger.hs (4)
40-44: Validate lazy vs. strict IO usage inrunDebug.
DebugusesIO (Either e a). Please ensure that effect sequencing aligns with your debugger's needs, especially if you anticipate large computations or want to capture partial results.
73-79: Monad instance chaining logic resembles a previously noted concern.
The usage offmap (debugRunT r) f valwas flagged in a past review. Verify that it correctly threads therand does not bypass normal monadic composition.
106-111: StraightforwardMonadDebuggerinstance.
The implementation is clear and follows the expected pattern.
113-119: Ensure_stackTraceis consistently updated elsewhere.
DebugContextcarries a list ofEState, but no current code references how_stackTraceis appended. Confirm the tracer logic is attached whenever errors or debug scenarios occur.src/Evaluator.hs (2)
254-256: Revisit performance of "pass-through" states.
Even though these lines are meant to forward the value upward, ensure no redundant stepping or overhead can accumulate if frequently triggered.
713-720: 🛠️ Refactor suggestionAvoid partial functions.
projectErrorthrows a runtime error if called on a non-errorEState, which can hide logical mistakes. Similarly,projectKontduplicates such partial logic. Consider returningMaybe EvalErroror using pattern matching in the caller for safer code.-projectError _ = error "projectError not used on an error!" +projectError (Up _ _ _) = Nothing +projectError (Down _ _ _) = NothingLikely invalid or redundant comment.
src/Pretty.hs (2)
5-5: LambdaCase is generally beneficial for concise pattern matching.
Your use ofLambdaCaseis straightforward. No concerns here.
641-641: Good coverage forEvalResultprinting.
TheExampleResultformatting is clear, especially with the breakdown of associated data. Sufficient as is.src/Expander/Monad.hs (8)
208-212: LGTM! The monad definition changes look good.The transition from
ExceptTtoDebugmonad is well-implemented, with proper deriving of necessary typeclasses.
215-215: Verify execExpand's error handling.The function now uses
runDebugwithinitialContextinstead ofrunExceptT. Ensure that partial expansions are consistently handled.
599-599: LGTM! Error handling in getDeclGroup is consistent.The error message for incomplete module expansion is clear and follows the new error handling pattern.
775-775: LGTM! Constructor error handling is clear.The error message for unknown constructors is informative and follows the new error handling pattern.
785-785: LGTM! Datatype error handling is consistent.The error message for unknown datatypes follows the new error handling pattern.
851-851: LGTM! Task progress error handling is clear.The error message for no progress in tasks is well-structured and uses the new debug functionality.
867-870: LGTM! Macro evaluation error handling is consistent.The error handling for macro evaluation errors is well-implemented using the new debug functionality.
929-929: LGTM! Circular imports error handling is clear.The error message for circular imports is informative and includes the import stack for better debugging.
src/Expander.hs (4)
61-61: LGTM! Debugger import is correctly added.The import is necessary for the new error handling functionality.
153-156: LGTM! File loading error handling is clear.The error handling for file loading and reading is well-implemented using the new debug functionality.
199-199: LGTM! Import error handling is consistent.The error message for non-exported identifiers is clear and follows the new error handling pattern.
1411-1440: LGTM! Macro action error handling is consistent.The error handling for macro actions and identifier equality checks is well-implemented using the new debug functionality.
examples/non-examples/stack-traces/error-in-case-constructor.kl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
src/Pretty.hs (2)
711-747: Group related patterns inprintKont.Consider grouping related patterns (e.g., all case-related patterns) together for better code organization. Also, some TODOs need attention:
- Line 717: Body prints uniques instead of var names
- Line 731: Clarify if data/type case differs from case in concrete syntax
757-834: Address TODOs in pattern implementations.Several TODOs remain in the code:
- Line 757: Testing cons functionality
- Line 783: Reporting and testing ident-related patterns
- Line 803, 807, 813: Potential need for passthrough in macro-related patterns
Based on the PR objectives, removing remaining TODOs is part of the goals.
Would you like me to help implement solutions for these TODOs?
src/Evaluator.hs (4)
11-25: Fix incorrect module description.The module description states "Converting state from the CEK machine to stack trace" but this module actually implements the CEK machine itself. The description seems to be copied from another module.
160-168: Enhance documentation for InCasePattern.The note explains that InCasePattern is used for debugger capture, but it could be more precise about:
- What exactly is being captured
- How this information is used by the debugger
- Why this is better than host runtime evaluation
199-199: Consider custom Show instances for better debugging.While deriving Show is functional, custom Show instances could provide more readable and focused debug output by:
- Formatting continuations in a stack-like manner
- Showing relevant parts of the environment
- Providing concise error state representations
Also applies to: 211-211
631-670: Consider performance impact of pattern capture.While capturing patterns in continuations improves debugging, it may impact performance in production. Consider:
- Adding a compile-time flag to conditionally include pattern capture
- Using a more lightweight representation for patterns in production builds
examples/primitives-documentation.golden (1)
7-8: Address TODO placeholders.The placeholders
#<TODO:Jeff:WHAT-TO-PUT-HERE>need to be replaced with appropriate values for theabsandnegatefunctions.Would you like me to help implement the missing values for these functions?
src/Expander/Primitives.hs (1)
752-753: Improve closure identifiers for better stack traces.The closure identifiers have been updated, but some could be more descriptive:
"TODO:Jeff:WHAT-TO-PUT-HERE"needs a proper identifier"bil"and"bir"could be more descriptive"bsp-l"and"bsp-r"could be more descriptiveApply this diff to improve the identifiers:
- ValueClosure $ HO "TODO:Jeff:WHAT-TO-PUT-HERE" $ + ValueClosure $ HO "unary-integer-operator" $ - ValueClosure $ HO "bil" $ + ValueClosure $ HO "binary-integer-left" $ - ValueClosure $ HO "bir" $ + ValueClosure $ HO "binary-integer-right" $ - ValueClosure $ HO "bsp-l" $ + ValueClosure $ HO "binary-string-predicate-left" $ - ValueClosure $ HO "bsp-r" $ + ValueClosure $ HO "binary-string-predicate-right" $Also applies to: 758-761, 777-780
examples/which-problem.golden (1)
3-6: Consider using consistent closure naming.The changes use a mix of
#<_>and#<x>for closures:
#<_>for unused closures in lines 3-4- Mix of
#<x>and#<_>in thebothfunction in lines 5-6Consider using
#<_>consistently for all unused closures to maintain a clear convention.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
examples/contract.golden(1 hunks)examples/datatypes.golden(1 hunks)examples/error.golden(1 hunks)examples/eta-case.golden(1 hunks)examples/fix.golden(1 hunks)examples/higher-kinded.golden(1 hunks)examples/implicit-conversion-test.golden(1 hunks)examples/monad.golden(1 hunks)examples/non-examples/stack-traces/error-in-arg.golden(1 hunks)examples/non-examples/stack-traces/error-in-bind-tail.golden(1 hunks)examples/non-examples/stack-traces/error-in-bind-tail.kl(1 hunks)examples/non-examples/stack-traces/error-in-constructor.golden(1 hunks)examples/non-examples/stack-traces/error-in-let.golden(1 hunks)examples/prelude-test.golden(1 hunks)examples/primitives-documentation.golden(2 hunks)examples/product-type.golden(1 hunks)examples/tiny-types.golden(1 hunks)examples/unknown-type.golden(1 hunks)examples/which-problem.golden(1 hunks)src/Evaluator.hs(13 hunks)src/Expander.hs(32 hunks)src/Expander/Primitives.hs(8 hunks)src/Pretty.hs(7 hunks)src/Value.hs(2 hunks)
✅ Files skipped from review due to trivial changes (7)
- examples/product-type.golden
- examples/implicit-conversion-test.golden
- examples/error.golden
- examples/fix.golden
- examples/unknown-type.golden
- examples/prelude-test.golden
- examples/contract.golden
🚧 Files skipped from review as they are similar to previous changes (5)
- examples/non-examples/stack-traces/error-in-arg.golden
- examples/non-examples/stack-traces/error-in-let.golden
- examples/non-examples/stack-traces/error-in-bind-tail.golden
- examples/non-examples/stack-traces/error-in-constructor.golden
- examples/non-examples/stack-traces/error-in-bind-tail.kl
🔇 Additional comments (23)
src/Pretty.hs (4)
5-5: LGTM! Well-structured module header.The addition of
LambdaCaseextension and new exports (ppBind,hardline,nest) are appropriate for the stack trace functionality.Also applies to: 11-20
40-40: LGTM! Appropriate import for stack traces.The import of evaluation-related types is necessary for implementing stack trace functionality.
526-527: LGTM! Improved closure pretty-printing.The changes make the output more concise and readable by using a consistent format for both first-order and higher-order closures.
696-707: Consider handling empty traces differently.Based on past review feedback, empty traces can be confusing. Consider suppressing the trace in empty cases or special-casing empty continuations at the outside.
src/Evaluator.hs (1)
715-723:⚠️ Potential issueImprove type safety of error handling functions.
The current implementation could panic at runtime. Consider using Haskell's type system to prevent invalid usage:
- Return
Maybetypes instead of using error- Use pattern matching in a way that the compiler can verify exhaustiveness
Apply this diff to improve type safety:
-projectError :: EState -> EvalError -projectError (Er err _env _kont) = err -projectError _ = error "projectError not used on an error!" +projectError :: EState -> Maybe EvalError +projectError (Er err _env _kont) = Just err +projectError _ = Nothing -projectKont :: EState -> Kont -projectKont (Er _ _ k) = k -projectKont (Up _ _ k) = k -projectKont (Down _ _ k) = k +projectKont :: EState -> Maybe Kont +projectKont (Er _ _ k) = Just k +projectKont (Up _ _ k) = Just k +projectKont (Down _ _ k) = Just kexamples/primitives-documentation.golden (1)
46-48: LGTM!The closure references have been updated to be more descriptive and consistent with the function signatures.
src/Expander/Primitives.hs (3)
77-77: LGTM!The Debugger module import aligns with the PR's objective to enhance stack trace functionality.
523-524: LGTM!The error handling in
typeConstructorhas been updated to usedebugfor wrong type arity, which is consistent with the PR's error handling approach.Also applies to: 535-536
593-593: LGTM!The error handling in
makeLocalTypehas been updated to usedebugfor invalid types.src/Expander.hs (4)
61-61: LGTM!The Debugger module import aligns with the PR's objective to enhance stack trace functionality.
153-156: LGTM!The error handling in
loadModuleFilehas been updated to usedebugfor file existence and reader errors.
309-312: LGTM!The error handling in
evalModhas been updated to usedebugfor macro evaluation errors.Also applies to: 915-919
1324-1329: LGTM!The error handling in
expandOneFormhas been updated to usedebugfor macro-related errors.examples/tiny-types.golden (1)
5-6: LGTM! Improved closure naming enhances readability.The changes replace generic
#<closure>labels with more descriptive names:
#<x>for boolean and syntax transformations#<eq?>for the macro that checks boolean equalityThis aligns with the PR's goal of improving clarity and removing non-deterministic IDs.
Also applies to: 11-11
examples/higher-kinded.golden (1)
2-2: LGTM! Consistent closure naming in higher-kinded types.The changes consistently use
#<x>for closures in bothof-unitandof-unit-unitfunctions while preserving the type signatures. This improves readability without affecting type safety.Also applies to: 5-6
examples/datatypes.golden (1)
6-6: LGTM! Descriptive closure names improve code clarity.The changes use purpose-indicating names for closures:
#<n>for the Nat operation closure#<letter>for Alphabet-related closuresThis makes the code more self-documenting by clearly indicating the purpose of each closure.
Also applies to: 8-9
examples/monad.golden (2)
1-4: LGTM! Improved identifier clarity.The replacement of generic
#<closure>with more descriptive identifiers (#<x>,#<inst>, etc.) enhances code readability while maintaining the same type signatures and functionality.
11-11: LGTM! Consistent identifier naming.The replacement of
#<closure>with#<inst>maintains consistency with earlier changes while preserving the type signature for the monadic bind operation.src/Value.hs (3)
55-59: LGTM! Enhanced error message clarity.The improved
valueTextimplementation now provides more descriptive closure identifiers in error messages, correctly handling both first-order (FO) and higher-order (HO) closures. This aligns well with the PR's objective to enhance error message clarity.
91-91: LGTM! Enhanced closure type with descriptive names.The addition of a
Textfield to theHOconstructor enables storing descriptive names for higher-order closures, supporting better error messages while maintaining backward compatibility.
94-95: LGTM! Improved debug output.The updated
Showinstance now includes closure identifiers in the string representation, providing more informative debug output that aligns with the PR's objective to enhance stack traces.examples/eta-case.golden (2)
13-14: LGTM! Improved variable naming enhances readability.The replacement of generic
#<closure>with meaningful variable names#<x>and#<xs>makes the code more readable while maintaining the same type signatures. This change aligns well with the PR's goal of improving clarity.Also applies to: 16-17
1-22:✅ Verification successful
Verify test coverage for stack trace functionality.
The file includes comprehensive test cases with source location annotations (e.g.,
eta-case.kl:188.13-195.38), but we should ensure it covers all stack trace scenarios mentioned in the PR objectives.Let's check for related test files:
🏁 Script executed:
#!/bin/bash # Description: Find related test files that might contain stack trace tests # Search for files with similar patterns fd -e golden -e kl 'eta-case|stack.*trace' examples/ # Search for stack trace related tests in source files rg -l 'stack.*trace|error.*message' examples/Length of output: 624
Test Coverage for Stack Trace Functionality Verified
We confirmed that stack trace scenarios are covered across the codebase. In addition to the annotations in examples/eta-case.golden and examples/eta-case.kl, multiple tests under examples/non-examples/stack-traces/ (such as error-in-let.golden, error-in-bind-head.golden, error-in-case-constructor.golden, error-in-arg.golden, error-in-constructor.golden, error-in-list.golden, and error-in-pure-macro.golden) address various error conditions.
- Review these files to ensure all the stack trace cases described in the PR objectives are explicitly validated.
- Confirm that the edge-case scenarios have the expected stack trace outputs.
|
Alright I was able to show the names of the value closures by adding some for example, instead of we output because of this combinator: This seems kludgy to me. At which results in: Notice the |
It makes sense that
hmm, that's the name of the argument, not the name of the function. While I understand that the name of the argument is easier to obtain since it's right there in the lambda, I am sure that the user would prefer to see How about using the SrcLoc of the lambda to identify it? This would work even if the lambda is never given a name. For this reason I think it makes perfect sense for both lambdas in lessThan to have the name |
Yes I wanted to go down this path with
totally agree! |
|
On second thought, I think that this printing In an |
|
Note that if instead of requiring the function symbol being fully evaluated before the argument like this: then I get this output: which makes sense. |
This is not a bug. A closure is an expression with a bunch of variables that and one extra variable. The other variables are closed by the captured environment. The extra variable is from the definition of the closure, recall that in the kernel lang each closure must at least be unary. So this was my misunderstanding. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Expander.hs (1)
153-153: Prefer 'unless' for clarity
In Haskell, usingunless existspis often clearer thanwhen (not existsp).- when (not existsp) $ debug $ NoSuchFile $ show file + unless existsp $ debug $ NoSuchFile $ show file
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/non-examples/stack-traces/error-in-let.golden(1 hunks)examples/primitives-documentation.golden(2 hunks)src/Expander.hs(31 hunks)src/Expander/Primitives.hs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/non-examples/stack-traces/error-in-let.golden
- src/Expander/Primitives.hs
🔇 Additional comments (78)
examples/primitives-documentation.golden (4)
1-30: Improved closure identifiers enhance debugging clarity.The changes to primitive function closures from generic
#<closure>to function-specific identifiers (like#<open-syntax>,#<+>, etc.) significantly improves stack trace readability and error message clarity. This aligns well with the PR objectives to refine stack trace output.
19-24: Consistent naming pattern for string comparison functions.I notice that the string comparison functions (string=?, string/=?, etc.) now use the same closure identifiers as their numeric counterparts (#<=>, #</=>, etc.). This is a good approach for consistency and makes the error messages more intuitive.
50-50: Successfully updated write function closure identifier.The "write" function's closure identifier has been updated from generic
#<closure>to the more descriptive#<write>, maintaining consistency with the overall approach.
46-48:✅ Verification successful
Verify if parameter names are intended for higher-order functions.
For higher-order functions like
id,const, andcompose, the closure identifiers now show parameter names (#<x>,#<x>,#<f>) rather than function names. Based on the PR comments, there was discussion about whether using argument names vs. function names is more appropriate. Could you verify this is the intended approach?For example, would
#<id>,#<const>, and#<compose>be more consistent with other primitives?
🏁 Script executed:
#!/bin/bash # Check how other higher-order functions are represented in other golden files rg -l "pair \"(id|const|compose)" --type-add golden:*.golden --type golden | xargs cat | grep -A 1 "pair \"\\(id\\|const\\|compose\\)"Length of output: 406
Verification Complete: Parameter names are intended.
The golden files consistently use the parameter names for higher-order functions—as seen with
(pair "id" #<x>),(pair "const" #<x>), and(pair "compose" #<f>)—which aligns with other primitives (e.g.,(pair "stdout" #<output port>)). Based on our review and prior discussions, this behavior is correct and intentional.src/Expander.hs (74)
61-61: Use of Debugger import looks good
No issues found with this new import line.
156-156: Consistent debug usage
This change aligns well with the new error handling approach.
199-199: Looks good
Usingdebughere is consistent with the revised error strategy.
309-310: Minor logic addition looks fine
Appending IO results to a sequence is straightforward and appears correct.
371-371: Consistent debug invocation
Good job replacing direct exceptions with debug calls for unknown identifiers.
409-409: Parameter usage
The new lambda parameter naming (ValueSyntax stx) is clear and readable.
426-427: Clarity improvement
Splitting the scheme declaration and name definition makes the code more readable.
429-431: Nested closures
These nested closures appear correct for handling the two-argument function.
462-463: String equality
Implementation forstring=?is straightforward and appears correct.
471-473: String append
Combining strings with a closure-based approach is consistent with the rest of the code.
487-491: Substring functionality
This newly introduced logic for substring checks edge cases properly.
501-501: String length
Simple usage ofT.lengthlooks correct.
505-505: Downcase closure
Implementation cleanly leveragesT.toLower.
513-513: Titlecase usage
UsingT.toTitleis appropriate for this function.
517-517: Foldcase
This follows the same consistent pattern as the other string functions.
521-521: String comparison
Declaring the scheme for [tString, tString] -> Bool is coherent.
528-528: Unary integer prim
No issues with referencingPrims.unaryIntegerPrim.
534-534: Binary integer function
The scheme definition for a two-integer function is correct.
541-541: Binary integer predicate
Implementation remains consistent with the code’s style.
546-547: pure-IO
The type signature and closure usage look good for this IO monad function.
554-555: bind-IO
This logic accurately lifts values into and out of the IOAction.
559-559: Higher-order function usage
Parameter_stris a minor detail but clearly shows the function is unused. Looks fine.
566-567: Replace direct error call for consistency
Previously, we recommended replacingerrorcalls withdebug. This line reintroduces a directerrorcall.- Left err -> error (T.unpack (pretty err)) + Left err -> debug $ MacroEvaluationError p (evalErrorType "closure" err)
583-583: Write function
Sending output to the handle is appropriate; no concerns.
795-796: Import prefix spec
New pattern matches forprefixappear correct.
802-802: PrefixImports
ReturningPrefixImports subSpec pis aligned with the rest of the code.
822-822: Edge case debugging
RaisingNotExportSpec blamefor zero-arg rename forms is consistent with the codebase style.
828-828: Edge case
NotExportSpec blameagain provides consistent error reporting.
829-829: Shift branch
Detecting"shift"keyword is properly introduced.
830-830: Case pattern
Case-based approach is consistent.
835-835: Invalid shift
Another consistent usage ofdebug $ NotExportSpec blame.
836-836: Unrecognized arguments
Same pattern for debugging invalid import specs.
837-837: Default branch
Maintains the same error approach.
839-839: ExportIdents
Graceful fallback toExportIdents xs.
851-852: Rename fallback
TriggersNotExportSpec blamefor unrecognized rename patterns.
907-907: MetaVar branching
Handling aTMetaVarin type-check logic is correct.
917-917: MacroEvaluationError
Debugging macro action errors is consistent with the approach.
921-921: MacroEvaluationError
Catching evaluator errors properly.
927-930: New dependency detection
Ensures tasks are rescheduled if new dependencies appear.
931-933: Continue if no new deps
Logic to link the core if no dependencies remain is correct.
948-953: Defining environment
Stores the result of the expanded expression in the environment for the phase.
1001-1001: Still stuck
Consistent pattern to wait if core isn’t linked yet.
1016-1017: Generalization fallback
Line 1016 logs an internal error if the type is missing; line 1017 proceeds on success. Looks consistent.
1079-1080: Type pattern info
Again, usingdebug $ InternalErrorfor missing pattern info is consistent.
1216-1216: PrimExprMacro branch
This pattern match inside theexpandOneFormfunction is correct.
1226-1226: Delegate to impl
Callingimpl t dest stxis a straightforward solution for expression macros.
1227-1227: EConstructor branch
Handling constructor expansions is correct for expressions or patterns.
1229-1229: Argument count check
Ensures the caller supplies the exact number of constructor arguments.
1237-1238: Pattern arg count
Properly checking subPats matchesargTypes.
1250-1250: WrongSyntacticCategory
Clear debug usage for mismatched categories.
1256-1256: ModuleCat
Another consistent branch for module macros.
1270-1270: TypeCat mismatch
Same approach for type expansions.
1297-1297: NotValidType
This helps identify invalid type syntax quickly.
1301-1304: EIncompleteDefn and EIncompleteMacro
Forking tasks to await definition or incomplete macro expansions is a proper design.
1316-1316: debug $ ValueNotMacro
Good consistency continuing the debug approach.
1317-1317: Handling 'Right mv'
Switching on macro results is clearly organized.
1326-1326: ValueNotSyntax
Properly flags unexpected expansions.
1327-1327: ValueNotMacro
Same pattern, clarifying the error context.
1329-1331: InternalError for missing transformer
Matches previous patterns of debug usage for missing macros.
1335-1335: Module must be identifier-headed
Error message for module expansions is consistent.
1336-1336: All declarations
Same logic for declaration expansions.
1337-1337: Identifier-headed declarations
Continuing the same pattern.
1338-1338: All type expansions
Maintains uniform approach withdebug.
1339-1339: NotValidType
Again consistent with the approach for type expansions.
1371-1372: Leaf decl
Completes a declaration tree node with no further content. Looks fine.
1384-1385: Expand decl forms
Forking the tail expansions ensures a multi-declaration structure.
1420-1420: MacroRaisedSyntaxError
Explicit debug call clarifies errors during macro expansions.
1421-1421: MacroActionIdentEq introduction
Branching introduced here is consistent with other macro actions.
1434-1434: Unknown => fallback
Returning false for unknown bindings is coherent with free-identifier=? logic.
1436-1436: Construct boolean
Usingflip primitiveCtor []is structured.
1442-1442: guard for invalid syntax object
Checks that we have aValueSyntax; good consistency.
1443-1443: compareFree
No immediate issues with this approach.
1459-1459: New closure for MacroActionValue
This function’s multi-closure approach is consistent with other expansions.
1463-1465: Replace direct error call
Use oferror "Impossible!"should be replaced with a debug call for consistent error handling.- _ -> error "Impossible!" + _ -> debug $ InternalError "Impossible!"
|
alright I've fixed the names, now more tests and cleanup and this should be good to go |
doyougnu
force-pushed
the
doyougnu/stack-traces
branch
2 times, most recently
from
234de98 to
7321c5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/Evaluator.hs (1)
705-708:⚠️ Potential issueHandle non-error cases gracefully in
projectError
If this function is called on a non-Erstate, it callserror. This can lead to runtime crashes. Consider returningMaybe EvalErroror a more descriptive error to avoid unexpected program termination.-projectError :: EState -> EvalError -projectError (Er err _env _kont) = err -projectError _ = error "projectError not used on an error!" +projectError :: EState -> Maybe EvalError +projectError (Er err _env _kont) = Just err +projectError _ = Nothingsrc/Debugger.hs (1)
73-79: 🛠️ Refactor suggestionRevisit the
bindimplementation.Chaining the monadic computations via
fmap (debugRunT r) f valis unconventional and may cause confusion. A clearer approach is to explicitly callrunDebug (f val) r, ensuring the monadic bind logic is transparent and consistent:instance Monad (Debug r e) where Debug m >>= f = Debug $ \r -> do ma <- m r case ma of Left err -> return (Left err) - Right val -> fmap (debugRunT r) f val + Right val -> runDebug (f val) r
🧹 Nitpick comments (32)
src/Expander.hs (9)
153-153: Review error handling for missing file scenario.
Logging withdebugis good for visibility, but consider whether you should halt or raise a higher-level error if the file is essential. A user might be left unaware of a missing critical file if it's only logged.
917-917: EnsureMacroEvaluationErroris the correct diagnostic.
When the value is not the expected macro action, you log it as a'macro action' error. Confirm that your diagnostic messages are understandable and help developers identify the root cause.
921-921: Investigate repeatedMacroEvaluationErrorhandling.
Multipledebugcalls forMacroEvaluationErrormight complicate the flow if the macro was partially evaluated. Consider whether consistent upstream or downstream logic can unify these diagnostic messages.
939-939: Assess concurrency implications of modifying the transformer env.
modifyState $ over (expanderCurrentTransformerEnvs ...)merges a new env with the old. If the expander can run in parallel (or in a future concurrency scenario), ensure appropriate synchronization or invariants.
982-982: Evaluate immediate next step for empty macro action continuation.
The code lumps this case intoContinueMacroAction dest value [], but the next lines proceed to evaluate. Double-check that no additional state checks are needed before proceeding.
985-985: Repeated retrieval ofcurrentPhase.
Callingp <- currentPhasein multiple consecutive lines might hint at a refactoring point to retrieve once and share. This can reduce repeated lookups and improve clarity.
987-987: Log message potentially missing important context.
While callingdebug $ MacroEvaluationError p ..., consider including more details (like function name or local environment state) to ensure easier debugging.
998-998: Check macro action error categorization.
We log'macro action'in multiple places. Distinguishing between well-known and truly unknown macro failures can help with triage when debugging.
1297-1298: Improve clarity ofNotValidTypemessage.
Line 1297 logs an error, but consider describing the invalid type form or the original syntax snippet that caused it, to help debugging.- _ -> debug $ NotValidType stx + _ -> debug $ NotValidType (show stx)src/Pretty.hs (4)
5-5: New usage ofLambdaCaseextension.
If you only useLambdaCasein a small section, consider scoping or limiting usage. Otherwise, ensure consistent usage across the codebase for readability.Do you want help refactoring any new
\caseexpressions?
10-20: Expanded exports might affect public API stability.
Exporting more symbols (Doc, Pretty(..), ppBind, etc.) can impact other modules relying on these definitions. Confirm no accidental oversharing of internals that might lock you into supporting them.
621-621: Enhance clarity of printing FO closures.
Currently printingFO foas#<...>is minimal. If debugging or stack tracing closures is a priority, consider including environment or unique identifiers to help in diagnosing closure contexts.
623-623: Add detail toHOclosure printing.
Similar toFO, consider whether you need more data than#<...>. This can be valuable for advanced debugging of high-order closures.src/StackTraces.hs (1)
1-41: Validate correctness of newStackTracesmodule.
This file introduces stack-trace handling for the CEK machine. Overall, the approach looks sound. A few points to consider:
- Dependency injection: If
printErr,printEnv, etc. rely on external definitions, ensure consistent versioning and no cyclical imports.- User experience: If these traces are for end-users, consider structured logs or a togglable verbosity.
- Testing coverage: Thoroughly test each function with varied
EStateto confirm correct formatting in corner cases.If you need test scaffolding or multi-phase checks for error states, let us know.
src/Expander/Monad.hs (9)
599-599: Improve error context
WhenNothing -> debug $ InternalError "Incomplete module after expansion", consider including the module name or location for clearer debugging.
610-610: Align error message with source info
For"Missing decl after expansion", it might help to include theDeclPtror a relevant location so users can quickly pinpoint the missing declaration.
619-619: Add diagnostic details
For"Missing expr after expansion", consider referencing the expression’s pointer or source to distinguish multiple missing expressions. This aids debugging.
623-623: Include scheme pointer in error
For"Missing scheme after expansion", you might include theSchemePtror some context about the scheme’s origin.
632-632: Maintain consistent error string
You are using"Missing expr after expansion"in multiple places. Ensure consistent capitalization, phrasing, and contextual data to unify error reporting throughout.
648-649: Optional: unify repetitive error handling
Here, you debug"Missing type after expansion". Repetitive error strings could be factored out into a helper or consistent function, so you can store location/identifier details in one place.
667-667: Reduce duplicated “Missing expr” messages
You have repeated"Missing expr after expansion"in several branches. A single helper function wrappingdebug $ InternalErrormight centralize the logic and further aid testing.
677-677: Consider verbose logging
Fordebug $ MacroEvaluationError p e_state, consider including further details aboute_state(e.g., partial macro calls) to aid in diagnosing macro expansion failures.
851-851: Add context for “NoProgress”
When you logNoProgress, specifying which tasks or expansions are stuck helps clarify potential deadlock or infinite loop issues.src/Expander/Primitives.hs (4)
9-9: Revisit-Wno-name-shadowingusage
Disabling name-shadowing warnings may mask unintended overlaps. Unless absolutely necessary, consider removing or addressing shadowing at the source.
756-757:binaryIntegerPrimmight need overflow handling
Integer operations can exceed typical ranges. Ensure that large inputs do not cause runtime issues or performance degradations.
764-765: HarmonizebinaryIntegerPredwith other integer wrappers
This predicate function follows the pattern ofbinaryIntegerPrim; consider unifying them into a single helper if their logic remains similar.
775-780: Consider mergingbinaryStringPredwith integer-based counterparts
Similar tobinaryIntegerPred, you could centralize both logic flows (string vs. integer) into parameterized helper functions. This reduces duplication and future maintenance overhead.src/Evaluator.hs (4)
11-14: Keep module headers concise
The four header lines provide general context about the CEK machine. Ensure they remain up-to-date and consistent with the actual code’s purpose.
24-26: Consolidate commentary
Your opening block comment and these lines about “Converting state from the CEK machine…” may overlap. Consider moving all module-level docs into a single{-# LANGUAGE ... #-}block or reorganizing for clarity.
30-30: Expand on CEK approach
“The Klister interpreter is a straightforward implementation...” is a good start. Adding a bit more about how the debugger hooks into the CEK machine can help new contributors.
70-71: ExportingEStateandKont
Exposing these internals is useful for debugging, but ensure you retain flexibility to switch or refactor them without breaking consumers.src/Debugger.hs (1)
121-131: Consider removing or clarifying the commented-out code.These comments reference next steps and partial functionalities. If the code is no longer relevant, removing it would reduce clutter. If it is a placeholder, adding a more explicit TODO or follow-up reference can help collaborators track outstanding tasks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (52)
examples/contract.golden(1 hunks)examples/datatypes.golden(1 hunks)examples/error.golden(1 hunks)examples/eta-case.golden(1 hunks)examples/fix.golden(1 hunks)examples/higher-kinded.golden(1 hunks)examples/implicit-conversion-test.golden(1 hunks)examples/monad.golden(1 hunks)examples/non-examples/bad-lexical-env.golden(1 hunks)examples/non-examples/bad-lexical-env.kl(1 hunks)examples/non-examples/error.golden(1 hunks)examples/non-examples/stack-traces/.#error-in-cons-head.golden(1 hunks)examples/non-examples/stack-traces/error-in-arg.golden(1 hunks)examples/non-examples/stack-traces/error-in-arg.kl(1 hunks)examples/non-examples/stack-traces/error-in-bind-head.golden(1 hunks)examples/non-examples/stack-traces/error-in-bind-head.kl(1 hunks)examples/non-examples/stack-traces/error-in-bind-tail.golden(1 hunks)examples/non-examples/stack-traces/error-in-bind-tail.kl(1 hunks)examples/non-examples/stack-traces/error-in-case-constructor.golden(1 hunks)examples/non-examples/stack-traces/error-in-case-constructor.kl(1 hunks)examples/non-examples/stack-traces/error-in-cons-head.golden(1 hunks)examples/non-examples/stack-traces/error-in-cons-head.kl(1 hunks)examples/non-examples/stack-traces/error-in-constructor.golden(1 hunks)examples/non-examples/stack-traces/error-in-constructor.kl(1 hunks)examples/non-examples/stack-traces/error-in-let.golden(1 hunks)examples/non-examples/stack-traces/error-in-let.kl(1 hunks)examples/non-examples/stack-traces/error-in-list.golden(1 hunks)examples/non-examples/stack-traces/error-in-list.kl(1 hunks)examples/non-examples/stack-traces/error-in-pure-macro.golden(1 hunks)examples/non-examples/stack-traces/error-in-pure-macro.kl(1 hunks)examples/non-examples/stack-traces/in-arg-error.golden(1 hunks)examples/non-examples/stack-traces/in-arg-error.kl(1 hunks)examples/prelude-test.golden(1 hunks)examples/primitives-documentation.golden(2 hunks)examples/product-type.golden(1 hunks)examples/tiny-types.golden(1 hunks)examples/unknown-type.golden(1 hunks)examples/which-problem.golden(1 hunks)flake.nix(2 hunks)klister.cabal(1 hunks)repl/Main.hs(3 hunks)src/Debugger.hs(1 hunks)src/Evaluator.hs(12 hunks)src/Expander.hs(31 hunks)src/Expander/Error.hs(3 hunks)src/Expander/Monad.hs(13 hunks)src/Expander/Primitives.hs(7 hunks)src/Expander/Syntax.hs(6 hunks)src/Expander/TC.hs(9 hunks)src/Pretty.hs(6 hunks)src/StackTraces.hs(1 hunks)src/Value.hs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (44)
- examples/non-examples/stack-traces/in-arg-error.golden
- examples/non-examples/stack-traces/error-in-pure-macro.kl
- klister.cabal
- examples/non-examples/stack-traces/error-in-list.golden
- examples/implicit-conversion-test.golden
- examples/fix.golden
- examples/non-examples/stack-traces/error-in-pure-macro.golden
- examples/non-examples/stack-traces/error-in-constructor.golden
- examples/non-examples/stack-traces/error-in-cons-head.golden
- examples/non-examples/stack-traces/error-in-case-constructor.golden
- examples/non-examples/stack-traces/in-arg-error.kl
- examples/non-examples/error.golden
- examples/non-examples/stack-traces/error-in-arg.kl
- src/Expander/Syntax.hs
- examples/product-type.golden
- examples/non-examples/stack-traces/error-in-bind-tail.kl
- examples/non-examples/stack-traces/error-in-bind-tail.golden
- examples/non-examples/stack-traces/error-in-let.golden
- examples/non-examples/stack-traces/error-in-cons-head.kl
- examples/eta-case.golden
- examples/non-examples/stack-traces/error-in-let.kl
- examples/non-examples/bad-lexical-env.golden
- examples/unknown-type.golden
- flake.nix
- examples/datatypes.golden
- examples/non-examples/stack-traces/error-in-constructor.kl
- examples/non-examples/stack-traces/error-in-bind-head.kl
- examples/non-examples/stack-traces/.#error-in-cons-head.golden
- examples/non-examples/stack-traces/error-in-bind-head.golden
- examples/error.golden
- examples/non-examples/bad-lexical-env.kl
- examples/non-examples/stack-traces/error-in-list.kl
- src/Expander/Error.hs
- examples/which-problem.golden
- src/Expander/TC.hs
- examples/prelude-test.golden
- examples/non-examples/stack-traces/error-in-case-constructor.kl
- repl/Main.hs
- examples/tiny-types.golden
- examples/contract.golden
- examples/monad.golden
- src/Value.hs
- examples/non-examples/stack-traces/error-in-arg.golden
- examples/primitives-documentation.golden
🔇 Additional comments (28)
examples/higher-kinded.golden (2)
2-2: Improved closure identification formatThe change from
#<closure>to#<x>provides a more descriptive identifier for the closure, which aligns with the PR objective of eliminating non-deterministic unique IDs in stack traces.
5-6: Consistent format for closure representationThe replacement of
#<closure>with#<x>in these lines maintains consistency with the changes in line 2, providing more meaningful identifiers in stack traces.Note that line 6 appears to be a duplicate of line 5. Is this intentional or should one of these entries be using a different identifier or function?
src/Expander.hs (17)
61-61: Consider consistency when importing theDebuggermodule.
It's unclear if this import is meant strictly for production logging or local debugging. If it's local-only, consider protecting it behind a conditional debug flag or removing it before merging to avoid unintended overhead or confusion.
156-156: Evaluate whether to propagateReaderErrorfurther.
Here you log an error withdebug, but continuing execution might lead to undefined behavior. Unless you’re sure the system can gracefully handle a partially read module, consider throwing or returning an explicit error.
199-199: Re-check the import resolution flow.
WhenNothing -> debug $ NotExported x p, the code logs but doesn't explicitly halt. Confirm that ambiguous or missing exports won't cause larger issues later, especially if the build or runtime expects them to exist.
309-309: Validate internal error branch in theRuncase.
The_ -> debug $ InternalError ...path might mask unhandled variants ofValueIOAction. Ensure you have tested or enumerated all possible outcomes to prevent silent failures in production.
360-360: Clarify the handling of ambiguous bindings.
It's logging anAmbiguouswarning but appears not to fail or disambiguate. Consider if an ambiguous binding at runtime should raise an error or if there's a resolution strategy.
935-935: Check for leftover partial expansions.
When we logNothing -> debug ...in the macro environment, ensure partial expansions don't stay in a corrupt state. It may help to finalize the environment or revert it upon error.
961-961: Confirm correct usage offorkExpandSyntax.
forkExpandSyntax (ExprDest t dest) stxspawns an expansion that might proceed asynchronously. Confirm that any shared state and subsequent tasks remain consistent while expansions happen in parallel.
990-990: Repeated retrieval ofcurrentPhase.
This is similar to line 985. If you fetch the phase once earlier in the flow, you can pass it around or store it, avoiding potential drift or repeated lookups.
997-997: Consider standardizing approach tocurrentPhase.
Reiterates the same suggestion about repeated calls tocurrentPhase—potential for a local variable or passing it along.
1079-1079: Clarify fallback path for missing type pattern info.
This debug line may suggest missing initialization or a delayed pattern. If type pattern info is critical, consider blocking or returning a user-facing error instead of just a debug output.
1179-1180: Check correctness ofWrongSyntacticCategory.
Here you log the mismatch withDeclarationCat, but there's no immediate fallback or alternative path for the syntax. Confirm that continuing after this log doesn't yield deeper issues.
1182-1182: Inconsistent category error handling.
Similar to the previous comment,WrongSyntacticCategoryfor a type might need a more direct approach, ensuring the expansion halts or the user is informed.
1192-1192: Revisit expression-cat mismatch.
Same pattern as lines 1180+ withWrongSyntacticCategory. Multiple repeated logs might hide the root cause if they are not aggregated or summarized for the user.
1226-1226: Handle argument count errors more explicitly.
WhenWrongArgCount stx ctor ...logs a mismatch, consider giving the user or developer a clear message about the cause or hint on how to fix it.
1238-1238: Ensure consistent error messages for argument count mismatch.
You log a secondWrongArgCountscenario. Prefer reusing or centralizing that logic to standardize user-facing error messages.
1326-1327: Macro expansions in user macros can be error-prone.
Line 1326 continues expansion, line 1327 logs if the result is not a macro. Confirm that unwrapping each step in user macros is robust. Could partial expansions cause the environment to break?
1349-1350: Potential unreachable spot for missing type patterns.
Similar to lines 1335-1339, verify whether these error logs occur in realistic scenarios. If not, consider throwing a more direct error or eliminating the branch for clarity.src/Pretty.hs (1)
42-42: Check for circular dependencies.
ImportingEvaluator (EvalResult(..), EvalError(..), TypeError(..), Kont(..), EState(..))here suggests a deeper coupling. Ensure you won't create cyclical references or hamper modular design.src/Expander/Monad.hs (3)
177-177: Ensure usage of the newly importedDebuggermodule
It appears you are importing theDebuggermodule to replace previous error handling/logging mechanisms. Confirm that all debug calls consistently rely on this module's capabilities, removing any now-redundant imports or references.
208-210: Validate the switch to theDebugmonad
By wrappingReaderT ExpanderContextwith(Debug DebugContext ExpansionErr), you have replaced theExceptT-based error handling. Verify that partial expansions or intermediate states are still properly handled and re-queued. Consider systematically testing edge cases, such as incomplete macros, to ensure no silent failures.
215-215: Check consistency ofexecExpandwith the debug context
CallingrunDebug (runReaderT (runExpand act) ctx) initialContextintegrates the new debug context. This looks correct structurally. Ensure that any previously-thrown errors are now being sent todebug, and that there are no regressions in how expansions terminate or yield results.src/Expander/Primitives.hs (2)
77-77: Confirm necessity ofDebuggerimport
Similar to the expander monad, ensure the newly importedDebuggeris actively used for logging or error handling and that no old error mechanism remains.
750-752: Check for potential edge cases inunaryIntegerPrim
This function straightforwardly wraps a unary integer operation. Confirm that no negative or overflow edge cases require additional checks, particularly if your integer arithmetic can raise exceptions.src/Evaluator.hs (1)
83-85: Re-check exports for potential reusability
projectError,projectKont, andconstructErrorTypeare newly exported. Confirm whether they should remain internal for tight control over error state, or if external usage is beneficial.src/Debugger.hs (2)
45-46: RenamedebugRunTfor clarity.Since this function only flips the arguments of
runDebugand does not operate on a transformer stack, consider using a name likerunDebugFlippedor simply inlining its usage.
113-114: Add explanation for why_stackTraceis a list ofEState.
EStatealready contains a continuation, so it's not obvious why multipleEStatevalues are stored. An inline comment clarifying the design rationale would help maintainers understand how you are aggregating these states into a coherent stack trace.
doyougnu
force-pushed
the
doyougnu/stack-traces
branch
from
ef8b557 to
90b5451
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Pretty.hs (2)
887-901: Case pattern printing could be enhanced.The pattern printing for syntax patterns simplifies the output by only showing core pattern information without variable bindings.
Consider providing more detailed information about variable bindings in patterns, which could help with debugging complex macros where the capturing environment is important.
945-967: Consider improving ident equality reporting.The pretty-printing for identifier equality operations provides basic information but could be enhanced with more context.
For identifier equality operations, consider adding information about the comparison mode (free vs. bound) in the output to make it clearer what kind of equality is being tested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Evaluator.hs(12 hunks)src/Expander/Primitives.hs(7 hunks)src/Pretty.hs(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Evaluator.hs
🔇 Additional comments (19)
src/Expander/Primitives.hs (9)
76-76: Adding the Debugger module import.This import is part of the stack trace enhancement, allowing for improved error handling throughout the module.
522-524: Error handling improved using debug instead of exceptions.Good refactoring by replacing direct error throwing with the
debugfunction, which provides better stack traces and context for debugging.
534-536: Consistent error handling in pattern implementation.The error handling approach is consistent with the earlier change, using
debuginstead of throwing exceptions directly.
592-592: Using debug for invalid type errors.Consistent with the overall approach of using the debugger for error reporting instead of direct exceptions.
691-691: Improved error handling for unknown patterns.Using the debug function for reporting unknown patterns provides better diagnostics.
749-752: Enhanced function signatures with name parameter.Adding the function name parameter improves stack traces by making functions identifiable when errors occur.
755-760: Consistent naming enhancements for binary integer primitive.Good continuation of the pattern, passing the function name through to the
HOconstructor.
763-768: Consistent naming for binary integer predicates.The same pattern is applied consistently across all primitive functions, which is good for maintainability.
774-779: Completed function naming pattern for string predicates.The changes to the string predicate functions maintain consistency with the other primitive functions, completing the enhancement pattern.
src/Pretty.hs (10)
5-5: Adding LambdaCase is a good improvement.This language extension enables the more concise
\casesyntax that will be particularly useful for pattern matching in the pretty-printing functions.
10-20: Export list updates properly expose new functionality.The changes to the export list are well-organized and properly expose the new
ppBind,hardline, andnestfunctions which are needed for the stack trace formatting.
42-42: Properly extended import for stack trace types.This import now includes the necessary
KontandEStatetypes which are used in the new stack trace functionality.
479-479: Constructor pattern printing improvement.The indentation and layout logic for constructor patterns with arguments has been improved, making the output more readable.
622-623: Enhanced clarity of closure presentation.Improved the closure pretty printing to display more meaningful information:
- First-order (FO) closures now show their identifier value
- Higher-order (HO) closures now display a name
This change will make error messages more informative by providing better context about closures.
820-829: Well-structured stack trace printing implementation.The instances for
EStateandKontestablish the foundation for stack trace formatting. The implementation effectively delegates to specialized printing functions.
830-838: Comprehensive error state printing.The
printStackfunction properly handles all three evaluation states:
- Error states with appropriate stack trace formatting
- "Up" states
- "Down" states
This ensures that all possible evaluation states can be clearly presented to the user.
839-886: Thorough continuation stack printing implementation.The
printKontfunction handles a wide range of continuation types, providing detailed context about the execution point where an error occurred. The implementation includes proper handling of:
- Basic continuations (Halt, primitives, function applications)
- Constructor applications with field position information
- Case expressions with pattern information
This comprehensive approach will significantly improve error diagnostics.
968-982: Good handling of macro continuation states.The implementation properly handles the various macro continuation states, which is important for debugging macro expansion issues.
1002-1009: Utility function for case handling is well-implemented.The
do_casehelper function provides a clean way to pretty-print case patterns and their bodies, promoting code reuse across different case expression types.
| #<closure> : (Syntax → (Syntax → (Syntax → (Macro Syntax)))) | ||
| #<closure> : ∀(α : *). (α → α) | ||
| #<contract> : (Syntax → Syntax) | ||
| #<arg> : (Syntax → (Syntax → (Syntax → (Macro Syntax)))) |
There was a problem hiding this comment.
looks like things defined with defun are still capturing the argument not the function name
There was a problem hiding this comment.
- : TODO fix defun
There was a problem hiding this comment.
I think these do not make it to the evaluator. They are wired into the expander in the kernel. This means they are defined as decls here:
evalMod (Expanded em _) = execStateT (traverseOf_ (moduleBody . each) evalDecl em) mempty
where
evalDecl :: CompleteDecl -> StateT (Seq EvalResult) Expand ()
evalDecl (CompleteDecl d) =
...
There was a problem hiding this comment.
And the Decls are not represented by Core so the evaluator typically only sees the body of the Decl which makes sense. This is a good use case for a passthrough Kont. But I'll save that for a different PR.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Pretty.hs (1)
908-918: Address the TODO about testing the cons case.The code itself is well-structured, but there's a question about testing the cons head case.
Testing pair/cons operations typically involves verifying that head/car and tail/cdr operations correctly retrieve the correct elements. I can help implement test cases for the
InConsHdcontinuation if you'd like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Pretty.hs(8 hunks)
🔇 Additional comments (12)
src/Pretty.hs (12)
5-5: Good addition of LambdaCase extension.This language extension enables more concise pattern matching with the
\casesyntax, which is appropriately used later in the newppPatternfunction implementations.
10-20: Export list properly updated with new functionality.The export list has been nicely reformatted and expanded to include the new
ppBindfunction as well as additional formatting functions likehardlineandnestwhich are necessary for the stack trace functionality.
42-42: Appropriate import expansion.The updated import from
Evaluatornow explicitly includes the necessary types (EvalResult,EvalError,TypeError,Kont,EState) that are used in the new stack trace implementation.
479-479: Formatting change for constructor arguments.This change maintains consistent formatting for constructors with arguments, using proper indentation and grouping.
622-623: Improved closure representation in error messages.The updated pretty-printing for closures now:
- For first-order (FO) closures: Shows the actual identifier value
- For higher-order (HO) closures: Shows the name parameter
This matches the PR objective of improving stack trace clarity and enhances user experience by making closure identification more informative.
820-829: Well-structured stack trace section.Good organization with a clear section comment and implementation of
Prettyinstances forEStateandKonttypes that leverage the new printing functions.
830-838: Clear error state formatting in stack traces.The
printStackfunction effectively formats error states with:
- Proper error message formatting
- Clear "stack trace:" header
- Special handling for
UpandDownstatesThis implementation aligns well with the PR objective of refining stack trace output for better clarity.
839-853: Good continuation formatting for basic elements.The
printKontimplementation for basic continuations (Halt, InPrim, InFun, InArg) is well-structured and provides clear information about the execution context.
919-929: Good implementation of tail continuation pretty-printing.The implementation for
InConsTlmirrors theInConsHdcase with appropriate labeling for the tail component, creating consistent output for both parts of a pair.
930-996: Comprehensive continuation handling for various types.The implementation covers all continuation types with consistent formatting:
- Lists
- Identifiers
- Macros
- Atomic values
- Scope-related continuations
This thorough coverage ensures stack traces will be informative in all error cases.
998-1006: Well-implemented helper function for case printing.The
do_casehelper function provides a clean way to pretty-print case patterns and their bodies, reusing the existingppBindandppfunctions for consistent formatting.
854-860:❓ Verification inconclusive
Address the TODO comment about body printing.
There's a TODO comment about the body printing uniques instead of variable names.
🏁 Script executed:
#!/bin/bash # Check for other occurrences of this pattern and how they're handled grep -n "TODO.*uniques" --include="*.hs" src/Length of output: 74
Below is a new shell script to re-run the search recursively so we can verify if there are any additional occurrences of the pattern in the codebase:
#!/bin/bash # Recursively search for TODO comments mentioning 'uniques' in all Haskell files under the src directory grep -R "TODO.*uniques" src/Once you run this, please double-check that the only occurrence is in
src/Pretty.hsat lines 854–860. Then, update the code so that the body is printed correctly (instead of printing unique identifiers) by re-enabling or revising the commented-out line (e.g. replacing the current logic with one that prints the expected variable name).Action Required: Update body printing in src/Pretty.hs
- In
src/Pretty.hs(lines 854–860), the commented-outb <- pp e bodyshould be replaced or updated so that the variable’s name—not an internal unique—is printed correctly.- Verify (with the new recursive search) that no other code locations exhibit the same issue.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Pretty.hs (1)
831-835: Consider adding color to error messages.Error messages could benefit from colorization to improve readability, particularly distinguishing between the error itself and the stack trace that follows.
Consider using ANSI color codes or a library like
ansi-terminalto make errors stand out more:printStack :: Env Var () -> EState -> State Renumbering (Doc VarInfo) printStack e (Er err _env k) = do er <- pp e err kont <- pp e k - return $ vsep [ er , text "stack trace:"] <> kont + return $ vsep [ annotate (color Red . bold) er , text "stack trace:"] <> kont
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/non-examples/stack-traces/error-in-bind-tail.kl(1 hunks)examples/non-examples/stack-traces/error-in-cons-head.kl(1 hunks)examples/non-examples/stack-traces/error-in-list.kl(1 hunks)src/Pretty.hs(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/non-examples/stack-traces/error-in-cons-head.kl
- examples/non-examples/stack-traces/error-in-bind-tail.kl
- examples/non-examples/stack-traces/error-in-list.kl
🔇 Additional comments (11)
src/Pretty.hs (11)
5-5: Good addition of LambdaCase extension.The LambdaCase language extension enables more concise and readable pattern matching in lambda functions, which is nicely utilized in the new stack trace rendering code (like in the pattern printing functions at lines 886-896).
11-20: Module exports updated to support stack tracing.The export list has been well-organized to include the additional functions needed for stack trace formatting, particularly
ppBind,hardline, andnestwhich are essential for the new pretty-printing functionality.
42-42: Import statement expanded to support stack traces.The import from
Evaluatorhas been appropriately expanded to include all necessary types for implementing stack trace functionality.
479-479: Improved pattern-arms formatting.The line now uses proper indentation, improving code readability and consistency with the rest of the codebase.
622-623: Enhanced closure display for better debugging.The pretty-printing for closures has been improved to provide more meaningful information by showing the identifier name for both first-order and higher-order closures, which aligns with the PR objective of improving stack trace clarity.
680-681: Proper handling of unused parameters.Using underscore prefixes for unused variables (like
_modName) follows Haskell best practices to avoid compiler warnings while maintaining code clarity.
823-834: Well-structured stack trace implementation.The implementation of
Prettyinstances forEStateandKontalong with the supportingprintStackfunction provides clear and structured error reporting, which is a key objective of this PR.
855-858: TODO comment needs to be addressed or documented.There's a TODO comment about body printing that shows uniques instead of variable names, which directly relates to the PR objective of "removing the unique id printing (which is non-deterministic)".
This TODO should either be resolved or documented as a known limitation. Consider reviewing how variable names are displayed in stack traces to ensure consistency with the PR objectives.
886-896: Consider enhancing pattern printing.SyntaxPatternIdentifier i _ SyntaxPatternInteger i _ SyntaxPatternString i _ SyntaxPatternCons ... SyntaxPatternList ... SyntaxPatternAny SyntaxPatternEmptyIn
printKont, you display only the main pattern parts. If debugging intricate macros, you might want the variable names or capturing environment. Evaluate if more detail is necessary for diagnosing pattern failures.
908-929: Implementation for cons handling looks good, but needs testing.The implementation for
InConsHdandInConsTlcontinuations appears thorough with proper formatting and scope information, addressing part of the PR objectives to enhance stack trace output.Since there was a TODO about testing these cases, consider adding test cases that specifically exercise these continuations to verify they work as expected.
975-978: Stack trace for bind macro tail is well-implemented.The handling of bind macro tail continuations is well-implemented and provides clear context about where in the macro expansion process the error occurred, which directly supports the PR objective of improved stack trace clarity.
|
Basically done, I want to do another pass over the code then squash |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/Debugger.hs (1)
133-135: 💡 Verification agent❓ Verification inconclusive
Implement or remove commented function signature
The commented function signature
enterDebuggersuggests an important part of the API that hasn't been implemented.Either implement this function or remove the comment if it's no longer needed. If it represents future work, consider creating a GitHub issue to track it.
🏁 Script executed:
#!/bin/bash # Check if enterDebugger is referenced elsewhere in the codebase grep -r "enterDebugger" --include="*.hs" . | grep -v "src/Debugger.hs"Length of output: 68
Action: Remove or properly implement the commented-out
enterDebuggersignatureThe grep-based check across the codebase indicates that
enterDebuggerisn’t referenced anywhere outside of the commented snippet in src/Debugger.hs. This suggests the signature is likely obsolete. Please consider one of the following actions:
- If the function is no longer needed: Remove the commented-out code to eliminate confusion.
- If future implementation is intended: Either implement the function or open a GitHub issue to track its development.
♻️ Duplicate comments (1)
src/Debugger.hs (1)
72-78:⚠️ Potential issueFix issue in
Monadinstance implementationThe implementation of
>>=appears incorrect. Currently,fmap (debugRun r) f valdoesn't properly sequence the monadic operations.The implementation should be:
instance Monad (Debug r e) where Debug m >>= f = Debug $ \r -> do ma <- m r case ma of Left err -> return (Left err) - Right val -> fmap (debugRun r) f val + Right val -> runDebug (f val) rThe problem is that
f valproduces aDebug r e b, and you need to run this computation with the contextr, not applydebugRun rinsidefmap.
🧹 Nitpick comments (4)
src/Debugger.hs (4)
24-25: Add explicit export listThe comment on line 25 indicates intent to add an explicit export list, but none has been implemented. Explicit exports help document the module's public API, prevent accidental exports, and improve maintainability.
Consider adding an export list specifying the intended public interface:
-module Debugger where - -- DYG explicit export list +module Debugger + ( -- * Core Debug monad + Debug(..) + , debugRun + , mapDebug + , withDebug + , ask' + -- * Debugger type class + , MonadDebugger(..) + -- * Context + , DebugContext(..) + , initialContext + ) where
112-114: Add documentation forDebugContextdata typeThe
DebugContextdata type lacks documentation explaining howEStateis used to represent a stack trace.Add a comment clarifying the role of
EStatein stack trace representation:+-- | Context for debugging operations +-- * `NoContext` represents the absence of debugging context +-- * `DebugContext` contains the current evaluation state, which includes +-- continuation information that can be used to construct stack traces data DebugContext = NoContext | DebugContext { _stackTrace :: EState } deriving Show
119-129: Remove or implement commented out code and TODOsThese lines contain commented out code and TODOs that should either be implemented or removed before merging.
Consider either:
- Implementing the functionality described in the TODOs
- Moving TODOs to issue tracking system
- Removing them if they're no longer relevant
If these represent future improvements, consider adding them as GitHub issues with links in code comments instead of inline TODOs.
96-101: Improve type class documentation with examplesThe
MonadDebuggertype class would benefit from more detailed documentation showing how to use it.Enhance the documentation with a brief example:
-- | Type class that defines the interface for any debugger. Each instance is a -- debugger in their own right +-- +-- Example usage: +-- +-- @ +-- computation :: MonadDebugger MyError m => m Result +-- computation = do +-- result <- riskyOperation `catch` \err -> fallbackOperation +-- if invalid result +-- then debug (InvalidResult result) +-- else pure result +-- @ class (Monad io, MonadIO io) => MonadDebugger e io | io -> e where -- conceptually this is throw debug :: e -> io a -- conceptually this is catch with a handler catch :: io a -> (e -> io a) -> io a
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Debugger.hs(1 hunks)
🔇 Additional comments (1)
src/Debugger.hs (1)
44-45: Consider renamingdebugRunfor clarityThe name
debugRundoesn't clearly convey that this is about running a Debug computation with a context.Consider a more descriptive name like
runDebugWithContextor justrunDebug(since it's not a transformer as noted in previous comments):-debugRun :: r -> Debug r e a -> IO (Either e a) -debugRun = flip runDebug +runDebugWithContext :: r -> Debug r e a -> IO (Either e a) +runDebugWithContext = flip runDebugThis would also require updating references elsewhere in the code.
| @@ -608,7 +619,8 @@ instance Pretty VarInfo (ExprF Syntax) where | |||
| pure $ parens (group (vsep ppXs)) | |||
|
|
|||
| instance Pretty VarInfo Closure where | |||
| pp _ _ = pure $ text "#<closure>" | |||
| pp _ (FO fo) = pure $ "#<" <> text (_stxValue (_closureIdent fo)) <> ">" | |||
| pp _ (HO n _) = pure $ "#<" <> text n <> ">" | |||
There was a problem hiding this comment.
FYI here are the changes to the pretty printer that print the closure name rather than the type "closure". In some cases this seems desirable, but the more I look at it having "foo" also seems reasonable. Thoughts?
| where | ||
| the_closure = case c of | ||
| (FO fo) -> _stxValue $ _closureIdent fo | ||
| (HO n _) -> n |
doyougnu
force-pushed
the
doyougnu/stack-traces
branch
from
9f4eee1 to
4982d60
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
examples/primitives-documentation.golden (1)
49-49: Inconsistent closure identifier format for stdout.Unlike other primitives that use the pattern
#<function-name>, thestdoutpair uses#<output port>. This inconsistency might confuse users when encountering it in stack traces.-(pair "stdout" #<output port>) : (Pair String Output-Port) +(pair "stdout" #<stdout>) : (Pair String Output-Port)src/Expander/Syntax.hs (4)
32-32: Transition from throw to debug.Similar to the previous function, ensure that non-empty inputs are truly recoverable. If these conditions imply fatal errors, consider short-circuiting or clarifying with comments.
49-49: Integer validation.Confirm integer parsing failure is genuinely something that can be recovered from with a simple debug log. Debug messages alone may not be sufficient for user feedback in all scenarios.
65-65: Logging invalid module name.
NotModNamediagnostic. Double-check that a user eventually sees actionable feedback to fix the module name, beyond just logging.
145-146: Potential partial success path.For an empty shape, we do a normal return, but for non-empty shapes, we log and continue. Ensure that all calling sites can handle partial expansions.
src/Expander.hs (5)
156-156: Reader error logs.
ReaderErroris now debug-logged. Ensure enough context is provided to the user if the file contents are invalid or unreadable.
986-986: Syntax mismatch logging.Confirm that a mismatch in the syntax macro path does not lead to incomplete expansions that might puzzle the user without a salient error message.
991-991: Re-logging macro evaluation errors.Avoid duplicates if multiple expansions trigger the same error path. Consider grouping repeated logs or re-throwing if necessary.
1335-1335: Module heading internal error.“All modules should be identifier-headed” is enforced as a debug call. If truly unreachable, consider throwing a final error or an assertion to avoid partial expansions.
1347-1347: Pattern heading safety net.“All patterns should be identifier-headed.” As above, confirm if the code is safe to continue in the presence of a logged error.
src/Pretty.hs (1)
622-623: Closure pretty-printer.Now displaying closure names (e.g., the
_stxValuefor aFO) or the textual name forHO. This increased clarity is helpful for debugging. Consider also showing environment captures if needed.src/Expander/Primitives.hs (1)
749-750: Enhanced closure identification with function names.Added function name parameter to primitive functions (
unaryIntegerPrim,binaryIntegerPrim,binaryIntegerPred,binaryStringPred) and their closures, improving error messages and debugging by labeling higher-order functions with descriptive names.Consider adding documentation comments for these functions to explain the purpose of the new Text parameter.
Also applies to: 755-756, 759-760, 763-764, 767-768, 774-775, 778-779
src/Debugger.hs (1)
122-129: Well-documented future work plans.The comments outline future enhancements to the debugger, indicating thoughtful design considerations for expanding its capabilities.
Consider converting these TODOs into actual tickets in your issue tracking system to ensure they're properly tracked.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
examples/contract.golden(1 hunks)examples/datatypes.golden(1 hunks)examples/error.golden(1 hunks)examples/eta-case.golden(1 hunks)examples/fix.golden(1 hunks)examples/higher-kinded.golden(1 hunks)examples/implicit-conversion-test.golden(1 hunks)examples/monad.golden(1 hunks)examples/non-examples/bad-lexical-env.golden(1 hunks)examples/non-examples/bad-lexical-env.kl(1 hunks)examples/non-examples/error.golden(1 hunks)examples/non-examples/stack-traces/error-in-arg.golden(1 hunks)examples/non-examples/stack-traces/error-in-arg.kl(1 hunks)examples/non-examples/stack-traces/error-in-bind-head.golden(1 hunks)examples/non-examples/stack-traces/error-in-bind-head.kl(1 hunks)examples/non-examples/stack-traces/error-in-bind-tail.golden(1 hunks)examples/non-examples/stack-traces/error-in-bind-tail.kl(1 hunks)examples/non-examples/stack-traces/error-in-case-constructor.golden(1 hunks)examples/non-examples/stack-traces/error-in-case-constructor.kl(1 hunks)examples/non-examples/stack-traces/error-in-cons-head.golden(1 hunks)examples/non-examples/stack-traces/error-in-cons-head.kl(1 hunks)examples/non-examples/stack-traces/error-in-constructor.golden(1 hunks)examples/non-examples/stack-traces/error-in-constructor.kl(1 hunks)examples/non-examples/stack-traces/error-in-let.golden(1 hunks)examples/non-examples/stack-traces/error-in-let.kl(1 hunks)examples/non-examples/stack-traces/error-in-list.golden(1 hunks)examples/non-examples/stack-traces/error-in-list.kl(1 hunks)examples/non-examples/stack-traces/error-in-pure-macro.golden(1 hunks)examples/non-examples/stack-traces/error-in-pure-macro.kl(1 hunks)examples/non-examples/stack-traces/in-arg-error.golden(1 hunks)examples/non-examples/stack-traces/in-arg-error.kl(1 hunks)examples/prelude-test.golden(1 hunks)examples/primitives-documentation.golden(2 hunks)examples/product-type.golden(1 hunks)examples/tiny-types.golden(1 hunks)examples/unknown-type.golden(1 hunks)examples/which-problem.golden(1 hunks)flake.nix(2 hunks)klister.cabal(1 hunks)repl/Main.hs(3 hunks)src/Debugger.hs(1 hunks)src/Evaluator.hs(12 hunks)src/Expander.hs(31 hunks)src/Expander/Error.hs(3 hunks)src/Expander/Monad.hs(13 hunks)src/Expander/Primitives.hs(7 hunks)src/Expander/Syntax.hs(6 hunks)src/Expander/TC.hs(9 hunks)src/Pretty.hs(8 hunks)src/Value.hs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (42)
- examples/non-examples/stack-traces/in-arg-error.golden
- examples/non-examples/stack-traces/error-in-arg.golden
- examples/non-examples/stack-traces/error-in-case-constructor.golden
- examples/non-examples/stack-traces/error-in-list.golden
- examples/non-examples/bad-lexical-env.golden
- examples/non-examples/stack-traces/error-in-cons-head.golden
- examples/non-examples/stack-traces/error-in-cons-head.kl
- examples/fix.golden
- examples/non-examples/stack-traces/in-arg-error.kl
- examples/non-examples/stack-traces/error-in-pure-macro.golden
- examples/non-examples/stack-traces/error-in-bind-head.golden
- examples/non-examples/stack-traces/error-in-bind-head.kl
- examples/non-examples/stack-traces/error-in-let.kl
- klister.cabal
- examples/error.golden
- examples/non-examples/stack-traces/error-in-pure-macro.kl
- examples/non-examples/stack-traces/error-in-arg.kl
- examples/non-examples/stack-traces/error-in-let.golden
- src/Expander/Error.hs
- examples/implicit-conversion-test.golden
- examples/non-examples/bad-lexical-env.kl
- flake.nix
- examples/tiny-types.golden
- examples/non-examples/stack-traces/error-in-bind-tail.golden
- examples/product-type.golden
- examples/non-examples/stack-traces/error-in-constructor.golden
- examples/non-examples/stack-traces/error-in-bind-tail.kl
- examples/non-examples/stack-traces/error-in-case-constructor.kl
- examples/unknown-type.golden
- examples/datatypes.golden
- examples/non-examples/stack-traces/error-in-list.kl
- examples/non-examples/stack-traces/error-in-constructor.kl
- examples/non-examples/error.golden
- examples/prelude-test.golden
- examples/higher-kinded.golden
- examples/which-problem.golden
- examples/eta-case.golden
- src/Value.hs
- repl/Main.hs
- examples/monad.golden
- examples/contract.golden
- src/Evaluator.hs
🔇 Additional comments (67)
examples/primitives-documentation.golden (5)
1-30: More descriptive closure identifiers improve readability.The change from generic
#<closure>to specific identifiers like#<open-syntax>,#<+>, etc. aligns with the PR objective of improving stack trace clarity. These descriptive names will make error messages more informative when these functions appear in stack traces.
19-24: Consistent operator naming pattern between number and string comparisons.Good consistency in applying the same naming pattern for both numeric and string comparison operators. This unified approach will help users understand error messages regardless of the data type being compared.
50-50: Consistent naming for I/O write function.The closure identifier for the
writefunction follows the descriptive naming pattern established for other primitives, which is good for consistency.
46-48:❓ Verification inconclusive
Parameter names shown instead of function names for higher-order functions.
For
id,const, andcompose, the closures show parameter names (#<x>,#<f>) rather than function names. This differs from other primitives that show function names and matches the discussion in PR comments about argument names vs. function names.Consider whether this inconsistency is intentional and how it impacts error message clarity for higher-order functions. Based on the PR comments, this might be related to how closures work in the context of the language.
🏁 Script executed:
#!/bin/bash # Check how these higher-order functions are implemented compared to other primitives grep -r "id.*closure\\|const.*closure\\|compose.*closure" --include="*.hs" src/Length of output: 150
Action: Verify closure identifier display for higher-order functions.
The documentation for the primitives
"id","const", and"compose"still shows parameter names (e.g.#<x>and#<f>) rather than explicit function names. The grep output fromsrc/Expander.hsindicates that the closure identifier is obtained viaclosureIdent(i.e.ident = view closureIdent clos), which suggests that this behavior may be intentional and part of the closure handling mechanism.Please verify whether this design choice meets the intended error message clarity and consistency across primitives, or if the higher-order functions should instead display their function names similar to other primitives.
9-14:❓ Verification inconclusive
Verify that symbolic operators remain clear in stack traces.
While the symbolic representation of comparison operators (e.g.,
#<>>,#<>=>,#<<>) is consistent with their function names, ensure these operators remain clear when they appear in stack traces. Consider testing how these look in actual error messages.
🏁 Script executed:
#!/bin/bash # Find examples of stack trace errors involving these comparison operators grep -r ">#\\|>=#\\|<#\\|<=#\\|=#\\|/=#" --include="*.golden" examples/Length of output: 68
Manual Verification Required for Symbolic Operator Clarity in Stack Traces
The original review comment's concern remains valid: we need to ensure that the symbolic operators (like
#<>>,#<>=>, etc.) are displayed clearly in stack traces. The initial grep command produced no evidence in the golden files. To verify this further, please run the following updated shell script, which searches for the exact operator tokens in the examples:
- Next Steps:
- If the script finds instances of these operators in error messages, confirm that they remain clear.
- If it still produces no output, please manually simulate error conditions or review runtime logs to verify that the stack traces display these symbolic operators properly.
🏁 Script executed:
#!/bin/bash # Improved search for explicit symbolic operator tokens in stack trace examples. grep -R "#<>>\|#<>=>\|#<<>\|#<<=>\|#<=>\|#</=>" --include="*.golden" examples/Length of output: 1468
Please Verify Actual Runtime Stack Traces for Clarity of Symbolic Operators
- The golden file (
examples/primitives-documentation.golden) clearly shows the expected symbolic operator tokens (e.g.,#<>>,#<>=>, etc.) that match their intended function names.- However, the current verification only confirms their appearance in documentation examples. It remains necessary to manually verify that these symbols are rendered clearly in actual stack traces (error messages) during runtime.
- Action Required: Please simulate error scenarios or review application logs to ensure that these symbolic operators are not misrepresented or ambiguous in stack traces. This check is critical to confirm that the clarity in the golden file translates to clarity in runtime errors.
src/Expander/Syntax.hs (12)
6-6: Acknowledge new language extension.The addition of
TypeOperatorsis appropriate if you're leveraging custom or symbolic type operators. Ensure that all necessary operators are indeed in use to justify enabling this extension.
18-18: New debugger import.Bringing in the
Debuggermodule aligns with the transition to debug-based error reporting. Confirm that this import won't introduce any unused-symbol warnings.
28-28: Switched to debug-based error reporting.Replacing the previous error/throw mechanism with
debug (NotIdentifier ...)looks consistent. Verify that continuing execution after logging an error is the desired behavior here.
36-36: Refactor to debug approach.The
mustBeConsfunction now callsdebug(NotCons other). Confirm if downstream code is ready to handle a debug path instead of an exception path.
40-40: Consistency with debug calls.The
mustBeConsConsfunction also follows the new pattern. Same caution regarding whether or not the code flow can proceed gracefully after logging.
45-45: Ensure post-debug flow is correct.For
mustBeList, check if calling code expects a structured list. Logging the error and continuing might cause unexpected behavior if the caller does not handle the invalid path.
53-53: Check string fallback logic.As with the other “mustBe…” checks, ensure that downstream logic gracefully handles the debug path when a string is not found.
60-60: Handling import errors.The
debug (ImportError err)pattern is consistent with the rest of the file. Confirm that any file-based errors are being flagged in a user-friendly way (e.g., logs or messages).
78-78: Length mismatch debugging.
NotRightLength lengths stxis helpful debug info. Ensure you are testing each scenario to confirm the displayed logs are sufficient.
80-80: Retaining flow after monitored error.Logging
NotListmight allow subsequent code to continue. Confirm that the calling code does not break due to a missing list structure.
157-158: Cons shape mismatch logs.Similar to earlier checks, ensure that
debug (NotCons other)anddebug (NotList other)will not lead to unexpected downstream logic if the shape was critically required.
167-167: Non-list shape fallback.Another instance of logging an invalid shape. Confirm consistency with the rest of your debugging error-handling strategy.
src/Expander.hs (13)
61-61: Debugger import alignment.This import fits the new debug-based approach used throughout. No immediate concerns.
153-153: File existence check logs.Replacing an exception/throw with
debugfor missing files. Consider whether a missing file should truly be recoverable or if a stronger failure mechanism is more appropriate.
199-199: Unexported identifier logging.
NotExported x pis captured via debug. Confirm that dependencies requiring an exported binding won’t silently fail if we only log instead of halting expansion.
370-370: Unknown identifier fallback.
debug (Unknown (Stx scs srcLoc x))logs the unknown reference. Check if subsequent logic doesn’t incorrectly assume a known identifier.
917-917: Macro evaluation error log.
MacroEvaluationError p $ constructErrorType "macro action" ...is consistent with the new approach. Just ensure macros that fail are short-circuited or controlled downstream.
921-921: Macro evaluation error (err variant).Same note as above; confirm that debugging an error is all that’s needed and that the pipeline detects and stops or recovers.
997-997: Short-circuiting macro action error.Likewise, watch out for partial expansions in a scenario that definitely cannot proceed if the macro action is invalid.
1016-1016: Missing type scenario.
InternalError "Type not found..."is debug-logged. Verify if missing types are genuinely expected or if this indicates a bigger logic bug that should abort.
1316-1316: Value not macro error.Check that the calling context indeed can continue if a value is incorrectly used as a macro. Potentially treat as a hard error if that’s never recoverable.
1326-1327: Non-syntax or invalid user macro.Logging the mismatch as
ValueNotSyntaxorValueNotMacrohelps debugging. Still confirm if there's a graceful fallback or if expansion is irretrievably broken.
1331-1331: Macro valued as plain value.This logs a mismatch, but be sure any partial expansions are prevented from producing invalid states or silent failures.
1337-1337: Declaration heading check.Same pattern as module heading. Confirm the code can’t proceed incorrectly if a declaration is not found or is the wrong shape.
1350-1350: Type pattern heading guard.Likewise, consider whether a logged debug message is enough or if you want a more robust error-handling approach for invalid type patterns.
src/Pretty.hs (4)
5-5: Enable LambdaCase.Using
LambdaCasecan simplify pattern matching on lambdas. Confirm usage to avoid unnecessary extension overhead.
10-20: Expanded export list.Exporting
ppBind,VarInfo(..), and more. Looks good for better reusability. Just ensure the rest of the codebase references these appropriately.
42-42: Evaluator types import.Importing
EStateandKontis crucial to pretty-print stack traces. This is in line with your new debugging approach. No immediate concerns.
821-1006: New stack trace and continuation pretty-printers.This large addition introduces helper functions (
printStack,printKont, etc.) to formatEStateandKontdata. Key considerations:
- Granularity: The verbose approach is excellent for debugging, but consider how it impacts performance if done frequently.
- Maintainability: The nested pattern matching can get unwieldy; keep an eye on code complexity if further expansions are added.
- Edge Cases: Confirm that special or unreachable states (e.g., partially applied continuations) are handled gracefully.
Overall, a solid improvement for developer debugging.
src/Expander/TC.hs (9)
20-20: New import for the debug system.The
Debuggermodule is now imported to support the new error handling approach that replaces direct error throwing with debug logging.
36-36: Replaced throwError with debug logging.Error handling for a dangling type metavar has been changed to use the new debug system instead of throwing an error directly.
82-83: Updated occurs check error handling.The occurs check failure error is now logged with
debugrather than throwing an exception, consistent with the new error handling approach.
113-113: Improved error handling for type variable mismatch.Error for mismatched number of type variables now uses the debug system, maintaining consistency with the global error handling approach.
198-198: Enhanced error handling for scheme generalization.The error for attempting to generalize in a scheme is now appropriately logged using the debug system.
292-293: Updated type mismatch error handling.Type mismatch errors are now logged with
debugrather than throwing exceptions, enabling better stack trace handling and error context retention.Also applies to: 296-296
305-305: Standardized type variable lookup error reporting.Error for missing type variables is now logged consistently using the debug system.
327-327: Unified kind mismatch error handling.Kind mismatch errors now use the debug system for better error reporting and consistency.
381-381: Standardized error handling for kind finding in open context.The error for attempting to find a kind in an open context now follows the global error handling pattern.
src/Expander/Monad.hs (11)
177-177: Added Debugger module import.The
Debuggermodule is imported to support the new debugging and error handling infrastructure.
208-212: Refactored Expand monad to use Debug.The
Expandmonad has been fundamentally changed to use the newDebug DebugContext ExpansionErrmonad instead ofExceptT ExpansionErr IO, providing enhanced debugging capabilities and stack trace information.
215-215: Updated execExpand to use runDebug.The
execExpandfunction now usesrunDebugwith the initial context to execute the monadic computation.It's important to confirm that partial expansions are consistently handled. Verify that incomplete tasks are properly handled or re-queued in the new Debug monad implementation to avoid silently skipped expansions.
599-600: Standardized error handling in getDeclGroup.Error for incomplete module after expansion now uses the debug system instead of throwing an exception directly.
610-611: Updated error handling in getDecl.Missing declaration errors are now logged through the debug system for better error tracking.
619-620: Unified error handling across zonkDecl.All error cases in the
zonkDeclfunction now use the debug system consistently, with source location information included where available.Also applies to: 623-624, 632-633, 648-649, 657-658, 661-662, 668-669
775-776: Enhanced constructor error reporting.Error for unknown constructors is now logged through the debug system with improved context.
785-786: Standardized datatype error handling.Error for unknown datatypes now uses the debug system for consistent error reporting.
851-852: Updated progress tracking in completely.The function now logs errors for tasks that cannot be completed, providing better visibility into blocked expansions.
867-870: Refactored evalInCurrentPhase error handling.Evaluation errors are now properly logged through the debug system with phase information, rather than being directly thrown.
929-931: Improved circular imports error handling.The circular imports error is now logged with debug information, providing better context about the import cycle.
src/Expander/Primitives.hs (5)
76-76: Added Debugger module import.The
Debuggermodule is imported to support the new debugging and error handling approach.
522-524: Updated type arity error handling.Error for incorrect type arity is now logged through the debug system instead of throwing an exception directly.
534-536: Standardized type pattern error handling.Error for type patterns with wrong arity now uses the debug system consistently.
592-592: Enhanced invalid type error handling.The error for invalid types in
makeLocalTypenow uses the debug system for better error tracking.
691-691: Updated unknown pattern error handling.Error for unknown patterns in
expandPatternCasenow uses the debug system, maintaining consistency across the codebase.src/Debugger.hs (8)
1-21: Well-structured module header and documentation.The module includes appropriate language extensions and comprehensive documentation explaining its purpose as a Common Lisp style debugger for Klister.
39-42: Well-designed Debug monad implementation.The
Debugnewtype is elegantly designed as a fusion ofReaderTandExceptTfor better control over the monad instance, capturing a computation that produces either a result or an error.
44-56: Comprehensive utility functions for Debug monad.The implementation includes essential functions like
debugRun,mapDebug,withDebug, andask'to support working with theDebugmonad.
82-92: Lifted MonadDebugger instances for common transformers.The implementation includes instances for
ReaderT,LazyState.StateT, andStrictState.StateT, allowing easy integration of debugging capabilities into various monad transformer stacks.
96-100: Clean MonadDebugger typeclass definition.The
MonadDebuggertypeclass is well-defined with essential methodsdebugandcatch, providing a clean interface for implementing debuggers.
104-110: Basic MonadDebugger implementation.The
Debug DebugContext einstance implements theMonadDebuggermethods with straightforward logic, creating a simple debugger that reports stack traces.
112-114: Simple DebugContext data structure.The
DebugContextdata type defines the context for debugging, including stack trace information.The comment suggests using
_stackTrace :: EState, but this should be clarified. Consider adding documentation explaining whyEStateis used instead of a stack of evaluator states, as suggested in a previous review comment.
116-117: Default initial context.The
initialContextfunction provides a sensible default context for starting debugging sessions.
| @@ -357,7 +357,7 @@ checkUnambiguous best candidates blame = | |||
| let bestSize = ScopeSet.size p best | |||
| let candidateSizes = map (ScopeSet.size p) (nub $ toList candidates) | |||
| if length (filter (== bestSize) candidateSizes) > 1 | |||
| then throwError (Ambiguous p blame candidates) | |||
| then debug (Ambiguous p blame candidates) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ambiguous binding resolution.
debug(Ambiguous p blame candidates) warns about ambiguous scope sets. Depending on usage, you might want to treat this as a hard error to avoid subtle shadowing bugs.
| throwError $ InternalError $ | ||
| other -> debug $ ValueNotSyntax other | ||
| other -> debug $ ValueNotMacro $ constructErrorType "error in user macro" other | ||
| Nothing -> debug $ InternalError $ | ||
| "No transformer yet created for " ++ shortShow ident ++ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Empty transformer scenario.
Nothing -> debug $ InternalError ... for macro creation. If this is truly impossible, consider an assertion or thorough fallback explanation for maintainers.
| instance Functor (Debug r e) where | ||
| fmap = mapDebug | ||
|
|
||
| instance Applicative (Debug r e) where | ||
| pure a = Debug $ const (return (Right a)) | ||
| Debug f <*> Debug v = Debug $ \rr -> do | ||
| mf <- f rr | ||
| case mf of | ||
| (Left fer) -> return (Left fer) | ||
| (Right k) -> do | ||
| mv <- v rr | ||
| case mv of | ||
| (Left ver) -> return (Left ver) | ||
| Right x -> return (Right (k x)) | ||
|
|
||
| instance Monad (Debug r e) where | ||
| Debug m >>= f = Debug $ \r -> do | ||
| ma <- m r | ||
| case ma of | ||
| Left err -> return (Left err) | ||
| Right val -> fmap (debugRun r) f val | ||
|
|
||
| instance MonadIO (Debug r e) where | ||
| liftIO = Debug . const . fmap Right | ||
|
|
||
| instance MonadDebugger e m => MonadDebugger e (ReaderT r m) where |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Standard typeclass instances for Debug monad.
The Functor, Applicative, Monad, and MonadIO instances are implemented correctly, enabling the use of standard monadic operations.
There's a potential issue in the Monad instance's bind implementation. The line fmap (debugRun r) f val looks suspicious and may not correctly apply the function in the expected monadic style. It should be running the result of f val (which is a Debug r e b) with context r. Verify this implementation with the following:
🏁 Script executed:
#!/bin/bash
# Look for other uses of fmap with debugRun to see if there are patterns
rg -B 5 -A 5 "fmap.*debugRun" src/
# Check if there are any issues reported related to this in tests or logs
rg "bind.*Debug" src/Length of output: 555
Action Needed: Clarify the bind implementation in the Debug Monad (src/Debugger.hs:57-82)
The implementation of the bind operator using
Right val -> fmap (debugRun r) f valrelies on the function functor (where fmap f g = f . g), which makes it equivalent to writing
Right val -> debugRun r (f val)—even though the current behavior is correct. However, this indirect style is confusing and risks misinterpretation. Please consider rewriting it for clarity (and verify that this change preserves the intended monadic semantics).
-
Suggested Change:
Right val -> fmap (debugRun r) f val + Right val -> debugRun r (f val) -
Next Steps:
- Manually verify that running
f valwith contextr(i.e. usingdebugRun r (f val)) indeed produces the expected behavior in all cases. - Confirm that no unintended side effects occur and that the test suite passes.
- Manually verify that running
Todos:
>>=tail andpair@gelisam @david-christiansen, sorry for the size of this one. I wanted to open this to get some eyes on it.
How can I test the
InMacroBindTLandpaircases? No matter what kind of klister I wrote, I could not get those to show up.Also, you'll notice that I chose to add a hardline to separate the error message from the error context. We should decide on a format in the next meeting
Summary by CodeRabbit
New Features
Enhancements