Upgrade chevrotain #268
Upgrade chevrotain #268
Conversation
Perhaps in a different change:Try to remove You will still get (fewer) errors regarding ambiguities because the new ambiguity ignoring is more granular. Those errors should be inspected, either we already handle those ambiguities in ways other than GATE, in which case they need to be ignored explicitly on the DSL method (new options). |
|
Remove "hacky" fix for CST Building while backtracking. |
Done ! I will try to delete the |
| } | ||
| ]); | ||
| ], | ||
| IGNORE_AMBIGUITIES: true |
There was a problem hiding this comment.
Ambiguous Alternatives Detected: <1 ,2> in inside Rule,
<Identifier, '='> may appears as a prefix path in all these alternatives.
It is because an elementValue could be a binaryExpression and could start with Identifier and '='. Do you think we must use a GATE or backtracking here ?
There was a problem hiding this comment.
if you use 3 or 4 tokens lookahead would that resolve in this case?
because you can increase the maxLookahead for this specific rule...
I don't remember the specific grammar rules, does the list alternative always have a comma somewhere? how far ahead is that comma?
There was a problem hiding this comment.
Unfortunately, it is not enough to increase the maxLookahead for this rule. The list alternative always has a comma somewhere, but as the elementValue could be an expression, it could have several annotations and thus we cannot look ahead at a specific step.
| { ALT: () => $.SUBRULE($.unannType) }, | ||
| { ALT: () => $.CONSUME(t.Var) } | ||
| ], | ||
| IGNORE_AMBIGUITIES: true |
There was a problem hiding this comment.
Not sure how to enforce not using 'var' in unannType
There was a problem hiding this comment.
can var be used as an identifier? is that the cause of the ambiguity?
There was a problem hiding this comment.
If that is the case (I have some fuzzy memory of limitations for the var keyword.
You may be able to define a IdentifierButNotVar Token using token categories.
There was a problem hiding this comment.
Yes, that's right. I think I will try to improve this in a following PR, adding a token and change some parser logic seems out of the scope of this PR
There was a problem hiding this comment.
Yes definitively out of scope
Concerning this one, It would be great if @Shaolans could have a look, as he added the "prettier-ignore" logic here |
I will take a look |
ac9359c to
4a70840
Compare
|
@bd82: Do you think it can be merged ? We could do the rest of the work (removing "hacky" fix for CST Building while backtracking & working on IGNORE_AMBIGUITIES cases in follow-up PRs) |
Yes, this looks good 👍 |
I started working on Chevrotain upgrade. It seems that it works with these changes, but the way I implemented it should be improved.
@bd82 if you could review this PR and give me your feedback, it would be great !
I especially modified some "is..." rules to make the tests pass and removed some errors throwing in the process, so I guess something should be improved here.