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

Precise error location in UNRECOGNIZED by attaching trivia to the expected nonterminal #1172

Merged
merged 12 commits into from
Dec 12, 2024

Conversation

beta-ziliani
Copy link
Contributor

@beta-ziliani beta-ziliani commented Nov 28, 2024

Fixes #841

In examples like the following:

// some trivia
unexpected

The parsing function was returning a terminal with UNRECOGNIZED kind and the entire text inside. Since a terminal cannot have trivia attached, we have to change the returning value to be a nonterminal. But which one? For a moment I thought of a new specific UNRECOGNIZED kind, but after discussing with Omar, it was clear that the better option was to return the expected one: if we are parsing a Statement, then return a Statement with the trivia and the UNRECOGNIZED terminal at the end. This is realized in the NoMatch structure with a new field for the optional expected nonterminal kind, which is essentially attached in the ParserResult::with_kind method.

The commit history contains a previous attempt in which the trivia was cached within the NoMatch struct. This added unnecessary complexity, so I decided against it.

Note1: In a couple of places the code will use a ParserResult::no_match(<default values>). Noting there was a ParseResult::default() function, I decided to use it instead, but I'm not sure if this is the expected use of the default function.

Note2: I see that the error is one off at the end, probably accounting EOF? But was already the case before, so there's no real change there.

@beta-ziliani beta-ziliani requested review from a team as code owners November 28, 2024 13:07
Copy link

changeset-bot bot commented Nov 28, 2024

🦋 Changeset detected

Latest commit: 72d4858

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@nomicfoundation/slang Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@beta-ziliani
Copy link
Contributor Author

Likely this deserves a changeset, since it's changing the way an UNRECOGNIZED node is return when it's in the top-most position.

Copy link
Contributor

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of questions.
Thanks!

Copy link
Contributor

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, modulo the two unresolved comments (adding comments and a changeset).

@beta-ziliani beta-ziliani added this pull request to the merge queue Dec 12, 2024
Merged via the queue into NomicFoundation:main with commit 6102886 Dec 12, 2024
1 check passed
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.

error range includes leading trivia, instead of just the offending token
2 participants