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.

Sign up for GitHub

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

Jump to bottom

Improve parser diagnostics #95211

Merged
merged 1 commit into from
Jun 14, 2022
Merged

Improve parser diagnostics #95211

merged 1 commit into from
Jun 14, 2022

Conversation

terrarier2111
Copy link
Contributor

@terrarier2111 terrarier2111 commented Mar 22, 2022 •
edited
Loading

This pr fixes #93867 and contains a couple of diagnostics related changes to the parser.
Here is a short list with some of the changes:

  • don't suggest the same thing that is the current token
  • suggest removing the current token if the following token is one of the suggestions (maybe incorrect)
  • tell the user to put a type or lifetime after where if there is none (as a warning)
  • reduce the amount of tokens suggested (via the new eat_noexpect and check_noexpect methods)

If any of these changes are undesirable, i can remove them, thanks!

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 22, 2022
@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2022
@terrarier2111 terrarier2111 force-pushed the improve-parser branch 2 times, most recently from aa01a0b to 3f8fdb7 Compare March 22, 2022 16:32
@terrarier2111 terrarier2111 changed the title Improves parser diagnostics, fixes #93867 Improve parser diagnostics Mar 22, 2022
@terrarier2111
Copy link
Contributor Author

r? @compiler-errors

could you take a look at this cuz i took some inspiration from your comments and you seemed to be interested in smth like this as well?

@compiler-errors
Copy link
Member

I cannot r+ this (and hence high-five bot didn't assign me), but I will certainly take a look!

@compiler-errors
Copy link
Member

lol, nvm, highfive-bot does not care about bors permissions apparently. anywho, I cannot approve this for you, so I will reassign this to someone who can. I will try to give it a review though!

r? rust-lang/compiler

@jackh726
Copy link
Member

@bors delegate=compiler-errors

@compiler-errors if you want to review this, go for it (I'll try to look at this regardless later today)

@bors
Copy link
Contributor

bors commented Mar 22, 2022

✌️ @compiler-errors can now approve this pull request

compiler-errors
compiler-errors requested changes Mar 22, 2022
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am concerned with a regression in parsing fn foo() where {}. Also left a few other comments.

src/test/ui/generic-associated-types/issue-93341.rs Outdated Show resolved Hide resolved
src/test/ui/moves/use_of_moved_value_copy_suggestions.rs Outdated Show resolved Hide resolved
src/test/ui/parser/doc-comment-in-stmt.stderr Outdated
@@ -19,6 +24,11 @@ LL | false
LL | /*! bar */
| ^^^^^^^^^^ unexpected token
|
help: Consider removing this token
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is somewhat misleading to call this a "token"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What name would you suggest, i am willing to adjust this

Copy link
Member

@compiler-errors compiler-errors Mar 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you special-case this for certain tokens?

Also, not sure if the suggestion to remove a comment is useful here, since comments are usually intentionally placed. This one might be better fixed with a separate suggestion.

Copy link
Contributor Author

@terrarier2111 terrarier2111 Mar 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you special-case this for certain tokens?

Also, not sure if the suggestion to remove a comment is useful here, since comments are usually intentionally placed. This one might be better fixed with a separate suggestion.

Oh, yea i can special case different token kinds and i can ignore comments i think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so i made it vary based on the kind of token

src/test/ui/parser/float-field-interpolated.stderr Outdated Show resolved Hide resolved
@terrarier2111
Copy link
Contributor Author

I am concerned with a regression in parsing fn foo() where {}. Also left a few other comments.

I fixed this regression by downgrading the diagnostic from an error to a warning

@terrarier2111 terrarier2111 force-pushed the improve-parser branch 2 times, most recently from e57b61b to 0839c08 Compare March 22, 2022 18:59
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@compiler-errors
Copy link
Member

@terrarier2111, is it too much work to split this up into several commits? We might want to land some of these diagnostics separately, possibly so it's easier to git-bisect later on, and because they are varying levels of opinionated. Or at least, several commits might be easier to review. For example, I could see the _noexpect changes landing immediately, but the remove token help might need some refining.

compiler-errors
compiler-errors reviewed Mar 22, 2022
View reviewed changes
compiler/rustc_parse/src/parser/diagnostics.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/parser/diagnostics.rs Outdated
Comment on lines 334 to 432
if !ignore
&& self.look_ahead(1, |next| {
is_ident_eq_keyword(next.kind.clone(), token.clone())
})
{
suggest_removing = true;
}
if let TokenType::Token(kind) = token.clone() {
if !ignore && self.look_ahead(1, |next| kind == next.kind) {
suggest_removing = true;
}
if kind == self.token.kind {
None
} else {
Some(token)
}
} else {
Some(token)
}
}
} else {
None
}
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind leaving comments explaining what this logic is checking for? Also, perhaps using return might help tighten some of the if-else blocks.

Copy link
Contributor Author

@terrarier2111 terrarier2111 Mar 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments there and used returns, do the comments suffice?

compiler/rustc_parse/src/parser/diagnostics.rs Outdated Show resolved Hide resolved
@terrarier2111
Copy link
Contributor Author

@terrarier2111, is it too much work to split this up into several commits? We might want to land some of these diagnostics separately, possibly so it's easier to git-bisect later on, and because they are varying levels of opinionated. Or at least, several commits might be easier to review. For example, I could see the _noexpect changes landing immediately, but the remove token help might need some refining.

Do you mean splitting these commits in different prs or just having multiple commits in this pr?

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Mar 27, 2022

☔ The latest upstream changes (presumably #94495) made this pull request unmergeable. Please resolve the merge conflicts.

compiler-errors
compiler-errors reviewed May 9, 2022
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for taking some time to get back to this. I have a couple of comments.

compiler/rustc_parse/src/parser/diagnostics.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/parser/expr.rs Show resolved Hide resolved
compiler-errors
compiler-errors reviewed May 13, 2022
View reviewed changes
compiler/rustc_parse/src/parser/mod.rs
@@ -548,6 +548,22 @@ impl<'a> Parser<'a> {
is_present
}

fn check_noexpect(&self, tok: &TokenKind) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you double check if the parser has other self.token == use cases that can be turned into check_noexpect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that i am able to do that as from what I was able to see that would require a decent amount of work and I just wanted to finish up the changes in this pr.

@compiler-errors compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 6, 2022
@compiler-errors
Copy link
Member

@terrarier2111, heads up that PRs should not include merge commits: https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy

@terrarier2111
Copy link
Contributor Author

terrarier2111 commented Jun 6, 2022 •
edited
Loading

@compiler-errors Yea, i know i wanted to finish this later

@terrarier2111 terrarier2111 force-pushed the improve-parser branch 2 times, most recently from 7a47722 to 70589ba Compare June 6, 2022 19:37
@compiler-errors
Copy link
Member

Heads up that you messed up the commit log I think: https://github.com/rust-lang/rust/pull/95211/commits

@terrarier2111
Copy link
Contributor Author

Heads up that you messed up the commit log I think: https://github.com/rust-lang/rust/pull/95211/commits

Oh.. yea how could I solve this problem?

@compiler-errors
Copy link
Member

perhaps git pull --rebase origin master (where origin is whatever remote you've set up for rust-lang/rust, not your personal fork) -- please also make sure the changes you've made in cf7e08607c580bd130a27adc0a7700e333f3e92b didn't get lost as well.

@terrarier2111
Copy link
Contributor Author

Okay, well so i tried fixing it and actually made it worse :c
I think the issue is that i did "git push origin +MY_BRANCH" instead of "git push origin +master" because the master version didn't work for some reason

@compiler-errors
Copy link
Member

I'm not sure if I can help -- I'm not familiar with the git push +branch syntax. Perhaps reset your branch and cherry-pick your specific commits? No idea.

@terrarier2111
Copy link
Contributor Author

@compiler-errors do I have to do anything else for this to be good to go?

@compiler-errors
Copy link
Member

This looks fine enough to merge now. Maybe we actually don't need the noexpect methods, but those are easy to remove.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 13, 2022

📌 Commit 21fdd54 has been approved by compiler-errors

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 13, 2022
@JohnTitor JohnTitor mentioned this pull request Jun 13, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 13, 2022
@bors
Auto merge of rust-lang#98075 - JohnTitor:rollup-nqwodnk, r=JohnTitor
3bdec3c 
Rollup of 4 pull requests

Successful merges:

 - rust-lang#95211 (Improve parser diagnostics)
 - rust-lang#95243 (Add Apple WatchOS compile targets)
 - rust-lang#97385 (Add WIP stable MIR crate)
 - rust-lang#97508 (Harden bad placeholder checks on statics/consts)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e3a3c00 into rust-lang:master Jun 14, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors requested changes

@klensy klensy klensy left review comments

@notriddle notriddle notriddle left review comments

Assignees

@compiler-errors compiler-errors

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.

Misleading diagnostic on missing comma
10 participants
@terrarier2111 @rust-highfive @compiler-errors @jackh726 @bors @rust-log-analyzer @notriddle @klensy @matthewjasper @rustbot