Skip to content

Commit 7b30dd7

Browse files
enklhtenklht
committed
refactor: ♻️ reorganize Cargo.toml dependencies, enhance Context struct with clone_source and call path management
1 parent 74ee02d commit 7b30dd7

6 files changed

Lines changed: 336 additions & 17 deletions

File tree

‎Cargo.toml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ version = "0.1.0"
44
edition = "2024"
55

66
[dependencies]
7-
env_logger = "0.11.8"
87
log = "0.4.29"
98
rustc-hash = "2.1.1"
9+
10+
[dev-dependencies]
11+
env_logger = "0.11.8"

‎REFACTORING_PLAN.md‎

Lines changed: 305 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,305 @@
1+
# Packrust Code Review and Refactoring Plan
2+
3+
## Executive Summary
4+
5+
Packrust is a well-implemented packrat parser combinator library with proper left recursion handling. The codebase is relatively small (~500 lines) and shows good understanding of advanced parsing concepts. However, there are several areas for improvement in code quality, API design, error handling, testing, and documentation.
6+
7+
---
8+
9+
## Critical Issues
10+
11+
### 1. **Missing Error Recovery in Examples**
12+
**Severity**: MEDIUM
13+
- **Issue**: Examples panic or fail silently without good error messages.
14+
- **Impact**: Users can't understand why parsing failed or where.
15+
16+
---
17+
18+
## High Priority Issues
19+
20+
### 4. **Poor Error Messages**
21+
**Severity**: HIGH
22+
- **Location**: [lib.rs](lib.rs#L79-L85), various places
23+
- **Problems**:
24+
- Left recursion errors are generic: `"failed to resolve left recursion"`
25+
- `keyword()` has empty error reason: `String::from("")`
26+
- No context about what was expected vs. found
27+
- Visual error indicator might be misaligned with UTF-8 characters
28+
- **Impact**: Difficult debugging for users. Makes the library feel unpolished.
29+
30+
**Example of current output**:
31+
```
32+
abc123
33+
^^
34+
expected EOF found a
35+
```
36+
37+
**Better would be**:
38+
```
39+
Error at position 2:
40+
abc123
41+
^^
42+
Expected end of input, but found 'a' (position 2)
43+
```
44+
45+
### 5. **Inadequate Logging with `info!()` Calls**
46+
**Severity**: MEDIUM
47+
- **Issue**: Using `info!()` for implementation details that should be `trace!()` or `debug!()`:
48+
- Line 82: `"left recursion detected"`
49+
- Line 104: `"start left recursion expansion"`
50+
- Line 118: `"cache update"`
51+
- Line 122: `"cache fix"`
52+
- **Impact**: Logs spam the output when trying to use the library normally.
53+
- **Fix**: Use `trace!()` for fine-grained details, `debug!()` for important events.
54+
55+
### 6. **Weak Test Coverage**
56+
**Severity**: MEDIUM
57+
- **Location**: [combinators.rs](combinators.rs#L265-L323) - only 10 basic unit tests
58+
- **Missing tests**:
59+
- Left recursion cases (the core feature!)
60+
- Multiple left-recursive calls
61+
- Cache behavior and eviction
62+
- Error message quality
63+
- `lazy()` with cycles
64+
- Complex nested expressions
65+
- Edge cases: empty input, very long inputs
66+
- **Impact**: Regression risks. Hard to maintain and refactor.
67+
68+
### 7. **Hardcoded Naming Convention Issues**
69+
**Severity**: MEDIUM
70+
- **Location**: [combinators.rs](combinators.rs#L68), `and()` combinator
71+
- **Issue**:
72+
```rust
73+
let name = format!("({}{})", self.name, right.name);
74+
```
75+
- Creates names like `(digitdigit)` which is confusing
76+
- Should use operators: `(a & b)`, `(a | b)`, `(a*)`, etc.
77+
- Names don't reflect actual operator semantics
78+
- **Impact**: Logging output is hard to read; difficult to debug complex parsers.
79+
80+
### 8. **Missing Combinator Methods**
81+
**Severity**: MEDIUM
82+
- **Missing common combinators**:
83+
- `sep_by` (parse list with separator)
84+
- `one_or_more` (alias for `many()` that fails if zero matches)
85+
- `peek` / `lookahead`
86+
- `not` / negative lookahead
87+
- `between` / surrounded parsing
88+
- `attempt` / backtracking control
89+
- `range()` for character ranges
90+
- `whitespace()` / `ws()` helpers
91+
- **Impact**: Users need to write boilerplate for common parsing patterns.
92+
93+
---
94+
95+
## Medium Priority Issues
96+
97+
### 9. **Incomplete Documentation**
98+
**Severity**: MEDIUM
99+
- **Issues**:
100+
- No rustdoc comments on public functions
101+
- No explanation of cache invalidation strategy
102+
- No guide for complex parser construction
103+
- README examples are minimal
104+
- No performance characteristics documented
105+
- **Fix**: Add comprehensive rustdoc with examples.
106+
107+
### 10. **Generic Constraints Unclear**
108+
**Severity**: MEDIUM
109+
- **Issue**: Bounds `T: Clone + 'static` are repeated everywhere but not well justified.
110+
- **Problem**: Users don't understand why `Clone` is required (answer: caching), or why `'static` (answer: type erasure in `Any`).
111+
- **Fix**:
112+
- Add comments explaining bounds
113+
- Consider using lifetime parameters to relax `'static`
114+
- Provide helper for types that can't derive Clone
115+
116+
### 12. **Architecture: Context Mutation Complexity**
117+
**Severity**: MEDIUM
118+
- **Location**: [context.rs](context.rs) and [lib.rs](lib.rs) interaction
119+
- **Issue**:
120+
- Complex cache eviction logic with multiple data structures
121+
- `pending_evictions` map with dependent lists is hard to reason about
122+
- Recursive `execute_cache_eviction` could stack overflow on deep dependencies
123+
- No comments explaining why this design was chosen
124+
- **Impact**: Maintenance burden. Hard to verify correctness.
125+
- **Suggestion**: Add detailed comments or consider refactoring into smaller functions.
126+
127+
### 13. **No Benchmarks**
128+
**Severity**: LOW
129+
- **Issue**: No benchmark suite to measure performance
130+
- **Impact**: Can't validate that refactorings maintain performance
131+
- **Fix**: Add `benches/` directory with criterion benchmarks
132+
133+
### 14. **Missing Debug Trait Implementation**
134+
**Severity**: LOW
135+
- **Issue**: `Parser<T>` derives Debug but `RawParser<T>` (a closure) cannot be debugged
136+
- **Impact**: Printing parser structures is not useful
137+
- **Fix**: Implement custom Debug that shows name + id, not the closure
138+
139+
### 15. **Keyword Parser Limitations**
140+
**Severity**: LOW
141+
- **Location**: [combinators.rs](combinators.rs#L152-L172)
142+
- **Issue**:
143+
- Doesn't check for word boundaries (would match "integer" for keyword "int")
144+
- Empty error reason is confusing
145+
- Returns `String` instead of `()` or `&str`
146+
- **Suggestion**: Add optional word boundary checking, better error messages
147+
148+
---
149+
150+
## Code Quality Issues
151+
152+
### 16. **Inconsistent Naming**
153+
**Severity**: LOW
154+
- `andl` / `andr` are non-standard abbreviations. Better: `and_left()`, `and_right()`
155+
- `lr_stack` should be more descriptive: `left_recursion_stack`
156+
- `CacheKey` alias obscures that it's `(ParserId, Pos)`
157+
158+
### 17. **Magic Numbers**
159+
**Severity**: LOW
160+
- Atomic ID counter starts at 0 (fine, but undocumented assumption)
161+
- Character index assumes single-byte operations (breaks with multi-byte UTF-8)
162+
163+
### 18. **Unused Flexibility in lazy()**
164+
**Severity**: LOW
165+
- **Location**: [combinators.rs](combinators.rs#L183-L208)
166+
- The `get_parser` closure-based approach is clever but unintuitive
167+
- No documentation on why this design was chosen
168+
- Consider: Could use a macro or builder pattern instead
169+
170+
---
171+
172+
## Potential Bugs / Edge Cases
173+
174+
### 19. **UTF-8 Support Unclear**
175+
**Severity**: MEDIUM
176+
- **Issue**: Code uses `Vec<char>` (correct) but position tracking is by char index, not byte index
177+
- **Problem**: Error messages display with space-based positioning that might not align correctly with variable-width characters
178+
- **Example**: Emoji or accented characters could cause misaligned error pointers
179+
- **Fix**: Document that positions are 0-indexed char positions, or add byte offset tracking
180+
181+
### 20. **No Input Validation**
182+
**Severity**: LOW
183+
- No checks for invalid UTF-8 strings
184+
- `satisfy()` uses `ctx.source.get(pos)` safely, so no buffer overflow risk
185+
- But no clear constraints documentation
186+
187+
### 21. **Recursion Depth Not Bounded**
188+
**Severity**: LOW
189+
- Deep left-recursive grammars could cause stack overflow
190+
- `execute_cache_eviction` is recursive and could overflow
191+
- No limits on cache size (could consume all memory on infinite recursion)
192+
- **Fix**: Document these limitations, consider add depth limits
193+
194+
---
195+
196+
## Testing & Documentation Gaps
197+
198+
### 22. **Example Clarity**
199+
**Severity**: LOW
200+
- Examples are good for demonstrating features but:
201+
- No explanation of the grammars
202+
- `baab_baab.rs` references academic papers without accessible explanation
203+
- `arithmetics.rs` is incomplete (commented-out Sub, Div)
204+
- **Fix**: Add comments, complete examples, add simple "hello world" example
205+
206+
---
207+
208+
## Refactoring Plan (Prioritized)
209+
210+
### Phase 1: Critical Fixes (Must Do)
211+
1. **Fix Cargo.toml edition** to "2021"
212+
2. **Add error handling tests** to catch panics from type casting
213+
3. **Improve error messages**:
214+
- Replace empty strings with meaningful errors
215+
- Fix left recursion error message
216+
- Better keyword error context
217+
218+
### Phase 2: High-Impact Improvements
219+
4. **Fix logging levels**:
220+
- Change `info!()` → `trace!()` or `debug!()`
221+
- Make logging truly optional with feature flag
222+
223+
5. **Add comprehensive test suite**:
224+
- Left recursion test cases
225+
- Multiple left-recursive calls
226+
- Cache behavior verification
227+
- Error message assertion tests
228+
229+
6. **Add rustdoc**:
230+
- Every public function needs doc comments with examples
231+
- Explain bounds (`Clone`, `'static`)
232+
- Document left recursion behavior
233+
- Add module-level overview
234+
235+
### Phase 3: API Improvements
236+
7. **Improve combinator names**:
237+
- `andl` → `and_left()`, `andr` → `and_right()`
238+
- Add operator-like names: `(a & b)`, `(a | b)` in debug output
239+
240+
8. **Add common combinators**:
241+
- `many1()` / `one_or_more()`
242+
- `sep_by()` / `sep_by1()`
243+
- `range(char, char)` for character ranges
244+
- `whitespace()` / `digit()` / `alpha()` helpers
245+
246+
9. **Improve type safety**:
247+
- Consider using a type-safe cache instead of `Any`
248+
- Or use generics more carefully with markers
249+
250+
### Phase 4: Polish & Optimization
251+
10. **Add benchmarks** with criterion
252+
11. **Improve error types**:
253+
- Consider `ParseError` with structured fields
254+
- Support error chains/recovery
255+
256+
12. **Complete examples**:
257+
- Uncomment Sub/Div in arithmetics
258+
- Add simple "hello world" example
259+
- Add comments explaining grammars
260+
261+
13. **UTF-8 documentation**:
262+
- Clearly document position semantics
263+
- Test with unicode characters
264+
- Consider byte offset option
265+
266+
14. **Performance optimization** (if needed):
267+
- Profile memory usage
268+
- Consider cache size limits with LRU eviction
269+
- Optimize hot paths based on profiler
270+
271+
---
272+
273+
## Estimated Effort
274+
275+
| Phase | Effort | Time |
276+
|-------|--------|------|
277+
| Phase 1 | Critical | 1-2 hours |
278+
| Phase 2 | High | 4-6 hours |
279+
| Phase 3 | Medium | 4-8 hours |
280+
| Phase 4 | Polish | 3-6 hours |
281+
| **Total** | | **12-22 hours** |
282+
283+
---
284+
285+
## Nice-to-Have Features (Future)
286+
287+
- [ ] Parser visualization/debugging tools
288+
- [ ] Better error recovery suggestions
289+
- [ ] PEG grammar syntax (DSL) instead of combinators
290+
- [ ] Performance profiling tools
291+
- [ ] Incremental parsing support
292+
- [ ] Serialization of parsers (for caching compiled parsers)
293+
294+
---
295+
296+
## Conclusion
297+
298+
Packrust demonstrates solid understanding of advanced parsing concepts. The main weaknesses are:
299+
1. **Polish**: Error messages and logging could be better
300+
2. **Testing**: Need comprehensive test suite for reliability
301+
3. **Documentation**: Users need clear guidance and examples
302+
4. **Type Safety**: The `Any`-based cache needs rethinking
303+
5. **API Completeness**: Missing common combinators
304+
305+
The priority should be **Phase 1 + 2** (error handling, tests, docs) to make the library production-ready, then **Phase 3** (API completeness) for usability.

‎examples/baab_baab.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,12 @@
55
use packrust::*;
66

77
fn main() {
8+
env_logger::init();
9+
810
// S -> A '-' A
911
// A -> B 'b' / 'b'
1012
// B -> B 'a' / A 'a'
11-
12-
let parser = {
13+
let s = {
1314
let a = lazy("A", |a| {
1415
let b = {
1516
let a = a.clone();
@@ -25,6 +26,6 @@ fn main() {
2526
.end()
2627
.map(|_| "parse success");
2728

28-
let res = parser.run("baab-baab");
29+
let res = s.run("baab-baab");
2930
println!("{:?}", res);
3031
}

‎src/combinators.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ where
3131
let (pos, val) = (self.raw_parser)(pos, ctx)?;
3232
let Some(val) = f(val) else {
3333
return Err(ParseError {
34-
source: ctx.source.iter().collect(),
34+
source: ctx.clone_source(),
3535
pos,
3636
reason: String::from("try map failed: got None"),
3737
});
@@ -110,7 +110,7 @@ where
110110
let (pos, val) = self.parse(pos, ctx)?;
111111
match ctx.source.get(pos) {
112112
Some(c) => Err(ParseError {
113-
source: ctx.source.iter().collect(),
113+
source: ctx.clone_source(),
114114
pos,
115115
reason: format!("expected EOF found {}", c),
116116
}),
@@ -129,12 +129,12 @@ pub fn satisfy(name: impl Into<String>, f: impl Fn(char) -> bool + 'static) -> P
129129
Rc::new(move |pos, ctx: &mut Context| match ctx.source.get(pos) {
130130
Some(&c) if f(c) => Ok((pos + 1, c)),
131131
Some(c) => Err(ParseError {
132-
source: ctx.source.iter().collect(),
132+
source: ctx.clone_source(),
133133
pos,
134134
reason: format!("expected {} got {}", name, c),
135135
}),
136136
None => Err(ParseError {
137-
source: ctx.source.iter().collect(),
137+
source: ctx.clone_source(),
138138
pos,
139139
reason: format!("expected {} got EOF", name),
140140
}),
@@ -166,7 +166,7 @@ pub fn keyword(keyword: impl Into<String>) -> Parser<String> {
166166
Ok((pos + keyword.len(), name.clone()))
167167
} else {
168168
Err(ParseError {
169-
source: ctx.source.iter().collect(),
169+
source: ctx.clone_source(),
170170
pos,
171171
reason: String::from(""),
172172
})

0 commit comments

Comments
 (0)