Version 8: Extract core pipeline into library#39
Closed
alejandro-vaz wants to merge 3 commits intomainfrom
Closed
Conversation
Walkthroughv8 extracts the pipeline into a new mathsys library (lib.rs), bumps version to 8.0.0, tightens MODULE tokens to require Changes
Sequence Diagram(s)sequenceDiagram
participant Use as Use (Level1)
participant FS as File System
participant Tokenizer
participant Parser
participant Solver
participant Settings
Use->>FS: read module file "<name>.msm"
FS-->>Use: file content
Use->>Tokenizer: tokenize(content)
Tokenizer-->>Parser: tokens
Parser-->>Solver: AST / Start
Solver-->>Use: resolved Start
Use->>Settings: consult settings
Use->>Use: set self.start (module wired)
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
main.rs (2)
42-51:⚠️ Potential issue | 🟠 MajorIndex-based target dispatch is a silent breakage hazard.
If anyone reorders or inserts an entry in
TARGETS, every single index here silently points to the wrong command. You're oneTARGETSedit away fromhelprunninglatex. Use the string names directly instead ofTARGETS[i].0.🛡️ Proposed fix — match on string literals directly
- match &transformers.settings.target.clone().unwrap_or(Target {name: String::from("help")}).name as &str { - target if target == TARGETS[0].0 => help(&mut transformers)?, - target if target == TARGETS[1].0 => println!("Running Mathsys v{} on {}/{}/{}", version(&mut transformers)?, OS, ARCH, rustcv()), - target if target == TARGETS[2].0 => println!("{:#?}", tokens(&mut transformers)?), - target if target == TARGETS[3].0 => {let len = length(&mut transformers)?; println!("Token length: {len} / {MAXLEN} ({}%)", len as f32 / MAXLEN as f32 * 100.0)}, - target if target == TARGETS[4].0 => println!("{}", {check(&mut transformers)?; "Valid"}), - target if target == TARGETS[5].0 => println!("{:#?}", ast(&mut transformers)?), - target if target == TARGETS[6].0 => println!("{}", latex(&mut transformers)?), - other => return Err(Issue::UnknownTarget(other.to_string())) + match &transformers.settings.target.clone().unwrap_or(Target {name: String::from("help")}).name as &str { + "help" => help(&mut transformers)?, + "version" => println!("Running Mathsys v{} on {}/{}/{}", version(&mut transformers)?, OS, ARCH, rustcv()), + "tokens" => println!("{:#?}", tokens(&mut transformers)?), + "length" => {let len = length(&mut transformers)?; println!("Token length: {len} / {MAXLEN} ({}%)", len as f32 / MAXLEN as f32 * 100.0)}, + "check" => println!("{}", {check(&mut transformers)?; "Valid"}), + "ast" => println!("{:#?}", ast(&mut transformers)?), + "latex" => println!("{}", latex(&mut transformers)?), + other => return Err(Issue::UnknownTarget(other.to_string())) };
29-29: 🧹 Nitpick | 🔵 TrivialFile extension check via
split(".")is naive — handle dotless filenames and paths with dots.
file.split(".").last().unwrap()works forfoo.msmbut consider a path likemy.project/input— it'd matchinputas the extension (correctly not matchingmsm), but the intent is murky. Usestd::path::Path::extension()for robust extension checking.♻️ Proposed fix
- file if file.split(".").last().unwrap() == "msm" => Argument::File(File { + file if std::path::Path::new(file.as_str()).extension().is_some_and(|ext| ext == "msm") => Argument::File(File {syntax/level2.rs (1)
43-43: 🧹 Nitpick | 🔵 TrivialBare
panic!()with no message — you're making future-you's life harder.When this inevitably blows up on unexpected input, you'll get a panic with zero context. At minimum, include the offending
itemin the message so you can actually debug it.🔧 Proposed fix
- other => panic!() + other => panic!("Expression::summon: unexpected item {other:?}")
🤖 Fix all issues with AI agents
In @.github/CHANGELOG.md:
- Around line 210-222: Add a blank line before and after each level-3 heading
(e.g., "### Added", "### Changed", "### Removed", "### Fixed") in the "v8 --
Semantic solver" section (and the section title "v8 -- Semantic solver" itself)
so they match the v7 formatting, and ensure the file ends with a single trailing
newline; update the changelog text around those headings (including the "1. Full
issue coverage now." and list blocks) to preserve markdown paragraph separation
and add the final newline.
In @.gitignore:
- Around line 14-15: The current gitignore entry '*.msm' will ignore all .msm
files globally; change this to a more scoped pattern (for example '/*.msm' to
only ignore root-level .msm files) or move test .msm files into a dedicated
ignored directory and update the .gitignore accordingly so you don't
accidentally hide future module/example .msm files; update the '*.msm' entry in
the .gitignore to the chosen scoped pattern or directory rule.
In `@file.msm`:
- Around line 1-150: The file contains 150 identical repeated lines "e = lim n->
inf of (1 + 1/n)^n^" which is almost certainly an accidental paste; remove the
duplicates and keep a single canonical line (or a small set of distinct test
cases) instead of the repeated block, or if the repetition is intentional for
stress-testing add a top-of-file comment stating that and replace the
hard‑copied repeats with a programmatic generator (e.g., a loop or a test
fixture) so the intent is clear; look for the repeated literal "e = lim n-> inf
of (1 + 1/n)^n^" to locate and fix the issue.
In `@lib.rs`:
- Around line 65-77: The impl block for Transformers is crammed into one line
which reduces readability; reformat the impl Transformers { pub fn
new(arguments: &[Argument]) -> Result<Self, Issue> { ... } } into a multi-line,
idiomatic block: place impl Transformers on its own line, open a block, put pub
fn new(...) on a new line, and format the return Ok(Transformers { tokenizer:
Tokenizer::new(), parser: Parser::new(), solver: Solver::new(), settings:
Settings::set(arguments)?, time: Time::now() }) across multiple indented lines
so each field (tokenizer, parser, solver, settings, time) is on its own line and
the function braces are clear; keep function signature and error handling
(Settings::set(...)? ) unchanged.
- Around line 142-148: The Noise::change method's boolean parameter (shift) is
unclear at callsites like Settings::apply; replace it with a self-documenting
parameter by either renaming the parameter to increase: bool or better yet
define a small Direction enum (e.g., enum Direction {Up, Down}) and change pub
fn change(&mut self, shift: bool) to pub fn change(&mut self, dir: Direction)
(or increase: bool) and update all callsites (e.g., Settings::apply where
self.noise.change(false) appears) to use Direction::Down/Direction::Up (or
increase: false/true) so intent is explicit; keep the match logic but switch on
the enum variant (or boolean name) to preserve behavior.
- Line 179: The code currently swallows a second file/target silently
(Argument::File => if self.file.is_none() { self.file = Some(file.clone()) } and
similarly for Argument::Target), so change the branches to detect duplicates and
surface them: if self.file.is_none() set it as before, else emit a clear warning
or return an error (e.g., eprintln! or return Err with a message) indicating a
duplicate file argument; do the same for the Argument::Target branch (check
self.target/self.targets) so duplicate targets are not silently dropped. Ensure
the messages include the offending value (file or target) so users know what was
ignored.
- Around line 16-26: The nested modules tokenizer::tokenizer, parser::parser,
and solver::solver create awkward callsites; fix by either renaming the inner
modules (e.g., change tokenizer::tokenizer -> tokenizer::core, parser::parser ->
parser::core, solver::solver -> solver::core and update the corresponding mod
file names) or re-export the key types at the outer module level (add pub use
statements in the outer modules to expose tokenizer::Tokenizer, parser::Parser,
solver::Solver as tokenizer::Tokenizer, parser::Parser, solver::Solver) and
update any callsites accordingly; ensure the mod declarations (pub mod
tokenizer, pub mod parser, pub mod solver) remain correct and adjust file/module
names to match the chosen approach.
- Around line 86-127: The five pipeline functions (tokens, length, check, ast,
latex) duplicate the same read→tokenize→parse→solve→modules chain; extract a
shared helper (e.g., run_pipeline or prepare_start) that accepts &mut
Transformers and returns the intermediate results needed (at minimum: content or
tokens, and for the full pipeline a Start or whatever type the solver returns)
so each public function calls that helper instead of repeating steps; update
check, ast, and latex to call the helper and then perform their final actions
(e.g., start.modules(...) or start.latex()), keep tokens and length either
calling a lightweight helper that only performs read+tokenize or leave them
as-is but prefer a second helper for read+tokenize to avoid duplication. Ensure
the helper surfaces the same Result<..., Issue> errors and uses the existing
symbols transformers.settings, transformers.tokenizer.run,
transformers.parser.run, transformers.solver.run, and Start::modules.
- Around line 6-10: The crate currently enables unstable features in lib.rs
(try_trait_v2, default_field_values, if_let_guard) which require a nightly
compiler; add a rust-toolchain.toml pinning a minimum working nightly (or
alternatively add a clear README note) so contributors use the correct
toolchain. Update the repository by creating rust-toolchain.toml specifying the
nightly version that successfully compiles these features, or add a prominent
section in README referencing the exact nightly needed and why (point to the
feature names in lib.rs) so users won't attempt to build on stable Rust.
In `@solver/solver.rs`:
- Around line 76-84: The resolve function currently panics for any NonTerminal
pair that isn't the specific Declaration vs. Equation arm; update the resolve
method (the function named resolve on the solver struct operating on
&NonTerminal) to handle all expected pair combinations by adding explicit match
arms for pairs like (Node, Node), (Definition, Equation), etc., and change its
signature to return a Result<bool, ResolveError> (or equivalent) instead of
panicking so unexpected pairs yield an Err rather than crashing; also remove the
mutable struct field opposite and instead thread an opposite flag as a local
parameter (e.g., resolve(&mut self, first: &NonTerminal, second: &NonTerminal,
opposite: bool)) or use a local stack/recursion parameter so recursive calls
call resolve(second, first, !opposite) safely without shared mutable state.
- Around line 50-56: The current winner-selection initializes winner = &built[0]
and then only compares when both winner.1 and contender.1 match
Partition::NonTerminal, so if built[0] is a Token you may never compare and end
up "winning" a Token unintentionally; change the logic in the selection loop
(the block using built, winner, select, Partition::NonTerminal, and resolve) to
either (a) initialize winner to the first entry whose partition is NonTerminal
(scan built for the first Partition::NonTerminal) and fall back to the first
Token only if no NonTerminal exists, or (b) filter built into NonTerminals up
front and run the comparison only on that filtered list, then explicitly handle
the all-Token case afterward; also add a brief comment documenting the chosen
behavior so the intent is clear.
- Around line 38-74: The code can panic when candidates becomes empty before
candidates.pop().unwrap(); change select to return Result<Partition<'resolving>,
Issue> (e.g., fn select(...) -> Result<Partition<'resolving>, Issue>) and add a
guard after the while loop: if candidates.is_empty() { return
Err(Issue::EmptyDerivations(node.clone() or suitable context)); }. Propagate the
Result through all recursive calls to self.select (use ?), convert existing
returns like Partition::... into Ok(...), and update all call sites to handle
the Result; ensure resolve comparisons still work by matching
Ok(Partition::NonTerminal(...)) where needed or by unwrapping via ? before
comparing. This prevents the unwrap panic and surfaces an explicit error when
derivations are exhausted.
In `@syntax/level1.rs`:
- Around line 130-140: Change Use::load to propagate file-read errors instead of
returning Ok(()): when File { name: self.module.clone().into() }.read() fails,
map that error to Issue::FileNotFound (or the appropriate Issue variant) and
return Err(...) so callers get a clear diagnostic instead of silent success.
Also avoid reusing the same mutable Solver for nested module loads: before
calling start.modules(tokenizer, parser, solver, settings) create and pass a
fresh solver instance (e.g., clone or construct a new Solver based on the
current one) so nested solver.run / resolve calls cannot flip the outer
solver.opposite state and corrupt outer resolution. Ensure you update the call
site to use the new mutable solver reference when invoking start.modules.
In `@syntax/start.rs`:
- Line 39: Start::modules currently calls Use::load which re-enters
Start::modules, so add cycle detection by tracking modules being loaded (e.g., a
HashSet<String> of module paths) either on Solver or passed as an extra
parameter (e.g., &mut HashSet<String>) and have Use::load/Start::modules check
that set and return a proper Issue error if the module path is already present
to prevent recursion; also refactor the one-line impl Start::modules into a
properly formatted multi-line function for readability and to clearly handle
insertion/removal of the module path from the tracking set around the recursive
load call.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib.rs`:
- Around line 163-169: Add a one-line comment immediately before the Settings
{..} usage in the Settings::set function explaining that the shorthand relies on
the nightly-only feature `default_field_values` (i.e., the struct initializers
like `file: Option<File> = None` and `noise: Noise = Noise::Normal`) so readers
know this is using the `#![feature(default_field_values)]` syntax and is
intentionally default-initializing remaining fields; reference the Settings
struct name and the Settings {..} expression in the comment.
- Line 81: The help() function currently signals "user asked for help" by
returning Err(Issue::GetHelp), conflating normal control flow with errors;
change help() to return a non-error control value (e.g., create and return a new
enum such as Control::Help or Result type like Result<Control, Issue>) or alter
its signature to return Result<Option<()>, Issue> so callers can distinguish
Help from actual errors; update the help(transformers: &mut Transformers)
signature and all call sites that check for Err(Issue::GetHelp) to instead match
the new Control::Help (or Some/None) variant and document the new behavior.
- Around line 181-184: The match on flag values currently uses positional
indices into FLAGLIASES inside the Argument::Flag arm which breaks if FLAGLIASES
is reordered; change the handling to resolve the flag name without relying on
indices — either match the literal strings (e.g. "help", "noise-off",
"noise-on") or search FLAGLIASES for the incoming flag.value and branch on the
found alias, then call the same actions (return Err(Issue::GetHelp),
self.noise.change(false), self.noise.change(true)) accordingly so behavior is
tied to the alias value rather than its array position.
- Around line 81-84: Remove the unused &mut Transformers parameter from the
function signatures of help and version (change pub fn help(transformers: &mut
Transformers) -> Result<(), Issue> to pub fn help() -> Result<(), Issue> and pub
fn version(transformers: &mut Transformers) -> Result<usize, Issue> to pub fn
version() -> Result<usize, Issue>), leaving their bodies unchanged, then update
all call sites (notably in main.rs) to call help() and version() with no
arguments; ensure any imports or variable bindings related to passing
Transformers at those call sites are adjusted/removed.
In `@solver/context.rs`:
- Around line 5-6: Context is currently an empty struct passed as &mut
throughout the call tree (Context and Context::new) which is fine as a scaffold
but should be annotated or given a private placeholder so future readers don't
wonder; update the Context definition to either include a brief TODO comment
(e.g. "// TODO: store resolver state here") and/or add a private zero-sized
placeholder field (for example _marker: std::marker::PhantomData<()>) and keep
Context::new returning the same shape so callers don't change.
In `@solver/solver.rs`:
- Line 39: Replace the panic-prone pool.get(node).unwrap() with an
error-propagating lookup: change the code that constructs candidates to use
pool.get(node).ok_or(Issue::SyntaxError)? so a missing entry yields a handled
SyntaxError rather than panicking; update the caller/select function signature
to return Result<Partition, Issue> (as other unwrap fixes require) so the ?
operator can propagate that error type, and adjust downstream call sites of
select/get to handle the Result accordingly (refer to the pool.get(node) call
and the candidates binding using SmallVec<[Backpointer<'resolving>;
MINPOINTERS]>).
- Around line 65-66: The match on Partition::Token currently calls
ORDER.get(&token.kind).unwrap(), which will panic on unknown token kinds, and
the catch-all other => continue silently drops partitions; change this to
propagate a proper error instead of panicking by replacing the unwrap with a
lookup that returns a Result (e.g., ORDER.get(&token.kind).ok_or(...)?) so
select can return an Err for unknown kinds, and remove the silent other =>
continue by either explicitly handling all Partition variants (so you don't lose
AST nodes) or, if dropping certain partitions is intentional (e.g., whitespace),
add a clear comment next to that branch and explicitly match those kinds (e.g.,
Partition::Whitespace) so the behavior is explicit rather than implicit; update
references around Partition::Token, Responsibility::Total, ORDER.get,
children.push(Item::Token) and select's Result propagation accordingly.
- Around line 33-35: The single complex statement in run should be split into
clear steps: first find the start backpointer by iterating pool and using
matches!(backpointer.symbol, Part::NonTerminal(Object::Start)) and return
Err(Issue::SyntaxError) if None; next call self.select(pool, found_backpointer,
&mut Context::new()) and capture the Result; then match the returned Partition
to Partition::NonTerminal(NonTerminal::Start(start)) and return
Err(Issue::SyntaxError) for any other variant; finally return Ok(start). Use the
existing identifiers (run, FastMap, Backpointer, matches!, select, Context::new,
Partition::NonTerminal(NonTerminal::Start(start)), Issue::SyntaxError) so each
operation is separable and errors are easy to trace.
---
Duplicate comments:
In `@lib.rs`:
- Around line 100-128: Extract the repeated read→tokenize→parse→solve→modules
sequence into a private helper (e.g., fn build_start(transformers: &mut
Transformers) -> Result<Start, Issue>) that performs: read file from
transformers.settings.file, run transformers.tokenizer.run,
transformers.parser.run, transformers.solver.run and call start.modules(...)
with the same mutable references and settings, then returns the Start; replace
the bodies of check, ast, and latex to call this helper (check calls build_start
and drops/ignores the Start, ast returns the Start, latex calls build_start and
returns start.latex()), preserving existing error handling and signatures.
- Line 180: The code currently swallows duplicate Argument::File and
Argument::Target by only setting self.file/self.target when None; update the
handling in the match arms for Argument::File(file) and Argument::Target(tgt) to
detect when self.file or self.target is already Some(...) and surface that as an
error or explicit warning instead of silently ignoring it—e.g., return a
Result::Err or call the existing logger with a clear "duplicate argument"
message referencing Argument::File / self.file and Argument::Target /
self.target so duplicates are reported to the caller.
- Around line 6-10: lib.rs currently enables unstable nightly-only features
(#![feature(try_trait_v2)], #![feature(default_field_values)],
#![feature(if_let_guard)]) without pinning a toolchain; add a
rust-toolchain.toml to the repo that pins a compatible nightly channel (or a
specific nightly date) so contributors can build without guessing toolchain,
commit that file alongside the change, and keep the feature attributes in lib.rs
unchanged (or alternatively remove/replace those feature usages if you want to
target stable instead).
In `@solver/solver.rs`:
- Around line 75-78: The resolve function (fn resolve) currently panics on any
NonTerminal pair not matching Declaration/Equation — locate resolve in solver.rs
(and the NonTerminal/Level1 types) and change its API to return a Result<bool,
ResolveError> (or similar error type) instead of panicking; remove the
panic!("{first:?} && {second:?}") and return an Err carrying a descriptive error
(include the two NonTerminal variants and context info), update the recursive
call to propagate/convert errors (i.e., use ? or map_err) so callers can handle
ambiguity rather than crashing, and adjust any callers of resolve to handle the
Result.
- Around line 41-60: The code currently calls candidates.pop().unwrap() which
can panic if candidates was emptied by candidates.retain during the while loop;
update the end of the algorithm (after the while loop that manipulates
candidates and index) to handle the empty-case without panicking: check if
candidates.is_empty() and return an appropriate error/None (or propagate a
Result) from the surrounding function instead of unwrap, or restore a fallback
candidate selection path; replace the direct unwrap with safe handling (e.g., if
let Some(derivation) = candidates.pop() { … } else { return Err(...) }) and
adjust the function signature/return (caller) accordingly so functions like
select, resolve, and callers of the solver handle this error path.
- Around line 50-55: The current winner initialization uses let mut winner =
&built[0] which can leave a Partition::Token as winner when no NonTerminal
exists; change the logic to locate the first Partition::NonTerminal in built
(e.g., find or position searching for Partition::NonTerminal) and initialize
winner to that item before running the loop that compares with
self.resolve(first, second, context, false); if no NonTerminal is found keep the
original behavior (or explicitly handle the all-Token case) so comparisons using
Partition::NonTerminal and resolve only run when both sides are NonTerminal.
| })}} | ||
|
|
||
| //> PIPELINE -> HELP | ||
| pub fn help(transformers: &mut Transformers) -> Result<(), Issue> {return Err(Issue::GetHelp)} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Using Err for help output is a control-flow smell.
help() always returns Err(Issue::GetHelp). "The user asked for help" isn't an error — it's a successful, intentional action. If your CLI dispatch treats all Err variants as exit conditions, that's understandable, but it conflates legitimate errors (file not found, syntax error) with intentional commands. At minimum, document this design choice. Ideally, model it as a separate enum or a distinct return path so callers don't have to guess whether an Err is actually bad.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib.rs` at line 81, The help() function currently signals "user asked for
help" by returning Err(Issue::GetHelp), conflating normal control flow with
errors; change help() to return a non-error control value (e.g., create and
return a new enum such as Control::Help or Result type like Result<Control,
Issue>) or alter its signature to return Result<Option<()>, Issue> so callers
can distinguish Help from actual errors; update the help(transformers: &mut
Transformers) signature and all call sites that check for Err(Issue::GetHelp) to
instead match the new Control::Help (or Some/None) variant and document the new
behavior.
Comment on lines
+81
to
+84
| pub fn help(transformers: &mut Transformers) -> Result<(), Issue> {return Err(Issue::GetHelp)} | ||
|
|
||
| //> PIPELINE -> VERSION | ||
| pub fn version(transformers: &mut Transformers) -> Result<usize, Issue> {return Ok(VERSION)} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how help/version are called and search for dispatch patterns
echo "=== Searching for help() and version() calls ==="
rg -n 'help\(|version\(' --type rust -C3
echo -e "\n=== Searching for function pointer or trait patterns ==="
rg -n 'fn.*transformers.*Result' --type rust -C2
echo -e "\n=== Looking for any dispatch or call patterns ==="
rg -n '\bhelp\b|\bversion\b' --type rust -B2 -A2 | head -60Repository: alejandro-vaz/mathsys
Length of output: 5435
🏁 Script executed:
#!/bin/bash
# Let's read lib.rs to understand the full context
echo "=== Reading lib.rs to see the complete picture ==="
wc -l lib.rsRepository: alejandro-vaz/mathsys
Length of output: 126
🏁 Script executed:
#!/bin/bash
# Double-check there's no trait or impl pattern requiring these signatures
echo "=== Searching for trait definitions or impl blocks ==="
rg -n 'trait.*Transformer|impl.*Transformer' --type rust -A5
echo -e "\n=== Checking if help/version stored in data structures ==="
rg -n 'help|version' --type rust -B1 -A1 | grep -i 'vec\|array\|map\|fn\(' | head -20Repository: alejandro-vaz/mathsys
Length of output: 535
Remove the unused transformers parameter from help() and version().
These functions don't touch transformers at all—help() just returns an error and version() returns a constant. Keeping &mut Transformers forces callers to pass it in for nothing, and it unnecessarily locks them out of holding other references. Drop the parameter entirely; there's no dispatch pattern here.
♻️ Fix
-pub fn help(transformers: &mut Transformers) -> Result<(), Issue> {return Err(Issue::GetHelp)}
+pub fn help() -> Result<(), Issue> {return Err(Issue::GetHelp)}
-pub fn version(transformers: &mut Transformers) -> Result<usize, Issue> {return Ok(VERSION)}
+pub fn version() -> Result<usize, Issue> {return Ok(VERSION)}Update the call sites in main.rs to match.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib.rs` around lines 81 - 84, Remove the unused &mut Transformers parameter
from the function signatures of help and version (change pub fn
help(transformers: &mut Transformers) -> Result<(), Issue> to pub fn help() ->
Result<(), Issue> and pub fn version(transformers: &mut Transformers) ->
Result<usize, Issue> to pub fn version() -> Result<usize, Issue>), leaving their
bodies unchanged, then update all call sites (notably in main.rs) to call help()
and version() with no arguments; ensure any imports or variable bindings related
to passing Transformers at those call sites are adjusted/removed.
Comment on lines
+163
to
+169
| pub struct Settings { | ||
| file: Option<File> = None, | ||
| pub target: Option<Target> = None, | ||
| pub noise: Noise = Noise::Normal | ||
| } impl Settings { | ||
| pub fn set(arguments: &[Argument]) -> Result<Settings, Issue> { | ||
| let mut settings = Settings {..}; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Settings {..} default-init syntax relies on default_field_values — document the intent inline.
Line 169 uses Settings {..} which is only possible because of #![feature(default_field_values)] and the = None / = Noise::Normal initializers on lines 164-166. This is a nightly-only pattern that most Rust developers won't recognize. A one-line comment at line 169 would save a lot of confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib.rs` around lines 163 - 169, Add a one-line comment immediately before the
Settings {..} usage in the Settings::set function explaining that the shorthand
relies on the nightly-only feature `default_field_values` (i.e., the struct
initializers like `file: Option<File> = None` and `noise: Noise =
Noise::Normal`) so readers know this is using the
`#![feature(default_field_values)]` syntax and is intentionally
default-initializing remaining fields; reference the Settings struct name and
the Settings {..} expression in the comment.
Comment on lines
+181
to
+184
| Argument::Flag(flag) => match &flag.value.to_lowercase() as &str { | ||
| name if name == FLAGLIASES[0].1 => return Err(Issue::GetHelp), | ||
| name if name == FLAGLIASES[1].1 => self.noise.change(false), | ||
| name if name == FLAGLIASES[2].1 => self.noise.change(true), |
There was a problem hiding this comment.
Flag matching via positional FLAGLIASES[0], [1], [2] is fragile — reorder the array and you silently break behavior.
If someone inserts a new flag or reorders FLAGLIASES, lines 182–184 will silently map to the wrong actions. Match on the string literal directly, or look up the flag name from the array by searching rather than hard-coding indices.
🛡️ Proposed fix — match on string literals
Argument::Flag(flag) => match &flag.value.to_lowercase() as &str {
- name if name == FLAGLIASES[0].1 => return Err(Issue::GetHelp),
- name if name == FLAGLIASES[1].1 => self.noise.change(false),
- name if name == FLAGLIASES[2].1 => self.noise.change(true),
+ "help" => return Err(Issue::GetHelp),
+ "quiet" => self.noise.change(false),
+ "verbose" => self.noise.change(true),
other => return Err(Issue::UnknownFlag(flag.clone()))
},This removes the coupling between the array order and the behavior, while the array itself remains the single source of truth for display/help text.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib.rs` around lines 181 - 184, The match on flag values currently uses
positional indices into FLAGLIASES inside the Argument::Flag arm which breaks if
FLAGLIASES is reordered; change the handling to resolve the flag name without
relying on indices — either match the literal strings (e.g. "help", "noise-off",
"noise-on") or search FLAGLIASES for the incoming flag.value and branch on the
found alias, then call the same actions (return Err(Issue::GetHelp),
self.noise.change(false), self.noise.change(true)) accordingly so behavior is
tied to the alias value rather than its array position.
Comment on lines
+5
to
+6
| //> CONTEXT -> STRUCT | ||
| pub struct Context {} impl Context {pub fn new() -> Self {return Context {}}} No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Empty struct shipped as &mut — that's fine as a scaffold, just don't forget to come back.
Context is passed as &mut through the entire select/resolve call tree in solver.rs but has zero fields. It compiles, it's harmless, but it's dead weight until you actually store state in it. If this is intentional scaffolding for the next version, a brief // TODO comment would save your future self some head-scratching.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@solver/context.rs` around lines 5 - 6, Context is currently an empty struct
passed as &mut throughout the call tree (Context and Context::new) which is fine
as a scaffold but should be annotated or given a private placeholder so future
readers don't wonder; update the Context definition to either include a brief
TODO comment (e.g. "// TODO: store resolver state here") and/or add a private
zero-sized placeholder field (for example _marker: std::marker::PhantomData<()>)
and keep Context::new returning the same shape so callers don't change.
Comment on lines
+33
to
35
| pub fn run<'resolving>(&mut self, pool: &FastMap<Backpointer<'resolving>, FastSet<SmallVec<[Backpointer<'resolving>; MINPOINTERS]>>>) -> Result<Start, Issue> { | ||
| let Partition::NonTerminal(NonTerminal::Start(start)) = self.select(pool, pool.iter().map(|item| item.0).find(|backpointer| if let Part::NonTerminal(Object::Start) = backpointer.symbol {true} else {false}).ok_or(Issue::SyntaxError)?, &mut Context::new()) else {return Err(Issue::SyntaxError)}; | ||
| return Ok(start); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Line 34 is doing way too much — break it up before it breaks you.
This single line finds the start backpointer, calls select, destructures the result, and handles the error case. That's 4 logical operations crammed into one statement. When this inevitably fails at runtime, the error will point to "line 34" and you'll have no idea which part blew up.
♻️ Proposed refactor for readability and debuggability
- pub fn run<'resolving>(&mut self, pool: &FastMap<Backpointer<'resolving>, FastSet<SmallVec<[Backpointer<'resolving>; MINPOINTERS]>>>) -> Result<Start, Issue> {
- let Partition::NonTerminal(NonTerminal::Start(start)) = self.select(pool, pool.iter().map(|item| item.0).find(|backpointer| if let Part::NonTerminal(Object::Start) = backpointer.symbol {true} else {false}).ok_or(Issue::SyntaxError)?, &mut Context::new()) else {return Err(Issue::SyntaxError)};
- return Ok(start);
- }
+ pub fn run<'resolving>(&mut self, pool: &FastMap<Backpointer<'resolving>, FastSet<SmallVec<[Backpointer<'resolving>; MINPOINTERS]>>>) -> Result<Start, Issue> {
+ let root = pool.iter()
+ .map(|item| item.0)
+ .find(|bp| matches!(bp.symbol, Part::NonTerminal(Object::Start)))
+ .ok_or(Issue::SyntaxError)?;
+ let mut context = Context::new();
+ let partition = self.select(pool, root, &mut context);
+ let Partition::NonTerminal(NonTerminal::Start(start)) = partition else {
+ return Err(Issue::SyntaxError);
+ };
+ Ok(start)
+ }Note: also replaces the verbose if let … {true} else {false} with matches!().
📝 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.
Suggested change
| pub fn run<'resolving>(&mut self, pool: &FastMap<Backpointer<'resolving>, FastSet<SmallVec<[Backpointer<'resolving>; MINPOINTERS]>>>) -> Result<Start, Issue> { | |
| let Partition::NonTerminal(NonTerminal::Start(start)) = self.select(pool, pool.iter().map(|item| item.0).find(|backpointer| if let Part::NonTerminal(Object::Start) = backpointer.symbol {true} else {false}).ok_or(Issue::SyntaxError)?, &mut Context::new()) else {return Err(Issue::SyntaxError)}; | |
| return Ok(start); | |
| pub fn run<'resolving>(&mut self, pool: &FastMap<Backpointer<'resolving>, FastSet<SmallVec<[Backpointer<'resolving>; MINPOINTERS]>>>) -> Result<Start, Issue> { | |
| let root = pool.iter() | |
| .map(|item| item.0) | |
| .find(|bp| matches!(bp.symbol, Part::NonTerminal(Object::Start))) | |
| .ok_or(Issue::SyntaxError)?; | |
| let mut context = Context::new(); | |
| let partition = self.select(pool, root, &mut context); | |
| let Partition::NonTerminal(NonTerminal::Start(start)) = partition else { | |
| return Err(Issue::SyntaxError); | |
| }; | |
| Ok(start) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@solver/solver.rs` around lines 33 - 35, The single complex statement in run
should be split into clear steps: first find the start backpointer by iterating
pool and using matches!(backpointer.symbol, Part::NonTerminal(Object::Start))
and return Err(Issue::SyntaxError) if None; next call self.select(pool,
found_backpointer, &mut Context::new()) and capture the Result; then match the
returned Partition to Partition::NonTerminal(NonTerminal::Start(start)) and
return Err(Issue::SyntaxError) for any other variant; finally return Ok(start).
Use the existing identifiers (run, FastMap, Backpointer, matches!, select,
Context::new, Partition::NonTerminal(NonTerminal::Start(start)),
Issue::SyntaxError) so each operation is separable and errors are easy to trace.
| }); | ||
| fn select<'resolving, 'active>(&mut self, pool: &'active FastMap<Backpointer<'resolving>, FastSet<SmallVec<[Backpointer<'resolving>; MINPOINTERS]>>>, node: &'active Backpointer<'resolving>, context: &mut Context) -> Partition<'resolving> { | ||
| return if let Part::Token(token) = &node.symbol {Partition::Token(token.clone())} else { | ||
| let mut candidates = pool.get(node).unwrap().clone().into_iter().collect::<Vec<SmallVec<[Backpointer<'resolving>; MINPOINTERS]>>>(); |
There was a problem hiding this comment.
pool.get(node).unwrap() — another panic waiting to happen.
If node isn't a key in pool (e.g., due to a grammar edge case or a bug in the parser), this crashes with no useful context. Use .ok_or(Issue::SyntaxError)? instead — which requires select to return Result<Partition, Issue>, which you should be doing anyway given the other .unwrap() issues downstream.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@solver/solver.rs` at line 39, Replace the panic-prone pool.get(node).unwrap()
with an error-propagating lookup: change the code that constructs candidates to
use pool.get(node).ok_or(Issue::SyntaxError)? so a missing entry yields a
handled SyntaxError rather than panicking; update the caller/select function
signature to return Result<Partition, Issue> (as other unwrap fixes require) so
the ? operator can propagate that error type, and adjust downstream call sites
of select/get to handle the Result accordingly (refer to the pool.get(node) call
and the candidates binding using SmallVec<[Backpointer<'resolving>;
MINPOINTERS]>).
Comment on lines
+65
to
+66
| Partition::Token(token) if let Responsibility::Total = ORDER.get(&token.kind).unwrap().1 => children.push(Item::Token(token)), | ||
| other => continue |
There was a problem hiding this comment.
ORDER.get(&token.kind).unwrap() panics on unknown token kinds; other => continue silently drops data.
Two issues here:
- Line 65: If a token kind isn't in
ORDER, you get a panic. Use.ok_or(…)?(onceselectreturnsResult). - Line 66:
other => continuesilently discards any partition that doesn't match the above arms. If this is intentional (e.g., whitespace tokens), add a comment. If not, you're losing AST nodes and will have a fun time debugging why output is incomplete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@solver/solver.rs` around lines 65 - 66, The match on Partition::Token
currently calls ORDER.get(&token.kind).unwrap(), which will panic on unknown
token kinds, and the catch-all other => continue silently drops partitions;
change this to propagate a proper error instead of panicking by replacing the
unwrap with a lookup that returns a Result (e.g.,
ORDER.get(&token.kind).ok_or(...)?) so select can return an Err for unknown
kinds, and remove the silent other => continue by either explicitly handling all
Partition variants (so you don't lose AST nodes) or, if dropping certain
partitions is intentional (e.g., whitespace), add a clear comment next to that
branch and explicitly match those kinds (e.g., Partition::Whitespace) so the
behavior is explicit rather than implicit; update references around
Partition::Token, Responsibility::Total, ORDER.get, children.push(Item::Token)
and select's Result propagation accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores
Chores