-
Notifications
You must be signed in to change notification settings - Fork 664
feat: pass error kind via parameter #1788
feat: pass error kind via parameter #1788
Conversation
crates/rslint_parser/src/parser.rs
Outdated
// The kind of unknown token the parser wanted to try to guess at that position | ||
unknown_syntax_kind: SyntaxKind, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this comment a bit hard to understand. One reason is that Unknown
kinds can never be tokens but always must be nodes (they group a set of tokens/nodes). I'm also not sure if guess
is the right word here. My understanding of the method is:
- It tries to recover from the error by seeing if any token in the recovery set OR
{
and}
are present - If that's the case, emit a parse error but don't add any node
- Otherwise, wrap the next token in an error node
So it's mainly a way to deal with unexpected tokens. I believe the caller should precisely know what kind of Unknown
node we expect (which is why you pass it).
Maybe something like this:
The kind of the unknown node the parser inserts if it isn't able to recover because the current token is neither in the recovery set nor any of `{` or `}`.
We may want to rename that method if we fully understand its use case because I had no idea what it's doing by just reading its name.
Nit: Should this comment be part of the method documentation?
Future: The method starts to have many parameters. We may want to think of an alternative API, especially if we need to add more parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's mainly a way to deal with unexpected tokens. I believe the caller should precisely know what kind of Unknown node we expect (which is why you pass it).
What do you think if changed the API in this way:
- call
err_recover
by passing a parameter calledexpected_node
; - in case the parser expects a
JS_STATEMENT
, we then passSyntaxKind::JS_STATEMENT
; - the API will be called like
p.err_recover(err, T!['{'], true, JS_STATEMENT)
err_recover
will mapJS_STATEMENT
toJS_UNKNOWN_STATEMENT
using amatch
into a variable calledunresolved_node
orunknown_node
. This variable will the node we will use to replaceERROR
Would that make sense?
We may want to rename that method if we fully understand its use case because I had no idea what it's doing by just reading its name.
Yes, I plan to rename the methods in order to better fit what we are trying to do. But I didn't want to put too much stuff and I wanted to lay ground for the future PRs.
Future: The method starts to have many parameters. We may want to think of an alternative API, especially if we need to add more parameters.
This is something I already had in mind. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be neat but there are no kinds of unions like expression and statements.
crates/rslint_parser/src/parser.rs
Outdated
&mut self, | ||
recovery: TokenSet, | ||
include_braces: bool, | ||
unknown_syntax_kind: SyntaxKind, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we add the same comment for unknown_syntax_kind
as you added to err_recover
?
Deploying with Cloudflare Pages
|
f30abf2
to
189c901
Compare
Summary
First PR that implements #1759
This change is very simple, every time we need to mark error inside the tree, we explicitly pass the
SyntaxKind::ERROR
as parameter instead of having it hardcoded.This change will make things much easier and smoother when applying specific errors during the parsing phase.
Test Plan
Existing CI