Skip to content

recurse into refs when comparing tys for diagnostics #118730

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Dec 9, 2023

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Dec 8, 2023

before:
image

after:
image

this diff from the test suite is also quite nice imo:

@@ -4,8 +4,8 @@ error[E0308]: mismatched types
 LL |     debug_assert_eq!(iter.next(), Some(value));
    |                                   ^^^^^^^^^^^ expected `Option<<I as Iterator>::Item>`, found `Option<&<I as Iterator>::Item>`
    |
-   = note: expected enum `Option<<I as Iterator>::Item>`
-              found enum `Option<&<I as Iterator>::Item>`
+   = note: expected enum `Option<_>`
+              found enum `Option<&_>`

@rustbot
Copy link
Collaborator

rustbot commented Dec 8, 2023

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 8, 2023
@jyn514
Copy link
Member Author

jyn514 commented Dec 8, 2023

note that the decision of whether to fully elide the type as _ is inconsistent right now - it never happens for functions, fn pointers, tuples, refs, or adts, but it does happen for primitives and ADTs behind a raw pointer. that is probably not intended? might be worth moving the t1 == t2 comparison up above the big match for consistency.

if t1 == t2 && !self.tcx.sess.verbose() {
// The two types are the same, elide and don't highlight.
(DiagnosticStyledString::normal("_"), DiagnosticStyledString::normal("_"))

Comment on lines +405 to +406
= note: expected reference `&Foo<_, B>`
found struct `Foo<_, A>`
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! Interesting that we have a likely improvement here!

@@ -4,8 +4,8 @@ error[E0308]: mismatched types
LL | debug_assert_eq!(iter.next(), Some(value));
| ^^^^^^^^^^^ expected `Option<<I as Iterator>::Item>`, found `Option<&<I as Iterator>::Item>`
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like maybe we should use the expected/found ones now instead for the label, as they end up being significantly shorter.

Comment on lines +12 to +13
= note: expected reference `&_`
found type parameter `_`
Copy link
Member

Choose a reason for hiding this comment

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

I wish we didn't use _ as a placeholder here, but whatever. This is still pretty helpful.

Copy link
Member Author

@jyn514 jyn514 Dec 8, 2023

Choose a reason for hiding this comment

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

@compiler-errors
Copy link
Member

This makes sense. Thanks for the PR, @jyn514.

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Dec 8, 2023

📌 Commit eb53721 has been approved by compiler-errors

It is now in the queue for this repository.

@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 Dec 8, 2023
@compiler-errors
Copy link
Member

@bors r=estebank,compiler-errors

@bors
Copy link
Collaborator

bors commented Dec 8, 2023

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Collaborator

bors commented Dec 8, 2023

📌 Commit eb53721 has been approved by estebank,compiler-errors

It is now in the queue for this repository.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 8, 2023
…er-errors

recurse into refs when comparing tys for diagnostics

before:
![image](https://github.com/rust-lang/rust/assets/23638587/bf6abd62-c7f3-4c09-a47e-31b6e129de19)

after:
![image](https://github.com/rust-lang/rust/assets/23638587/b704d728-ddba-4204-aebe-c07dcbbcb55c)

this diff from the test suite is also quite nice imo:
```diff
`@@` -4,8 +4,8 `@@` error[E0308]: mismatched types
 LL |     debug_assert_eq!(iter.next(), Some(value));
    |                                   ^^^^^^^^^^^ expected `Option<<I as Iterator>::Item>`, found `Option<&<I as Iterator>::Item>`
    |
-   = note: expected enum `Option<<I as Iterator>::Item>`
-              found enum `Option<&<I as Iterator>::Item>`
+   = note: expected enum `Option<_>`
+              found enum `Option<&_>`
```
@estebank
Copy link
Contributor

estebank commented Dec 8, 2023

Future work: highlight the relevant part of trait objects and impl Traits.

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 8, 2023
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#117586 (Uplift the (new solver) canonicalizer into `rustc_next_trait_solver`)
 - rust-lang#118692 (remove redundant imports)
 - rust-lang#118694 (Add instance evaluation and methods to read an allocation in StableMIR)
 - rust-lang#118715 (privacy: visit trait def id of projections)
 - rust-lang#118730 (recurse into refs when comparing tys for diagnostics)
 - rust-lang#118736 (temporarily revert "ice on ambguity in mir typeck")

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2023
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#117586 (Uplift the (new solver) canonicalizer into `rustc_next_trait_solver`)
 - rust-lang#118502 (fix: correct the arg for 'suggest to use associated function syntax' diagnostic)
 - rust-lang#118694 (Add instance evaluation and methods to read an allocation in StableMIR)
 - rust-lang#118715 (privacy: visit trait def id of projections)
 - rust-lang#118730 (recurse into refs when comparing tys for diagnostics)
 - rust-lang#118736 (temporarily revert "ice on ambguity in mir typeck")

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 943fa33 into rust-lang:master Dec 9, 2023
@rustbot rustbot added this to the 1.76.0 milestone Dec 9, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2023
Rollup merge of rust-lang#118730 - jyn514:cmp_refs, r=estebank,compiler-errors

recurse into refs when comparing tys for diagnostics

before:
![image](https://github.com/rust-lang/rust/assets/23638587/bf6abd62-c7f3-4c09-a47e-31b6e129de19)

after:
![image](https://github.com/rust-lang/rust/assets/23638587/b704d728-ddba-4204-aebe-c07dcbbcb55c)

this diff from the test suite is also quite nice imo:
```diff
`@@` -4,8 +4,8 `@@` error[E0308]: mismatched types
 LL |     debug_assert_eq!(iter.next(), Some(value));
    |                                   ^^^^^^^^^^^ expected `Option<<I as Iterator>::Item>`, found `Option<&<I as Iterator>::Item>`
    |
-   = note: expected enum `Option<<I as Iterator>::Item>`
-              found enum `Option<&<I as Iterator>::Item>`
+   = note: expected enum `Option<_>`
+              found enum `Option<&_>`
```
@jyn514 jyn514 deleted the cmp_refs branch December 14, 2023 16:00
# for free to join this conversation on GitHub. Already have an account? # 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.

6 participants