Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

should function combinators return a FnMut? #898

Closed
Geal opened this issue Apr 6, 2019 · 4 comments
Closed

should function combinators return a FnMut? #898

Geal opened this issue Apr 6, 2019 · 4 comments
Milestone

Comments

@Geal
Copy link
Collaborator

Geal commented Apr 6, 2019

some combinators return a impl FnMut(I) -> IResult<I,O,E> because a test was failing in tests/reborrow_fold.rs, but I'm not sure allowing mutability there is such a good idea. If people want to mutate data from inside a parser, using a Cell might be a better solution.

@Geal Geal added this to the 5.0 milestone Apr 6, 2019
@Geal Geal mentioned this issue Apr 6, 2019
22 tasks
@loewenheim
Copy link

If I’m not mistaken, Fn can’t work in all cases. For instance, consider fold_many0. If it returned an Fn, we could run it as many times as we wanted and thereby multiply the init argument, which can’t be right.

But maybe I’m making a mistake somewhere, I’m no expert on Rust’s type system.

@Geal
Copy link
Collaborator Author

Geal commented Apr 13, 2019

for fold_many0, we would need to make init Clone. In general, we have to assume any parser can be called multiple times, so they should at least return Fn or FnMut.
I added FnMut in delimited: https://github.com/Geal/nom/blob/master/src/sequence/mod.rs#L93-L94
to make this test compile: https://github.com/Geal/nom/blob/master/tests/reborrow_fold.rs#L12-L16
But now I'm not sure this is something that should be supported

@Geal
Copy link
Collaborator Author

Geal commented Apr 27, 2019

all nom parsers are now Fn instead of FnOnce and FnMut. We should be able to call them multiple times, and they're not meant to modify their environment

@Geal Geal closed this as completed Apr 27, 2019
@z2oh
Copy link

z2oh commented Jul 15, 2019

I was upgrading a parser from 4.0 to 5.0 and ran into an issue with fold_many0. I used this function to accumulate parts of an arithmetic expression into an AST. I made a decision a while ago to make my AST nodes non-clonable (since I figured this should never need to happen and I didn't want to be able to circumvent the borrow checker). I was able to fix my issue by having a custom implementation of the fold_many0 function that returned impl FnOnce instead of impl Fn.

Is it worth polluting the multi namespace with fold_many*_once variants of the fold_many functions to avoid the Clone bound where it is not needed? I would be happy to make a PR for this if desired, otherwise I can stick to my custom implementations of the functions.

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

No branches or pull requests

3 participants