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

Improve parser (recovery) for expressions #11188

Closed
Veykril opened this issue Jan 4, 2022 · 0 comments · Fixed by #14128
Closed

Improve parser (recovery) for expressions #11188

Veykril opened this issue Jan 4, 2022 · 0 comments · Fixed by #14128
Labels
A-parser parser issues C-enhancement Category: enhancement E-unknown It's unclear if the issue is E-hard or E-easy without digging in

Comments

@Veykril
Copy link
Member

Veykril commented Jan 4, 2022

The general approach here is to look at our syntax tree (show syntax tree), at the InteliiJ syntax tree (PSI viewer plugin), at the Kotlin parser, and make the recovery work.

I don't think we should special-case the arg list parsing though, Kotlin doesn't seem to do that:

https://github.com/JetBrains/kotlin/blob/dc568426bd4b73a377187c32991ffd98e9ec1106/compiler/psi/src/org/jetbrains/kotlin/parsing/KotlinExpressionParsing.java#L1825

From what I've looked at, the problem is our expression parsing. Here are some problemeatic cases:

  • fn main() { call(x::) } -- here, the ) get's eaten by PATH_EXPR, this shouldn't be happening. Culprit is somewhere around here. It semes that for expression paths we shoudl be using a different recovery set
  • fn main() { f(x+) } is a similar case, binary expressions eats )
  • fn main() { f(x:) } this one is fun! I think this one works in IntelliJ Rust, becaues it supprts type ascription, and we don't. I think what I'd expect to see here is that the whole x: gets parsed as a qualified path.

On trick we should steal from kotlin for arg list itself is this condition:
https://github.com/JetBrains/kotlin/blob/dc568426bd4b73a377187c32991ffd98e9ec1106/compiler/psi/src/org/jetbrains/kotlin/parsing/KotlinExpressionParsing.java#L1839-L1842

Originally posted by @matklad in #10195 (comment)
Looked some more into this, not super trivial to fix. The problem is, to make a resonable recovery in (expr,*), we need first to make sure that expr parsing itself is reasonable. Today it is not: in (1+), expression will eat ). Just fixing that by adding ) to

https://github.com/rust-analyzer/rust-analyzer/blob/a6e6f9c58b951459a73497618fc523d96de24fba/crates/parser/src/grammar/expressions/atom.rs#L62

doesn't work -- it actually breaks our stmt* parsing in blocks, which assumes that statement always consumes at least one token. It seems like we want to remove recovery from here:

https://github.com/rust-analyzer/rust-analyzer/blob/a6e6f9c58b951459a73497618fc523d96de24fba/crates/parser/src/grammar/expressions/atom.rs#L150-L152

and instead add it to the toplevel, to whatever code actually parses expressions. Will try to investigate further.

Originally posted by @matklad in #10195 (comment)

cc #10410 #10173

@Veykril Veykril added E-unknown It's unclear if the issue is E-hard or E-easy without digging in A-parser parser issues labels Jan 4, 2022
@Veykril Veykril added the C-enhancement Category: enhancement label Feb 8, 2023
@bors bors closed this as completed in 4456800 Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parser parser issues C-enhancement Category: enhancement E-unknown It's unclear if the issue is E-hard or E-easy without digging in
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant