seq! for tuples errors when capturing locally-defined parsers.

[aDotInTheVoid]

Please complete the following tasks

• I have searched the discussions
• I have searched the open and rejected issues

rust version

rustc 1.77.0 (aedd173a2 2024-03-17)

winnow version

0.6.5

Minimal reproducible code

use winnow::combinator::seq;
use winnow::prelude::*;

pub fn doesnt_work(input: &mut &str) -> PResult<(i32, i32)> {
    let mut foo = "foo".map(|_| 10);
    let mut bar = "bar".map(|_| 20);
    seq!(foo, bar).parse_next(input)
}

Steps to reproduce the bug with the above code

cargo check

Actual Behaviour

error[E0525]: expected a closure that implements the `FnMut` trait, but this closure only implements `FnOnce`
  --> good-crates/parser/examples/mcve.rs:18:5
   |
18 |     seq!(foo, bar).parse_next(input)
   |     ^^^^^---^^^^^^
   |     |    |
   |     |    closure is `FnOnce` because it moves the variable `foo` out of its environment
   |     this closure implements `FnOnce`, not `FnMut`
   |     the requirement to implement `FnMut` derives from here
   |     required by a bound introduced by this call
   |
   = note: required for `{closure@/home/alona/.cargo/registry/src/index.crates.io-6f17d22bba15001f/winnow-0.6.5/src/macros/seq.rs:88:44: 88:64}` to implement `winnow::Parser<&str, (i32, i32), ContextError>`
note: required by a bound in `trace`
  --> /home/alona/.cargo/registry/src/index.crates.io-6f17d22bba15001f/winnow-0.6.5/src/combinator/debug/mod.rs:45:18
   |
43 | pub fn trace<I: Stream, O, E>(
   |        ----- required by a bound in this function
44 |     name: impl crate::lib::std::fmt::Display,
45 |     parser: impl Parser<I, O, E>,
   |                  ^^^^^^^^^^^^^^^ required by this bound in `trace`
   = note: this error originates in the macro `$crate::seq` which comes from the expansion of the macro `seq` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0525`.

Expected Behaviour

This should compile.

If seq! is used to make a struct (instead of a tuple) it does compile.

pub struct Res {
    pub a: i32,
    pub b: i32,
}

pub fn does_work(input: &mut &str) -> PResult<Res> {
    let mut foo = "foo".map(|_| 10);
    let mut bar = "bar".map(|_| 20);
    seq! { Res{ a: foo, b: bar } }.parse_next(input)
}

works fine.

Additional Context

Manually expanding the seq! macro (with RA) for the broken example gives:

pub fn doesnt_work(input: &mut &str) -> PResult<(i32, i32)> {
    let mut foo = "foo".map(|_| 10);
    let mut bar = "bar".map(|_| 20);
    {
        winnow::combinator::trace("tuple", move |input: &mut _| {
            use winnow::Parser;
            (foo, bar).map(|t| ((t.0), (t.1))).parse_next(input)
        })
    }
    .parse_next(input)
}

This gives a more descriptive error:

error[E0525]: expected a closure that implements the `FnMut` trait, but this closure only implements `FnOnce`
  --> good-crates/parser/examples/mcve.rs:19:44
   |
19 |           winnow::combinator::trace("tuple", move |input: &mut _| {
   |           -------------------------          -^^^^^^^^^^^^^^^^^^^
   |           |                                  |
   |  _________|__________________________________this closure implements `FnOnce`, not `FnMut`
   | |         |
   | |         required by a bound introduced by this call
20 | |             use winnow::Parser;
21 | |             (foo, bar).map(|t| ((t.0), (t.1))).parse_next(input)
   | |              --- closure is `FnOnce` because it moves the variable `foo` out of its environment
22 | |         })
   | |_________- the requirement to implement `FnMut` derives from here
   |
   = note: required for `{closure@good-crates/parser/examples/mcve.rs:19:44: 19:64}` to implement `winnow::Parser<&str, (i32, i32), ContextError>`
note: required by a bound in `trace`
  --> /home/alona/.cargo/registry/src/index.crates.io-6f17d22bba15001f/winnow-0.6.5/src/combinator/debug/mod.rs:45:18
   |
43 | pub fn trace<I: Stream, O, E>(
   |        ----- required by a bound in this function
44 |     name: impl crate::lib::std::fmt::Display,
45 |     parser: impl Parser<I, O, E>,
   |                  ^^^^^^^^^^^^^^^ required by this bound in `trace`

For more information about this error, try `rustc --explain E0525`.

Manually expanding the example that does work gives:

pub fn does_work(input: &mut &str) -> PResult<Res> {
    let mut foo = "foo".map(|_| 10);
    let mut bar = "bar".map(|_| 20);
    {
        winnow::combinator::trace("Res", move |input: &mut _| {
            use winnow::Parser;
            let a = foo.parse_next(input)?;
            let b = bar.parse_next(input)?;
            #[allow(clippy::redundant_field_names)]
            Ok(Res { a: a, b: b })
        })
    }
    .parse_next(input)
}

I think the reason this works is that foo and bar don't need to be captured by value here, as .parse_next takes &mut self.

Adding .by_ref() to the tuple example version may fix it.

[epage]

Thanks for reporting this!

I worked on a fix but it appears to be a breaking change. Before, we moved the parser and the caller could declare their parsers as immutable. With this change, they now need to make their parsers mutable.

I'm assuming this can be worked around on the callers side by adding .by_ref() on their side.

I'm thinking this is likely to not be enough to justify a breaking release right now but will need to wait until the motivation is strong enough to release one.

Side note: #502 only partially brings their behavior in alignment which is why I didn't mark this as a fix but a step. I suspect the way to fix this would be to stop using ().parse_next() completely and instead assign each parse result to a variable using the same pre-filled list of variable names that the macros for implementing Parser for tuples does. Hopefully this would let us reduce the scope of #237 affecting users of seq!.