Skip to content

Initial winnow implementation - #871

Open
lu-zero wants to merge 5 commits into
reubeno:mainfrom
lu-zero:winnow
Open

Initial winnow implementation#871
lu-zero wants to merge 5 commits into
reubeno:mainfrom
lu-zero:winnow

Conversation

@lu-zero

@lu-zero lu-zero commented Dec 25, 2025

Copy link
Copy Markdown
Contributor

It has two implementations, one still using the tokenizer the other directly parsing a str input.

Most of the code had been machine-ported from the peg implementation and then refined so it is a bit faster than the original, there is still potentially more to refactor but we can start reasoning about it.

@github-actions

github-actions Bot commented Dec 25, 2025

Copy link
Copy Markdown

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
clone_shell_object 17.70 μs 17.82 μs 0.13 μs ⚪ Unchanged
eval_arithmetic 0.15 μs 0.15 μs 0.00 μs ⚪ Unchanged
expand_one_string 1.64 μs 1.66 μs 0.02 μs ⚪ Unchanged
for_loop 31.45 μs 32.12 μs 0.67 μs 🟠 +2.12%
full_peg_complex 57.80 μs 58.19 μs 0.38 μs ⚪ Unchanged
full_peg_for_loop 6.25 μs 6.32 μs 0.07 μs ⚪ Unchanged
full_peg_nested_expansions 16.02 μs 16.06 μs 0.04 μs ⚪ Unchanged
full_peg_pipeline 4.23 μs 4.27 μs 0.03 μs ⚪ Unchanged
full_peg_simple 1.81 μs 1.81 μs 0.00 μs ⚪ Unchanged
function_call 3.41 μs 3.43 μs 0.02 μs ⚪ Unchanged
instantiate_shell 54.95 μs 54.95 μs -0.00 μs ⚪ Unchanged
instantiate_shell_with_init_scripts 26445.53 μs 26570.51 μs 124.98 μs ⚪ Unchanged
parse_peg_bash_completion 2125.72 μs 2139.84 μs 14.12 μs ⚪ Unchanged
parse_peg_complex 20.94 μs 21.02 μs 0.09 μs ⚪ Unchanged
parse_peg_for_loop 2.08 μs 2.04 μs -0.04 μs 🟢 -1.73%
parse_peg_pipeline 2.09 μs 2.13 μs 0.04 μs 🟠 +2.11%
parse_peg_simple 1.08 μs 1.09 μs 0.01 μs 🟠 +1.11%
run_echo_builtin_command 16.31 μs 16.29 μs -0.02 μs ⚪ Unchanged
tokenize_sample_script 3.39 μs 3.46 μs 0.07 μs ⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
Overall Coverage 🟢 75.93% 🟢 75.93% ⚪ 0%

Minimum allowed coverage is 70%, this run produced 75.93%
Maximum allowed coverage difference is -5%, this run produced 0%

Test Summary: bash-completion test suite

Outcome Count Percentage
✅ Pass 1582 75.01
❗️ Error 17 0.81
❌ Fail 156 7.40
⏩ Skip 339 16.07
❎ Expected Fail 13 0.62
✔️ Unexpected Pass 2 0.09
📊 Total 2109 100.00

@lu-zero
lu-zero force-pushed the winnow branch 8 times, most recently from 95731cd to 39dbcaa Compare December 27, 2025 07:15
@reubeno

reubeno commented Dec 27, 2025

Copy link
Copy Markdown
Owner

Thanks for cleaning up the changes. I still need to actually read through the winnow-based parsers, but what do you think about arranging a runtime switch to select the parser we want to use? If you're open to it, I'm happy to help with some of the glue code for this too.

I'm thinking we could extract a Parser trait (or similar name) that must implement a fn that takes text as input and yields an AST. We could implement this for the PEG-based impl as well as the winnow-based impl(s). An option on Shell could select the desired impl, with default going to the (legacy) PEG one.

We could then arrange to run all of our YAML-based integration tests against all parsers without needing to recompile brush, and present results. This would give us a way to track functional parity and help with side-by-side comparison. (And ditto for the benchmarks you've set up.) I'd want to update the PR checks to run tests against multiple parsers, but only block if we fail tests on the PEG one. This would give us a clear way to track progress and evaluate when we'd be ready to switch over the default.

@lu-zero

lu-zero commented Dec 28, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for cleaning up the changes. I still need to actually read through the winnow-based parsers, but what do you think about arranging a runtime switch to select the parser we want to use? If you're open to it, I'm happy to help with some of the glue code for this too.

Sounds feasible and shouldn't be too hard to achieve given what we currently have. Just we could clean up a little the options.

I'm thinking we could extract a Parser trait (or similar name) that must implement a fn that takes text as input and yields an AST. We could implement this for the PEG-based impl as well as the winnow-based impl(s). An option on Shell could select the desired impl, with default going to the (legacy) PEG one.

Yes, I have the two winnow impls because I gradually did the translation and benchmarked and cleaned up till I was confident winnow can be faster and overall nicer than the status quo. If we can ditch the tokenization from the public API we should spare quite a bit of allocations and we can focus on cleaning up even more the winnow_str impl.

We could then arrange to run all of our YAML-based integration tests against all parsers without needing to recompile brush, and present results. This would give us a way to track functional parity and help with side-by-side comparison. (And ditto for the benchmarks you've set up.) I'd want to update the PR checks to run tests against multiple parsers, but only block if we fail tests on the PEG one. This would give us a clear way to track progress and evaluate when we'd be ready to switch over the default.

Yes, we might want to do a bit of refactor so that we can make the location a no-op for the tests since that part will have discrepancies.

@lu-zero
lu-zero marked this pull request as ready for review January 11, 2026 12:32
@reubeno reubeno added the area: parsing Issues or PRs related to tokenization or parsing label Jan 17, 2026
@lu-zero
lu-zero force-pushed the winnow branch 5 times, most recently from 57f48f3 to 1e54749 Compare January 21, 2026 20:54
@lu-zero

lu-zero commented Jan 23, 2026

Copy link
Copy Markdown
Contributor Author

@reubeno this weekend we could try to do a first review pass to make sure all the not too beautiful bits are needed for performance, but it seems that we are getting closer to an acceptable version.

lu-zero added 2 commits August 2, 2026 20:36
Unblocks the fmt and spell-check CI checks. The 6 here_docs test
failures (3 stale _peg negative assertions + 3 _winnow snapshot
mismatches from main's AST evolution) remain to be addressed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: parsing Issues or PRs related to tokenization or parsing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants