execute macro actions in phase 1 #240
Conversation
WalkthroughThe recent changes introduce significant restructuring and renaming of data types and patterns across multiple modules. Key modifications include splitting Changes
Sequence Diagram(s) (Beta)No sequence diagrams are needed for these changes as they primarily involve renaming, restructuring, and minor functional updates. Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
Thinking about this more, I am even more convinced that I am right: in Racket, there is no separation between the pure function and the Macro effects, they happen at the same time. So of course they run in the same phase. |
gelisam
added
bug
|
@coderabbitai review |
Actions PerformedReview triggered.
|
![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
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/Expander.hs (2 hunks)
- src/Expander/Monad.hs (2 hunks)
- src/Phase.hs (2 hunks)
Additional Context Used
GitHub Check Runs (2)
ubuntu-latest / ghc 9.2.5 success (23)
src/Expander.hs: [warning] 413-413:
Pattern match(es) are non-exhaustive
src/Expander.hs: [warning] 431-431:
Pattern match(es) are non-exhaustive
src/Expander.hs: [warning] 433-433:
Pattern match(es) are non-exhaustive
src/Expander.hs: [warning] 413-413:
Pattern match(es) are non-exhaustive
src/Expander.hs: [warning] 431-431:
Pattern match(es) are non-exhaustive
src/Expander.hs: [warning] 433-433:
Pattern match(es) are non-exhaustivestack / ghc 9.2.8 success (13)
src/Expander.hs: [warning] 413-413:
Pattern match(es) are non-exhaustive
src/Expander.hs: [warning] 431-431:
Pattern match(es) are non-exhaustive
src/Expander.hs: [warning] 433-433:
Pattern match(es) are non-exhaustive
Path-based Instructions (3)
src/Phase.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Expander/Monad.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Expander.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
Additional comments not posted (3)
src/Phase.hs (1)
37-39: The implementation ofposteriorcorrectly calculates a phase that is one step earlier than the given phase.src/Expander/Monad.hs (1)
411-414: The implementation ofinLaterPhasecorrectly defers operations to a later phase using theposteriorfunction.src/Expander.hs (1)
1435-1436: Ensure identifier resolution is consistent with new phase adjustments.
src/Expander.hs
Outdated
| case res of | ||
| StuckOnType loc ty env cases kont -> | ||
| forkAwaitingTypeCase loc prob ty env cases kont | ||
| inEarlierPhase $ forkAwaitingTypeCase loc prob ty env cases kont |
There was a problem hiding this comment.
I'm afraid I have opened a can of worms.
First, without this inEarlierPhase, the Macro actions which precede (type-case ...) execute in phase 1 while the Macro actions which follow it execute in phase 0. So something needs to be fixed.
Unfortunately, adding this inEarlierPhase is not the correct solution, because when the macro returns a Syntax object after the (type-case ...), that Syntax is expanded in phase 1 instead of phase 0. For example,
#lang "prelude.kl"
(import (shift "prelude.kl" 1))
(datatype (T)
(mkT))
(define-macros
([my-macro
(lambda (stx)
(pure '(mkT)))]))
(example (the (T) (my-macro)))
returns (mkT) as expected, but
#lang "prelude.kl"
(import (shift "prelude.kl" 1))
(datatype (T)
(mkT))
(define-macros
([my-macro
(lambda (stx)
(>>= (which-problem)
(lambda (problem)
(case problem
[(expression type)
(type-case type
[(else _)
(pure '(mkT))])]))))]))
(example (the (T) (my-macro)))
Fails with Unknown: <mkT> because mkT is only bound in phase 0, not in phase 1. This seems relatively easy to fix: instead of forking the job in an earlier phase, fork a job which executes the Macro action in an earlier phase and then returns the Syntax in the current phase.
There was a problem hiding this comment.
The second, more important problem occurs when type-case matches on (T). Since the Macro action now executes in phase 1, type T, which is only bound in phase 0, is not found. The solution seems simple: just look up T in the later phase, right? Well... what about else? If we look up else in the later phase, then it is now else which is not found!
This makes sense: type-pattern is a Problem, so it should be possible to write macros which expand to a type-pattern. Since the body of my-macro is in phase 1, those macros should be from phase 2, and so should else. So type-case is in a difficult situation where it encounters identifiers and it does not know in which phase it should expand them.
I think the solution is, sadly, to reject the syntax
(type-case type
[(T) ...]
[(else _) ...])
In favor of a dedicated phase 2 macro which specifies that its argument is a type from phase 0.
(type-case type
[(the T) ...]
[(else _) ...])
or perhaps
(type-case type
['(T) ...]
[(else _) ...])
There was a problem hiding this comment.
Or maybe datatype should bind the type-pattern macros at a different phase? That is,
(datatype (Maybe A)
(nothing)
(just A))
should bind a Maybe macro at phase 0 which expects to be run in the type Problem at phase 0, and a Maybe macro at phase 1 which expects to be run in the type-pattern Problem at phase 1.
One obvious problem with this idea is that if I then try to make Maybe available in the macro definition:
(import "maybe-datatype.kl")
(import (shift "maybe-datatype.kl" 1))
I now have two conflicting Maybe macros at phase 1: the one which expects to run in the type-pattern Problem at phase 1, and the shifted one which expects to run in the type Problem at phase 1. Maybe it's time to implement the strategy we discussed, where multiple macros are allowed to have the same name, as long as all but one indicate that they do not expect to be called in that context (#241)?
There was a problem hiding this comment.
Instead of giving a new meaning to the or quote, I am currently implementing a new primitive macro called type-constructor, used like this:
(type-case type
[(type-constructor Either a b) ...])
There was a problem hiding this comment.
The worms continue to come out of the can.
A datatype is uniquely identified by a module, a phase, and their true name (a String). But when we (import "either-datatype.kl"), we don't add that true name to the bindings of that phase. We add the primitive type macro named Either to the bindings of that phase. The name of the datatype is not necessarily the same as the name of the macro, because of scopes and shadowing. For example,
(datatype (Either A B)
(left A)
(right B))
(datatype (Either A B)
(left A)
(right B))
will define two datatypes, whose true names are Either and Either0 respectively. And since the second Either shadows the first, after importing that module the primitive type macro named Either now refers to the datatype whose true name is Either0.
Anyway, for that reason, (type-constructor Either a b) should not simply look up the datatype named Either, because the true name for that datatype might be Either0. Instead, it must use the name Either, and find that it refers to a primitive type macro.
A primitive type macro happens to be represented by the data constructor EPrimTypeMacro, but there is nothing in the Klister codebase which ever matches on a data constructor of EValue, so it would be weird for type-constructor to look up the EValue for Either, check if it is an EPrimTypeMacro, and then find the true name from that somehow. Instead, macros are always expanded (expandOneForm is the one function which does match on the EValue data constructors in order to accomplish this).
What does a type macro expand to? While we usually think of a macro as a function which produces a Syntax object, it is actually only user macros which expand to Syntax objects. Primitive macros have the side-effect of filling in ("linking") the mortise (e.g. a TypePatternPtr) at which the macro is expanded with a tenon (e.g. a TypePattern).
Primitive type macros, in particular, can either be expanded to fill a type mortise or a type-pattern mortise. In this case, a type-pattern mortise makes more sense.
This means that (type-constructor Either a b) is a phase 0 type-pattern would work by expanding (Either a b) in phase 0 type-pattern. I don't like the idea of (Either a b) being a valid type-pattern, it is too error-prone because matching on (Either a b) will work some of the time (when "either-datatype.kl" and (shift "either-datatype.kl" 1) are both imported), whereas at other time it will mysteriously fail (when Either refers to a different type at phase 0 and phase 1... which it does) and the type-constructor wrapper will be needed.
Which means that the final worm coming out the can is that I should create a new Problem, the type-constructor Problem. The type-pattern (type-constructor Either a b) at phase 1 will expand Either at phase 0 in the type-constructor Problem, obtaining the true name, and then using it to construct a TypePattern.
Phew, what an adventure!
previously, "else" was considered a "pattern primitive", even though it is valid in both the pattern and the type pattern Problems. And "with-unknown-macro" was considered a "universal primitive", because it works in any Problem. Now, a "pattern primitive" means a pattern which is _only_ valid in the pattern Problem, and a "type pattern primitive" (a new term!) means a pattern which is _only_ valid in the type pattern Problem. There are no such primitives yet, but I plan to add a type pattern primitive, this commit is in preparation for that. Finally, a "poly-problem primitive" now means a primitive which works in more than one Problem; all of them in the case of "with-unknown-type", and two of them in the case of "else".
forkAwaitingTypeCase schedules a job which (eventually) executes the Macro actions which follow the type-case. shifting the phase before forking did cause that Macro action to be executed in a shifted phase, but also had the unintended effect of shifting the phase at which to interpret the Syntax returned by the macro. The solution: don't shift the fork call, but do shift the Macro action it eventually executes.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
src/Expander.hs (2)
Line range hint
415-415: Address potential non-exhaustive pattern matching.- traverse_ (uncurry addTypePatternPrimitive) typePatternPrims + traverse_ (uncurry addTypePatternPrimitive) (typePatternPrims ++ [("default", defaultTypePatternImpl)])Consider adding a default case to handle unexpected inputs gracefully. This will prevent runtime errors due to non-exhaustive pattern matching.
Line range hint
433-433: Ensure all pattern matches are exhaustive to prevent runtime exceptions.- traverse_ (uncurry addPatternPrimitive) patternPrims + traverse_ (uncurry addPatternPrimitive) (patternPrims ++ [("default", defaultPatternImpl)]) - traverse_ (uncurry addTypePatternPrimitive) typePatternPrims + traverse_ (uncurry addTypePatternPrimitive) (typePatternPrims ++ [("default", defaultTypePatternImpl)])Adding default cases for pattern matching in
addPatternPrimitiveandaddTypePatternPrimitivefunctions will safeguard against potential runtime errors from unexpected cases.Also applies to: 435-435
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- src/Core.hs (4 hunks)
- src/Evaluator.hs (3 hunks)
- src/Expander.hs (15 hunks)
- src/Expander/Error.hs (8 hunks)
- src/Expander/Monad.hs (3 hunks)
- src/Expander/Primitives.hs (7 hunks)
- src/Expander/Syntax.hs (3 hunks)
- src/Pretty.hs (1 hunks)
- toy.kl (1 hunks)
Additional Context Used
GitHub Check Runs (3)
ubuntu-latest / ghc 9.4 success (13)
src/Expander/Syntax.hs: [warning] 70-70:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 104-104:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 110-110:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 110-110:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 116-116:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 116-116:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 116-116:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 122-122:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 122-122:
The use of ‘~’ without TypeOperatorsubuntu-latest / ghc 9.6 success (13)
src/Expander/Syntax.hs: [warning] 70-70:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 104-104:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 110-110:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 110-110:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 116-116:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 116-116:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 116-116:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 122-122:
The use of ‘~’ without TypeOperators
src/Expander/Syntax.hs: [warning] 122-122:
The use of ‘~’ without TypeOperatorsstack / ghc 9.2.8 success (13)
src/Expander.hs: [warning] 415-415:
Pattern match(es) are non-exhaustive
src/Expander.hs: [warning] 433-433:
Pattern match(es) are non-exhaustive
src/Expander.hs: [warning] 435-435:
Pattern match(es) are non-exhaustive
src/Expander/Primitives.hs: [warning] 820-820:
Pattern match(es) are non-exhaustive
src/Expander/Primitives.hs: [warning] 826-826:
Pattern match(es) are non-exhaustive
src/Expander/Primitives.hs: [warning] 828-828:
Pattern match(es) are non-exhaustive
src/Expander/Primitives.hs: [warning] 834-834:
Pattern match(es) are non-exhaustive
src/Expander/Primitives.hs: [warning] 836-836:
Pattern match(es) are non-exhaustive
src/Expander/Primitives.hs: [warning] 845-845:
Pattern match(es) are non-exhaustive
src/Expander/Primitives.hs: [warning] 847-847:
Pattern match(es) are non-exhaustive
Path-based Instructions (8)
src/Expander/Syntax.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Expander/Error.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Evaluator.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Core.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Pretty.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Expander/Primitives.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Expander/Monad.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Expander.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
Additional comments not posted (20)
toy.kl (2)
32-41: Simplified macro definition aligns with the PR objectives to handle macro actions consistently in a specific phase.
42-42: Example usage correctly demonstrates the new behavior ofmy-macro.src/Expander/Syntax.hs (1)
Line range hint
83-132: UsingNonEmptylists improves the robustness by ensuring that functions handle cases where at least one item is expected.src/Expander/Error.hs (2)
Line range hint
50-65: Enhanced error handling usingNonEmptylists and new error types improves clarity and precision.Also applies to: 95-95
109-119: Introduction of new syntactic categories likely supports expanded functionality and new features in the expander.src/Evaluator.hs (2)
Line range hint
287-303: Changes ensure correct pattern matching for data constructors, enhancing the correctness of the evaluator.
318-318: Improved handling of type patterns ensures robust matching of type constructors and their arguments.src/Core.hs (4)
76-77: RenamedTypePatterntoTypeCtorPatternandTypePatternVarto better distinguish between constructors and variable patterns.
81-81: RenamedConstructorPatternFtoDataCtorPatternand addedPatternVar. This change clarifies the distinction between data constructors and pattern variables, aligning with the changes inTypePattern.
394-395: UpdatedalphaCheckimplementations forDataCtorPatternandTypeCtorPatternto reflect the new pattern structures. Ensure that these changes are consistent with the intended equality checks for these types.Also applies to: 404-405, 407-408
602-602: UpdatedshortShowimplementations forDataCtorPatternandTypeCtorPattern. These changes are necessary to ensure that the new pattern names are correctly represented in debug outputs or logs.Also applies to: 610-610, 612-613
src/Pretty.hs (1)
213-215: UpdatedppBindimplementations forTypeCtorPatternandDataCtorPatternto reflect the new pattern structures. These changes ensure that the pretty-printing functionality aligns with the new data types and their usage.Also applies to: 222-222
src/Expander/Primitives.hs (3)
11-11: Introduction of new type aliases for primitives enhances modularity.These type aliases (
DeclPrim,ExprPrim,PatternPrim,TypePatternPrim,ModulePrim,PolyProblemPrim) help in making the code more readable and maintainable by clearly separating different kinds of primitives.Also applies to: 20-20, 49-49, 51-51, 53-53, 56-56
565-565: ThemakeModulefunction now properly handles module expansion.This change ensures that the module expansion process is isolated, preventing conflicts by checking if a module is already being expanded. This is a robust design choice that prevents state corruption.
669-669: Introduction ofelsePatternfor handling different syntactic categories.This function adds flexibility in pattern matching by accommodating different types of patterns (
PatternDest,TypePatternDest). It's a good use of Haskell's pattern matching capabilities to handle various cases distinctly.src/Expander/Monad.hs (2)
412-415: The implementation ofinLaterPhasecorrectly adjusts the phase context for executing actions in a later phase.Please ensure that the
posteriorfunction is correctly defined and behaves as expected.
241-243: The addition ofEPrimPatternMacro,EPrimTypePatternMacro, andEPrimPolyProblemMacroto theEValuedata type enhances the flexibility and capability of the macro system to handle various pattern and problem types.These changes are well-integrated and follow the established patterns in the codebase.
src/Expander.hs (3)
391-392: Add new primitives for handling type patterns and polymorphic problems.This addition aligns with the PR's objective to handle macro actions and evaluations more consistently across phases. The introduction of
addTypePatternPrimitiveandaddPolyProblemPrimitivefunctions will help in managing type patterns and polymorphic problems more effectively within the kernel initialization.
743-750: Introduce new type pattern primitives to the kernel.The implementation of
addTypePatternPrimitivefunction is a crucial addition for handling type patterns effectively. This change supports the PR's goal of enhancing macro handling by ensuring that type patterns are processed correctly in the kernel.
783-785: Add new primitives for handling polymorphic problems.The introduction of
addPolyProblemPrimitivefunction is a significant enhancement for dealing with polymorphic issues in macros. This aligns with the PR's objectives to improve macro handling and error resolution.
| (TypeCtorPattern $ TyF ctor [ (varStx, var) | ||
| | (_, varStx, var) <- varInfo | ||
| ]) |
There was a problem hiding this comment.
Ensure the new pattern matching cases are exhaustive.
The pattern matching in the TypeCtorPattern and TypePatternVar seems non-exhaustive. Consider adding default cases or handling all possible patterns to avoid runtime errors.
-- For TypeCtorPattern
_ -> error "Unhandled pattern case for TypeCtorPattern"
-- For TypePatternVar
_ -> error "Unhandled pattern case for TypePatternVar"Also applies to: 682-682
I want to add a type pattern macro in phase 1 which matches on a type constructor in phase 0. The only way to lookup a type constructor is to expand the name of that type constructor (so that hygiene rules apply properly). That means we now need macros which return type constructors, not types nor type patterns.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
src/Evaluator.hs (1)
Line range hint
16-16: Remove the redundant import ofData.Textto clean up the code.- import qualified Data.Text as Tsrc/Core.hs (1)
Line range hint
25-25: Remove the redundant import ofData.Listto clean up the code.- import Data.List
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
examples/primitives-documentation.goldenis excluded by!**/*.golden
Files selected for processing (24)
- examples/primitives-documentation.kl (1 hunks)
- src/Binding.hs (2 hunks)
- src/Binding/Info.hs (1 hunks)
- src/Core.hs (6 hunks)
- src/Datatype.hs (2 hunks)
- src/Evaluator.hs (5 hunks)
- src/Expander.hs (22 hunks)
- src/Expander/Error.hs (8 hunks)
- src/Expander/Monad.hs (6 hunks)
- src/Expander/Primitives.hs (8 hunks)
- src/Expander/Task.hs (3 hunks)
- src/ModuleName.hs (2 hunks)
- src/PartialCore.hs (2 hunks)
- src/Phase.hs (2 hunks)
- src/Pretty.hs (4 hunks)
- src/ScopeSet.hs (2 hunks)
- src/ShortShow.hs (1 hunks)
- src/SplitCore.hs (4 hunks)
- src/Syntax/SrcLoc.hs (3 hunks)
- src/Syntax/Syntax.hs (2 hunks)
- src/Type.hs (2 hunks)
- stdlib/prelude.kl (1 hunks)
- tests/Test.hs (4 hunks)
- toy.kl (1 hunks)
Files skipped from review due to trivial changes (6)
- examples/primitives-documentation.kl
- src/Binding.hs
- src/ModuleName.hs
- src/ShortShow.hs
- src/Type.hs
- stdlib/prelude.kl
Files skipped from review as they are similar to previous changes (4)
- src/Expander/Error.hs
- src/Expander/Monad.hs
- src/Expander/Primitives.hs
- toy.kl
Additional context used
Path-based instructions (14)
src/Binding/Info.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Syntax/SrcLoc.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/PartialCore.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Phase.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Datatype.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Expander/Task.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Syntax/Syntax.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/ScopeSet.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/SplitCore.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Evaluator.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Core.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Pretty.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
tests/Test.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
src/Expander.hs (1)
Pattern
**/*.hs: Dont mention changes to the import statements nor to the language extensions at the top of the file.Dont point out missing error-handling code, the type-checker already detects those mistakes at compile-time.
Do explain what the new code does and how changes to existing functions affect their behavior.
GitHub Check: ubuntu-latest / ghc 8.10.7
src/Evaluator.hs
[warning] 16-16:
The qualified import of ‘Data.Text’ is redundantsrc/Core.hs
[warning] 25-25:
The import of ‘Data.List’ is redundantsrc/Pretty.hs
[failure] 701-701:
• No instance for (Pretty VarInfo MacroDest)
[failure] 703-703:
• No instance for (Pretty VarInfo SplitType.SplitTypePtr)
[failure] 704-704:
• No instance for (Pretty VarInfo AfterTypeTask)
[failure] 707-707:
• No instance for (Pretty
[failure] 719-719:
• No instance for (Pretty VarInfo Var) arising from a use of ‘pp’
GitHub Check: stack / ghc 9.2.8
src/Evaluator.hs
[warning] 16-16:
The qualified import of ‘Data.Text’ is redundantsrc/Core.hs
[warning] 25-25:
The import of ‘Data.List’ is redundantsrc/Pretty.hs
[failure] 701-701:
• No instance for (Pretty VarInfo MacroDest)
[failure] 703-703:
• No instance for (Pretty VarInfo SplitType.SplitTypePtr)
[failure] 704-704:
• No instance for (Pretty VarInfo AfterTypeTask)
[failure] 707-707:
• No instance for (Pretty
[failure] 719-719:
• No instance for (Pretty VarInfo Var) arising from a use of ‘pp’
Additional comments not posted (15)
src/Binding/Info.hs (1)
Line range hint
1-10: The implementation ofBindingInfolooks good and aligns with Haskell's data type declaration standards.Consider addressing the TODO comment in a future update to enhance functionality.
src/Syntax/SrcLoc.hs (1)
Line range hint
1-20: TheSrcLocandSrcPosdata types are well-defined, and theAlphaEqinstance is correctly implemented.src/PartialCore.hs (1)
Line range hint
9-31: The introduction ofPartialPatternandPartialCorealong with their associated functions is well-implemented, using idiomatic Haskell patterns liketraverseandMaybe.src/Phase.hs (1)
Line range hint
6-34: ThePhasetype and its manipulation functions (runtime,prior,posterior) are correctly implemented. ThePhasedinstances are also correctly defined to handle phase shifts.src/Datatype.hs (1)
Line range hint
1-50: The data typesDatatype,Constructor, and their associated information types are well-defined. The use ofHashableandAlphaEqinstances is appropriate and well-implemented.src/Expander/Task.hs (1)
32-33: The addition of theTypeCtorDestconstructor toMacroDestis well-integrated and aligns with the changes in type handling across the codebase.src/SplitCore.hs (1)
83-97: IntroducedTypeCtorPtrto handle type constructors. Ensure that this new type is integrated correctly across the system.Verification successful
The
TypeCtorPtrtype is introduced and used in several files, indicating it is integrated into the system. Here are the specific locations:
src/SplitCore.hs: Definition and instances.src/Pretty.hs: Pretty instance.src/Expander.hs: Functions usingTypeCtorPtr.src/Expander/Task.hs: TypeCtorDest usage.src/Expander/Primitives.hs: TypeCtorPrim type alias.src/Expander/Monad.hs: EPrimTypeCtorMacro usage.To ensure proper integration, let's review the specific lines in each file where
TypeCtorPtris used.
The
TypeCtorPtrtype is used consistently across multiple files, indicating proper integration into the system. Here are the specific locations and contexts where it is used:
src/Pretty.hs:
- Line 745:
instance Pretty VarInfo TypeCtorPtr wheresrc/Expander.hs:
- Line 764:
addTypeCtorPrimitive :: Text -> (TypeCtorPtr -> Syntax -> Expand ()) -> Expand ()- Line 1176:
expandOneTypeCtor :: TypeCtorPtr -> Syntax -> Expand ()- Line 1240:
requireTypeCtorCat :: Syntax -> MacroDest -> Expand TypeCtorPtrsrc/Expander/Task.hs:
- Line 32:
| TypeCtorDest TypeCtorPtrsrc/Expander/Primitives.hs:
- Line 598:
type TypeCtorPrim = TypeCtorPtr -> Syntax -> Expand ()src/Expander/Monad.hs:
- Line 242:
| EPrimTypeCtorMacro (TypeCtorPtr -> Syntax -> Expand ())These usages show that
TypeCtorPtris integrated into various parts of the system, including pretty-printing, expansion tasks, and monadic operations. The integration appears consistent and correct.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `TypeCtorPtr` is used consistently across the system. # Test: Search for the type usage. Expect: Consistent usage across relevant files. rg --type haskell $'TypeCtorPtr'Length of output: 1027
Script:
#!/bin/bash # Review the specific lines in each file where `TypeCtorPtr` is used. # Extract lines from src/Pretty.hs rg --type haskell --line-number --context 5 $'TypeCtorPtr' src/Pretty.hs # Extract lines from src/Expander.hs rg --type haskell --line-number --context 5 $'TypeCtorPtr' src/Expander.hs # Extract lines from src/Expander/Task.hs rg --type haskell --line-number --context 5 $'TypeCtorPtr' src/Expander/Task.hs # Extract lines from src/Expander/Primitives.hs rg --type haskell --line-number --context 5 $'TypeCtorPtr' src/Expander/Primitives.hs # Extract lines from src/Expander/Monad.hs rg --type haskell --line-number --context 5 $'TypeCtorPtr' src/Expander/Monad.hsLength of output: 3256
src/Pretty.hs (2)
215-217: Ensure correct binding for new pattern types.The new pattern types
TypeCtorPatternandTypePatternVarare correctly handled in thePrettyBinderinstance, ensuring they are pretty-printed appropriately.
220-224: Check the handling ofDataPatternandDataCtorPattern.The implementation for
DataPatternandDataCtorPatternin thePrettyBinderinstance looks correct and should handle the pretty-printing of these patterns as expected.tests/Test.hs (2)
610-611: The update in the function signature ofgenLamfromConstructorPatterntoDataPatternaligns with the global renaming and restructuring of pattern data types across the codebase. This ensures consistency and clarity in type usage.
623-626: The update in the function signature ofgenCoreFfromConstructorPatterntoDataPatternis consistent with the renaming and restructuring of pattern data types throughout the codebase. This change enhances clarity and maintains consistency in type usage.src/Expander.hs (4)
55-55: The import ofData.List.NonEmptywith explicit import ofNonEmpty((:|))is a good practice as it ensures that only the necessary parts of the module are imported, reducing namespace clutter and potential conflicts.
601-609: The expansion of theProblemdatatype to includetype-constructoris a direct reflection of the PR's focus on enhancing type handling capabilities. This change should help in better categorizing problems related to type constructors, which is crucial for debugging and error reporting in language processing.
803-805: The addition ofaddPolyProblemPrimitiveto handle polymorphic problems is a significant enhancement. This should allow for more flexible and powerful macro processing capabilities, especially in handling various polymorphic scenarios.
391-393: The addition of new primitives (addTypePatternPrimitive,addTypeCtorPrimitive,addPolyProblemPrimitive) is aligned with the PR's objective to handle macro actions and type patterns more robustly. Ensure that these primitives are well-documented and tested to maintain code quality.Would you like assistance in creating documentation or tests for these new primitives?
| pp env (AwaitingDefn _x n _b _defn _t _dest stx) = | ||
| "(AwaitingDefn" <+> pp env n <+> pp env stx <> ")" | ||
| pp env (AwaitingMacro dest t) = | ||
| "(AwaitingMacro" <+> pp env dest <+> pp env t <> ")" |
There was a problem hiding this comment.
Resolve missing instances for pretty-printing.
+ instance Pretty VarInfo MacroDest where
+ pp _ = viaShow -- Placeholder, adjust as needed
+ instance Pretty VarInfo SplitType.SplitTypePtr where
+ pp _ = viaShow -- Placeholder, adjust as needed
+ instance Pretty VarInfo AfterTypeTask where
+ pp _ = viaShow -- Placeholder, adjust as needed
+ instance Pretty VarInfo Var where
+ pp _ = viaShow -- Placeholder, adjust as neededThe static analysis has identified missing instances for Pretty VarInfo for several types. These instances are necessary for the pp function to work correctly across all usages in this file.
Also applies to: 703-703, 704-704, 707-707, 719-719
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.
| "(AwaitingMacro" <+> pp env dest <+> pp env t <> ")" | |
| "(AwaitingMacro" <+> pp env dest <+> pp env t <> ")" | |
| instance Pretty VarInfo MacroDest where | |
| pp _ = viaShow -- Placeholder, adjust as needed | |
| instance Pretty VarInfo SplitType.SplitTypePtr where | |
| pp _ = viaShow -- Placeholder, adjust as needed | |
| instance Pretty VarInfo AfterTypeTask where | |
| pp _ = viaShow -- Placeholder, adjust as needed | |
| instance Pretty VarInfo Var where | |
| pp _ = viaShow -- Placeholder, adjust as needed |
Tools
GitHub Check: ubuntu-latest / ghc 8.10.7
[failure] 701-701:
• No instance for (Pretty VarInfo MacroDest)
GitHub Check: stack / ghc 9.2.8
[failure] 701-701:
• No instance for (Pretty VarInfo MacroDest)
| addTypePatternPrimitive :: | ||
| Text -> (TypePatternPtr -> Syntax -> Expand ()) -> Expand () | ||
| addTypePatternPrimitive name impl = do | ||
| let val = EPrimTypePatternMacro impl | ||
| b <- freshBinding | ||
| bind b val | ||
| addToKernel name runtime b | ||
|
|
||
| addTypeCtorPrimitive :: | ||
| Text -> (TypeCtorPtr -> Syntax -> Expand ()) -> Expand () | ||
| addTypeCtorPrimitive name impl = do | ||
| let val = EPrimTypeCtorMacro impl | ||
| b <- freshBinding | ||
| bind b val | ||
| addToKernel name runtime b |
There was a problem hiding this comment.
Tip
Codebase Verification
The typePatternPrims and typeCtorPrims lists are defined but not populated anywhere in the codebase. This indicates that the integration of addTypePatternPrimitive and addTypeCtorPrimitive might be incomplete, as these functions rely on these lists being populated to function correctly.
src/Expander.hs:typePatternPrimsandtypeCtorPrimsare defined but remain empty.
Analysis chain
The introduction of addTypePatternPrimitive and addTypeCtorPrimitive functions is crucial for handling new types of patterns and constructors more effectively in the expander module. It's important to ensure that these functions are integrated correctly with the rest of the system to handle the respective patterns and constructors.
To ensure the correct integration of addTypePatternPrimitive and addTypeCtorPrimitive, we need to verify the definitions and usage of typePatternPrims and typeCtorPrims in src/Expander.hs.
The typePatternPrims and typeCtorPrims are defined as empty lists in src/Expander.hs. To ensure proper integration, we need to check if these lists are populated elsewhere in the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of new type pattern and constructor primitives.
# Test: Search for the integration points of these new primitives. Expect: Correct integration with the system.
rg --type haskell $'addTypePatternPrimitive|addTypeCtorPrimitive'
Length of output: 424
Script:
#!/bin/bash
# Description: Verify the definitions and usage of typePatternPrims and typeCtorPrims in src/Expander.hs
# Search for the definitions of typePatternPrims and typeCtorPrims
rg --type haskell 'typePatternPrims|typeCtorPrims' src/Expander.hs
Length of output: 343
Script:
#!/bin/bash
# Description: Verify if typePatternPrims and typeCtorPrims are populated elsewhere in the codebase
# Search for any assignments or modifications to typePatternPrims and typeCtorPrims
rg --type haskell 'typePatternPrims|typeCtorPrims'
Length of output: 423
test command: ghcid --test=':set -XOverloadedStrings' --test='ShortShow.shortShow $ Core.Core $ Core.CoreDataCase (Syntax.SrcLoc.SrcLoc "" (Syntax.SrcLoc.SrcPos 0 0) (Syntax.SrcLoc.SrcPos 0 0)) (Core.Core $ Core.CoreInteger 42) [(Core.DataPattern $ Core.DataCtorPattern (Datatype.Constructor (ModuleName.KernelName ModuleName.kernelName) "just") [], Core.Core $ Core.CoreInteger 42)]'
As discussed here, if
my-macrois a user-defined macro whose(-> Syntax (Macro Syntax))ismyMacroImpland(my-macro)is encountered at phase 0, then the current code will evaluate(myMacroImpl '(my-macro))at phase 1 but will execute the resulting(Macro Syntax)at phase 0. This seems wrong to me, I think it should also execute at phase 1.I have changed the code so that the
(Macro Syntax)is also executed at phase 1. The only consequence of this change which was found by the test suite was to breakfree-identifier=?. Consider this code:When the
(free-identifier=? x 'my-keyword)action is executed, then the current implementation will look up "my-keyword" and "my-keyword" in the current phase (previously phase 0, now phase 1) and determine whether they point to the same binding. In phase 0, they do point to the same binding, sofree-identifier=?return true, but at phase 1 there is no binding for "my-keyword", sofree-identifier=?returns false.Now, we do want
free-identifier=?to return true, so I have changedfree-identifier=?so that instead of looking up "my-keyword" in the current phase (now phase 1), it now looks it up in phase 0, thus resolving the issue. This change makes sense to me: if e.g. the macro had a local variable named "my-keyword", we would not wantxto resolve to that local variable, we still wantxto resolve to the top-leveldefine. Sofree-identifier=?should look up the variables in the environment of the macro's call site, not in the current environment.Does this make sense? Did I find and fix a bug, or is my intuition completely wrong?