Rewrite it in rust (regex, utils, nfa, dfa) - #219
modelconsumer wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughIntroduces a complete new Rust crate ( ChangesRust crate introduction
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Regex as "Regex::from_pattern"
participant Tnfa as "Tnfa::from_rules"
participant Tdfa as "Tdfa::for_rules"
participant Jit as "Jit::jit"
participant Compiled as "JittedDfa"
Caller->>Regex: parse pattern string
Regex-->>Caller: AnchoredRegex AST
Caller->>Tnfa: build NFA from rules
Tnfa-->>Caller: Tnfa (states, capture tags)
Caller->>Tdfa: determinize NFA into TDFA
Tdfa-->>Caller: Tdfa (states, register operations)
Caller->>Jit: compile TDFA to native code
Jit-->>Caller: JittedDfa function pointer
Caller->>Compiled: execute(input)
Compiled-->>Caller: matched rule index + end pointer
Poem — skipped for this response as it is optional and not requested. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
38b0d2c to
c09886f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/README.md (1)
1-66: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReformat to satisfy markdownlint.
This file currently trips markdownlint rules for heading style, blank lines around headings/fenced blocks, and the final fence is missing a language tag. (github.com)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/README.md` around lines 1 - 66, Reformat the Rust README to satisfy markdownlint by fixing the heading style and adding the required blank lines around headings and fenced code blocks. Update the affected sections in the README content so the headings render consistently, ensure each fenced block is surrounded by proper spacing, and add a language tag to the final code fence near the tracing instructions.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/build.rs`:
- Around line 6-21: The build script only reruns when build.rs changes, so
generated_bindings.hpp can become stale when cbindgen.toml or the Rust sources
change. Update the logic in generate_c_bindings and the build entrypoint in
build.rs to emit rerun-if-changed directives for the cbindgen config and the
Rust source tree used by cbindgen. Make sure the watch list covers the inputs
that affect cbindgen output so Cargo regenerates the header when those files
change.
In `@rust/Cargo.toml`:
- Around line 29-32: The Cargo feature setup enables the jit/Cranelift path by
default, which makes an experimental dependency part of every normal build.
Update the package feature declarations around default and jit so Cranelift JIT
is not default-on unless that is a deliberate decision, and ensure any related
feature wiring in the same Cargo.toml section is adjusted consistently so users
must opt in explicitly.
- Around line 55-58: The PyO3 dependency is pinned to an affected version, so
update the pyo3 entry in Cargo.toml from the current 0.27 floor to 0.29.0 or
newer. Keep the existing dependency shape for the [dependencies.pyo3] block, but
raise the version so the maturin-built Python extension uses a fixed PyO3
release.
In `@rust/docs/parsing-spec-file.md`:
- Around line 98-133: The grammar code fence in the parsing-spec documentation
is unlabeled, so add an explicit language tag to the fenced block to enable
highlighting and satisfy markdownlint; update the fence around the
root_pattern/alternation grammar snippet to use a suitable label such as ebnf or
text, and make the same kind of change to the earlier example fence in this
document if you want the file fully lint-clean.
In `@rust/docs/parsing.md`:
- Around line 64-109: The pseudocode blocks in the parsing docs are missing
fenced language tags, which triggers markdownlint and disables highlighting.
Update both code fences in the DFA and tagged DFA examples in parsing.md to use
an explicit fence label, choosing text for plain pseudocode or rust if you want
Rust highlighting. Keep the surrounding prose unchanged and ensure the fenced
examples remain easy to locate by their DFA and submatch extraction sections.
- Around line 161-165: The TODO section in the parsing docs is incomplete
because the “anchors” and “leaf ambiguity” topics are still unresolved. Update
the documentation by either filling in those explanations in the parsing.md TODO
section or removing the TODO entirely if the content is moved elsewhere, and
make sure the relevant parsing semantics are fully described where the TODO
currently appears.
- Around line 1-3: The opening sentence in the parsing documentation is still
placeholder text; update the intro under the parsing heading in the docs page to
a real summary that explains what the page covers. Use the existing parsing
section content and the nearby “Parsing Specification File” reference as
context, and replace the draft-style sentence with a concise overview that
matches the page’s purpose.
In `@rust/src/lib.rs`:
- Around line 1-5: The crate-wide legacy macro imports in the Rust library root
are unused and should be removed or modernized. Update the `lib.rs` setup by
dropping the `#[macro_use] extern crate tracing;` and `#[macro_use(Serialize,
Deserialize)] extern crate serde;` declarations, and if `debug!` or the serde
derives are actually needed, bring them into scope explicitly at the specific
use sites with normal `use` imports instead of relying on `macro_use`.
In `@rust/src/regex/pattern_parsing.rs`:
- Around line 294-298: The From<Infallible> implementation for RegexError is
recursively calling itself via infallible.into(), so replace the body with the
standard uninhabited-match pattern inside the From<std::convert::Infallible>
impl to make the conversion unreachable without recursion. Keep the change
localized to the RegexError conversion in pattern_parsing.rs and ensure the from
method no longer delegates back to itself.
- Around line 232-238: The placeholder handling in
replace_with_placeholders/number_captures is mutating a Regex that may still
share its inner Arc<SubRule> because lookup returns only a shallow clone, which
can make Arc::get_mut(...).unwrap() panic later. Update the placeholder
replacement path in pattern_parsing so the Regex pulled from
get_placeholder.lookup(name) is deep-cloned or made uniquely owned before
assigning it into item, ensuring each reused placeholder has its own mutable
copy before capture numbering runs.
In `@rust/src/utils/macros.rs`:
- Around line 25-47: The `spec!` macro is being exported as part of the normal
crate API instead of being test-only. Gate the `mod test` block and the
`#[macro_export] macro_rules! spec` helper behind `#[cfg(test)]` so it is only
compiled for tests, and keep its `unwrap()`-based parsing logic out of the
public release surface. Use the existing `spec!`, `mod test`, and
`ParsingSpecBuilder` symbols to relocate the helper into test-only code without
changing its behavior in tests.
In `@rust/src/utils/serde.rs`:
- Around line 16-17: The ArrayVisitor derive is introducing an unnecessary T:
Debug requirement that leaks into SerdeArray deserialization support. Remove the
#[derive(Debug, Clone)] bound from ArrayVisitor<T, N> and replace it with an
implementation that does not constrain T, then update the Deserialize for
SerdeArray<[T; N]> and Visitor for ArrayVisitor<T, N> bounds so they only
require the traits actually used by deserialize and visit_seq. Keep the focus on
ArrayVisitor, SerdeArray, and the Visitor impl so the public utility accepts T:
Deserialize types even when T does not implement Debug.
- Around line 58-72: The array deserialization logic in visit_seq currently
reports the expected length N as the observed length when seq.next_element()
returns None, which makes Error::invalid_length misleading. Update the failure
path in visit_seq so it uses the number of elements already collected
(elements.len()) when constructing the invalid_length error, keeping the rest of
the Vec<T> to [T; N] conversion flow unchanged.
---
Outside diff comments:
In `@rust/README.md`:
- Around line 1-66: Reformat the Rust README to satisfy markdownlint by fixing
the heading style and adding the required blank lines around headings and fenced
code blocks. Update the affected sections in the README content so the headings
render consistently, ensure each fenced block is surrounded by proper spacing,
and add a language tag to the final code fence near the tracing instructions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a9aa3a00-fe7a-43a8-834c-41a638066cff
📒 Files selected for processing (24)
CMakeLists.txtrust/.gitignorerust/.rust-analyzer.tomlrust/Cargo.tomlrust/README.mdrust/build.rsrust/cbindgen.tomlrust/compile_flags.txtrust/docs/parsing-spec-file.mdrust/docs/parsing.mdrust/pyproject.tomlrust/rust-toolchain.tomlrust/rustfmt.tomlrust/src/lib.rsrust/src/regex.rsrust/src/regex/pattern_parsing.rsrust/src/utils.rsrust/src/utils/convert.rsrust/src/utils/escaping.rsrust/src/utils/macros.rsrust/src/utils/nom.rsrust/src/utils/serde.rsrust/src/utils/tarjan_scc.rstaskfiles/lint.yaml
| println!("cargo:rerun-if-changed=build.rs"); | ||
|
|
||
| let root_dir: PathBuf = std::env::current_dir().unwrap(); | ||
|
|
||
| // Ignore error; don't choke rustfmt/rust-analyzer/rustc just because there's a syntax error. | ||
| let _ = generate_c_bindings(&root_dir); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| fn generate_c_bindings(root_dir: &Path) -> Result<(), Box<dyn Error>> { | ||
| cbindgen::Builder::new() | ||
| .with_config(cbindgen::Config::from_file("cbindgen.toml")?) | ||
| .with_crate(root_dir) | ||
| .generate()? | ||
| .write_to_file(root_dir.join("cxx").join("log_surgeon").join("generated_bindings.hpp")); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo root: '; pwd
printf '\nFiles of interest:\n'
git ls-files rust/build.rs rust/Cargo.toml rust/cbindgen.toml rust/src 2>/dev/null || true
printf '\n--- rust/build.rs ---\n'
cat -n rust/build.rs
printf '\n--- cargo/rust related files ---\n'
rg -n --hidden --glob 'rust/**' 'rerun-if-changed|CARGO_MANIFEST_DIR|current_dir|cbindgen' rustRepository: y-scope/log-surgeon
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- rust/Cargo.toml ---\n'
cat -n rust/Cargo.toml
printf '\n--- rust/cbindgen.toml ---\n'
cat -n rust/cbindgen.toml
printf '\n--- references to generated_bindings.hpp ---\n'
rg -n --hidden --glob 'rust/**' 'generated_bindings.hpp|log_surgeon/generated_bindings.hpp|cxx/log_surgeon' rustRepository: y-scope/log-surgeon
Length of output: 7497
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
root = Path('rust')
for rel in ['Cargo.toml', 'cbindgen.toml']:
p = root / rel
print(f'--- {p} ({p.stat().st_size} bytes) ---')
print(p.read_text())
PYRepository: y-scope/log-surgeon
Length of output: 5507
Watch cbindgen inputs. rust/build.rs:6-21 only watches build.rs, so edits to rust/cbindgen.toml or the Rust source tree can leave rust/cxx/log_surgeon/generated_bindings.hpp stale. Add rerun-if-changed entries for those inputs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/build.rs` around lines 6 - 21, The build script only reruns when
build.rs changes, so generated_bindings.hpp can become stale when cbindgen.toml
or the Rust sources change. Update the logic in generate_c_bindings and the
build entrypoint in build.rs to emit rerun-if-changed directives for the
cbindgen config and the Rust source tree used by cbindgen. Make sure the watch
list covers the inputs that affect cbindgen output so Cargo regenerates the
header when those files change.
| default = [ | ||
| "jit", | ||
| ] | ||
| jit = [] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial
jit (Cranelift) is enabled by default while cranelift-jit is explicitly "extremely experimental" upstream.
Since default = ["jit"], any consumer of cargo build gets Cranelift JIT compiled in by default. The cranelift-jit crate's own description flags itself as experimental, which is a meaningful stability risk to inherit as a default-on feature this early in a regex-engine rewrite.
Worth confirming this is an intentional risk acceptance, and considering whether jit should remain opt-in until the engine stabilizes.
Also applies to: 50-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/Cargo.toml` around lines 29 - 32, The Cargo feature setup enables the
jit/Cranelift path by default, which makes an experimental dependency part of
every normal build. Update the package feature declarations around default and
jit so Cranelift JIT is not default-on unless that is a deliberate decision, and
ensure any related feature wiring in the same Cargo.toml section is adjusted
consistently so users must opt in explicitly.
| [dependencies.pyo3] | ||
| version = "0.27" | ||
| default-features = false | ||
| optional = true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What are the CVE/RustSec advisory details for pyo3 versions before 0.29, specifically the Sync bound and out-of-bounds read fixes?
💡 Result:
PyO3 versions before 0.29 are affected by two security vulnerabilities disclosed in June 2026, which were addressed in version 0.29.0 [1][2]. 1. Out-of-bounds Read (RUSTSEC-2026-0176) This vulnerability affected PyO3 versions >=0.24.0, <0.29.0 [1][3]. It involved optimized implementations of Iterator::nth and DoubleEndedIterator::nth_back for BoundListIterator and BoundTupleIterator [1][3]. These methods used unchecked usize addition (index + n) before bounds-checking, which could cause integer overflow or underflow [1][3]. This allowed potential out-of-bounds reads in memory [1][3]. PyO3 0.29.0 corrected these methods to use checked arithmetic [1][3]. 2. Missing Sync Bound (RUSTSEC-2026-0177 / GHSA-chgr-c6px-7xpp) This vulnerability affected PyO3 versions prior to 0.29.0 [2][4]. The function PyCFunction::new_closure (including the temporary new_closure_bound in the 0.21–0.22 series) required the provided closure to be Send + 'static, but it lacked a Sync bound [2][4]. Because the resulting PyCFunction could be invoked from any Python thread, this missing bound could lead to data races, especially in free-threaded Python environments where the Global Interpreter Lock (GIL) does not enforce serial execution [2][4]. PyO3 0.29.0 added the necessary Sync bound to resolve this [2][4].
Citations:
- 1: https://rustsec.org/advisories/RUSTSEC-2026-0176.html
- 2: https://rustsec.org/advisories/RUSTSEC-2026-0177.html
- 3: https://osv.dev/vulnerability/RUSTSEC-2026-0176
- 4: GHSA-chgr-c6px-7xpp
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Cargo.toml excerpt =="
nl -ba rust/Cargo.toml | sed -n '1,140p'
echo
echo "== Search for pyo3 usage across rust/ =="
rg -n --hidden -S '\bpyo3\b|PyO3|PyCFunction::new_closure|new_closure_bound' rust
echo
echo "== Feature declarations in rust/Cargo.toml =="
rg -n --hidden -S '^\[features\]|^\[dependencies\.pyo3\]|^\[dependencies\.[^]]+\]' rust/Cargo.tomlRepository: y-scope/log-surgeon
Length of output: 222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== rust/Cargo.toml excerpt =="
sed -n '1,180p' rust/Cargo.toml
echo
echo "== pyo3 references under rust/ =="
rg -n --hidden -S '\bpyo3\b|PyO3|PyCFunction::new_closure|new_closure_bound' rust || true
echo
echo "== feature/dependency markers in rust/Cargo.toml =="
rg -n --hidden -S '^\[features\]|^\[dependencies\.pyo3\]|^\[dependencies\.[^]]+\]' rust/Cargo.toml || trueRepository: y-scope/log-surgeon
Length of output: 2164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== rust source files =="
find rust/src -maxdepth 2 -type f | sort
echo
echo "== outline of likely Python interface file(s) =="
for f in rust/src/python_interface.rs rust/src/lib.rs; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,260p' "$f"
echo
fi
done
echo
echo "== pyo3 attribute/API search in rust/src =="
rg -n --hidden -S '#\[py|pyo3::|PyCFunction|new_closure|BoundListIterator|BoundTupleIterator|#[[:space:]]*pymodule|#[[:space:]]*pyfunction' rust/src || trueRepository: y-scope/log-surgeon
Length of output: 3157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository references to python_interface / pyo3 =="
rg -n --hidden -S 'python_interface|PyO3|pyo3|maturin|extension-module|new_closure|PyCFunction' .
echo
echo "== files that mention the python feature =="
rg -n --hidden -S 'feature\s*=\s*"python"|cfg\(feature\s*=\s*"python"\)|python\s*=' rust . || trueRepository: y-scope/log-surgeon
Length of output: 1654
Upgrade PyO3 to 0.29.0 or newer
pyo3 = "0.27" is still in the affected range for the 0.29.0 security fixes, and the python feature wires it into the maturin-built extension. Bump the floor to 0.29.0+ so the shipped Python build picks up the fixes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/Cargo.toml` around lines 55 - 58, The PyO3 dependency is pinned to an
affected version, so update the pyo3 entry in Cargo.toml from the current 0.27
floor to 0.29.0 or newer. Keep the existing dependency shape for the
[dependencies.pyo3] block, but raise the version so the maturin-built Python
extension uses a fixed PyO3 release.
| ``` | ||
| // A root pattern may be "anchored". | ||
| root_pattern: "^"? alternation "$"? | ||
|
|
||
| alternation: sequence ("|" sequence)* | ||
|
|
||
| sequence: suffixed_term+ | ||
|
|
||
| suffixed_term: term repetition_suffix? | ||
|
|
||
| term: | ||
| "." | ||
| bracketed_ranges | ||
| symbol | ||
| "(" alternation ")" | ||
| "(?<" name ">" alternation ")" // A subrule. | ||
|
|
||
| repetition_suffix: | ||
| "*" // 0 or more. | ||
| "+" // 1 or more. | ||
| "?" // 0 or 1. | ||
| "{" decimal_integer "}" // Repeat exactly this many times. | ||
| "{" decimal_integer "," decimal_integer "}" // Repeat min to max times (inclusive). | ||
|
|
||
| bracketed_ranges: "[" bracketed_item+ "]" | ||
|
|
||
| symbol: | ||
| "\" escaped_character | ||
| unescaped_character | ||
|
|
||
| bracketed_item: | ||
| "^" | ||
| "\^" | ||
| symbol "-" symbol // Character range, inclusive. | ||
| symbol | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the grammar fence.
This block is unlabeled, so it misses syntax highlighting and trips markdownlint. Tag it as ebnf or text; the earlier example fence should be tagged as well if you want the file lint-clean.
Suggested edit
-```
+```ebnf📝 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.
| ``` | |
| // A root pattern may be "anchored". | |
| root_pattern: "^"? alternation "$"? | |
| alternation: sequence ("|" sequence)* | |
| sequence: suffixed_term+ | |
| suffixed_term: term repetition_suffix? | |
| term: | |
| "." | |
| bracketed_ranges | |
| symbol | |
| "(" alternation ")" | |
| "(?<" name ">" alternation ")" // A subrule. | |
| repetition_suffix: | |
| "*" // 0 or more. | |
| "+" // 1 or more. | |
| "?" // 0 or 1. | |
| "{" decimal_integer "}" // Repeat exactly this many times. | |
| "{" decimal_integer "," decimal_integer "}" // Repeat min to max times (inclusive). | |
| bracketed_ranges: "[" bracketed_item+ "]" | |
| symbol: | |
| "\" escaped_character | |
| unescaped_character | |
| bracketed_item: | |
| "^" | |
| "\^" | |
| symbol "-" symbol // Character range, inclusive. | |
| symbol | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 98-98: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/docs/parsing-spec-file.md` around lines 98 - 133, The grammar code fence
in the parsing-spec documentation is unlabeled, so add an explicit language tag
to the fenced block to enable highlighting and satisfy markdownlint; update the
fence around the root_pattern/alternation grammar snippet to use a suitable
label such as ebnf or text, and make the same kind of change to the earlier
example fence in this document if you want the file fully lint-clean.
Source: Linters/SAST tools
| ## Parsing in Log Surgeon | ||
| An overview of the title. | ||
| See also: [Parsing Specification File][parsing-spec]. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the placeholder opening sentence.
“An overview of the title.” reads like draft text, not documentation. Replace it with a real summary of what this page covers.
Suggested edit
- An overview of the title.
+ An overview of how Log Surgeon parses and matches log events.📝 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.
| ## Parsing in Log Surgeon | |
| An overview of the title. | |
| See also: [Parsing Specification File][parsing-spec]. | |
| ## Parsing in Log Surgeon | |
| An overview of how Log Surgeon parses and matches log events. | |
| See also: [Parsing Specification File][parsing-spec]. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/docs/parsing.md` around lines 1 - 3, The opening sentence in the parsing
documentation is still placeholder text; update the intro under the parsing
heading in the docs page to a real summary that explains what the page covers.
Use the existing parsing section content and the nearby “Parsing Specification
File” reference as context, and replace the draft-style sentence with a concise
overview that matches the page’s purpose.
| Self::Placeholder { name, item } => { | ||
| let Some(placeholder): Option<Regex> = get_placeholder.lookup(name) else { | ||
| return Err(RegexErrorKind::UndefinedPlaceholder(name.clone())); | ||
| }; | ||
| **item = placeholder; | ||
| Ok(()) | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how placeholders/captures are constructed and whether lookups can yield captures.
fd -e rs | xargs rg -nP 'get_mut|make_mut|Arc::new|fn lookup|cloned\(\)' -g '*.rs' -C2Repository: y-scope/log-surgeon
Length of output: 2489
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Regex/SubRule definitions and clone implementation paths.
sed -n '1,220p' rust/src/regex/pattern_parsing.rs
printf '\n---\n'
rg -n "enum Regex|struct SubRule|impl Clone for Regex|derive\\(Clone\\)|Arc::new\\(SubRule|Capture\\(" rust/src/regex -n -C2Repository: y-scope/log-surgeon
Length of output: 8227
Deep-clone placeholder regexes before mutating captures. rust/src/regex/pattern_parsing.rs:25-26 clones the stored Regex, so a reused placeholder can keep sharing the inner Arc<SubRule> and make Arc::get_mut(...).unwrap() in replace_with_placeholders/number_captures panic. Use a deep clone or Arc::make_mut before numbering captures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/regex/pattern_parsing.rs` around lines 232 - 238, The placeholder
handling in replace_with_placeholders/number_captures is mutating a Regex that
may still share its inner Arc<SubRule> because lookup returns only a shallow
clone, which can make Arc::get_mut(...).unwrap() panic later. Update the
placeholder replacement path in pattern_parsing so the Regex pulled from
get_placeholder.lookup(name) is deep-cloned or made uniquely owned before
assigning it into item, ensuring each reused placeholder has its own mutable
copy before capture numbering runs.
| impl From<std::convert::Infallible> for RegexError { | ||
| fn from(infallible: std::convert::Infallible) -> Self { | ||
| infallible.into() | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
From<Infallible> body recurses infinitely.
infallible.into() resolves back to this same From<Infallible> for RegexError impl, so the body is unconditional recursion. Although Infallible is uninhabited (so the path is unreachable at runtime), this trips rustc's unconditional_recursion lint and is semantically wrong. Use the uninhabited-match idiom instead.
🐛 Proposed fix
impl From<std::convert::Infallible> for RegexError {
- fn from(infallible: std::convert::Infallible) -> Self {
- infallible.into()
- }
+ fn from(infallible: std::convert::Infallible) -> Self {
+ match infallible {}
+ }
}📝 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.
| impl From<std::convert::Infallible> for RegexError { | |
| fn from(infallible: std::convert::Infallible) -> Self { | |
| infallible.into() | |
| } | |
| } | |
| impl From<std::convert::Infallible> for RegexError { | |
| fn from(infallible: std::convert::Infallible) -> Self { | |
| match infallible {} | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/regex/pattern_parsing.rs` around lines 294 - 298, The
From<Infallible> implementation for RegexError is recursively calling itself via
infallible.into(), so replace the body with the standard uninhabited-match
pattern inside the From<std::convert::Infallible> impl to make the conversion
unreachable without recursion. Keep the change localized to the RegexError
conversion in pattern_parsing.rs and ensure the from method no longer delegates
back to itself.
| #[macro_use] | ||
| mod test { | ||
| #[macro_export] | ||
| macro_rules! spec { | ||
| ($definition:expr) => {{ | ||
| use $crate::parsing_spec::ParsingSpec; | ||
| use $crate::parsing_spec::ParsingSpecBuilder; | ||
|
|
||
| let definition: &::std::primitive::str = $definition; | ||
|
|
||
| // Canonicalize spec by round-tripping. | ||
| let spec: ParsingSpec = ParsingSpecBuilder::from_parsing_spec_definition(definition) | ||
| .unwrap() | ||
| .build(); | ||
| let roundtrip: String = spec.to_parsing_spec_definition(); | ||
| let spec: ParsingSpec = ParsingSpecBuilder::from_parsing_spec_definition(&roundtrip) | ||
| .unwrap() | ||
| .build(); | ||
|
|
||
| spec | ||
| }}; | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
spec! test helper is exported unconditionally, not gated to test builds.
mod test (despite the name) isn't #[cfg(test)], so #[macro_export] macro_rules! spec! is always compiled and always part of the crate's public macro namespace — usable by downstream consumers in release builds, not just tests. It also .unwrap()s on parse failures (fine for tests, not for production API surface) and depends on crate::parsing_spec::{ParsingSpec, ParsingSpecBuilder} being part of the stable contract.
♻️ Suggested fix
-#[macro_use]
-mod test {
+#[cfg(test)]
+#[macro_use]
+mod test {
#[macro_export]
macro_rules! spec {📝 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.
| #[macro_use] | |
| mod test { | |
| #[macro_export] | |
| macro_rules! spec { | |
| ($definition:expr) => {{ | |
| use $crate::parsing_spec::ParsingSpec; | |
| use $crate::parsing_spec::ParsingSpecBuilder; | |
| let definition: &::std::primitive::str = $definition; | |
| // Canonicalize spec by round-tripping. | |
| let spec: ParsingSpec = ParsingSpecBuilder::from_parsing_spec_definition(definition) | |
| .unwrap() | |
| .build(); | |
| let roundtrip: String = spec.to_parsing_spec_definition(); | |
| let spec: ParsingSpec = ParsingSpecBuilder::from_parsing_spec_definition(&roundtrip) | |
| .unwrap() | |
| .build(); | |
| spec | |
| }}; | |
| } | |
| } | |
| #[cfg(test)] | |
| #[macro_use] | |
| mod test { | |
| #[macro_export] | |
| macro_rules! spec { | |
| ($definition:expr) => {{ | |
| use $crate::parsing_spec::ParsingSpec; | |
| use $crate::parsing_spec::ParsingSpecBuilder; | |
| let definition: &::std::primitive::str = $definition; | |
| // Canonicalize spec by round-tripping. | |
| let spec: ParsingSpec = ParsingSpecBuilder::from_parsing_spec_definition(definition) | |
| .unwrap() | |
| .build(); | |
| let roundtrip: String = spec.to_parsing_spec_definition(); | |
| let spec: ParsingSpec = ParsingSpecBuilder::from_parsing_spec_definition(&roundtrip) | |
| .unwrap() | |
| .build(); | |
| spec | |
| }}; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/utils/macros.rs` around lines 25 - 47, The `spec!` macro is being
exported as part of the normal crate API instead of being test-only. Gate the
`mod test` block and the `#[macro_export] macro_rules! spec` helper behind
`#[cfg(test)]` so it is only compiled for tests, and keep its `unwrap()`-based
parsing logic out of the public release surface. Use the existing `spec!`, `mod
test`, and `ParsingSpecBuilder` symbols to relocate the helper into test-only
code without changing its behavior in tests.
| #[derive(Debug, Clone)] | ||
| struct ArrayVisitor<T, const N: usize>(PhantomData<[T; N]>); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Unnecessary Debug bound restricts which element types SerdeArray supports.
Deserialize for SerdeArray<[T; N]> and Visitor for ArrayVisitor<T, N> both require T: std::fmt::Debug, but nothing in deserialize/visit_seq actually calls a Debug method on T or on Self. This looks like a side effect of #[derive(Debug, Clone)] on ArrayVisitor<T, N> (line 16-17), which naively adds a T: Debug bound even though PhantomData<[T; N]> doesn't need it. As written, any T: Deserialize type that doesn't implement Debug can't be wrapped in SerdeArray, unnecessarily narrowing this public utility's applicability.
Consider hand-writing Debug for ArrayVisitor (or dropping the derive) so the Debug bound isn't forced onto Deserialize/Visitor.
Also applies to: 35-38, 48-51
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/utils/serde.rs` around lines 16 - 17, The ArrayVisitor derive is
introducing an unnecessary T: Debug requirement that leaks into SerdeArray
deserialization support. Remove the #[derive(Debug, Clone)] bound from
ArrayVisitor<T, N> and replace it with an implementation that does not constrain
T, then update the Deserialize for SerdeArray<[T; N]> and Visitor for
ArrayVisitor<T, N> bounds so they only require the traits actually used by
deserialize and visit_seq. Keep the focus on ArrayVisitor, SerdeArray, and the
Visitor impl so the public utility accepts T: Deserialize types even when T does
not implement Debug.
| #[inline] | ||
| fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error> | ||
| where | ||
| A: SeqAccess<'de>, | ||
| { | ||
| let mut elements: Vec<T> = Vec::with_capacity(N); | ||
| for _ in 0..N { | ||
| if let Some(e) = seq.next_element()? { | ||
| elements.push(e); | ||
| } else { | ||
| return Err(Error::invalid_length(N, &self)); | ||
| } | ||
| } | ||
| Ok(<[T; N]>::try_from(elements).unwrap()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
invalid_length reports the wrong count.
Error::invalid_length(N, &self) at line 68 always reports N (the expected length) as the observed length, even though the failure happens precisely because fewer than N elements were available. Idiomatic serde array implementations pass the actual count already read (elements.len()), so the error message is meaningful (e.g. "invalid length 2, expected an array of length 5") instead of self-contradictory (currently it would read "invalid length 5, expected an array of length 5").
🐛 Proposed fix
for _ in 0..N {
if let Some(e) = seq.next_element()? {
elements.push(e);
} else {
- return Err(Error::invalid_length(N, &self));
+ return Err(Error::invalid_length(elements.len(), &self));
}
}📝 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.
| #[inline] | |
| fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error> | |
| where | |
| A: SeqAccess<'de>, | |
| { | |
| let mut elements: Vec<T> = Vec::with_capacity(N); | |
| for _ in 0..N { | |
| if let Some(e) = seq.next_element()? { | |
| elements.push(e); | |
| } else { | |
| return Err(Error::invalid_length(N, &self)); | |
| } | |
| } | |
| Ok(<[T; N]>::try_from(elements).unwrap()) | |
| } | |
| #[inline] | |
| fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error> | |
| where | |
| A: SeqAccess<'de>, | |
| { | |
| let mut elements: Vec<T> = Vec::with_capacity(N); | |
| for _ in 0..N { | |
| if let Some(e) = seq.next_element()? { | |
| elements.push(e); | |
| } else { | |
| return Err(Error::invalid_length(elements.len(), &self)); | |
| } | |
| } | |
| Ok(<[T; N]>::try_from(elements).unwrap()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/utils/serde.rs` around lines 58 - 72, The array deserialization
logic in visit_seq currently reports the expected length N as the observed
length when seq.next_element() returns None, which makes Error::invalid_length
misleading. Update the failure path in visit_seq so it uses the number of
elements already collected (elements.len()) when constructing the invalid_length
error, keeping the rest of the Vec<T> to [T; N] conversion flow unchanged.
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/src/dfa.rs`:
- Around line 34-49: Tdfa should not be round-tripped with serde while relying
on skipped runtime state, because deserialization drops fields used by
lookup_transition and capture execution. Fix this by removing the
Serialize/Deserialize derives from Tdfa or by adding custom (de)serialization
that rebuilds all runtime fields such as tags, number_of_registers, final
operations, and the ASCII cache before the DFA is used again. Update the Tdfa
type and its construction/deserialization path so the runtime metadata is always
restored consistently.
In `@rust/src/dfa/jit.rs`:
- Around line 88-129: The returned JIT entrypoint in jit() is currently a bare
JittedDfa produced from get_finalized_function, which can outlive the Jit-owned
JITModule. Update Jit::jit and the JittedDfa return type so the function pointer
is wrapped in a lifetime-bound type tied to Jit, rather than exposing an
unconstrained extern "C" fn. Use the existing symbols jit, JittedDfa, and
get_finalized_function to locate the return path and ensure callers cannot use
the compiled code after Jit is dropped.
- Around line 361-431: The multibyte UTF-8 decode path in the JIT logic is
reusing a shifted/masked `ch_a` for both length detection and payload assembly,
which causes 3- and 4-byte sequences to be decoded incorrectly. Update the
multibyte handling in this `jit.rs` block to preserve the original lead byte
before shifting, use that original value when computing `bits_6_5` and
`is_2_bytes`/`is_3_bytes`, and only apply the per-length masks/shifts to
separate temporary values when building `ch` for the 2-, 3-, and 4-byte cases.
- Around line 109-124: The JIT timing code in the dfa/jit module uses an
undefined now! macro, which prevents compilation. Replace those timing calls
with a valid source of timestamps such as Instant::now(), or add the missing
macro/import consistently where the timing is recorded in the JIT path
(including the repeated timing blocks referenced by the same symbols). Keep the
change localized around the define_function, clear_context,
finalize_definitions, and get_finalized_function timing/debug flow so the
existing measurement logic still works.
In `@rust/src/lib.rs`:
- Around line 41-55: Make enable_tracing() idempotent by replacing the direct
tracing_subscriber::fmt() .init() call with a non-panicking initialization path.
In the enable_tracing function, use .try_init() and explicitly ignore the
already-initialized error so repeated calls or pre-existing subscribers do not
panic, while keeping the existing EnvFilter and fmt subscriber configuration
unchanged.
In `@rust/src/nfa/regex_construction.rs`:
- Around line 199-211: The bounded repetition handling in regex_construction.rs
is only applying negative_tags for the first skipped optional repetition, which
leaves later skipped capture-producing repetitions untagged and can misalign
TDFA histories. In the loop inside the bounded `{m,n}` construction, update the
logic around the `sub_skip` path so every optional repetition that may be
skipped gets passed through `negative_tags`, not just the `i == 0` case, and
keep the behavior localized to the NFA-building flow in the regex construction
routine.
In `@rust/src/regex.rs`:
- Around line 65-68: The Display implementation for Regex is escaping the
pattern output, so it prints Rust-literal text instead of the raw regex. Update
the fmt logic in the Display impl for Regex to write the string returned by
to_pattern() directly, and keep escape_default() only in Debug so
Regex::Literal('.') renders as \. rather than \\..
In `@rust/src/regex/pattern_parsing.rs`:
- Around line 265-269: The qualified capture name construction in
`pattern_parsing` currently always formats with `"{}.{}"`, so top-level captures
end up with a leading dot from `maybe_parent.map_or("", ...)`. Update the logic
where `sub_rule.qualified_name` is assigned to special-case the top-level case
and build the name from just `sub_rule.name` when there is no parent, while
keeping the existing parent-prefixed form for nested captures.
- Around line 232-237: The placeholder handling in the regex substitution path
leaves nested placeholders unresolved after assigning into the Placeholder
branch. Update the logic in the match arm for Self::Placeholder in
pattern_parsing.rs so that, after replacing item with the looked-up Regex, the
newly inserted regex is traversed or re-parsed for further placeholder expansion
instead of stopping at Ok(()). Use the Self::Placeholder branch and the
placeholder lookup/assignment flow to locate the fix, and ensure chained
placeholders do not remain as Regex::NIL.
In `@rust/src/utils/convert.rs`:
- Around line 1-16: The crate-specific LocalTryInto shim is unnecessary here and
can conflict with the standard prelude try_into method. Remove the LocalTryInto
trait and its blanket impl in convert.rs, then implement the standard
TryFrom<&str> for AnchoredRegex directly so callers can use the normal
conversion API without the custom wrapper.
In `@rust/src/utils/escaping.rs`:
- Around line 109-111: The escaping logic in `escape_char`/`escape_default`
produces Rust-style Unicode escapes that `unescape()` cannot parse reliably, so
update one side to keep round-tripping valid. Either change the formatter path
that emits escaped chars to produce the pair-padded `\u{..}` form expected by
`unescape()`, or extend `unescape()` to accept standard Rust Unicode escape
syntax for the emitted values. Ensure the fix is applied consistently across the
`escaping.rs` helpers so escaped output remains parseable by `unescape()`.
In `@rust/src/utils/nom.rs`:
- Around line 56-57: Remove the explicit type-underscore annotations in the
parser setup inside NomUtils::parse_until_close; both cut_inside and cut_close
can rely on type inference, so update those let bindings to drop the : _
annotations while keeping the existing cut(...) expressions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 150fa69e-2b29-4a2f-bb7e-cc64e030fa42
📒 Files selected for processing (14)
rust/src/dfa.rsrust/src/dfa/compressed.rsrust/src/dfa/jit.rsrust/src/lib.rsrust/src/nfa.rsrust/src/nfa/graph_dot_output.rsrust/src/nfa/regex_construction.rsrust/src/nfa/search_decomposition.rsrust/src/regex.rsrust/src/regex/pattern_parsing.rsrust/src/utils.rsrust/src/utils/convert.rsrust/src/utils/escaping.rsrust/src/utils/nom.rs
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct Tdfa { | ||
| states: Vec<DfaState>, | ||
| #[serde(skip)] | ||
| kernels: BTreeMap<Kernel, usize>, | ||
| #[serde(skip)] | ||
| pub tags: Vec<CaptureTag>, | ||
| /// Bijection between indices of start/end capture pair tags. | ||
| #[serde(skip)] | ||
| tag_pairs: Vec<usize>, | ||
| /// During construction, this is the "current" count; | ||
| /// after construction, this is the "total required". | ||
| /// The first `tags.len()` are initial registers for the corresponding tags. | ||
| /// The second `tags.len()` (i.e. `tags.len()..(2 * tags.len())`) are the corresponding final registers. | ||
| #[serde(skip)] | ||
| pub number_of_registers: usize, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not deserialize Tdfa with skipped runtime state.
A deserialized Tdfa loses tags, register counts, final operations, and the ASCII cache. Since lookup_transition trusts ascii_cache for ASCII, a round-tripped DFA will reject ASCII matches; capture execution also cannot recover the skipped tag/register metadata. Prefer removing serde derives from Tdfa or implement custom deserialization that rebuilds all runtime fields.
🛡️ Safer minimal fix
-#[derive(Debug, Clone, Serialize, Deserialize)]
+#[derive(Debug, Clone)]
pub struct Tdfa {Also applies to: 78-104, 333-343
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/dfa.rs` around lines 34 - 49, Tdfa should not be round-tripped with
serde while relying on skipped runtime state, because deserialization drops
fields used by lookup_transition and capture execution. Fix this by removing the
Serialize/Deserialize derives from Tdfa or by adding custom (de)serialization
that rebuilds all runtime fields such as tags, number_of_registers, final
operations, and the ASCII cache before the DFA is used again. Update the Tdfa
type and its construction/deserialization path so the runtime metadata is always
restored consistently.
| pub fn jit(&mut self, dfa: &Tdfa) -> Result<JittedDfa, ()> { | ||
| let mut sig: Signature = self.module.make_signature(); | ||
| let ptr_ty: Type = self.module.isa().pointer_type(); | ||
|
|
||
| sig.params.push(AbiParam::new(ptr_ty)); // input_ptr | ||
| sig.params.push(AbiParam::new(ptr_ty)); // input_ptr_end | ||
| sig.params.push(AbiParam::new(types::I32)); // anchor | ||
| sig.params.push(AbiParam::new(ptr_ty)); // new input ptr | ||
| sig.returns.push(AbiParam::new(types::I16)); // rule | ||
|
|
||
| let func: FuncId = self.module.declare_anonymous_function(&sig).unwrap(); | ||
|
|
||
| self.context.func.signature = sig; | ||
|
|
||
| (Compilation { | ||
| module: &mut self.module, | ||
| asm: FunctionBuilder::new(&mut self.context.func, &mut self.function_context), | ||
| ptr_ty, | ||
| }) | ||
| .compile(func, dfa); | ||
|
|
||
| now!(t0); | ||
| self.module.define_function(func, &mut self.context).unwrap(); | ||
| now!(t1); | ||
| // println!("func: {}", ctx.func.display()); | ||
| self.module.clear_context(&mut self.context); | ||
|
|
||
| self.module.finalize_definitions().unwrap(); | ||
| now!(t2); | ||
|
|
||
| let code: *const u8 = self.module.get_finalized_function(func); | ||
| assert!(!code.is_null()); | ||
| now!(t3); | ||
|
|
||
| debug!( | ||
| "jitted (define function, clear context and finalize definitions, get finalized): {:?}", | ||
| [t1.duration_since(t0), t2.duration_since(t1), t3.duration_since(t2),] | ||
| ); | ||
|
|
||
| let func: JittedDfa = unsafe { std::mem::transmute::<*const u8, JittedDfa>(code) }; | ||
|
|
||
| Ok(func) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Tie the jitted function pointer to the Jit lifetime.
get_finalized_function returns code owned by the JITModule; returning a bare extern "C" fn lets callers invoke it after Jit is dropped. Return a lifetime-bound wrapper instead of a raw function pointer.
🧰 Tools
🪛 Clippy (1.96.0)
[error] 109-109: cannot find macro now in this scope
(error)
[error] 111-111: cannot find macro now in this scope
(error)
[error] 116-116: cannot find macro now in this scope
(error)
[error] 120-120: cannot find macro now in this scope
(error)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/dfa/jit.rs` around lines 88 - 129, The returned JIT entrypoint in
jit() is currently a bare JittedDfa produced from get_finalized_function, which
can outlive the Jit-owned JITModule. Update Jit::jit and the JittedDfa return
type so the function pointer is wrapped in a lifetime-bound type tied to Jit,
rather than exposing an unconstrained extern "C" fn. Use the existing symbols
jit, JittedDfa, and get_finalized_function to locate the return path and ensure
callers cannot use the compiled code after Jit is dropped.
| now!(t0); | ||
| self.module.define_function(func, &mut self.context).unwrap(); | ||
| now!(t1); | ||
| // println!("func: {}", ctx.func.display()); | ||
| self.module.clear_context(&mut self.context); | ||
|
|
||
| self.module.finalize_definitions().unwrap(); | ||
| now!(t2); | ||
|
|
||
| let code: *const u8 = self.module.get_finalized_function(func); | ||
| assert!(!code.is_null()); | ||
| now!(t3); | ||
|
|
||
| debug!( | ||
| "jitted (define function, clear context and finalize definitions, get finalized): {:?}", | ||
| [t1.duration_since(t0), t2.duration_since(t1), t3.duration_since(t2),] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Replace the undefined now! macro.
Clippy reports now! is not in scope, so this file will not compile. Use Instant::now() directly or define/import the macro.
🐛 Proposed fix
- now!(t0);
+ let t0 = std::time::Instant::now();
self.module.define_function(func, &mut self.context).unwrap();
- now!(t1);
+ let t1 = std::time::Instant::now();
...
- now!(t0);
+ let t0 = std::time::Instant::now();
...
- now!(t1);
+ let t1 = std::time::Instant::now();Also applies to: 135-135, 244-252
🧰 Tools
🪛 Clippy (1.96.0)
[error] 109-109: cannot find macro now in this scope
(error)
[error] 111-111: cannot find macro now in this scope
(error)
[error] 116-116: cannot find macro now in this scope
(error)
[error] 120-120: cannot find macro now in this scope
(error)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/dfa/jit.rs` around lines 109 - 124, The JIT timing code in the
dfa/jit module uses an undefined now! macro, which prevents compilation. Replace
those timing calls with a valid source of timestamps such as Instant::now(), or
add the missing macro/import consistently where the timing is recorded in the
JIT path (including the repeated timing blocks referenced by the same symbols).
Keep the change localized around the define_function, clear_context,
finalize_definitions, and get_finalized_function timing/debug flow so the
existing measurement logic still works.
Source: Linters/SAST tools
| let ch_b: Value = self.asm.ins().band_imm(ch_b, 0b0011_1111); | ||
|
|
||
| let ch_a: Value = self.asm.ins().band_imm(ch_a, 0b0001_1111); | ||
| let ch_a: Value = self.asm.ins().ishl_imm(ch_a, 6); | ||
|
|
||
| let ch: Value = self.asm.ins().bor(ch_a, ch_b); | ||
|
|
||
| // As per the table above, for a non-ascii code point, | ||
| // the 5th and 6th bits identify whether it is a 2/3/4-byte encoded value. | ||
| let bits_6_5: Value = self.asm.ins().ushr_imm(ch_a, 4); | ||
| let bits_6_5: Value = self.asm.ins().band_imm(bits_6_5, 0b0000_0011); | ||
|
|
||
| let is_2_bytes: Value = self.asm.ins().icmp_imm(IntCC::UnsignedLessThan, bits_6_5, 2); | ||
|
|
||
| self.asm.ins().brif( | ||
| is_2_bytes, | ||
| success_b, | ||
| &[BlockArg::Value(next_input_ptr_2), BlockArg::Value(ch)], | ||
| multi_byte_3_b, | ||
| &[], | ||
| ); | ||
| self.asm.seal_block(multi_byte_3_b); | ||
|
|
||
| { | ||
| self.asm.switch_to_block(multi_byte_3_b); | ||
|
|
||
| let (next_input_ptr_3, ch_c): (Value, Value) = | ||
| self.read_byte::<WITH_BOUNDS_CHECK, 2>(input_ptr, n_bytes_remaining, exit_b, last_match); | ||
|
|
||
| let ch_c: Value = self.asm.ins().band_imm(ch_c, 0b0011_1111); | ||
|
|
||
| let ch_b: Value = self.asm.ins().ishl_imm(ch_b, 6); | ||
|
|
||
| let ch_a: Value = self.asm.ins().band_imm(ch_a, 0b0000_1111); | ||
| let ch_a: Value = self.asm.ins().ishl_imm(ch_a, 12); | ||
|
|
||
| let ch: Value = self.asm.ins().bor(ch_a, ch_b); | ||
| let ch: Value = self.asm.ins().bor(ch, ch_c); | ||
|
|
||
| let is_3_bytes: Value = self.asm.ins().icmp_imm(IntCC::UnsignedLessThan, bits_6_5, 3); | ||
|
|
||
| self.asm.ins().brif( | ||
| is_3_bytes, | ||
| success_b, | ||
| &[BlockArg::Value(next_input_ptr_3), BlockArg::Value(ch)], | ||
| multi_byte_4_b, | ||
| &[], | ||
| ); | ||
| self.asm.seal_block(multi_byte_4_b); | ||
|
|
||
| { | ||
| self.asm.switch_to_block(multi_byte_4_b); | ||
|
|
||
| let (next_input_ptr_4, ch_d): (Value, Value) = | ||
| self.read_byte::<WITH_BOUNDS_CHECK, 3>(input_ptr, n_bytes_remaining, exit_b, last_match); | ||
|
|
||
| let ch_d: Value = self.asm.ins().band_imm(ch_d, 0b0011_1111); | ||
|
|
||
| let ch_c: Value = self.asm.ins().ishl_imm(ch_c, 6); | ||
| let ch_b: Value = self.asm.ins().ishl_imm(ch_b, 12); | ||
|
|
||
| let ch_a: Value = self.asm.ins().band_imm(ch_a, 0b0000_0111); | ||
| let ch_a: Value = self.asm.ins().ishl_imm(ch_a, 18); | ||
|
|
||
| let ch: Value = self.asm.ins().bor(ch_a, ch_b); | ||
| let ch: Value = self.asm.ins().bor(ch, ch_c); | ||
| let ch: Value = self.asm.ins().bor(ch, ch_d); | ||
|
|
||
| self.asm | ||
| .ins() | ||
| .jump(success_b, &[BlockArg::Value(next_input_ptr_4), BlockArg::Value(ch)]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Decode multibyte UTF-8 from the original lead byte.
The code masks/shifts ch_a before using it to decide 2/3/4-byte length and later masks that shifted value again, so 3- and 4-byte characters are decoded as 2-byte sequences or assembled incorrectly. Preserve the original lead byte for length checks and payload extraction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/dfa/jit.rs` around lines 361 - 431, The multibyte UTF-8 decode path
in the JIT logic is reusing a shifted/masked `ch_a` for both length detection
and payload assembly, which causes 3- and 4-byte sequences to be decoded
incorrectly. Update the multibyte handling in this `jit.rs` block to preserve
the original lead byte before shifting, use that original value when computing
`bits_6_5` and `is_2_bytes`/`is_3_bytes`, and only apply the per-length
masks/shifts to separate temporary values when building `ch` for the 2-, 3-, and
4-byte cases.
| pub fn enable_tracing() { | ||
| use tracing_subscriber::filter::EnvFilter; | ||
| use tracing_subscriber::fmt::format::FmtSpan; | ||
|
|
||
| // Note: [`tracing_subscriber::fmt::SubscriberBuilder::without_time`] | ||
| // disables _both_ timestamps per log message _and_ timing events/showing their duration. | ||
| // Call `.with_timer()` with an empty formatter to _just_ disable timestamps in each log printed. | ||
| tracing_subscriber::fmt() | ||
| .with_timer(()) | ||
| .with_target(false) | ||
| .with_span_events(FmtSpan::CLOSE) | ||
| .with_file(true) | ||
| .with_line_number(true) | ||
| .with_env_filter(EnvFilter::from_env("LOG_SURGEON_LOG")) | ||
| .init() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and search for uses of enable_tracing.
git ls-files 'rust/src/lib.rs' 'rust/**' | sed -n '1,120p'
printf '\n--- rust/src/lib.rs (selected range) ---\n'
cat -n rust/src/lib.rs | sed -n '1,120p'
printf '\n--- enable_tracing usages ---\n'
rg -n "enable_tracing\\s*\\(" rust -g '!target' -g '!**/target/**'
printf '\n--- tracing init patterns ---\n'
rg -n "\\.init\\(\\)|\\.try_init\\(\\)|set_global_default\\(" rust -g '!target' -g '!**/target/**'Repository: y-scope/log-surgeon
Length of output: 4296
Make tracing setup idempotent.
enable_tracing() is public, so calling it after another subscriber is already installed will panic at .init(). Use .try_init() and ignore the already-initialised error instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/lib.rs` around lines 41 - 55, Make enable_tracing() idempotent by
replacing the direct tracing_subscriber::fmt() .init() call with a non-panicking
initialization path. In the enable_tracing function, use .try_init() and
explicitly ignore the already-initialized error so repeated calls or
pre-existing subscribers do not panic, while keeping the existing EnvFilter and
fmt subscriber configuration unchanged.
| Self::Placeholder { name, item } => { | ||
| let Some(placeholder): Option<Regex> = get_placeholder.lookup(name) else { | ||
| return Err(RegexErrorKind::UndefinedPlaceholder(name.clone())); | ||
| }; | ||
| **item = placeholder; | ||
| Ok(()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve placeholders inside substituted placeholder regexes.
After assigning placeholder into item, the newly inserted regex is not traversed. A placeholder that expands to another placeholder remains unresolved and can silently carry Regex::NIL into later construction.
🐛 Proposed fix
let Some(placeholder): Option<Regex> = get_placeholder.lookup(name) else {
return Err(RegexErrorKind::UndefinedPlaceholder(name.clone()));
};
**item = placeholder;
- Ok(())
+ item.replace_with_placeholders(get_placeholder)📝 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.
| Self::Placeholder { name, item } => { | |
| let Some(placeholder): Option<Regex> = get_placeholder.lookup(name) else { | |
| return Err(RegexErrorKind::UndefinedPlaceholder(name.clone())); | |
| }; | |
| **item = placeholder; | |
| Ok(()) | |
| Self::Placeholder { name, item } => { | |
| let Some(placeholder): Option<Regex> = get_placeholder.lookup(name) else { | |
| return Err(RegexErrorKind::UndefinedPlaceholder(name.clone())); | |
| }; | |
| **item = placeholder; | |
| item.replace_with_placeholders(get_placeholder) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/regex/pattern_parsing.rs` around lines 232 - 237, The placeholder
handling in the regex substitution path leaves nested placeholders unresolved
after assigning into the Placeholder branch. Update the logic in the match arm
for Self::Placeholder in pattern_parsing.rs so that, after replacing item with
the looked-up Regex, the newly inserted regex is traversed or re-parsed for
further placeholder expansion instead of stopping at Ok(()). Use the
Self::Placeholder branch and the placeholder lookup/assignment flow to locate
the fix, and ensure chained placeholders do not remain as Regex::NIL.
| sub_rule.qualified_name = Arc::from(format!( | ||
| "{}.{}", | ||
| maybe_parent.map_or("", |(_, name)| name), | ||
| sub_rule.name | ||
| )); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Avoid leading dots in top-level qualified capture names.
For a top-level capture, maybe_parent.map_or("", ...) still flows into "{}.{}", producing names like .field. That leaks incorrect capture identifiers to downstream consumers.
🐛 Proposed fix
- sub_rule.qualified_name = Arc::from(format!(
- "{}.{}",
- maybe_parent.map_or("", |(_, name)| name),
- sub_rule.name
- ));
+ sub_rule.qualified_name = match maybe_parent {
+ Some((_, parent_name)) => Arc::from(format!("{parent_name}.{}", sub_rule.name)),
+ None => Arc::from(sub_rule.name.as_str()),
+ };📝 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.
| sub_rule.qualified_name = Arc::from(format!( | |
| "{}.{}", | |
| maybe_parent.map_or("", |(_, name)| name), | |
| sub_rule.name | |
| )); | |
| sub_rule.qualified_name = match maybe_parent { | |
| Some((_, parent_name)) => Arc::from(format!("{parent_name}.{}", sub_rule.name)), | |
| None => Arc::from(sub_rule.name.as_str()), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/regex/pattern_parsing.rs` around lines 265 - 269, The qualified
capture name construction in `pattern_parsing` currently always formats with
`"{}.{}"`, so top-level captures end up with a leading dot from
`maybe_parent.map_or("", ...)`. Update the logic where `sub_rule.qualified_name`
is assigned to special-case the top-level case and build the name from just
`sub_rule.name` when there is no parent, while keeping the existing
parent-prefixed form for nested captures.
| /// Crate-defined [`TryInto`] to get around Rust's forsaken orphan rules. | ||
| pub trait LocalTryInto<T> { | ||
| type Error; | ||
|
|
||
| fn try_into(self) -> Result<T, Self::Error>; | ||
| } | ||
|
|
||
| impl<T, U> LocalTryInto<U> for T | ||
| where | ||
| U: TryFrom<T>, | ||
| { | ||
| type Error = U::Error; | ||
|
|
||
| fn try_into(self) -> Result<U, Self::Error> { | ||
| U::try_from(self) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the trait definition and nearby context.
if [ -f rust/src/utils/convert.rs ]; then
echo "== rust/src/utils/convert.rs =="
cat -n rust/src/utils/convert.rs
else
echo "Missing rust/src/utils/convert.rs"
fi
# Find all references to LocalTryInto / try_into in the Rust sources.
echo
echo "== References =="
rg -n --hidden --glob '!target' 'LocalTryInto|local_try_into|try_into\(' rust/ || true
# Show the relevant module tree and trait imports if present.
echo
echo "== utils module files =="
fd -a -t f '^mod\.rs$|convert\.rs$' rust/src || trueRepository: y-scope/log-surgeon
Length of output: 1233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the only known consumer of LocalTryInto.
cat -n rust/src/regex.rs | sed -n '1,120p'
# Check whether the crate ever calls `.try_into()` unqualified in these sources.
echo
echo "== unqualified try_into call sites =="
rg -n --hidden --glob '!target' '\.try_into\(' rust/src || trueRepository: y-scope/log-surgeon
Length of output: 4568
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find Rust package manifests and edition settings.
fd -a -t f 'Cargo.toml$' rust . || true
echo
echo "== manifest excerpts =="
for f in $(fd -a -t f 'Cargo.toml$' rust . 2>/dev/null); do
echo "-- $f --"
rg -n '^(edition|name|version)\s*=' "$f" || true
doneRepository: y-scope/log-surgeon
Length of output: 498
Use the standard TryFrom impl here. AnchoredRegex is local, so this crate-specific try_into shim is unnecessary and can collide with the prelude’s try_into() method. Implement TryFrom<&str> for AnchoredRegex directly instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/utils/convert.rs` around lines 1 - 16, The crate-specific
LocalTryInto shim is unnecessary here and can conflict with the standard prelude
try_into method. Remove the LocalTryInto trait and its blanket impl in
convert.rs, then implement the standard TryFrom<&str> for AnchoredRegex directly
so callers can use the normal conversion API without the custom wrapper.
| // `\t`, `\r`, `\n`, `\\`, non-printable ASCII, non-ASCII unicode characters. | ||
| // [`char::escape_default`] also escapes quotes, which aren't relevant to us, but it doesn't hurt. | ||
| ch.escape_default().fmt(fmt) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== file map ==\n'
git ls-files rust/src/utils/escaping.rs rust/src/utils -z | tr '\0' '\n'
printf '\n== outline ==\n'
ast-grep outline rust/src/utils/escaping.rs --view expanded || true
printf '\n== relevant symbols ==\n'
rg -n "escape_default|unescape|escape\\(|escape_default" rust/src/utils/escaping.rs rust/src/utils -g '*.rs' || true
printf '\n== targeted read ==\n'
sed -n '1,220p' rust/src/utils/escaping.rsRepository: y-scope/log-surgeon
Length of output: 5358
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' rust/src/utils/escaping.rs
rg -n "escape_default|unescape|\\u\\{" rust/src/utils -g '*.rs'Repository: y-scope/log-surgeon
Length of output: 4048
Keep escaped output parseable by unescape() — char::escape_default() emits Rust-style \u{...} escapes with a non-padded hex length, but unescape() only accepts hex digits in pairs. That breaks round-tripping for some Unicode chars; either emit pair-padded \u{..} escapes here or widen unescape() to accept standard Rust unicode escapes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/utils/escaping.rs` around lines 109 - 111, The escaping logic in
`escape_char`/`escape_default` produces Rust-style Unicode escapes that
`unescape()` cannot parse reliably, so update one side to keep round-tripping
valid. Either change the formatter path that emits escaped chars to produce the
pair-padded `\u{..}` form expected by `unescape()`, or extend `unescape()` to
accept standard Rust Unicode escape syntax for the emitted values. Ensure the
fix is applied consistently across the `escaping.rs` helpers so escaped output
remains parseable by `unescape()`.
| let mut cut_inside: _ = cut(inside); | ||
| let mut cut_close: _ = cut(NomUtils::parse_char::<CLOSE, E>.or(expected_close)); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Remove the explicit type-underscore annotations.
Clippy flags both let mut ...: _ declarations; inference works without the annotations.
♻️ Proposed cleanup
- let mut cut_inside: _ = cut(inside);
- let mut cut_close: _ = cut(NomUtils::parse_char::<CLOSE, E>.or(expected_close));
+ let mut cut_inside = cut(inside);
+ let mut cut_close = cut(NomUtils::parse_char::<CLOSE, E>.or(expected_close));📝 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.
| let mut cut_inside: _ = cut(inside); | |
| let mut cut_close: _ = cut(NomUtils::parse_char::<CLOSE, E>.or(expected_close)); | |
| let mut cut_inside = cut(inside); | |
| let mut cut_close = cut(NomUtils::parse_char::<CLOSE, E>.or(expected_close)); |
🧰 Tools
🪛 Clippy (1.96.0)
[warning] 56-56: variable declared with type underscore
(warning)
[warning] 57-57: variable declared with type underscore
(warning)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/src/utils/nom.rs` around lines 56 - 57, Remove the explicit
type-underscore annotations in the parser setup inside
NomUtils::parse_until_close; both cut_inside and cut_close can rely on type
inference, so update those let bindings to drop the : _ annotations while
keeping the existing cut(...) expressions unchanged.
Source: Linters/SAST tools
1 participant
Description
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Documentation
Chores