SchemeVar newtype #257
SchemeVar newtype #257
Conversation
WalkthroughThe changes across multiple modules enhance type handling and expand functionality within the expander system. Key modifications include the introduction of a new type Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Expander
participant Store
User->>Expander: Initialize Kernel
Expander->>Store: Create SchemeVars
Expander->>Expander: Expand Expressions
Expander->>Store: Retrieve Values with (!)
Store-->>Expander: Return Value
Expander-->>User: Return Result
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (7)src/Expander.hs (7)
The function
The function
The
The
The inclusion of the comment reminds developers to add corresponding patterns in
The type arguments
The code appropriately generates 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: 8
🧹 Outside diff range and nitpick comments (4)
src/Util/Store.hs (1)
89-91: LGTM: Consider adding a Haddock comment.The implementation is correct and efficient, using
IM.toAscListand maintaining consistency withtoList. Consider adding a Haddock comment to document that the list is sorted by keys.-- | Returns a list of key/value pairs in ascending key order. toAscList :: HasKey p => Store p v -> [(p,v)]src/Type.hs (2)
39-44: LGTM! Consider adding documentation.The
SchemeVarnewtype and its instances are well-designed. The derived instances provide all necessary functionality for type variables in a type system.Consider adding Haddock documentation to explain:
- The purpose and invariants of
SchemeVar- Why
firstSchemeVarstarts at 0- Whether there are any constraints on valid
SchemeVarvalues
Line range hint
39-125: Well-structured type system enhancement.The introduction of
SchemeVarand its systematic integration throughout the type system is a good architectural improvement that:
- Enhances type safety by preventing accidental mixing of scheme variable IDs with raw natural numbers
- Makes the code more maintainable by making scheme variable usage explicit
- Provides a foundation for potential future extensions to the type system
The changes follow the "make invalid states unrepresentable" principle of type-driven design.
src/Expander/Monad.hs (1)
Line range hint
4-4: Add tests for the expander functionality.While the codebase demonstrates excellent practices in terms of documentation, error handling, and state management, there's a TODO comment indicating missing tests. Given the complexity of the expander system and the recent type variable representation changes, comprehensive tests would be valuable.
Would you like me to help create a test suite for this functionality?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/Expander.hs(4 hunks)src/Expander/Monad.hs(1 hunks)src/Expander/Primitives.hs(2 hunks)src/Expander/TC.hs(4 hunks)src/Pretty.hs(4 hunks)src/Type.hs(3 hunks)src/Util/Key.hs(1 hunks)src/Util/Store.hs(3 hunks)
🔇 Additional comments (16)
src/Util/Key.hs (2)
7-7: LGTM!
The import of Numeric.Natural is necessary for the new HasKey instance.
17-19: Implementation looks correct, but let's verify key usage patterns.
The implementation is sound for typical key-value store usage where keys are expected to be small, non-negative integers.
Let's verify our assumptions about key usage patterns:
✅ Verification successful
The previous results show that getKey/fromKey are primarily used in data structures (Store, Set) and for unique identifiers, but let's verify the complete context of Natural usage to ensure our assumptions about the safety of conversions are correct.
Implementation is safe for its intended use
The verification shows that Natural is used in controlled contexts:
- As binding levels and scheme variables in the type system
- For phase numbers in the module system
- For fixed-length list checking and type arity
- No evidence of large numeric operations or unsafe bounds
The HasKey Natural implementation using fromIntegral is appropriate because:
- Natural values are used for small, controlled counters (phases, binding levels)
- No instances of arithmetic that could lead to large numbers
- All conversions are within the safe range of Int for the observed use cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for patterns of key generation and usage to ensure they align with our assumptions
# about small, non-negative integers
# Look for key generation patterns
echo "Checking key generation patterns..."
rg -A 3 "getKey|fromKey" --type haskell
# Look for Natural key usage
echo "Checking Natural key usage patterns..."
rg -A 3 "Natural.*->.*Store|Store.*->.*Natural" --type haskell
Length of output: 7328
Script:
#!/bin/bash
# Let's check for Natural type usage in the codebase to understand the context better
echo "Checking Natural type usage..."
rg "Natural" --type haskell -B 2 -A 2
# Let's also check for any numeric bounds or range checks
echo "Checking for numeric bounds/range checks..."
rg "(maxBound|minBound|>=|<=|>|<).*Natural" --type haskell -B 2 -A 2
# Look for any imports of Natural to understand its source
echo "Checking Natural imports..."
rg "import.*Natural" --type haskell
Length of output: 6756
src/Util/Store.hs (1)
18-18: LGTM: Export list updated correctly.
The new functions are properly exported and maintain alphabetical ordering.
Also applies to: 22-22
src/Type.hs (2)
56-56: LGTM! Type-safe update to TSchemaVar.
The change from Natural to SchemeVar improves type safety by preventing accidental mixing of scheme variable IDs with other natural numbers.
125-125: Verify all TyLike instances are updated.
The signature change from Natural to SchemeVar is correct and consistent with the new type system.
Let's verify all instances have been updated:
✅ Verification successful
All TyLike instances are correctly updated with SchemeVar
Both instances of TyLike (TyF a and Ty) have been properly updated to use the new SchemeVar type in their tSchemaVar method implementations. No other instances exist that would need updating.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of TyLike that might need updating
ast-grep --pattern 'instance TyLike $_ $_'
Length of output: 1281
src/Pretty.hs (4)
314-314: LGTM: Variable reference updated correctly.
The change from typeVarNames to schemeVarNames is consistent with the renaming.
Line range hint 320-334: LGTM: Clear and descriptive variable naming.
The renaming from typeVarNames to schemeVarNames better reflects its purpose in the context of scheme variables. The implementation remains clean and efficient.
348-348: LGTM: Consistent use of new helper function.
The pretty-printing of TSchemaVar correctly uses the new schemeVarName helper function.
335-336: 🛠️ Refactor suggestion
Consider adding bounds checking.
The schemeVarName function could potentially crash if the index exceeds the list length. Consider adding bounds checking or using a safe indexing function.
-schemeVarName :: SchemeVar -> Text
-schemeVarName (SchemeVar n) = schemeVarNames !! fromIntegral n
+schemeVarName :: SchemeVar -> Text
+schemeVarName (SchemeVar n) =
+ let idx = fromIntegral n
+ in if idx < length schemeVarNames
+ then schemeVarNames !! idx
+ else error $ "SchemeVar index " ++ show n ++ " out of bounds"src/Expander/Monad.hs (1)
244-244: LGTM! Verify type variable handling across the codebase.
The change from Natural to SchemeVar for type variables improves type safety and semantic clarity. This aligns well with the PR objective of introducing the SchemeVar newtype.
Let's verify the consistency of this change across the codebase:
✅ Verification successful
Let me gather more context about the usage of SchemeVar to ensure complete verification.
Change from Natural to SchemeVar is consistently implemented
The verification shows that the transition from Natural to SchemeVar has been thoroughly and consistently implemented across the codebase:
SchemeVaris properly defined as a newtype wrapper overNaturalinsrc/Type.hs- All type variable handling code has been updated to use
SchemeVar:- Type constructor
ETypeVarinsrc/Expander/Monad.hs - Type instantiation in
src/Expander/TC.hs - Type variable preparation in
src/Expander/Primitives.hs - Type generalization in
src/Expander/TC.hs
- Type constructor
No remaining usages of Natural in type variable contexts were found, confirming complete migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining Natural usages in type variable contexts
# and verify SchemeVar adoption
# Search for potential Natural usage in type variable contexts
rg -p "Natural.*[Tt]ype[Vv]ar|[Tt]ype[Vv]ar.*Natural"
# Search for SchemeVar usage patterns to verify consistent adoption
rg -p "SchemeVar.*[Tt]ype[Vv]ar|[Tt]ype[Vv]ar.*SchemeVar"
Length of output: 272
Script:
#!/bin/bash
# Search for all SchemeVar definitions and usages
rg -p "SchemeVar" -A 3 -B 3
# Search for any remaining type variable related code
rg -p "[Tt]ype[Vv]ar" -A 3 -B 3
Length of output: 11803
src/Expander/Primitives.hs (1)
Line range hint 714-718: Confirmation: Function prepareTypeVar Correctly Updated to Use SchemeVar
The prepareTypeVar function's signature has been updated from accepting a Natural to a SchemeVar, which enhances type safety by encapsulating the Natural within a newtype. The implementation correctly handles the SchemeVar type, and the changes align with the overall refactoring effort.
src/Expander.hs (5)
547-552: bind-IO function definition is correct.
The bind-IO function is properly defined with the correct kinds, type variables, and implementation.
593-593: Addition of Maybe datatype is correct.
The Maybe datatype is correctly defined with kind [KStar] and constructors nothing and just.
595-600: Definition of List datatype is appropriate.
The List datatype is correctly defined with kind [KStar] and constructors nil and ::.
1228-1230: Type unification and instantiation logic is correctly implemented.
The unification of the type destination and datatype with type arguments, along with instantiation of argument types, is appropriate.
1243-1245: Pattern argument types are correctly instantiated.
The argument types for the pattern match are correctly instantiated using the type store.
pure-IO :: forall a. a -> IO a not pure-IO :: forall a b. a -> IO a Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
SchemeVarfor improved type handling.(!)for enhanced key-value retrieval in theStoretype.toAscListfunction for sorted key-value pairs in theStore.Bug Fixes
Documentation
Prettymodule.Refactor