fix: add error recovery for use_tree_list parsing #16349
Conversation
rustbot
added
the
S-waiting-on-review
There was a problem hiding this comment.
#4680 (comment) still applies here fwiw
But I think when parsing a {} delimited UseTree we can just skip unexpected tokens until we find the closing brace or a new item (ITEM_RECOVERY_SET). That should keep the balancing working.
| } | ||
|
|
||
| pub(super) const USE_TREE_LIST_RECOVERY_SET: TokenSet = | ||
| TokenSet::new(&[T![;], T![,], T![.], T![ident]]); |
There was a problem hiding this comment.
This should probably contain pub and other item keywords like struct etc. Also the T![.] makes little sense here I think. ITEM_RECOVERY_SET) is probably what we want to include.
There was a problem hiding this comment.
add T![.] here cause it's position on the keyboard is close to the T![,], some user may type it by mistakely when they want to type T![,] (as least I did 😄)
| // use b; | ||
| // struct T; | ||
| // fn test() {} | ||
| delimited(p, T!['{'], T!['}'], T![,], USE_TREE_LIST_RECOVERY_SET, |p: &mut Parser<'_>| { |
There was a problem hiding this comment.
USE_TREE_LIST_RECOVERY_SET is wrong here, that param in delimited is not for recovery but for the first set. So we want a USE_TREE_LIST_FIRST_SET that contains the tokens that use tree may start with.
There was a problem hiding this comment.
To re-iterate, I don't think matklad's comment on the previous PR applies here. The invariant that was mentioned is still upheld, either the } is eaten as the last child or we recover on an item in which case we stop the parse without consuming any closing one. The reparser checks the invariants for balancedness already so at worst we lose out on the re-parse in this case, but I think that is fine.
BUT, there is one problem I can see here I think. With this setup, the following might change now
struct implThing;
use crate::{impl$0};This should complete implThing, but with the changes here it might no longer do that, can you add a completion test for this? If it no longer works (as I suspect) we need to adjust the recovery strategy by doing an extra lookahead after the keyword token for , and } and recover accordingly
Veykril
added
S-waiting-on-author
|
actually the completion still works
|
|
Nice, thanks! |
|
☀️ Test successful - checks-actions |
This PR adds error recovery for USE_TREE_LIST parsing, avoid the wrong USE_TREE_LIST making the rest parsing incorrectly.
before
after
close #16227