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

See if gix-object/verbose-object-parsing-errors can include more parsing state like with nom #1099

Closed
Byron opened this issue Nov 9, 2023 · 3 comments · Fixed by #1104
Closed

Comments

@Byron
Copy link
Owner

Byron commented Nov 9, 2023

In #1096 we saw the verbose errors with nom would yield information about the current location of the cursor like this:

Err(Decode(Error { inner: VerboseError { errors: [("040000 css\0\xA7E/\xD7Ǡ\x91S\x96p\xF2\xE8S˦S\x98@-\xDF100644 favicon.ico\0p\x06\x9B\xADB\x8A\x93\xD4V\xCD\x15\x93\x9D\xB2\xF1\xD66v4\x90100644 help.html\0\xBDp\xC6M$\xC7\u{1e}D\xC8@OZ>\\\xA5TU\xAB\xAE\xE5040000 help\0\xE7eo7rg g\n\x18_:\x12\xE3<\xD2!\xFF\xF9\x82040000 images\0d\x97\xB2\x82C\xAF%n\t)\xDB\x05\xD0
q\xE2\xC3\x0c\xBD\xE1C100644 index.php\0\xFF\r\x08\xC4\x0b\xCF \xFB\xEE:]\x7fQӉb\xEC1\xD6\u{1c}40000 js\0^\"\xE0P\x94\xA0bi4\xCB/s\x19\xE9-\xA2\xD3C\xE7&040000 lib\0\xAB;\xBF\xBC\x88)Y\xE8p\n=\r\xF3\xAF\xCFH\x13\xB6\x0e\x8C100644 phpMake\0<\xB0\xFF\x96Uvq\xB4\xBB\xC6,\xC1!\x17\xC78\x99<j\xE5100644 sprocketize.php\0\x82L\xD6\u{1f}\xD8\x13\xC8\x0bq\x16\x0f\xF9\x86
`5no\x9B\x10b", Nom(Eof))] } }))

Seeing this might be useful for debugging.

When looking at code like this…

let failing = self.data;
self.data = &[];
#[allow(clippy::unit_arg)]
Some(Err(crate::decode::Error::with_err(
winnow::error::ErrMode::from_error_kind(&failing, winnow::error::ErrorKind::Verify),
)))

…it appears the cursor is passed to the error, but when displaying it, it appears completely empty:

if cfg!(feature = "verbose-object-parsing-errors") {
""
} else {
"object parsing failed"
}

Now I wonder if this is intentional, or if there is anything that can be done to get the 'auto-context' back.

Thanks @epage for sharing your insight.

Follow-up of #1098.

@epage
Copy link
Contributor

epage commented Nov 9, 2023

A ContextError doesn't capture information at the error-site, relying on the &mut Input at the top-level for it, reducing the size of the error.

There are a couple ways to go on this

  • Modify ParseError::with_err to capture the &mut Input (highest performance)
  • Write a custom error more like the old VerboseError (medium performance)
  • Switch to TreeError that captures all error information as you go (lowest performance, a lot of debug info).

For me, the question is on expectation for verbose-object-parsing-errors. Is this purely for debug and we can throw caution to the wind and capture as much debug info as possible or is this a mixed debug / production feature and we need to balance things?

@Byron
Copy link
Owner Author

Byron commented Nov 10, 2023

Thanks for chiming in and for all your help!

A ContextError doesn't capture information at the error-site, relying on the &mut Input at the top-level for it, reducing the size of the error.

Highest Performance

What's confusing me here is that it looks like it's getting the input passed, via failing in this case. Yet, what's displayed is empty. Probably the error doesn't format itself but relies on the parent implementation to do that?
Maybe the highest-performance option would then be to display the context nicely if it contains the input. It is, however, something that I'd intuitively ascribe to the inner error instead.

Medium Performance

I can imagine what the difference is in usability compared to the highest-performance version. The way I remember the nom version of this error is that it would just print the input remaining and possibly a context string (which is also reproduced now if given). I wouldn't want more.

Lowest Performance

I have tried the debug feature and it seemed to crash the tests somehow, at least when run from IntelliJ which sometimes has its own issues so I didn't take it too seriously. From what I saw, I think that's too much for this occasion.

For me, the question is on expectation for verbose-object-parsing-errors. Is this purely for debug and we can throw caution to the wind and capture as much debug info as possible or is this a mixed debug / production feature and we need to balance things?

I'd want to be able to use it in the production binary if fast enough - parsing is not usually a bottleneck right now so that might even be possible. If not, it's fine to let people compile a special binary. Right now, from what I see, verbose-object-parsing-errors isn't wired up anyway so it would be hard to discover or use even when compiling gix yourself.

@epage
Copy link
Contributor

epage commented Nov 10, 2023

I have tried the debug feature and it seemed to crash the tests somehow, at least when run from IntelliJ which sometimes has its own issues so I didn't take it too seriously. From what I saw, I think that's too much for this occasion.

This is separate from the trace feature. This is like nom-supreme's ErrorTree

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 a pull request may close this issue.

2 participants