Different result if using Parser trait's parse() vs calling the parser function directly

[beeb]

https://github.com/beeb/lintspec/blob/01eb3e90cb9ea1626c0a26a0e4d3b2ed221f3808/src/natspec.rs#L603-L625

See test above.

I have a parser (parse_comment) which takes in a &mut &str (required to have the Parser trait blanket implementation), wraps it in a LocatingSlice and then does a bunch of stuff with the spans.

If I use the parser via the parser trait: parse_comment.parse(my_comment), then in some cases I get a parsing error (generic error with no reason or context). Debugging the test case shows I shouldn't have an error, because the code path seems to enter the early return condition on L185 (and the alt returns an Ok()).

If I replace the call with parse_comment(&mut my_comment) then everything works.

It's really puzzling to me, the only intuition I would have is that in one case my string slice is a mutable reference and in the other it's not... I checked the trait's implementation but couldn't find a hint for the error. Any explanation or how to fix it would be greatly appreciated!

Thanks

[epage]

Calling the function directly is the equivalent of Parser::parse_next. Parser::parse is meant for the application to call.

[beeb]

Gotcha, but why would it give an error when calling Parser::parse and not parse_next? It certainly looks like it should consume the whole input.

[epage]

Parser::parse assumes it will parse all of the input (note it consumes the input, leaving no info where it left off). To help catch bugs with this, it applies an eof parser at the end.

[beeb]

Oooh, is it because my parser doesn't consume the input &mut &str (since it clones it inside of the LocatingSlice)? The fix would be simply to move the pointer to the end to "consume" the input right? Is take_while(0.., |_| true) suitable or any better way to "consume" the input dumbly?

[epage]

rest would consume the input but the fact that your parser isn't already would be a code smell to me and I would recommend double checking how you are doing things.

[beeb]

I want retrieve some spans related to items that get parsed so I had to wrap the input in a LocatingSlice. But I need to then convert the byte offsets into my own text index type which tracks chars, lines and columns, and for this I refer to the original string slice. Not sure how I could do it differently at the moment.

[epage]

I would expect the parser to accept a LocatingSlice.

Personally, I would defer lines and columns until you need to render as that is tightly coupled with what you are rendering to. chars having any real meaning to track is mostly a myth and I would recommend avoiding it entirely.

There are also performance issues related to this. At least deferring this until after you parse means that you can incrementally calculate these (e.g. https://github.com/crate-ci/typos/blob/fb1f64595962a79113c92d4879e6b3b2e8f524b4/crates/typos-cli/src/file.rs#L828-L851). Deferring until you render means you then only need to process a subset of the data. chars will have the biggest performance impact.

If there is a motivating case to still do it all this way,

• As a quick hack, you can take a checkpoint, reset_to_start, calculate the position, and then reset to your checkpoint
• We could consider providing access to the original input

[epage]

btw I posted #796 to improve some of the docs

[beeb]

Thanks for the reply and for the docs PR. The suggestion to use a LocatingSlice as input makes it so I have to export an additional type from winnow in my public API, which I would like to avoid if possible. Granted I already have ModalResult in my public API so one could argue it's not a big deal.

Note that what I generate in the end is not really a char offset but rather a bytes offset for utf8 encoding and code units offset for utf16 (via char.len_utf8() and char.len_utf16()).

I could consider shifting the work into the caller, but I think it will have no performance impact in my case as I already calculate these incrementally and only parse a subset of the document with winnow already.

Need to think about it... Thanks for the great pointers!