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

Rewrite the whole combinators module to use HRTB only where necessary. #1

Merged
merged 1 commit into from
Jan 13, 2016

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Jan 13, 2016

Most of the changes are straight-forward, except for the Fn<(T,)> workaround for rust-lang/rust#30867, which requires the two feature-gates.

let RPAREN = string(")").map(|_| RParen);
let WHITESPACE = character(char::is_whitespace).map(|_| Whitespace);
let IDENTIFIER = character(char::is_alphabetic).and_then(character(char::is_alphanumeric).star())
.buffer().map(mk_identifier);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I use Identifier here, I get an ICE. I will try to reduce and submit a Rust issue.

@asajeffrey
Copy link
Owner

OK, this is really cool. The crucial trick seems to be not mentioning the target type in Parser<S> or ParseTo<S,D>, and then only constraining D to be a Consumer<T> when necessary.

I added a test to make sure it works with different lifetimes:

#[test]
fn test_different_lifetimes() {
    fn go<'a,'b>(ab: &'a str, cd: &'b str) {
        fn tail(x:&str) -> &str { &x[1..] }
        let mut parser = string("abc").buffer().map(tail);
        let mut result = String::new();
        assert_eq!(parser.push_to(ab, &mut result), Undecided);
        assert_eq!(result, "");
        assert_eq!(parser.push_to(cd, &mut result), Matched(Some("d")));
        assert_eq!(result, "bc");
    }
    go("ab","cd");        
}

Interestingly, it passes with a named function, but not with an anonymous function, which is a bit annoying, and may be related to the issue with the lexer you were mentioning above.

Anyway, thanks for all the work, the parser combinators are a lot slicker now!

asajeffrey pushed a commit that referenced this pull request Jan 13, 2016
Rewrite the whole combinators module to use HRTB only where necessary.
@asajeffrey asajeffrey merged commit 7465446 into asajeffrey:master Jan 13, 2016
asajeffrey pushed a commit that referenced this pull request Jan 13, 2016
@asajeffrey
Copy link
Owner

Hmm, I realized that there's a problem with ParserConsumer.

#[test]
fn test_parser_consumer() {
    fn mk_parser<C,D>(mut consumer: C) 
    where C: for<'a> ParserConsumer<&'a str,D>, D: for<'a> Consumer<&'a str> {
        fn tail(x:&str) -> &str { &x[1..] }
        consumer.accept(string("abc").buffer().map(tail));
    }
    struct TestConsumer<'a>(&'a str);
    impl<'a,'b> ParserConsumer<&'b str, String> for TestConsumer<'a> {
        fn accept<P>(&mut self, mut parser: P) where P: ParseTo<&'b str,String> {
            let mut result = String::new();
            assert_eq!(parser.push_to(self.0, &mut result), Matched(Some("d")));
            assert_eq!(result, "bc");
        }
    }
    mk_parser::<TestConsumer,String>(TestConsumer("abcd"));
}

The problem is that the parser producer is getting to pick the lifetime 'a. Really the consumer should be picking that lifetime, not the producer. I think parser producers are going to have to be specialized, so no generic ParserConsumer trait. Parser producers/consumers are only used at top-level so I don't think that's too horrible.

@eddyb
Copy link
Contributor Author

eddyb commented Jan 13, 2016

The trait is fine, it's just how you impl it and how you abstract over it:

#[test]
fn test_parser_consumer() {
    fn mk_parser<'a,C,D>(mut consumer: C)
    where C: ParserConsumer<&'a str,D>, D: for<'b> Consumer<&'b str> {
        fn tail(x:&str) -> &str { &x[1..] }
        consumer.accept(string("abc").buffer().map(tail));
    }
    struct TestConsumer<'a>(&'a str);
    impl<'a> ParserConsumer<&'a str, String> for TestConsumer<'a> {
        fn accept<P>(&mut self, mut parser: P) where P: ParseTo<&'a str,String> {
            let mut result = String::new();
            assert_eq!(parser.push_to(self.0, &mut result), Matched(Some("d")));
            assert_eq!(result, "bc");
        }
    }
    mk_parser::<TestConsumer,String>(TestConsumer("abcd"));
}

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

Successfully merging this pull request may close these issues.

None yet

2 participants