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

Fix expected/found order on impl trait projection mismatch error #80828

Merged
merged 2 commits into from
Apr 2, 2021

Conversation

SNCPlay42
Copy link
Contributor

fixes #68561

This PR adds a new ObligationCauseCode used when checking the concrete type of an impl trait satisfies its bounds, and checks for that cause code in the existing test to see if a projection's normalized type should be the "expected" or "found" type.

The second commit adds a peel_derives to that test, which appears to be necessary in some cases (see projection-mismatch-in-impl-where-clause.rs, which would still give expected/found in the wrong order otherwise). This caused some other changes in diagnostics not involving impl trait, but they look correct to me.

@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 Jan 8, 2021
&msg,
proj_ty,
values.expected,
) || self.suggest_constraint(
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 added this to avoid losing a helpful suggestion in the impl-trait-return-missing-constraint test.

I wonder how much else of the expected_projection function makes sense to run in this case (when the projection type is found) as well.

= note: expected struct `Box<(dyn for<'r> Fn(&'r (dyn ToString + 'r)) -> impl Debug + 'static)>`
found struct `Box<(dyn for<'r> Fn(&'r (dyn ToString + 'r)) -> Option<String> + 'static)>`
= note: expected struct `Box<(dyn for<'r> Fn(&'r (dyn ToString + 'r)) -> Option<String> + 'static)>`
found struct `Box<(dyn for<'r> Fn(&'r (dyn ToString + 'r)) -> impl Debug + 'static)>`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change looks like it might be wrong to me. On the other hand I'm not sure why this test doesn't compile. Bar's Iterator impl defines its Item to be FooItem, but the compiler complains Bar doesn't satisfy impl Iterator<Item = FooItem> because the Item type is wrong? It seems the compiler is being inconsistent about whether ham gets to see the true type of FooRet, or something.

At any rate this test is a complicated setup involving a nightly feature, so I don't think it's a big problem if this pr worsens the diagnostic in such a case.

@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 Jan 25, 2021
@bors
Copy link
Contributor

bors commented Feb 2, 2021

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

@bors
Copy link
Contributor

bors commented Feb 7, 2021

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

@bors
Copy link
Contributor

bors commented Feb 18, 2021

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

@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 Mar 7, 2021
@bors
Copy link
Contributor

bors commented Mar 16, 2021

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

@SNCPlay42 SNCPlay42 force-pushed the opaque-projections branch from d666f6e to 525e23a Compare March 16, 2021 16:56
@bstrie bstrie 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 Mar 31, 2021
@JohnTitor
Copy link
Member

r? @estebank as per GitHub suggestions

@estebank
Copy link
Contributor

estebank commented Apr 1, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Apr 1, 2021

📌 Commit 525e23a 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 Apr 1, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 1, 2021
…ebank

Fix expected/found order on impl trait projection mismatch error

fixes rust-lang#68561

This PR adds a new `ObligationCauseCode` used when checking the concrete type of an impl trait satisfies its bounds, and checks for that cause code in the existing test to see if a projection's normalized type should be the "expected" or "found" type.

The second commit adds a `peel_derives` to that test, which appears to be necessary in some cases (see projection-mismatch-in-impl-where-clause.rs, which would still give expected/found in the wrong order otherwise). This caused some other changes in diagnostics not involving impl trait, but they look correct to me.
@bors
Copy link
Contributor

bors commented Apr 2, 2021

⌛ Testing commit 525e23a with merge 4fa76a4...

@bors
Copy link
Contributor

bors commented Apr 2, 2021

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 4fa76a4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 2, 2021
@bors bors merged commit 4fa76a4 into rust-lang:master Apr 2, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 2, 2021
@SNCPlay42 SNCPlay42 deleted the opaque-projections branch April 18, 2021 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

impl Trait error message expected/found order are confusing
9 participants