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

Recover invalid assoc type bounds using == #87566

Merged
merged 2 commits into from
Sep 17, 2021

Conversation

JohnTitor
Copy link
Member

Fix #87493
r? @estebank

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021
@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
@@ -1933,7 +1933,19 @@ impl<'a> Parser<'a> {
}
match self.parse_expr_res(Restrictions::CONST_EXPR, None) {
Ok(expr) => {
if token::Comma == self.token.kind || self.token.kind.should_end_const_arg() {
// Find a mistake like `MyTrait<Assoc == S::Assoc>`.
if token::EqEq == snapshot.token.kind {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this is the wrong place for this. Haven't dug into this, but I would have expected this to be in the same place as the associated type parsing code? There, we just "accept" with an error == when we really want =.

I guess this is potentially "ambiguous" if Assoc == S::Assoc gets treated as a const arg, which I guess is what currently happens.

Is there anything we can to be smarter here? Would we theoretically be allowed to have a variable named Assoc? I'm trying to think of if we had complete knowledge of all traits, types, variables, etc. in scope, are there any cases where we can know if this should be a bracketed const expr arg or an associated type binding?

Copy link
Contributor

Choose a reason for hiding this comment

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

I got somewhat confused as to why we didn't have a == binop here, but looking at the surrounding code I can now see that we discard whatever op this was and what is being parsed here is the RHS, namely a single path. So I'm getting around to thinking that this is not a bad place to make this check.

Comment on lines 1938 to 1965
err.span_suggestion(
snapshot.token.span,
"replace `==` with `=`",
"=".to_string(),
Applicability::MaybeIncorrect,
);
Copy link
Member

Choose a reason for hiding this comment

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

If this does stay here, it might be worth mentioning something like "if you meant to use an associated type binding"

"=".to_string(),
Applicability::MaybeIncorrect,
);
let value = self.mk_expr_err(expr.span);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be self.mk_expr_err(start.to(expr.span))? We'd want this to cover the whole Foo == Bar instead of just Bar.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

Please apply @jackh726's wording change and this can be merged.

@JohnTitor JohnTitor force-pushed the find-eqeq-on-assoc-type-bounds branch from 8625e34 to ee99bb3 Compare September 17, 2021 05:10
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 17, 2021

📌 Commit ee99bb3 has been approved by estebank

@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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 17, 2021
@estebank
Copy link
Contributor

Just realized that we should probably also check that the rhs is a path (because foo == 42 is more likely to be meant as a const expression), but we can iterate on that later.

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2021
…laumeGomez

Rollup of 10 pull requests

Successful merges:

 - rust-lang#86422 (Emit clearer diagnostics for parens around `for` loop heads)
 - rust-lang#87460 (Point to closure when emitting 'cannot move out' for captured variable)
 - rust-lang#87566 (Recover invalid assoc type bounds using `==`)
 - rust-lang#88666 (Improve build command for compiler docs)
 - rust-lang#88899 (Do not issue E0071 if a type error has already been reported)
 - rust-lang#88949 (Fix handling of `hir::GenericArg::Infer` in `wrong_number_of_generic_args.rs`)
 - rust-lang#88953 (Add chown functions to std::os::unix::fs to change the owner and group of files)
 - rust-lang#88954 (Allow `panic!("{}", computed_str)` in const fn.)
 - rust-lang#88964 (Add rustdoc version into the help popup)
 - rust-lang#89012 (Suggest removing `#![feature]` for library features that have been stabilized)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1b78967 into rust-lang:master Sep 17, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 17, 2021
@JohnTitor JohnTitor deleted the find-eqeq-on-assoc-type-bounds branch November 10, 2021 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Misleading error message on associated type bounds
8 participants