add match command to phino #355
Conversation
WalkthroughAdds a new CLI subcommand "match" that reads a program, parses a pattern and optional YAML condition, runs pattern matching, and prints results. Implements option parsing, logging propagation, condition parsing via Yaml.parseConditionString, match execution (performMatch'), and extensive CLI tests for match behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI (match)
participant IO as IO
participant Parser as Program Parser
participant YAML as Yaml.parseConditionString
participant Matcher as performMatch'
participant Out as Stdout
User->>CLI: phino match --pattern P [--when C] [FILE]
CLI->>IO: Read input (FILE or stdin)
IO-->>CLI: Program text
CLI->>Parser: Parse program
Parser-->>CLI: Program AST
alt --when provided
CLI->>YAML: parseConditionString(C)
YAML-->>CLI: Condition
else
CLI-->>CLI: Condition = None
end
CLI->>Matcher: performMatch'(pattern=P, condition?)
Matcher-->>CLI: Matches (formatted)
CLI->>Out: Print matches
Out-->>User: Match results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@maxonfjvipon Hello! Could you please check this pull request? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
src/Yaml.hs (1)
15-15: Avoid Char8 packing for YAML text
Data.ByteString.Char8.packcan corrupt non-ASCII. PreferencodeUtf8 . T.packas shown above.test/CLISpec.hs (2)
192-201: Test name vs. expectation mismatch for--whenThe example uses a YAML string that likely won’t parse to the intended meta-variable condition, and the test expects no output. Either rename to indicate invalid condition or provide a valid YAML that filters to a specific binding.
Example adjustment (filters to only
b ↦ 2):-it "handles when condition for filtering" $ +it "filters matches with a valid --when condition" $ withStdin "Q -> [[a -> 1, b -> 2, c -> 3]]" $ - testCLI ["match", "--pattern", "!x ↦ !y", "--when", "eq: [\"!x\", \"b\"]"] - [] -- When conditions need proper YAML format + testCLI ["match", "--pattern", "!x ↦ !y", "--when", "eq: [\"!x\", b]"] + ["b ↦"]
240-243: “stdin read errors” test likely not exercising an errorFeeding empty stdin does not trigger an IO read failure; it yields empty input and then parsing decides the outcome. Consider inducing an actual read error or rename the test to reflect “empty stdin produces no matches.”
src/CLI.hs (2)
152-162: CLI parser UXHelpful help texts; consider adding
metavar "YAML"for--whento hint the expected format (optional).
277-287: Print trailing newline for CLI ergonomics
putStrcan leave the shell prompt glued to the last line. PreferputStrLn.- result <- performMatch pattern cond prog SALTY - putStr result + result <- performMatch pattern cond prog SALTY + putStrLn resultsrc/Match.hs (1)
115-121: Dead code: formatMatchResult is unusedEither remove it or use it inside your final renderer. If you keep it, consider extending it to include substitution formatting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
phino.cabal(1 hunks)src/CLI.hs(8 hunks)src/Match.hs(1 hunks)src/Yaml.hs(2 hunks)test/CLISpec.hs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (macos-15)
🔇 Additional comments (8)
phino.cabal (1)
44-46: ExposeMatchmodule — looks goodLibrary exposure of
Matchis correct and aligns with the new CLI subcommand.test/CLISpec.hs (1)
97-171: Solid coverage of happy paths formatchBroad cases (attributes, ξ/Φ/⊥, nesting, ASCII) look good and use substring assertions to avoid brittle formatting checks.
src/CLI.hs (5)
23-24: Imports forperformMatchandparseConditionStringWiring the new command through these imports is correct.
Also applies to: 35-36
52-53: Command variant added
CmdMatchintegration intoCommandis clean and consistent.
70-77: Opts formatchlook rightFlags cover log level, pattern, optional condition, format, and input file. Good.
193-193: Command registration
matchis correctly registered with a clear description.
213-214: Log level propagationIncluding
CmdMatchinsetLogLevel'is correct.src/Match.hs (1)
99-114: Confirm RuleContext scope for conditionsfilterByCondition builds RuleContext using Program matchedScope. If conditions are intended to see the whole program (not just the nearest formation), this should use the original Program. Please confirm and adjust API to pass prog in.
Run this to inspect meetCondition/RuleContext expectations and usages:
#!/bin/bash set -euo pipefail rg -n -C3 'data\s+RuleContext|newtype\s+RuleContext|type\s+RuleContext' src rg -n -C2 '\bmeetCondition\b' src rg -n -C2 'RuleContext\s*\(' src
src/Match.hs
Outdated
| findMatches (BindingPattern ptnBinding) prog@(Program rootExpr) = | ||
| findBindingMatches ptnBinding rootExpr | ||
| where | ||
| -- Find all bindings that match the pattern | ||
| findBindingMatches :: Binding -> Expression -> [MatchResult] | ||
| findBindingMatches pattern expr = case expr of | ||
| ExFormation bds -> | ||
| let directMatches = concatMap (matchSingleBinding pattern) bds | ||
| deepMatches = concatMap (findInBinding pattern) bds | ||
| in directMatches ++ deepMatches | ||
| ExDispatch e _ -> findBindingMatches pattern e | ||
| ExApplication e bd -> | ||
| findBindingMatches pattern e ++ findInBinding pattern bd | ||
| _ -> [] | ||
|
|
||
| -- Try to match a single binding | ||
| matchSingleBinding :: Binding -> Binding -> [MatchResult] | ||
| matchSingleBinding pattern target = | ||
| case matchBinding pattern target defaultScope of | ||
| [] -> [] | ||
| substs -> map (\s -> MatchResult (bindingToExpression target) defaultScope s) substs | ||
|
|
||
| -- Find matches within a binding | ||
| findInBinding :: Binding -> Binding -> [MatchResult] | ||
| findInBinding pattern (BiTau _ e) = findBindingMatches pattern e | ||
| findInBinding _ _ = [] | ||
|
|
||
| -- Convert binding to expression for display | ||
| bindingToExpression :: Binding -> Expression | ||
| bindingToExpression bd = ExFormation [bd] | ||
|
|
There was a problem hiding this comment.
Fix incorrect scope propagation: matchedScope is always defaultScope, breaking --when filtering
You build MatchResult with defaultScope and also call matchBinding using defaultScope. As a result, filterByCondition evaluates conditions against the wrong scope. Thread the actual scope through traversal and use it both for matching and in MatchResult.
Apply this diff:
-findMatches (BindingPattern ptnBinding) prog@(Program rootExpr) =
- findBindingMatches ptnBinding rootExpr
+findMatches (BindingPattern ptnBinding) prog@(Program rootExpr) =
+ findBindingMatches ptnBinding rootExpr rootExpr
where
-- Find all bindings that match the pattern
- findBindingMatches :: Binding -> Expression -> [MatchResult]
- findBindingMatches pattern expr = case expr of
- ExFormation bds ->
- let directMatches = concatMap (matchSingleBinding pattern) bds
- deepMatches = concatMap (findInBinding pattern) bds
- in directMatches ++ deepMatches
- ExDispatch e _ -> findBindingMatches pattern e
- ExApplication e bd ->
- findBindingMatches pattern e ++ findInBinding pattern bd
+ findBindingMatches :: Binding -> Expression -> Expression -> [MatchResult]
+ findBindingMatches pattern expr scope = case expr of
+ ExFormation bds ->
+ let scope' = ExFormation bds
+ directMatches = concatMap (matchSingleBinding pattern scope') bds
+ deepMatches = concatMap (findInBinding pattern scope') bds
+ in directMatches ++ deepMatches
+ ExDispatch e _ ->
+ findBindingMatches pattern e scope
+ ExApplication e bd ->
+ findBindingMatches pattern e scope ++ findInBinding pattern scope bd
_ -> []
-- Try to match a single binding
- matchSingleBinding :: Binding -> Binding -> [MatchResult]
- matchSingleBinding pattern target =
- case matchBinding pattern target defaultScope of
+ matchSingleBinding :: Binding -> Expression -> Binding -> [MatchResult]
+ matchSingleBinding pattern scope target =
+ case matchBinding pattern target scope of
[] -> []
- substs -> map (\s -> MatchResult (bindingToExpression target) defaultScope s) substs
+ substs -> map (\s -> MatchResult (bindingToExpression target) scope s) substs
-- Find matches within a binding
- findInBinding :: Binding -> Binding -> [MatchResult]
- findInBinding pattern (BiTau _ e) = findBindingMatches pattern e
- findInBinding _ _ = []
+ findInBinding :: Binding -> Expression -> Binding -> [MatchResult]
+ findInBinding pattern scope (BiTau _ e) = findBindingMatches pattern e scope
+ findInBinding _ _ _ = []📝 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.
| findMatches (BindingPattern ptnBinding) prog@(Program rootExpr) = | |
| findBindingMatches ptnBinding rootExpr | |
| where | |
| -- Find all bindings that match the pattern | |
| findBindingMatches :: Binding -> Expression -> [MatchResult] | |
| findBindingMatches pattern expr = case expr of | |
| ExFormation bds -> | |
| let directMatches = concatMap (matchSingleBinding pattern) bds | |
| deepMatches = concatMap (findInBinding pattern) bds | |
| in directMatches ++ deepMatches | |
| ExDispatch e _ -> findBindingMatches pattern e | |
| ExApplication e bd -> | |
| findBindingMatches pattern e ++ findInBinding pattern bd | |
| _ -> [] | |
| -- Try to match a single binding | |
| matchSingleBinding :: Binding -> Binding -> [MatchResult] | |
| matchSingleBinding pattern target = | |
| case matchBinding pattern target defaultScope of | |
| [] -> [] | |
| substs -> map (\s -> MatchResult (bindingToExpression target) defaultScope s) substs | |
| -- Find matches within a binding | |
| findInBinding :: Binding -> Binding -> [MatchResult] | |
| findInBinding pattern (BiTau _ e) = findBindingMatches pattern e | |
| findInBinding _ _ = [] | |
| -- Convert binding to expression for display | |
| bindingToExpression :: Binding -> Expression | |
| bindingToExpression bd = ExFormation [bd] | |
| findMatches (BindingPattern ptnBinding) prog@(Program rootExpr) = | |
| findBindingMatches ptnBinding rootExpr rootExpr | |
| where | |
| -- Find all bindings that match the pattern | |
| findBindingMatches :: Binding -> Expression -> Expression -> [MatchResult] | |
| findBindingMatches pattern expr scope = case expr of | |
| ExFormation bds -> | |
| let scope' = ExFormation bds | |
| directMatches = concatMap (matchSingleBinding pattern scope') bds | |
| deepMatches = concatMap (findInBinding pattern scope') bds | |
| in directMatches ++ deepMatches | |
| ExDispatch e _ -> | |
| findBindingMatches pattern e scope | |
| ExApplication e bd -> | |
| findBindingMatches pattern e scope ++ findInBinding pattern scope bd | |
| _ -> [] | |
| -- Try to match a single binding | |
| matchSingleBinding :: Binding -> Expression -> Binding -> [MatchResult] | |
| matchSingleBinding pattern scope target = | |
| case matchBinding pattern target scope of | |
| [] -> [] | |
| substs -> map (\s -> MatchResult (bindingToExpression target) scope s) substs | |
| -- Find matches within a binding | |
| findInBinding :: Binding -> Expression -> Binding -> [MatchResult] | |
| findInBinding pattern scope (BiTau _ e) = | |
| findBindingMatches pattern e scope | |
| findInBinding _ _ _ = [] | |
| -- Convert binding to expression for display | |
| bindingToExpression :: Binding -> Expression | |
| bindingToExpression bd = ExFormation [bd] |
🤖 Prompt for AI Agents
In src/Match.hs around lines 39 to 69, the traversal always uses defaultScope
for matching and for constructing MatchResult; instead thread the current Scope
through the helper functions: change findBindingMatches, matchSingleBinding, and
findInBinding to accept a Scope parameter, start the top-level call with the
Program's actual scope (not defaultScope), pass that scope into recursive calls
for ExFormation, ExDispatch and ExApplication cases, call matchBinding with the
current scope, and construct MatchResult using that same scope
(bindingToExpression stays the same). Ensure every recursive call that descends
into expressions or bindings propagates the current scope rather than
substituting defaultScope.
src/Match.hs
Outdated
| findMatches (ExpressionPattern ptnExpr) prog@(Program rootExpr) = | ||
| let substs = matchProgram ptnExpr prog | ||
| in concatMap (findExprLocations ptnExpr rootExpr) substs | ||
| where | ||
| -- Find specific expression locations that match | ||
| findExprLocations :: Expression -> Expression -> Subst -> [MatchResult] | ||
| findExprLocations pattern expr subst = | ||
| let directMatch = case matchExpression pattern expr defaultScope of | ||
| [] -> [] | ||
| matches -> if subst `elem` matches | ||
| then [MatchResult expr defaultScope subst] | ||
| else [] | ||
| deepMatches = case expr of | ||
| ExFormation bds -> | ||
| concatMap (\bd -> findInBinding pattern bd (ExFormation bds) subst) bds | ||
| ExDispatch e _ -> | ||
| findExprLocations pattern e subst | ||
| ExApplication e bd -> | ||
| findExprLocations pattern e subst ++ | ||
| findInBinding pattern bd defaultScope subst | ||
| _ -> [] | ||
| in directMatch ++ deepMatches | ||
|
|
||
| -- Find matches within a binding | ||
| findInBinding :: Expression -> Binding -> Expression -> Subst -> [MatchResult] | ||
| findInBinding pattern (BiTau _ e) scope subst = | ||
| findExprLocations pattern e subst | ||
| findInBinding _ _ _ _ = [] | ||
|
|
There was a problem hiding this comment.
Propagate scope for expression matches too
Same issue for expression patterns: results carry defaultScope and matchExpression uses defaultScope. Thread the current scope and store it in MatchResult.
-findMatches (ExpressionPattern ptnExpr) prog@(Program rootExpr) =
- let substs = matchProgram ptnExpr prog
- in concatMap (findExprLocations ptnExpr rootExpr) substs
+findMatches (ExpressionPattern ptnExpr) prog@(Program rootExpr) =
+ let substs = matchProgram ptnExpr prog
+ in concatMap (findExprLocations ptnExpr rootExpr rootExpr) substs
where
-- Find specific expression locations that match
- findExprLocations :: Expression -> Expression -> Subst -> [MatchResult]
- findExprLocations pattern expr subst =
- let directMatch = case matchExpression pattern expr defaultScope of
+ findExprLocations :: Expression -> Expression -> Expression -> Subst -> [MatchResult]
+ findExprLocations pattern expr scope subst =
+ let directMatch = case matchExpression pattern expr scope of
[] -> []
- matches -> if subst `elem` matches
- then [MatchResult expr defaultScope subst]
- else []
+ matches ->
+ if subst `elem` matches
+ then [MatchResult expr scope subst]
+ else []
deepMatches = case expr of
- ExFormation bds ->
- concatMap (\bd -> findInBinding pattern bd (ExFormation bds) subst) bds
- ExDispatch e _ ->
- findExprLocations pattern e subst
- ExApplication e bd ->
- findExprLocations pattern e subst ++
- findInBinding pattern bd defaultScope subst
+ ExFormation bds ->
+ let scope' = ExFormation bds
+ in concatMap (\bd -> findInBinding pattern bd scope' subst) bds
+ ExDispatch e _ ->
+ findExprLocations pattern e scope subst
+ ExApplication e bd ->
+ findExprLocations pattern e scope subst
+ ++ findInBinding pattern bd scope subst
_ -> []
in directMatch ++ deepMatches
-- Find matches within a binding
- findInBinding :: Expression -> Binding -> Expression -> Subst -> [MatchResult]
- findInBinding pattern (BiTau _ e) scope subst =
- findExprLocations pattern e subst
- findInBinding _ _ _ _ = []
+ findInBinding :: Expression -> Binding -> Expression -> Subst -> [MatchResult]
+ findInBinding pattern (BiTau _ e) scope subst =
+ findExprLocations pattern e scope subst
+ findInBinding _ _ _ _ = []📝 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.
| findMatches (ExpressionPattern ptnExpr) prog@(Program rootExpr) = | |
| let substs = matchProgram ptnExpr prog | |
| in concatMap (findExprLocations ptnExpr rootExpr) substs | |
| where | |
| -- Find specific expression locations that match | |
| findExprLocations :: Expression -> Expression -> Subst -> [MatchResult] | |
| findExprLocations pattern expr subst = | |
| let directMatch = case matchExpression pattern expr defaultScope of | |
| [] -> [] | |
| matches -> if subst `elem` matches | |
| then [MatchResult expr defaultScope subst] | |
| else [] | |
| deepMatches = case expr of | |
| ExFormation bds -> | |
| concatMap (\bd -> findInBinding pattern bd (ExFormation bds) subst) bds | |
| ExDispatch e _ -> | |
| findExprLocations pattern e subst | |
| ExApplication e bd -> | |
| findExprLocations pattern e subst ++ | |
| findInBinding pattern bd defaultScope subst | |
| _ -> [] | |
| in directMatch ++ deepMatches | |
| -- Find matches within a binding | |
| findInBinding :: Expression -> Binding -> Expression -> Subst -> [MatchResult] | |
| findInBinding pattern (BiTau _ e) scope subst = | |
| findExprLocations pattern e subst | |
| findInBinding _ _ _ _ = [] | |
| findMatches (ExpressionPattern ptnExpr) prog@(Program rootExpr) = | |
| let substs = matchProgram ptnExpr prog | |
| in concatMap (findExprLocations ptnExpr rootExpr rootExpr) substs | |
| where | |
| -- Find specific expression locations that match | |
| findExprLocations :: Expression -> Expression -> Expression -> Subst -> [MatchResult] | |
| findExprLocations pattern expr scope subst = | |
| let directMatch = case matchExpression pattern expr scope of | |
| [] -> [] | |
| matches -> | |
| if subst `elem` matches | |
| then [MatchResult expr scope subst] | |
| else [] | |
| deepMatches = case expr of | |
| ExFormation bds -> | |
| let scope' = ExFormation bds | |
| in concatMap (\bd -> findInBinding pattern bd scope' subst) bds | |
| ExDispatch e _ -> | |
| findExprLocations pattern e scope subst | |
| ExApplication e bd -> | |
| findExprLocations pattern e scope subst | |
| + findInBinding pattern bd scope subst | |
| _ -> [] | |
| in directMatch ++ deepMatches | |
| -- Find matches within a binding | |
| findInBinding :: Expression -> Binding -> Expression -> Subst -> [MatchResult] | |
| findInBinding pattern (BiTau _ e) scope subst = | |
| findExprLocations pattern e scope subst | |
| findInBinding _ _ _ _ = [] |
🤖 Prompt for AI Agents
In src/Match.hs around lines 70 to 98, expression-matching currently always uses
and records defaultScope; change findExprLocations and findInBinding to accept a
current Scope parameter, pass the appropriate scope when recursing (e.g.
propagate scope from the surrounding expression or binding), call
matchExpression with that current scope instead of defaultScope, and construct
MatchResult using the threaded scope so returned MatchResult entries carry the
actual scope rather than defaultScope.
src/Match.hs
Outdated
| -- | Format match results showing unique matches | ||
| formatUniqueMatches :: [MatchResult] -> PrintMode -> String | ||
| formatUniqueMatches results mode = | ||
| let uniqueExprs = nub (map matchedExpression results) | ||
| formatted = map (\e -> case mode of | ||
| SWEET -> prettyExpression' e | ||
| SALTY -> prettyExpression e) uniqueExprs | ||
| in unlines formatted | ||
|
|
There was a problem hiding this comment.
Output doesn’t meet Issue #73: mappings are not printed
formatUniqueMatches renders only expressions, and performMatch returns that string. The spec expects lines like “t ↦ ξ.k” showing the substitution. Please format and print the substitution(s) from MatchResult.substitution (ideally stable-sorted and optionally deduped by (matchedExpression, substitution)).
I can wire a formatter that pretty-prints each (var ↦ term) pair from Subst in SALTY/SWEET modes and update performMatch to use it. Want me to draft that?
Also applies to: 151-154
🤖 Prompt for AI Agents
In src/Match.hs around lines 122-130 (and similarly 151-154),
formatUniqueMatches currently prints only matched expressions but the spec
(Issue #73) requires printing substitutions like "t ↦ ξ.k"; change it to iterate
MatchResult values and for each produce a stable-sorted, optionally deduplicated
output of (matchedExpression, substitution) pairs, where the substitution is
rendered as a list of "var ↦ term" entries; use nubBy on the tuple
(matchedExpression, substitution) if dedupe is desired, sort keys
deterministically (e.g. sortBy on variable name) to produce stable order, and
call the existing prettyExpression / prettyExpression' depending on PrintMode to
format terms and variable names, then join entries into lines and return unlines
of those lines; also update performMatch to call this new substitution-aware
formatter instead of the old expression-only function.
src/Yaml.hs
Outdated
| -- SPDX-License-Identifier: MIT | ||
|
|
||
| module Yaml where | ||
| module Yaml (module Yaml, parseConditionString) where |
There was a problem hiding this comment.
Invalid self re-export in module header breaks compilation
module Yaml (module Yaml, parseConditionString) attempts to re-export the current module, which isn't allowed (only imported modules can be re-exported). Use an explicit export list or revert to exporting everything.
Apply this minimal fix:
-module Yaml (module Yaml, parseConditionString) where
+module Yaml where📝 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.
| module Yaml (module Yaml, parseConditionString) where | |
| module Yaml where |
🧰 Tools
🪛 GitHub Check: build (macos-15)
[warning] 9-9:
‘parseConditionString’ is exported by ‘parseConditionString’ and ‘module Yaml’
🤖 Prompt for AI Agents
In src/Yaml.hs around line 9, the module header "module Yaml (module Yaml,
parseConditionString) where" illegally re-exports itself; replace the header
with a normal export that avoids self re-export—e.g. change the line to "module
Yaml where" (or alternatively provide an explicit export list that includes
parseConditionString and any other intended exports) so the module compiles.
src/Yaml.hs
Outdated
| -- | Parse a condition from a string (for use with --when flag) | ||
| parseConditionString :: String -> IO Condition | ||
| parseConditionString str = do | ||
| -- Try to parse as YAML | ||
| case Yaml.decodeEither' (BSC.pack str) of | ||
| Right cond -> pure cond | ||
| Left err -> error $ "Failed to parse condition: " ++ show err |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer UTF-8 and IO exceptions over error in parseConditionString
Use proper UTF-8 encoding and raise exceptions in the IO monad to integrate with the CLI handler.
Apply:
-parseConditionString :: String -> IO Condition
-parseConditionString str = do
- -- Try to parse as YAML
- case Yaml.decodeEither' (BSC.pack str) of
- Right cond -> pure cond
- Left err -> error $ "Failed to parse condition: " ++ show err
+parseConditionString :: String -> IO Condition
+parseConditionString str =
+ case Yaml.decodeEither' (encodeUtf8 (T.pack str)) of
+ Right cond -> pure cond
+ Left err -> throwIO (userError ("Failed to parse condition: " <> show err))Additionally, add imports (and drop BSC if unused elsewhere):
- import qualified Data.ByteString.Char8 as BSC
+ import Control.Exception (throwIO)
+ import qualified Data.Text as T
+ import Data.Text.Encoding (encodeUtf8)📝 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.
| -- | Parse a condition from a string (for use with --when flag) | |
| parseConditionString :: String -> IO Condition | |
| parseConditionString str = do | |
| -- Try to parse as YAML | |
| case Yaml.decodeEither' (BSC.pack str) of | |
| Right cond -> pure cond | |
| Left err -> error $ "Failed to parse condition: " ++ show err | |
| -- In your imports, replace or add: | |
| import Control.Exception (throwIO) | |
| import qualified Data.Text as T | |
| import Data.Text.Encoding (encodeUtf8) | |
| -- | Parse a condition from a string (for use with --when flag) | |
| parseConditionString :: String -> IO Condition | |
| parseConditionString str = | |
| case Yaml.decodeEither' (encodeUtf8 (T.pack str)) of | |
| Right cond -> pure cond | |
| Left err -> throwIO (userError ("Failed to parse condition: " <> show err)) |
🤖 Prompt for AI Agents
In src/Yaml.hs around lines 197 to 203, replace the use of BSC.pack and the call
to error with UTF-8 encoding and an IO exception: convert the input String to
Text and call Data.Text.Encoding.encodeUtf8 before passing to
Yaml.decodeEither', and on Left use throwIO (e.g. throwIO . userError $ "Failed
to parse condition: " ++ show err) instead of error; add imports for Data.Text
(qualified or Data.Text.pack), Data.Text.Encoding (encodeUtf8), and
Control.Exception (throwIO), and remove the BSC import if it is no longer used
elsewhere.
|
@yarkivaev we already have |
|
@maxonfjvipon Hello! I've removed Match.hs. Could you please check the pull request again? |
|
@yarkivaev I'd recommend you to investigate how As you see - matching is only one step in this rewriting process. When something's wrong, it's quite hard to tell where exactly error is. In order to add more traceability to the The only new thing here is parsing |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/CLI.hs (3)
157-167: CLI UX: consider PATTERN as a positional arg.
grep-like tools usually accept the pattern positionally. Keeping--patternis fine; adding a positionalPATTERN(and resolving precedence) improves ergonomics.
75-82: Optional: support --sweet printing for match output.You always format matches in SALTY. Consider a
--sweetflag (like rewrite) to mirror output style options.
12-41: Tidy imports (minor).
Control.Exception.Base,matchExpressionDeep, andBuilder (buildExpression, buildBinding)appear unused here; also you splitParser/Prettyacross two imports. Consolidating/removing unused imports avoids -Wall warnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
stack.yaml.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
src/CLI.hs(10 hunks)src/Yaml.hs(2 hunks)test/CLISpec.hs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Yaml.hs
- test/CLISpec.hs
🔇 Additional comments (2)
src/CLI.hs (2)
197-198: Subcommand wiring LGTM.Command is registered with a clear description.
214-219: Log-level propagation LGTM.
CmdMatchparticipates in global log-level handling correctly.
| CmdMatch OptsMatch {..} -> do | ||
| input <- readInput inputFile | ||
| prog <- parseProgram input inputFormat | ||
| cond <- case whenCondition of | ||
| Nothing -> pure Nothing | ||
| Just condStr -> Just <$> parseConditionString condStr | ||
| result <- performMatch' pattern cond prog SALTY | ||
| putStr result | ||
| where |
There was a problem hiding this comment.
Make match exit non-zero when no matches (grep semantics).
Currently prints nothing and exits 0 on no matches. Return exit code 1 when no matches to align with grep-like behavior required by the PR objectives.
Apply this diff:
@@
CmdMatch OptsMatch {..} -> do
input <- readInput inputFile
prog <- parseProgram input inputFormat
cond <- case whenCondition of
Nothing -> pure Nothing
Just condStr -> Just <$> parseConditionString condStr
- result <- performMatch' pattern cond prog SALTY
- putStr result
+ result <- performMatch' pattern cond prog SALTY
+ if null (filter (not . isSpace) result)
+ then exitWith (ExitFailure 1)
+ else putStr resultAdditionally add missing imports outside this hunk:
import Data.Char (isSpace, toLower, toUpper)
import System.Exit (ExitCode (..), exitFailure, exitWith)🤖 Prompt for AI Agents
In src/CLI.hs around lines 282 to 290, the CmdMatch branch currently prints
match output but always exits 0 even when there are no matches; update it so
that if result is empty (no matches) the program exits with a non-zero status
(exit code 1) to match grep semantics, otherwise print the result and exit
normally; implement this by checking whether result is empty and calling
exitFailure or exitWith (ExitFailure 1) when empty, and retain the existing
successful exit behavior when non-empty. Also add the missing imports at the top
of the file: import Data.Char (isSpace, toLower, toUpper) and import System.Exit
(ExitCode (..), exitFailure, exitWith).
| performMatch' :: String -> Maybe Condition -> Program -> PrintMode -> IO String | ||
| performMatch' patternStr maybeCond prog@(Program rootExpr) mode = do | ||
| logDebug (printf "Searching for pattern: %s" patternStr) | ||
| case parseExpression patternStr of | ||
| Right expr -> do | ||
| let substs = matchProgram expr prog | ||
| logDebug (printf "Found %d matches for expression pattern" (length substs)) | ||
| filtered <- filterByCondition' maybeCond substs prog | ||
| logDebug (printf "After filtering: %d matches" (length filtered)) | ||
| let exprs = extractMatchedExpressions expr filtered | ||
| formatMatches exprs mode | ||
| Left _ -> case parseBinding patternStr of | ||
| Right binding -> do | ||
| let substs = findBindingMatches binding rootExpr | ||
| logDebug (printf "Found %d matches for binding pattern" (length substs)) | ||
| filtered <- filterByCondition' maybeCond (map snd substs) prog | ||
| logDebug (printf "After filtering: %d matches" (length filtered)) | ||
| let matchedBindings = [bd | (bd, s) <- substs, s `elem` filtered] | ||
| formatBindingMatches matchedBindings mode | ||
| Left err -> throwIO $ userError $ "Invalid pattern: " ++ err |
There was a problem hiding this comment.
Print mappings “name ↦ value” instead of raw nodes.
The command currently emits only pretty-printed nodes, losing the meta-name → matched value mapping that the issue’s example requires (e.g., “t ↦ ξ.k”). Emit per-substitution mappings.
Apply this diff:
@@
case parseExpression patternStr of
Right expr -> do
let substs = matchProgram expr prog
logDebug (printf "Found %d matches for expression pattern" (length substs))
filtered <- filterByCondition' maybeCond substs prog
logDebug (printf "After filtering: %d matches" (length filtered))
- let exprs = extractMatchedExpressions expr filtered
- formatMatches exprs mode
+ formatSubsts filtered mode
Left _ -> case parseBinding patternStr of
Right binding -> do
let substs = findBindingMatches binding rootExpr
logDebug (printf "Found %d matches for binding pattern" (length substs))
filtered <- filterByCondition' maybeCond (map snd substs) prog
logDebug (printf "After filtering: %d matches" (length filtered))
- let matchedBindings = [bd | (bd, s) <- substs, s `elem` filtered]
- formatBindingMatches matchedBindings mode
+ formatSubsts filtered mode
Left err -> throwIO $ userError $ "Invalid pattern: " ++ err
@@
- extractMatchedExpressions :: Expression -> [Subst] -> [Expression]
- extractMatchedExpressions pattern substs =
- [expr | Subst m <- substs,
- MvExpression expr _ <- Map.elems m]
+ formatSubsts :: [Subst] -> PrintMode -> IO String
+ formatSubsts [] _ = pure ""
+ formatSubsts substs mode = do
+ let fmtExpr e = case mode of
+ SWEET -> prettyExpression' e
+ SALTY -> prettyExpression e
+ let lineOf (name, MvExpression e _) = Just (show name <> " ↦ " <> fmtExpr e)
+ lineOf (name, MvBinding b _) = Just (show name <> " ↦ " <> prettyBinding b)
+ lineOf _ = Nothing
+ let lines' =
+ [ ln
+ | Subst m <- substs
+ , (name, mv) <- Map.toList m
+ , Just ln <- [lineOf (name, mv)]
+ ]
+ pure (unlines lines')
- filterByCondition' :: Maybe Condition -> [Subst] -> Program -> IO [Subst]
+ filterByCondition' :: Maybe Condition -> [Subst] -> Program -> IO [Subst]
@@
- formatMatches :: [Expression] -> PrintMode -> IO String
- formatMatches [] _ = pure ""
- formatMatches exprs mode = do
- let formatted = map (\e -> case mode of
- SWEET -> prettyExpression' e
- SALTY -> prettyExpression e) exprs
- pure $ unlines formatted
- formatBindingMatches :: [Binding] -> PrintMode -> IO String
- formatBindingMatches [] _ = pure ""
- formatBindingMatches bds mode = do
- let formatted = map prettyBinding bds
- pure $ unlines formattedNote: This assumes the map key type in Subst has a sane Show instance (e.g., renders !a as a). If not, swap show name with the appropriate pretty-printer for meta-names. See verification comment below.
Also applies to: 355-375
| findBindingMatches pattern expr = case expr of | ||
| ExFormation bds -> | ||
| let direct = [(bd, s) | bd <- bds, s <- matchBinding pattern bd defaultScope] | ||
| nested = concatMap (searchInBinding pattern) bds | ||
| in direct ++ nested | ||
| ExDispatch e _ -> findBindingMatches pattern e | ||
| ExApplication e bd -> | ||
| findBindingMatches pattern e ++ searchInBinding pattern bd | ||
| _ -> [] | ||
| where | ||
| searchInBinding :: Binding -> Binding -> [(Binding, Subst)] | ||
| searchInBinding ptn (BiTau _ e) = findBindingMatches ptn e | ||
| searchInBinding _ _ = [] | ||
| extractMatchedExpressions :: Expression -> [Subst] -> [Expression] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Custom binding traversal may miss cases; prefer existing deep match utilities.
findBindingMatches manually walks a subset of constructors (ExFormation, ExDispatch, ExApplication) and only recurses into BiTau. This risks missing bindings under other nodes now or in the future. If Matcher exposes a deep-binding matcher (akin to matchExpressionDeep), reuse it; otherwise, build a uniform traversal that visits every binding occurrence.
Run this to inspect available constructors and matcher APIs, then decide whether to replace findBindingMatches:
🏁 Script executed:
#!/bin/bash
# Show Expression/Binding constructors (context for traversal coverage)
rg -nP -C2 'data\s+Expression\b|data\s+Binding\b' src
# Inspect Subst/MetaValue shape and key type (to choose a proper name pretty-printer)
rg -nP -C2 '\bdata\s+Subst\b|\bnewtype\s+Subst\b|\bdata\s+MetaValue\b|\btype\s+MetaVar\b|\bnewtype\s+Meta\b' src
# Look for any deep-binding matcher helpers
rg -nP -C2 '\bmatch.*Binding.*Deep\b|\bmatchBindingDeep\b|\bmatch.*Deep\b' srcLength of output: 2302
Replace custom findBindingMatches with Matcher.deep-match helpers.
Matcher already exposes matchExpressionDeep and matchBindingExpression which implement the deep traversal for ExFormation / ExDispatch / ExApplication and BiTau — reuse those instead of the manual recursion to avoid duplicating logic and missing cases. Location: src/CLI.hs (lines 341–354).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/CLI.hs (3)
329-337: Non‑matches exit code: decide whether to emulate grep semantics.Prior feedback recommended exiting with code 1 when no matches. That’s useful for scripting but would require adjusting the top-level handler to avoid logging an error on ExitFailure 1 and updating tests that currently expect success on no matches.
Do we want grep-like exit code (1) on zero matches? If yes, I’ll wire it in and update the handler and tests accordingly; otherwise, we keep current success semantics.
380-399: Emit meta-variable mappings instead of raw nodes.Right now expression patterns print only the matched nodes and binding patterns print pretty bindings. To align with the PR objective’s “mappings” output and make results uniform, format the substitutions directly as “name ↦ value” for both expression and binding patterns.
Apply this diff:
@@ - case parseExpression patternStr of - Right expr -> do - let substs = matchProgram expr prog - logDebug (printf "Found %d matches for expression pattern" (length substs)) - filtered <- filterByCondition' maybeCond substs prog - logDebug (printf "After filtering: %d matches" (length filtered)) - let exprs = extractMatchedExpressions expr filtered - formatMatches exprs mode + case parseExpression patternStr of + Right expr -> do + let substs = matchProgram expr prog + logDebug (printf "Found %d matches for expression pattern" (length substs)) + filtered <- filterByCondition' maybeCond substs prog + logDebug (printf "After filtering: %d matches" (length filtered)) + formatSubsts filtered mode Left _ -> case parseBinding patternStr of Right binding -> do let substs = findBindingMatches binding rootExpr logDebug (printf "Found %d matches for binding pattern" (length substs)) - filtered <- filterByCondition' maybeCond (map snd substs) prog + filtered <- filterByCondition' maybeCond (map snd substs) prog logDebug (printf "After filtering: %d matches" (length filtered)) - let matchedBindings = [bd | (bd, s) <- substs, s `elem` filtered] - formatBindingMatches matchedBindings mode + formatSubsts filtered mode Left err -> throwIO $ userError $ "Invalid pattern: " ++ err @@ - extractMatchedExpressions :: Expression -> [Subst] -> [Expression] - extractMatchedExpressions pattern substs = - [expr | Subst m <- substs, - MvExpression expr _ <- Map.elems m] + formatSubsts :: [Subst] -> PrintMode -> IO String + formatSubsts [] _ = pure "" + formatSubsts substs mode = do + let fmtExpr e = case mode of + SWEET -> prettyExpression' e + SALTY -> prettyExpression e + let lineOf (name, MvExpression e _) = Just (show name <> " ↦ " <> fmtExpr e) + lineOf (name, MvBinding b _) = Just (show name <> " ↦ " <> prettyBinding b) + lineOf _ = Nothing + let lines' = + [ ln + | Subst m <- substs + , (name, mv) <- Map.toList m + , Just ln <- [lineOf (name, mv)] + ] + pure (unlines lines') @@ - formatMatches :: [Expression] -> PrintMode -> IO String - formatMatches [] _ = pure "" - formatMatches exprs mode = do - let formatted = map (\e -> case mode of - SWEET -> prettyExpression' e - SALTY -> prettyExpression e) exprs - pure $ unlines formatted - formatBindingMatches :: [Binding] -> PrintMode -> IO String - formatBindingMatches [] _ = pure "" - formatBindingMatches bds mode = do - let formatted = map prettyBinding bds - pure $ unlines formattedNote: This assumes the meta-name key in Subst has a sensible Show instance (e.g., renders “!a” as “a”). If not, switch to the appropriate pretty-printer for meta names.
400-414: Deep-binding traversal may miss cases; reuse Matcher’s deep helpers or broaden recursion.Manual matching only visits ExFormation, ExDispatch, ExApplication and recurses from BiTau. Bindings under other nodes could be skipped now or in the future.
I can replace this with a helper that walks every binding occurrence or reuse a deep match from Matcher if available. To confirm coverage, run:
#!/bin/bash # Inspect constructors to ensure traversal covers all cases rg -nP -C2 'data\s+Expression\b|data\s+Binding\b' src # Check Matcher for deep binding helpers you can reuse rg -nP -C2 '\bmatch.*Binding.*Deep\b|\bmatchBindingDeep\b|\bmatch.*Deep\b' srcIf a deep-binding matcher exists, wire it here; otherwise, I’ll draft a uniform traversal visiting all constructors that can contain bindings.
🧹 Nitpick comments (3)
test/CLISpec.hs (3)
146-149: Consider asserting the meta-name mapping for expression patterns, not only values.PR goals mention mappings like “t ↦ ξ.k”. For expression patterns (e.g., "!e.c"), asserting “e ↦ ξ.b” would make output semantics consistent across pattern types. Current tests will still pass if CLI prints “e ↦ ξ.b” because you check substrings, but consider tightening the expectation.
Would you like me to update this test to also assert the meta name (e.g., include "e ↦ ξ.b")?
199-203: The --when test should cover a positive case too.Right now it only validates “filters to empty.” Add a case that keeps a specific binding when the condition is met to ensure integration with parseConditionString/meetCondition is exercised end‑to‑end.
I can add a passing condition test (e.g., eq matching a concrete name) if you want.
247-250: Rename for precision: this checks empty stdin, not an error.The title “handles stdin read errors gracefully” is misleading since empty stdin is valid input and returns success. Consider “handles empty stdin gracefully.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/CLI.hs(10 hunks)test/CLISpec.hs(1 hunks)
🔇 Additional comments (3)
test/CLISpec.hs (2)
104-121: Solid, focused coverage for the new “match” command.Good breadth across binding patterns, nested formations, xi/Φ/⊥/Δ/λ/ρ cases, and basic error handling. This will catch most regressions in the CLI surface.
171-178: Nice: file and XMIR input paths covered.This validates IOFormat plumbing and parser selection through the CLI.
src/CLI.hs (1)
198-208: CLI parser for “match” looks correct and consistent with the existing style.Options and help text are clear, integrate with log-level and input format as expected.
| logDebug "Reading from stdin" | ||
| getContents' `catch` (\(e :: SomeException) -> throwIO (CouldNotReadFromStdin (show e))) | ||
| performMatch' :: String -> Maybe Condition -> Program -> PrintMode -> IO String | ||
| performMatch' patternStr maybeCond prog@(Program rootExpr) mode = do |
There was a problem hiding this comment.
@yarkivaev the functionality of matching program with rule is already implemented in Rule module, the function matchProgramWithRule is what you need
|
@yarkivaev what's up with this one? If you're not planning to continue working on this PR, please close it |
|
@yarkivaev reminder |
2 participants
Closes #73
Summary by CodeRabbit
New Features
Tests