Skip to content

Conversation

@ratmice
Copy link
Collaborator

@ratmice ratmice commented Mar 11, 2025

Marking as a draft, because this is perhaps questionable.

Here is the example source producing the error.

%grmtools {
  !unicode
}
%%
. "dot"

There are two questionable things here, the first being the Eq implementation relying on regex::Error's to_string() impl,
because regex::Error is only PartialEq.

The second being when we run the example through nimbleparse we see the regex::Error includes lrlex's wrapping of the regular expression e.g. \A(:? unfortunately, regex doesn't really give us any kind of Span we otherwise might be able to use to translate this back to the lex source locations.

Previously this just produced "Invalid regular expression", with this change it now produces a reason.
So, I'm a bit torn on this because I do feel like the extra information is helpful.

[Error] in bad.l
    5| . "dot"
        Invalid regular expression: regex parse error:
        \A(?:.)
             ^
    error: pattern can match invalid UTF-8

@ltratt
Copy link
Member

ltratt commented Mar 11, 2025

This might be a dumb question but do we need PartialEq on LexErrorKind? I can't, off the top of my head, immediately think why it's there.

@ratmice
Copy link
Collaborator Author

ratmice commented Mar 11, 2025

It looks like it is used for merging spans of duplicate occurrences so that you get an error at a bunch of places,
rather than a bunch of errors each having a place, I think I added it, and for that reason.

@ltratt
Copy link
Member

ltratt commented Mar 11, 2025

Aha, that makes sense. On reflection, I wonder if it would be better to make this a "normal" function called something like is_mergeable_with(&self, other: Self)? It gives us a "nicely weaker" external API (since we no longer promise PartialEq to other users).

@ratmice
Copy link
Collaborator Author

ratmice commented Mar 11, 2025

It was also used in the testsuite, so I went with the more generic is_same_kind().

@ltratt
Copy link
Member

ltratt commented Mar 11, 2025

Please squash (and rebase to remove the conflict).

@ratmice ratmice marked this pull request as ready for review March 11, 2025 13:37
@ratmice
Copy link
Collaborator Author

ratmice commented Mar 11, 2025

Squashed.

@ltratt ltratt added this pull request to the merge queue Mar 11, 2025
Merged via the queue into softdevteam:master with commit 683d4fe Mar 11, 2025
2 checks passed
@ratmice ratmice deleted the regex_error branch March 11, 2025 14:33
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 this pull request may close these issues.

2 participants