deterministic unification variables #262
Conversation
I want to thread a state throughout the value being pretty-printed, so I have to change all the pretty-printing code to be monadic. to make this easier to review, this commit only contains the refactoring, I am not yet making use of the new state.
WalkthroughThis set of changes updates error reporting and pretty-printing functionality in multiple modules. It introduces a new error example that highlights a type mismatch, adds a demonstration file with a prelude declaration, and refactors pretty-printing functions in both error and task modules to use do notation and monadic wrapping. Additionally, the pretty-printing module now incorporates state management for renumbering, and new functions for creating and inspecting store instances are added. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PrettyModule
participant StateManager
Caller->>PrettyModule: Invoke pretty/prettyPrint functions
PrettyModule->>StateManager: Initialize state (St.empty)
StateManager-->>PrettyModule: Provide state context
PrettyModule->>PrettyModule: Execute pp functions in state monad
PrettyModule-->>Caller: Return formatted output
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Expander/Error.hs (1)
112-287: LGTM! Improved readability with monadic style.The refactoring to use
donotation and bind intermediate values makes the code more readable and maintainable. The consistent pattern of binding pretty-printed values before composing them improves code clarity.Consider extracting common patterns into helper functions to reduce repetition. For example:
+ppWithLabel :: Pretty VarInfo a => VarInfo -> Text -> a -> State Renumbering (Doc ann) +ppWithLabel env label x = do + ppX <- pp env x + pure $ hang 2 $ group $ vsep [text label, ppX] pp env (NotList stx) = do - ppStx <- pp env stx - pure $ hang 2 $ group $ vsep [text "Expected parens, but got", ppStx] + ppWithLabel env "Expected parens, but got" stx
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
examples/non-examples/type-errors/not-a-function.golden(1 hunks)examples/non-examples/type-errors/not-a-function.kl(1 hunks)src/Expander/Error.hs(3 hunks)src/Expander/Task.hs(1 hunks)src/Pretty.hs(6 hunks)src/Util/Store.hs(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/non-examples/type-errors/not-a-function.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 (13)
src/Pretty.hs (5)
14-14: New imports for Set and for
No issues identified; these are used within the updated pretty-printing logic as intended.Also applies to: 21-21
65-69: Great approach to monadic pretty-printing
Changing these functions to use theevalState St.emptymonadic approach ensures consistent state management for renumbering, aligning with the PR's objective for deterministic error messages.Also applies to: 72-76, 78-80, 82-87, 89-93
96-103: Documentation, state type definition, and refinedppsignature
These updates provide clarity on how unification variables are tracked viaRenumbering, improving maintainability and consistency in pretty-printing. No further concerns.Also applies to: 104-104, 107-107, 110-110
239-243: Enhanced syntax error printing
Enclosing the message and locations in angle brackets, separated by a semicolon, helps visually parse the error message. Nicely implemented.
617-626: Value printing additions
The separate handling of IO actions, output ports, integers, constructors, and strings provides clear, distinct behavior for each value type. Good job maintaining consistency.examples/non-examples/type-errors/not-a-function.golden (1)
1-2: Deterministic unification variable placeholders
"Expected (Integer → (?1 → ?2)) but got Integer" showcases clear, stable placeholders for unification variables, matching the PR's goal.src/Util/Store.hs (3)
17-18: Exportingemptyandsize
Exposing these methods enriches the API for handlingStoreobjects more flexibly.Also applies to: 27-28
72-74:emptyfunction
Providing a named constructor for an empty store improves code readability over relying onmempty.
102-103:sizefunction
Offering this helper method aligns with common container patterns and makes it straightforward to retrieve store length.src/Expander/Task.hs (1)
113-113: LGTM! Consistent with monadic pretty-printing refactor.The change to wrap the return value with
purealigns with the broader refactoring effort to make pretty-printing functions return monadic values.src/Expander/Error.hs (3)
19-19: LGTM! Required import for monadic traversal.The addition of
Data.Traversable (for)is necessary for the monadic traversal used in the refactored pretty-printing functions.
289-321: LGTM! Clear error message composition.The refactoring to use
donotation makes the complex error message composition more readable and easier to maintain. The structured binding of intermediate values improves code clarity.
325-330: LGTM! Consistent monadic style.The changes to wrap return values with
purealign with the broader refactoring effort to make pretty-printing functions return monadic values.
|
Hey @gelisam, FYI I won't be able to get to this until Wednesday. |
|
No rush! |
|
This LGTM, I really don't have any complaints or nits. It makes sense that the tests were not covering the meta ptrs. Hopefully this paves the way to more coverage. |
Improves error messages from
to
which is not only more readable, it is also more deterministic, since the first error message prints a different number every time. the extra determinism makes it possible to write non-examples which produce a type error involving unification variables.
Summary by CodeRabbit
New Features
Refactor
Chores