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

As is, new consumer design will probably break soon #105

Closed
soro opened this issue Oct 15, 2015 · 3 comments
Closed

As is, new consumer design will probably break soon #105

soro opened this issue Oct 15, 2015 · 3 comments
Milestone

Comments

@soro
Copy link
Contributor

soro commented Oct 15, 2015

If you try to compile the 1.0 branch with nightly you will get several of these warnings:

src/stream.rs:44:50: 44:57 note: this warning results from recent bug fixes and clarifications; it will become a HARD ERROR in the next release. See RFC 1214 for details.
src/stream.rs:44 if let &ConsumerState::Done(, ref o) = self.state() {
^~~~~~~
src/stream.rs:44:50: 44:57 note: ...so that the reference type &stream::ConsumerState<O, E, M> does not outlive the data it points at
src/stream.rs:44 if let &ConsumerState::Done(
, ref o) = self.state() {

I think this a legitimate error since without some explicit lifetime rust can't possibly generically tell that the state function below really does return a reference to something that lives as long as whatever is implementing Consumer. This would require it know something about the fact that it will be tied to a struct that contains a state field.

pub trait Consumer<I,O,E,M> {
...
  fn state(&self) -> &ConsumerState<O,E,M>;
...
}

You will also get a slew of these:

src/stream.rs:44:50: 44:57 warning: the parameter type M may not live long enough [E0309]
src/stream.rs:44 if let &ConsumerState::Done(_, ref o) = self.state() {

I've tried to fix this in various ways but unfortunately I just kept running into further problems and possibly outright bugs with the borrow checker and have decided to give up. I can get the Consumer trait to compile but I run into trouble with some of the producers and weird lifetime based signature mismatches. It also did require a very unpleasant amount of extra lifetime annotation.

Personally I'm starting to feel that, given all the breaking changes around borrowing, the safety might not be worth it and maybe it would be easier to make sure the consumers are constructed correctly via macros. All the more so because in principle handle is the only thing that needs to be implemented if the construction is passed a particular state struct.

@soro
Copy link
Contributor Author

soro commented Oct 15, 2015

Given the current state of unstable and bugs like rust-lang/rust#28970 I think it is premature to try to fix this (mea culpa), but it also means that going through with the design might not be worth it if it turns out to be nigh unfixable in an upcoming release.

@Geal
Copy link
Collaborator

Geal commented Oct 15, 2015

I see how that can become an issue. Using macros is always an option, but I need to write the code explicitely once, before I try to generate it.

I'll fix some remaining issues and remove the warnings, then I'll update my toolchain to test this bug.

@Geal Geal added this to the 4.0 milestone Sep 6, 2017
@Geal
Copy link
Collaborator

Geal commented Oct 3, 2017

The stream feature will be removed in the next nom version (4.0), since commit 564a934. If someone wants to extract the code and maintain it, feel free to do it, it's in src/stream.rs. Otherwise, I would recommend that you check out the (currently in nightly) generators feature, which is much nicer to use. Here's an example of using nom with generators.

@Geal Geal closed this as completed Oct 3, 2017
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

2 participants