Receive &T instead of T in AsChar, to eliminate <I as Stream>::Token: Copy?

[lo48576]

Currently (as of v0.4.1), winnow::stream::AsChar trait converts self into primitive copyable types char, bool, and usize. So it should be possible to implement this also for any &T where T: AsChar. In such case, blanket impl such as impl<'a, T> AsChar for &T where T: AsChar { ... } can be used, but winnow implements AsChar for &'a u8 and &'a char independently.
Are there any reasons for this?

If blanket impl is used, then some parsers will be able to drop <I as Stream>::Token: Copy trait bound.

One example of this is winnow::bytes::one_of. It runs any.verify(...) internally, but Parser::verify requires G: Fn(&O2) -> bool (where O2 is <I as Stream>::Token), so one_of requires Fn(&I::Token) -> bool, which cannot be possible without I::Token: Copy. With blanket impl, &I::Token (where I::Token: AsChar) will also implement AsChar, so implicit copy will no longer be required.

[lo48576]

Ah OK, if AsChar::as_char() and other methods took &self as its argument, blanket impl is easily possible as usual.
But currently methods of AsChar take self (with its ownership), so blanket impl is not possible...

Changing signatures of methods of AsChar will be difficult as it will affect numerous other parsers, but I think it's worth considering.

[lo48576]

Implemented as an experiment, and it was possible: main...lo48576:winnow:aschar-blanket-impl .

The core change is small (ContainsToken<T> and AsChar now receives &T instead of T), and <I as Stream>::Token: Copy is successfully be eliminated.

However, this means some simple closures like |c| c == 'c' are now unusable, and need to be rewritten as |c: &char| *c == 'c'. This is huge breaking change... The changes required on downstream will be quite simple but numerous and boring. I'm not sure if this can be mitigated, but it would be better to change earlier than later if this change will be introduced.

[lo48576]

One example of this is winnow::bytes::one_of.

Sorry this is not true, it depends not only on Parser::verify but also on ContainsToken::contains_token(&self, T) which also takes T with ownership.

[lo48576]

I was thinking about |input: I| any.verify(|c: &I::Token| c.as_char() == the_character) to create restricted version of one_of that does not require I::Token: Copy, and it would be possible if blanket impl for AsChar exists. This is what I wanted to say in that paragraph.

[epage]

Would be up for creating an issue for the use case that is blocked on this solution?

[lo48576]

I'm happy to create an issue if it helps, but I'm not blocked on this. Just wondered "why some of them (and my parsers depending on them) need Token: Copy and some others don't?", so I'm not in any trouble for now.

(Sorry if I'm misunderstanding your sentence... I'm not sure what "this solution" points.)

[epage]

This Discussion is focused on a solution (supporting non-Copy tokens). The use case where that is needed helps us understand the problem statement that leads to this.

The use case I can think of for this is distinct lexing and parsing passes where the lexed tokens contain owned types and you want to be able to do one_of(|token| matches!(token, String(_) | Number(_))). Normally I want end-users who need something to report issues and then work off of those so we have a good feedback loop that we are focusing on the right problems and providing the right solutions but I can see this being a chicken-and-egg problem where people might pass winnow up rather than create an issue.

However, I also feel the use case might be small because other parts of winnow (Stream::next_token) requires token.clone() to be done which is throwaway work if the token doesn't get passed along or we have to rollback due to an alt. This means people are more likely to want to have cheap tokens anyways.

So unsure whether we should move forward with this speculatively on the next breaking release

[lo48576]

Thank you, I understood.

The use case I imagined first is, characters stream that associates every line (and every character that belong to the line) to some commit metadata (that is shared through Arc or Rc), similar to git blame.

#[derive(Debug, Clone)]
struct VersionControlledChar {
    /// Last commit that modified the line the character belongs to.
    line_last_commit: Arc<Commit>, // `Rc` is also OK.
    /// The character.
    c: char,
}

// By this impl, `ContainsToken<VersionControlledChar> for T` will be implemented for many `T`.
impl AsChar for VersionControlledChar { ... }

#[derive(Debug, Clone)]
struct VersionControlledFileSlice {
    file: Arc<VersionControlledFile>,
    range_in_file: Range<usize>,
    /// Last commits that modified the lines.
    line_last_commits: Vec<Range<usize>, Arc<Commit>>,
}

#[derive(Debug, Clone)]
struct VersionControlledStream {
    file: Arc<VersionControlledFile>,
    cursor: usize,
    // ...
}
impl Stream for VersionControlledStream {
    type Token = VersionControlledChar;
    type Slice = VersionControlledSlice;

    fn next_token(&self) -> Option<(Self, Self::Token)> {
        let mut this = self.clone();
        // Get the char and its byte length.
        let (c, len) = this.file.get_char(self.cursor)?;
        // Get the line.
        let line = this.file.get_line_number(this.cursor);
        // Get the commit for the line.
        let line_last_commit = this.file.get_last_commit_of_line(line);
        // Advance the cursor.
        this.cursor += len;

        Some((this, VersionControlledChar { line_last_commit, c }))
    }
}

In this case,

• Token implements Clone but not Copy,
• cost of cloning Token is not so high (thanks to refcount),
• Token will implement AsChar (since it is essentially char with metadata) but needless Arc::clone will be necessary due to self receiver,
• cost of cloning stream itself is not so high (thanks to refcount), and
• the user will wish to run some parsers like one_of(&['ℹ️', '⚠️', '🚫']) to parse comments in specific format.

---

But this is just a virtual (not real) example to answer my own question "why Token is not always required to be Copy?", and I realized that Token will not implement Copy when it has associated shared data.
Currently I don't have strong need to support this kind of use case.

[epage]

FYI the upcoming release of Winnow 0.5

• Removes Stream: Clone bounds, instead relying on a new Stream::Checkpoint which is a location to reset the stream to
• Changes Token: Copy bounds to Token: Clone

I feel like updating AsChar methods to take &self doesn't fully solve the problem. The core problems are

• Stream::offset_for takes a fn(Token) -> bool, requiring clones of Token while iterating
• We rely on trait bounds for Token rather than &Token

#290 tried to clean this up but I ran into issues and put it on hold so I can continue the current release.

[epage]

0.5 is now out