chumsky icon indicating copy to clipboard operation
chumsky copied to clipboard

No `Delimiters` strategy advertized by error message

Open tailhook opened this issue 2 years ago • 7 comments

Just got this:

Start and end delimiters cannot be the same when using `NestedDelimiters`, consider using `Delimiters` instead

But I don't see any Delimiters structure or delimiters function. So it's probably in to do list?

tailhook avatar Jan 06 '22 01:01 tailhook

Ah, whoops. Delimiters was removed before the last release because it didn't quite work as expected, but it seems that I forgot to remove mention of it in the error message.

For now, something like SkipUntil will probably be a good substitute.

zesterer avatar Jan 06 '22 10:01 zesterer

Skip until seems to work. By my intention was also to emit Unclosed error for "..." and it looks like the only way to do that is nested_delimiters recovery (or manually).

tailhook avatar Jan 06 '22 11:01 tailhook

You could just use map_err afterwards to emit the error you care about.

zesterer avatar Jan 06 '22 12:01 zesterer

You could just use map_err afterwards to emit the error you care about.

The problem with map_err on quoted strings is that it catches all the inner errors (i.e. bad escape chars). So I have to filter what error I have to map and which to keep intact, which I figured out to only fix error on end of input condition.

But would be nice to have something ready for use. It took some time to figure out how to match that, and how backtracking works. I'm still not sure I understand it enough, just it looks like it works on few unit tests.

tailhook avatar Jan 06 '22 13:01 tailhook

Another issue with map_err (which is both for nested delimiters and non-nested delimiters) is that:

let x = inner1.delimited_by(just('"'), just('"')).map_err(|e| ...)
let y = inner2.delimited_by(just('{'), just('}'));
x.or(y)

This will yield unclosed " error. for the example input {. The reason for that is that when error is merged, the first Unclosed takes precedence. So even if I add .map_err() to y the error will still be incorrect.

So to fix the error I'm currently doing something like this:

just('"').ignore_then(
    inner1.then_ignore(just('"')).map_err_and_span(...)
)

Which is almost fine except:

  1. In map_err_and_span I get the span after the quote, so I have to do offset-1
  2. And delimited_by is then discouraged in real code that want to have nice error reporting?

Alternative is something like:

inner1.delimited_by(
  just('"'),
  just('"').map_err_and_span(...)
).map_err_and_span(...)

I.e. first map error specifically for quote and then add a span to that quote, although, this sounds like a bit more complex then I would like it to be.

Are there any better ways to implement Unclosed errors with delimited_by?

tailhook avatar Jan 08 '22 15:01 tailhook

(Apologies, I'm planning to get round to answering this but I've not had much time over the last few days)

zesterer avatar Jan 12 '22 11:01 zesterer

(Apologies, I'm planning to get round to answering this but I've not had much time over the last few days)

Sure. No pressure. I'm relatively satisfied with what I'm currently have, so take your time.

Some more quick food for the brain: it would be nice if it were possible to get spans of successful parts of the parser to the error handler, kinda like this:

just('"').then(filter(|c| c != '"')).then('"')
.map_err_with_spans(|_unexpected_err, (open_quote_span, _span_middle), error_span| {
  Unclosed { open_quote_span, error_span }
})

In current implementation I'm doing math on spans in map_err which is unfortunate if span contain more than byte offsets: line and column for the characters (this either needs access to original string, or need to rely on the first delimiter having known dimensions, which ruins abstractions a bit).

This specific syntax is not possible, I think, but maybe you can have better ideas how to express that or how it could be done with current tools.

tailhook avatar Jan 12 '22 12:01 tailhook

Error reason merging has been implemented in zero-copy, so both the original issue and the second issue should be resolved now.

zesterer avatar Feb 20 '23 22:02 zesterer