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

recurse into refs when comparing tys for diagnostics #118730

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
Contributor

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
Contributor

bors commented Dec 8, 2023

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

@bors
Copy link
Contributor

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
11 checks passed
@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
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.

6 participants