internal: Improve parser recovery for delimited lists #14128
Conversation
rustbot
added
the
S-waiting-on-review
Veykril
changed the title
Veykril
changed the title
Veykril
changed the title
Veykril
force-pushed
the
parser
branch
2 times, most recently
from
58db469 to
66ca869
Compare
Veykril
changed the title
Veykril
force-pushed
the
parser
branch
2 times, most recently
from
1a0f006 to
bb5a5e7
Compare
| } | ||
| if !p.at(delim) { | ||
| if p.at_ts(first_set) { | ||
| p.error(format!("expected {:?}", delim)); |
There was a problem hiding this comment.
[...] as this might get the parser stuck if I missed something
This is related to this right? Just a random unfinished thought: The recovery here is basically unbounded right? Does it make sense to bound it to some hardcoded recovery limit?
There was a problem hiding this comment.
It assumes that the parser function consumes at least one token if it returns true, if it doesn't then this will lock up. All use sites should work this way though (will add a comment for that behavior)
There was a problem hiding this comment.
Yes I thought so :) However, my question was somewhat unrelated to that 😅. I was just wondering whether we shouldn't recover indefinitely. But on second thought that probably doesn't make any sense...
Again, unrelated to the above. IIuc, this
fn test() {
hello(a <|>
return 1
...
}will pull the return expr into the arg list right? That's quite unfortunate.
There was a problem hiding this comment.
Oh that's what you meant with unbounded 😬. It will yes, but I don't think that's gonna be a problem, in this (broken syntax) spot, the only features that are relevant to the user are signature help and completions, neither of which would break from this (and assuming the return expression is termianted by a semicolon, nothing else after would be consumed either)
Veykril
force-pushed
the
parser
branch
2 times, most recently
from
127bd70 to
109ddec
Compare
| @@ -1126,5 +1126,5 @@ fn benchmark_syntax_highlighting_parser() { | |||
| .filter(|it| it.highlight.tag == HlTag::Symbol(SymbolKind::Function)) | |||
| .count() | |||
| }; | |||
| assert_eq!(hash, 1609); | |||
| assert_eq!(hash, 1608); | |||
There was a problem hiding this comment.
This somehow affects the outcome here, yet the parse trees don't differ on my machine 😕
|
@bors r+ |
|
☀️ Test successful - checks-actions |
Closes #11188, #10410, #10173
Should probably be merged after the stable release as this might get the parser stuck if I missed something