`Recursive` minor improvements

[wkordalski]

First of all, I would like to thank the authors of this crate.

I'm using it to parse programs written in my own programming language.
My parser utilizes Recursive::declare/define and the caching pattern (but with thread-locally stored parsers as Recursive is not Send nor Sync).

1. The first thing is missing documentation about cloning behavior of Recursive. Especially that one can safely clone the result of Recursive::declare and then call Recursive::define on one of the clones (because Rc<_> is used internally). I think this is an important information for the crate users. Or am I wrong about this?

2. The second thing is that Recursive::define takes &mut self. It is not necessary and counter-intuitive as the common case is "declare, then use it, and finally define". In my parser I ended up with code like:

   fn define_expr(parser: &Recursive<...>, cache: &ParserCache) {
       let mut parser = parser.clone();
       parser.define(...);
   }

I can write appropriate PR if the ideas get accepted.

[zesterer]

An improvement to the docs seems sensible. I'd be happy to accept a PR: otherwise, I'll get to it as and when.

&mut self is not strictly required today, it is true, but it's designed to facilitate more complex future APIs that might need it. It's also communicating something: the recursive parser should not be shared around and defined in multiple places. Is there any reason you can't pass &mut Recursive in?

[wkordalski]

Is there any reason you can't pass &mut Recursive in?

No. Because I always can turn &Recursive into &mut Recursive.

fn define_expr(expr_parser: &Recursive<...>) {
    let mut expr_parser = expr_parser.clone();
    expr_parser.define(...);
}

It only looks like "something wrong here".

the recursive parser should not be shared around and defined in multiple places

Isn't cloning the recursive parser a good example of sharing the recursive parser around?
All clones of a recursive parser points to the same shared internal state of the recursive parser.

And cloning (i.e. sharing) a recursive parser is necessary to both:

• use the recursive parser,
• and define the parser.

Current API does not prevent things like:

fn foo(parser: &mut Recursive) {
    parser.define(todo());
}

let mut parser = Recursive::declare();
// use the parser here...
foo(&mut parser);
foo(&mut parser);

An API that disallows such usage might look like this one:

struct Recursive<P> { ... }

// Recursive does not implement `Clone`

impl<P> Recursive<P> {
    fn declare() -> (Self, SharedRecursive<P>) { ... }
    fn define<T>(self, parser: T) { ... } // moving `Recursive` into `define`
}

// SharedRecursive parses using the parser passed to relevant `Recursive::define(...)`, can be cloned but don't allow to call `define(...)`
struct SharedRecursive<P> { ... }

impl<P> Clone for SharedRecursive<P> { ... }
impl<...> Parser<...> for SharedRecursive<...> { ... }

This way checking for at-most-one call of define is done in type system. But is it worth to add more complexity into type system?

Note also, that removing Clone impl from Recursive is as much breaking change as changing &self to &mut self is.

it's designed to facilitate more complex future APIs that might need it

What guarantees can be provided by &mut self if one can create infinitely many mutable references to the same shared state?

Summary

As I said at the beginning: it's not a blocker for me.

I just feel that current API design (define takes &mut self, Recursive is Clone) does not provide any more safety and is not more ergonomic than "define takes &self, Recursive is Clone".

Decision is yours. I just wanted to share my feelings about it.

[zesterer]

Yes, the API does not currently restrict this. There have been several proposals (like #664) that attempt to make this change. The API I'd like to introduce is #902, but I've not had the time yet to push it to a mergeable state due to personal commitments.