Unary prefix/suffix operator support for PrecClimber #344

segeljakt · 2018-11-23T21:16:11Z

Adds a new PrattParser, which works similar to PrecClimber but extends it with unary (prefix and suffix) operator handling.

To try it out, checkout this PR and run cargo run --example=calc in the terminal.

Given a grammar:

WHITESPACE   =  _{ " " | "\t" | NEWLINE }

program      =   { SOI ~ expr ~ EOI }
  expr       =   { prefix* ~ primary ~ postfix* ~ (infix ~ prefix* ~ primary ~ postfix* )* }
    infix    =  _{ add | sub | mul | div | pow }
      add    =   { "+" } // Addition
      sub    =   { "-" } // Subtraction
      mul    =   { "*" } // Multiplication
      div    =   { "/" } // Division
      pow    =   { "^" } // Exponentiation
    prefix   =  _{ neg }
      neg    =   { "-" } // Negation
    postfix  =  _{ fac }
      fac    =   { "!" } // Factorial
    primary  =  _{ int | "(" ~ expr ~ ")" }
      int    =  @{ (ASCII_NONZERO_DIGIT ~ ASCII_DIGIT+ | ASCII_DIGIT) }

You can now define a PrattParser:

let pratt = PrattParser::new()
      .op(Op::infix(Rule::add, Left) | Op::infix(Rule::sub, Left))
      .op(Op::infix(Rule::mul, Left) | Op::infix(Rule::div, Left))
      .op(Op::infix(Rule::pow, Right))
      .op(Op::postfix(Rule::fac))
      .op(Op::prefix(Rule::neg));

And use it like:

fn parse_to_i32(pairs: Pairs<Rule>, pratt: &PrattParser<Rule>) -> i32 {
  pratt
      .map_primary(|primary| match primary.as_rule() {
          Rule::int  => primary.as_str().parse().unwrap(),
          Rule::expr => parse_to_i32(primary.into_inner(), pratt),
          _          => unreachable!(),
      })
      .map_prefix(|op, rhs| match op.as_rule() {
          Rule::neg  => -rhs,
          _          => unreachable!(),
      })
      .map_postfix(|lhs, op| match op.as_rule() {
          Rule::fac  => (1..lhs+1).product(),
          _          => unreachable!(),
      })
      .map_infix(|lhs, op, rhs| match op.as_rule() {
          Rule::add  => lhs + rhs,
          Rule::sub  => lhs - rhs,
          Rule::mul  => lhs * rhs,
          Rule::div  => lhs / rhs,
          Rule::pow  => (1..rhs+1).map(|_| lhs).product(),
          _          => unreachable!(),
      })
      .parse(pairs)
}

Some things to consider:

Does not support ternary operators.
Precedence climbing like before uses recursion, though I'm not sure how deep you need to go for this to become a problem.
The current naming (prefix/suffix/infix) could be misinterpreted as binary operators with prefix/suffix/infix notation. Does it work, or should I name things more explicitly, e.g., unary_prefix/unary_suffix/binary_infix?

dragostis

Thanks a lot for your contribution! Really interesting work. I've taken a first look over the PR and there are a few issues that need to be resolved in order to get closer to merging this:

we need to find a way to support this in 2.1+; the way I see it, 3.0 is too far right now away in order lock into a particular implementation, so we need to find a way to deprecate the old types without breaking compatibility
prefix and postfix/suffix operators should potentially have their own precedence; this would require changing the algorithm
we should probably add another type in order not to restrain verification only in debug builds

dragostis · 2018-11-25T13:44:39Z

pest/src/prec_climber.rs

+        }
+    }
+
+    /// Defines `rule` as a suffix unary operator.


I'm more in favour of postfix rather than suffix.

dragostis · 2018-11-25T13:51:16Z

pest/src/prec_climber.rs

+/// Below is a [`PrecClimber`] that is able to climb an `expr` in the above grammar. The order
+/// of precedence corresponds to the order in which [`op`] is called. Thus, `mul` will
+/// have higher precedence than `add`. Operators can also be chained with `|` to give them equal
+/// precedence. In the current version, prefix operators precede suffix, and suffix


The issue here is that users might find confusing how, even though infix, prefix and postfix operators are mixed together, order matters only when Ops are infix. In other words, there is a redundant ordering complication here that has potential to be confusing.

In my opinion, the best course of action is to either logically separate prefix/postfix ops, such that they cannot be mixed in with infix operators, or extend the algorithm to deal with prefix/postfix operator precedence as well.

The algorithm should now be able to deal with prefix/postfix operator precedence

dragostis · 2018-11-25T13:53:48Z

pest/src/prec_climber.rs

+///             _          => unreachable!(),
+///         })
+///         .map_suffix(|lhs, op| match op.as_rule() {
+///             Rule::fac  => (1..=lhs).product(),


Kind of a papercut, but I believe this syntax will probably raise the minimum required Rust version. Might be a good idea to leave these kind of refactorings for when we tackle the 2018 edition refactor.

I changed it to (1..lhs+1)

dragostis · 2018-11-25T13:58:54Z

pest/src/prec_climber.rs

+///
+/// [`map_primary`]: struct.PrecClimber.html#method.map_primary
+/// [`PrecClimber`]: struct.PrecClimber.html 
+pub struct PrecClimberMap<'climber, 'pest, R, F, T>
    where


This doesn't look like the style specified in rustfmt.toml, but I might be mistaken. Is this formatted with rustfmt? Trailing commas style also seems to be different.

it should now be properly formatted

dragostis · 2018-11-25T14:05:19Z

pest/src/prec_climber.rs

+        pairs: P
+    ) -> T {
+
+        #[cfg(debug_assertions)]


Maybe it would be worthwhile to introduce a third type that this one would get compiled to. This would both offer a nice API, i.e. return a None if missing map_*s, and relive the user from possible headaches in release builds.

I changed so that the code panics either when:

The stream of tokens does not match the expected pattern.

No map function is specified for a certain kind of operator.

dragostis · 2018-11-25T14:09:27Z

pest/src/prec_climber.rs

+    }
+
+    /// Maps primary expressions with a closure `primary`.
+    pub fn map_primary<'climber, 'pest, X, T>(&'climber self, primary: X)


'i in pest stands for input. 'pest lifetime is too ambiguous.

Suggested change

pub fn map_primary<'climber, 'pest, X, T>(&'climber self, primary: X)

pub fn map_primary<'climber, 'i, X, T>(&'climber self, primary: X)

agausmann · 2019-05-03T08:29:57Z

pest/tests/calculator.rs

@@ -187,14 +190,15 @@ fn expression() {
 }

 #[test]
-fn prec_climb() {


I see tests for PrecClimber removed and replaced, but PrecClimber is still in the codebase. I don't think these should be removed until PrecClimber itself is.

agausmann · 2019-05-03T08:35:41Z

This would be a great feature to have! Personally I think the right way to incorporate this is to keep the public APIs involving PrecClimber at least until next major release, but supplement them with newer APIs that use PrattParser instead. There's nothing wrong with keeping two different algorithms around for a little while.

segeljakt · 2019-05-04T09:21:50Z

I recently added this to the pest-ast crate, you can use it like this with a grammar like this.

lberezy · 2020-02-25T01:32:02Z

I understand that this is a little bit old now and would probably requite a bit of fixing up, but is there any movement on this?

I think it would be quite handy to enable prefix/suffix operators like this.

dragostis · 2020-02-28T10:10:31Z

@lberezy, there hasn't been that much work here ever since, but I'm happy to merge this if it gets push forward.

segeljakt · 2020-04-06T14:25:48Z

Hi again, I refactored this into its own crate https://crates.io/crates/pratt. Feel free to copy it, add it as a dependency, or just point to it from to pest. You may close this PR if you want.

Closes pest-parser#461 based on pest-parser#344 Co-authored-by: Klas Segeljakt <klasseg@kth.se>

tomtau · 2022-09-20T14:25:48Z

@segeljakt @lberezy @dragostis I opened a new PR based on this one: #710 -- I had to make a few small changes in order to support no_std. I kept the original PrecClimber in order not to break the existing public API, but marked it with the #[deprecated] annotation.

Closes #461 based on #344 Co-authored-by: Tomas Tauber <me@tomtau.be> Co-authored-by: Klas Segeljakt <klasseg@kth.se>

segeljakt changed the title ~~Prec climber arity~~ Unary operator support for PrecClimber Nov 23, 2018

segeljakt changed the title ~~Unary operator support for PrecClimber~~ Unary prefix/suffix operator support for PrecClimber Nov 23, 2018

segeljakt force-pushed the prec-climber-arity branch from 04c6aa2 to ba220d2 Compare November 24, 2018 16:25

dragostis requested changes Nov 25, 2018

View reviewed changes

segeljakt force-pushed the prec-climber-arity branch 5 times, most recently from 725007f to fce43c4 Compare December 2, 2018 20:46

Update prec climber to support unary operators

960195d

segeljakt force-pushed the prec-climber-arity branch from fce43c4 to 960195d Compare December 3, 2018 19:20

agausmann reviewed May 3, 2019

View reviewed changes

CAD97 mentioned this pull request May 25, 2020

make PrecClimber support prefix and postfix operators #461

Closed

tomtau pushed a commit to tomtau/pest that referenced this pull request Sep 20, 2022

feature: unary prefix/suffix operator support via PrattParser

9b7f991

Closes pest-parser#461 based on pest-parser#344 Co-authored-by: Klas Segeljakt <klasseg@kth.se>

tomtau mentioned this pull request Sep 20, 2022

feature: unary prefix/suffix operator support via PrattParser #710

Merged

tomtau closed this Sep 20, 2022

tomtau added a commit that referenced this pull request Oct 1, 2022

feature: unary prefix/suffix operator support via PrattParser (#710)

9f7e4f7

Closes #461 based on #344 Co-authored-by: Tomas Tauber <me@tomtau.be> Co-authored-by: Klas Segeljakt <klasseg@kth.se>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unary prefix/suffix operator support for PrecClimber #344

Unary prefix/suffix operator support for PrecClimber #344

segeljakt commented Nov 23, 2018 •

edited

dragostis left a comment

dragostis Nov 25, 2018

segeljakt Dec 1, 2018

dragostis Nov 25, 2018

segeljakt Dec 1, 2018

dragostis Nov 25, 2018

segeljakt Dec 1, 2018

dragostis Nov 25, 2018

segeljakt Dec 1, 2018

dragostis Nov 25, 2018

segeljakt Dec 1, 2018

dragostis Nov 25, 2018

segeljakt Dec 1, 2018

agausmann May 3, 2019

agausmann commented May 3, 2019

segeljakt commented May 4, 2019

lberezy commented Feb 25, 2020

dragostis commented Feb 28, 2020

segeljakt commented Apr 6, 2020 •

edited

tomtau commented Sep 20, 2022

	pub fn map_primary<'climber, 'pest, X, T>(&'climber self, primary: X)
	pub fn map_primary<'climber, 'i, X, T>(&'climber self, primary: X)

Unary prefix/suffix operator support for PrecClimber #344

Unary prefix/suffix operator support for PrecClimber #344

Conversation

segeljakt commented Nov 23, 2018 • edited

dragostis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agausmann commented May 3, 2019

segeljakt commented May 4, 2019

lberezy commented Feb 25, 2020

dragostis commented Feb 28, 2020

segeljakt commented Apr 6, 2020 • edited

tomtau commented Sep 20, 2022

segeljakt commented Nov 23, 2018 •

edited

segeljakt commented Apr 6, 2020 •

edited