Skip to content

Pratt parser #798

Closed
lu-zero wants to merge 2 commits intowinnow-rs:mainfrom
lu-zero:pratt
Closed

Pratt parser #798
lu-zero wants to merge 2 commits intowinnow-rs:mainfrom
lu-zero:pratt

Conversation

@lu-zero
Copy link
Contributor

@lu-zero lu-zero commented Jul 6, 2025

Rebase of #614
Closes #131

I guess porting what we have in #620 could happen later or within this PR.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clippy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@coveralls
Copy link

coveralls commented Jul 6, 2025

Pull Request Test Coverage Report for Build 16239183961

Details

  • 61 of 97 (62.89%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.8%) to 47.428%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/combinator/expression.rs 61 97 62.89%
Totals Coverage Status
Change from base Build 16221869183: 0.8%
Covered Lines: 1669
Relevant Lines: 3519

💛 - Coveralls

@epage
Copy link
Collaborator

epage commented Jul 7, 2025

Could you clean up the commit history, per our CONTRIBUTING.md?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should include the example from #622

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably name that c_expr, rather than pratt. Trying to decide how it should show up in our docs, maybe a sub-page if Languages but have Arithmetic cross-link to it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should include the example from #622

I'm on vacation and haven't had much time to look at this code, but I started porting the example to the new API. If it helps: https://github.com/ssmendon/winnow/blob/devel-pratt/examples/pratt/parser.rs#L72-L163

All commits: https://github.com/ssmendon/winnow/commits/devel-pratt

Some notes:

@epage
Copy link
Collaborator

epage commented Jul 7, 2025

CC @39555

@lu-zero lu-zero force-pushed the pratt branch 3 times, most recently from d0629db to bb9a5d4 Compare July 12, 2025 14:33

/// Set the precedence level for the current expression
#[inline(always)]
pub fn current_precedence_level(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we could drop current_.

#[doc(alias = "shunting_yard")]
#[doc(alias = "precedence_climbing")]
#[inline(always)]
pub fn expression<I, ParseOperand, O, E>(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we pub use the main function in the parent module as the rest of the functions?

@codecov
Copy link

codecov bot commented Jul 12, 2025

Codecov Report

❌ Patch coverage is 62.88660% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.42%. Comparing base (18f20ee) to head (f878de8).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/combinator/expression.rs 62.88% 36 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #798      +/-   ##
==========================================
+ Coverage   46.72%   47.42%   +0.70%     
==========================================
  Files          26       27       +1     
  Lines        3416     3519     +103     
==========================================
+ Hits         1596     1669      +73     
- Misses       1820     1850      +30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines +289 to +296
impl<I: Stream, O, E: ParserError<I>> Parser<I, Postfix<I, O, E>, E>
for (i64, fn(&mut I, O) -> Result<O, E>)
{
#[inline(always)]
fn parse_next(&mut self, input: &mut I) -> Result<Postfix<I, O, E>, E> {
empty.value(Postfix(self.0, self.1)).parse_next(input)
}
}
Copy link
Contributor

@ssmendon ssmendon Jul 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for @39555.

I think you meant to implement this for Postfix instead of for the tuple? Like this:

impl<I: Stream, O, E: ParserError<I>> Parser<I, Postfix<I, O, E>, E> for Postfix<I, O, E> {
    #[inline(always)]
    fn parse_next(&mut self, input: &mut I) -> Result<Postfix<I, O, E>, E> {
        empty.value(self.clone()).parse_next(input)
    }
}

I noticed this while porting the example in #622 to my own fork: https://github.com/ssmendon/winnow/blob/pratt-parsing/examples/c_expression/parser.rs

Specifically, I noticed this syntax doesn't work due to unsatisfied trait bounds:

dispatch! {take(2usize);
    "++" => Postfix(20, |_: &mut _, a| Ok(Expr::PostIncr(Box::new(a)))),
    "--" => Postfix(20, |_: &mut _, a| Ok(Expr::PostDecr(Box::new(a)))),
    _ => fail,
},

Notes:

  • You can't implement the original trait outside of winnow due to trait orphan rules.
  • The example inside of crate::combinator::expression doesn't use any Postfix rules, but it is in the example (e.g. cargo test -F unstable-doc,std --all).

@ssmendon
Copy link
Contributor

I added my own PR in #804, which tries to address some of the review comments attached to this. I did not really look at the code in this PR, so I don't know in what ways our implementation is different (if at all).

@lu-zero
Copy link
Contributor Author

lu-zero commented Jul 27, 2025

Let me close this in favor of #804 then.

@lu-zero lu-zero closed this Jul 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pratt parsing support

5 participants

Comments