Provide a way to pass in a context to Stream::next_slice instead of just offset

[desaikd]

I'm working on a custom implementation of Stream where both Slice and Checkpoint are Self. I'm trying to add row and column count details to this buffer, with specific requirements that the current Location doesn't address.

While implementing this, I noticed that the parse_next function in impls.rs (line 399) stores the checkpoint and then resets and modifies the offset using next_slice. I'd like to do something similar for location data (rows and columns).

Currently, I've had to modify the reset function to preserve my custom location information:

let current_row = self.row;
let current_column = self.column;

*self = *checkpoint;
self.row = current_row;
self.column = current_column;

While this works for now, I'm concerned about potential issues in the future, given that reset is used in many places throughout winnow.

I'm wondering if it would be possible to modify next_slice to accept a context or Slice parameter, allowing me to pass in my row and column information there. This would eliminate the need to depend on reset for maintaining this custom state. The signature might look something like:

fn next_slice(&mut self, slice: Self::Slice) -> Self::Slice;

This change would provide more flexibility for implementations that need to track custom state beyond just the offset.

I'd appreciate your thoughts on this proposal and whether it aligns with the future direction of the winnow crate. Also, If you any other ideas on how I should do it that is helpful as well.

[epage]

I'm working on a custom implementation of Stream where both Slice and Checkpoint are Self. I'm trying to add row and column count details to this buffer, with specific requirements that the current Location doesn't address.

Could you go into more detail on this?

The intention behind Location is that line/column is display information and should be generated from byte offsets when needed, rather than pessimistically during parsing.

[epage]

@zslayton from #742 (comment)

Unfortunately, the pessimistic processing would seem to be necessary in order to support providing row/column information when reading from a sliding buffer over an input stream. The reader may not be able to re-visit the bytes in question. (To be clear: the ask here is not that Location be modified to support our use case. We just need some way to achieve our use case, and have observed that Location does not seem to be it.)

[epage]

Currently, I've had to modify the reset function to preserve my custom location information:

let current_row = self.row;
let current_column = self.column;

*self = *checkpoint;
self.row = current_row;
self.column = current_column;

I think we need to narrow on this before discussing next_slice

reset is meant to move back in where you are parsing. This is used by alt, opt, and many other places. To preserve row/column information through a reset does not seem correct as you will then move forward in parsing (either next_token or next_slice) which will, in theory, advance row/column despite those locations already being tracked. reset should restore these back to the position for where the checkpoint points to.

[zslayton]

The intention behind Location is that line/column is display information and should be generated from byte offsets when needed, rather than pessimistically during parsing.

Unfortunately, the pessimistic processing would seem to be necessary in order to support providing row/column information when reading from a sliding buffer over an input stream. The reader may not be able to re-visit the bytes in question. (To be clear: the ask here is not that Location be modified to support our use case. We just need some way to achieve our use case, and have observed that Location does not seem to be it.)

reset is meant to move back in where you are parsing. This is used by alt, opt, and many other places. To preserve row/column information through a reset does not seem correct as you will then move forward in parsing (either next_token or next_slice) which will, in theory, advance row/column despite those locations already being tracked. reset should restore these back to the position for where the checkpoint points to.

Agreed, the current implementation is a hacky workaround. It functions in the narrow use cases we've tested, but it's obviously fragile and does not follow the intended design. We're hoping to find an alternative solution.

Regarding next_slice, I've annotated a copy of the parse_next function body to illustrate where we experience the problem. (Forgive any over-explanation, I'm aware you know much of this already.)

fn parse_next(&mut self, input: &mut I) -> Result<<I as Stream>::Slice, E>

    // Save a copy of `input` (For us, `Self::Checkpoint` is `Self`)
    let checkpoint = input.checkpoint();

    // Run the parser.
    match (self.parser).parse_next(input) {       
      Ok(_) => {         

         // At this point `input` has been modified.
         // Its inner `offset`, `row`, and `column` fields have been updated.
         // Take note of the change in 'offset'.    
         let offset = input.offset_from(&checkpoint);

         // Discard all of the other information, including the new `row` and `column`.
         input.reset(&checkpoint);

         // Get a slice by applying the change in offset.
         let taken = input.next_slice(offset);
         
         // At this point, `taken` is correct, but important changes to `input` were lost.
         Ok(taken)             
      }
      Err(e) => Err(e),         
   }
}

In our case, Self::Checkpoint is the same type as Self.
If that were always true, this impl could just call checkpoint.next_slice(offset) instead of resetting input altogether.
However, since it (rightfully) isn't mandated by the API, we wondered if another approach might be possible.

Conceptually, we would need something like:

fn parse_next(&mut self, input: &mut I) -> Result<<I as Stream>::Slice, E>
    let checkpoint_before = input.checkpoint();
    match (self.parser).parse_next(input) {       
      Ok(_) => {
         let checkpoint_after = input.checkpoint();
         let offset = input.offset_from(&checkpoint_before);
         input.reset(&checkpoint_before);
         let taken = input.next_slice(offset);
         input.reset(&checkpoint_after);
         Ok(taken)             
      }
      Err(e) => Err(e),         
   }
}

Since this adds a second reset(), it probably isn't the way to go. We were hoping you might have better ideas.

[epage]

At this point, taken is correct, but important changes to input were lost.

What important changes were lost?

I feel like I'm being brought in mid-conversation on something with many assumptions made without going back to the beginning to explain the core problem trying to be solved.

[zslayton]

I feel like I'm being brought in mid-conversation on something with many assumptions made without going back to the beginning to explain the core problem trying to be solved.

Apologies, that's certainly not our intention.

We have a type, TextBuffer, that implements Stream.

Each parser receives a &mut self (that is, a &mut TextBuffer<'_>). Parsers that process whitespace modify the self.row and self.prev_newline_offset fields to track where the TextBuffer appears in the overall parent stream.

When parse_next resets input, we lose the updates made to offset, row and prev_newline_offset. The call to next_slice(offset) produces a new TextBuffer that has the correct data, but at that point input no longer has the correct row and prev_newline_offset. We're trying to find a way to preserve that bookkeeping.

[epage]

When parse_next resets input, we lose the updates made to offset, row and prev_newline_offset.

Reset is supposed to rewind the parser to a previous position, so that sounds "correct" to me.

The call to next_slice(offset) produces a new TextBuffer that has the correct data, but at that point input no longer has the correct row and prev_newline_offset. We're trying to find a way to preserve that bookkeeping.

Shouldn't input.next_slice(offset) update input with the new offset, current_row, etc? Outside of this specific call with reset, that seems needed for take_while, etc.

Similarly, looking at TextBuffer, I'm surprised the following don't update those values:

• consumed
• slice
• slice_to_end

[zslayton]

When parse_next resets input, we lose the updates made to offset, row and prev_newline_offset.
Reset is supposed to rewind the parser to a previous position, so that sounds "correct" to me.

Yes, agreed.

Shouldn't input.next_slice(offset) update input with the new offset, current_row, etc? Outside of this specific call with reset, that seems needed for take_while, etc.

Ion, the format being parsed, is a superset of JSON. At least for the moment, only the parsers that can legally process newlines actually perform bookkeeping for these fields. Parsers for floats, booleans, strings, etc will never accept a newline and so do not bother with bookkeeping. We could have all methods do newline bookkeeping as you describe, but it would perform a lot more work since it often isn't necessary.

[epage]

Ion, the format being parsed, is a superset of JSON. At least for the moment, only the parsers that can legally process newlines actually perform bookkeeping for these fields. Parsers for floats, booleans, strings, etc will never accept a newline and so do not bother with bookkeeping. We could have all methods do newline bookkeeping as you describe, but it would perform a lot more work since it often isn't necessary.

Ah, thats the other key piece of information.

Personally, I think trying to get both behaviors out of winnow like this is a losing proposition. All of the calls you are needing to customize are low level and have no concept of the intent with how they are being called. This would lead to a very brittle parser.

One alternative idea is you have a newtype for TextBuffer with one of the two behaviors (or maybe two separate newtypes) and you switch between them explicitly as needed.