Repository: Marwes/combine (language: rust)

Issue:
token and SliceStream do not play together well
First, thanks for the library.  It's a fun way to learn Rust!

I'd like to understand some of the API decisions the library makes.  First, my motivating use case is to parse a token stream (not a character stream), and my token's do not implement `Copy` because they include `String` data, like:
```
struct Token {
    Keyword(String),
    ...
}
```

If I use `SliceStream`, I can parse `&[Token]`.  However, I can't really use `token`, because the `Item` type of `SliceStream` is `&'a Token` and that's not something I can create with the correct lifetime.  It seems to me like `token` should have `where Item : Clone` and explicitly `.clone()` it's argument.  I see at https://github.com/Marwes/combine/blob/master/src/combinator.rs#L200 that we can't actually _parse_ without `Item : Clone`.

Is this deliberate?  Oversight?  Am I using `SliceStream` incorrectly?


Discussion / hints:
The reason for the `Clone` bound is that `combine` assumes that `Stream` is a type that can be cloned (which is necessary for the `or` parser which implements alternation). Since `Stream` can be cloned it follows that its tokens must be cloneable as well (any token that does not implement `Clone` could still be duplicated by cloning the stream `stream.uncons().clone()` vs `stream.clone().uncons()` so there is no reason that they would not be possible to clone). If we then also assume that streams and tokens are cheap to clone (if they were expensive to clone then parsing would be slow) then it follows that the predicates might as well take `Item` instead of `&Item` as it is much more convenient in common cases such as `Item = u8` or `Item = char`.

That was my reasoning in any case :).

As for the lifetime problem, that is not something I had considered actually. I can't think of anyway to make `token` available for that case as the `token` requires exactly the `Item` type since that is used to construct the error message if the parser fails https://github.com/Marwes/combine/blob/6a57d7f58117abb6ff62a0a6e169fcdbdf83b0f5/src/combinator.rs#L209. Using the the `satisfy` parser instead should work though.
Reasoning for `Copy` https://github.com/Marwes/combine/pull/44

Fix patch:
diff --git a/src/combinator.rs b/src/combinator.rs
index 797ad924..3d8fdaf3 100644
--- a/src/combinator.rs
+++ b/src/combinator.rs
@@ -2093,4 +2093,18 @@ mod tests {
                        errors: vec![Error::Unexpected('a'.into()), Error::Expected("digit".into())],
                    }));
     }
+
+    #[derive(Clone, PartialEq, Debug)]
+    struct CloneOnly {
+        s: String,
+    }
+
+    #[test]
+    fn token_clone_but_not_copy() {
+        // Verify we can use token() with a StreamSlice with an item type that is Clone but not
+        // Copy.
+        let input = &[CloneOnly { s: "x".to_string() }, CloneOnly { s: "y".to_string() }][..];
+        let result = token(CloneOnly { s: "x".to_string() }).parse(input);
+        assert_eq!(result, Ok((CloneOnly { s: "x".to_string() }, &[CloneOnly { s: "y".to_string() }][..])));
+    }
 }
diff --git a/src/primitives.rs b/src/primitives.rs
index bc28315a..9ae94f2b 100644
--- a/src/primitives.rs
+++ b/src/primitives.rs
@@ -711,7 +711,7 @@ impl<'a, T> Range for &'a [T] {
 }
 
 impl<'a, T> RangeStream for &'a [T]
-    where T: Copy + PartialEq
+    where T: Clone + PartialEq
 {
     #[inline]
     fn uncons_range(&mut self, size: usize) -> Result<&'a [T], Error<T, &'a [T]>> {
@@ -728,7 +728,7 @@ impl<'a, T> RangeStream for &'a [T]
         where F: FnMut(Self::Item) -> bool
     {
         let len = self.iter()
-            .take_while(|c| f(**c))
+            .take_while(|c| f((**c).clone()))
             .count();
         let (result, remaining) = self.split_at(len);
         *self = remaining;
@@ -760,7 +760,7 @@ impl<'a> StreamOnce for &'a str {
 }
 
 impl<'a, T> StreamOnce for &'a [T]
-    where T: Copy + PartialEq
+    where T: Clone + PartialEq
 {
     type Item = T;
     type Range = &'a [T];
@@ -771,7 +771,7 @@ impl<'a, T> StreamOnce for &'a [T]
         match self.split_first() {
             Some((first, rest)) => {
                 *self = rest;
-                Ok(*first)
+                Ok(first.clone())
             }
             None => Err(Error::end_of_input()),
         }
@@ -1711,4 +1711,17 @@ mod tests {
         let s: &[u8] = &[];
         assert_eq!(SliceStream(s).uncons_range(0), Ok(&[][..]));
     }
+
+    #[derive(Clone, PartialEq, Debug)]
+    struct CloneOnly {
+        s: String,
+    }
+
+    #[test]
+    fn parse_clone_but_not_copy() {
+        // This verifies we can parse slice references with an item type that is Clone but not Copy.
+        let input = &[CloneOnly { s: "x".to_string() }, CloneOnly { s: "y".to_string() }][..];
+        let result = ::range::take_while(|c: CloneOnly| c.s == "x".to_string()).parse(input);
+        assert_eq!(result, Ok((&[CloneOnly { s: "x".to_string() }][..], &[CloneOnly { s: "y".to_string() }][..])));
+    }
 }
diff --git a/src/range.rs b/src/range.rs
index fdb3d311..53b65cb1 100644
--- a/src/range.rs
+++ b/src/range.rs
@@ -183,6 +183,16 @@ mod tests {
     fn take_while_test() {
         let result = take_while(|c: char| c.is_digit(10)).parse("123abc");
         assert_eq!(result, Ok(("123", "abc")));
+        let result = take_while(|c: char| c.is_digit(10)).parse("abc");
+        assert_eq!(result, Ok(("", "abc")));
+    }
+
+    #[test]
+    fn take_while1_test() {
+        let result = take_while1(|c: char| c.is_digit(10)).parse("123abc");
+        assert_eq!(result, Ok(("123", "abc")));
+        let result = take_while1(|c: char| c.is_digit(10)).parse("abc");
+        assert!(result.is_err());
     }
 
     #[test]


Test patch: