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

consistently elide/don't elide types in diagnostics #118731

Open
jyn514 opened this issue Dec 8, 2023 · 4 comments
Open

consistently elide/don't elide types in diagnostics #118731

jyn514 opened this issue Dec 8, 2023 · 4 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Dec 8, 2023

right now, the decision of whether to fully elide the type as _ is inconsistent - it never happens for functions, fn pointers, tuples, refs where the pointee types are equal, 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("_"))

here is an example of it being inconsistent:

LL | fn say(self: &Pair<&str, isize>) {
| ^^^^^^^^^^^^^^^^^^ lifetime mismatch
|
= note: expected struct `Pair<&str, _>`

isize is elided but str is not.

i originally changed this to almost always elide the type, but @estebank is worried that will be confusing:
image
he suggested instead eliding if the type has either type params or projection types ("looks complicated", basically).

Originally posted by @jyn514 in #118730 (comment)

@rustbot label +A-diagnostics +E-medium

@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. A-diagnostics Area: Messages for errors, warnings, and lints E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Dec 8, 2023
@Noratrieb Noratrieb added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Dec 15, 2023
@Liewyec
Copy link
Contributor

Liewyec commented Jan 23, 2024

Hello, I would like to look at this. Is there something I need to be aware of?

@estebank
Copy link
Contributor

@Liewyec take a look at the conversation in the linked PR. The main thing to be careful with here is taking a look at the changes to the .stderr files after running python x.py test tests/ui --bless to verify that you're not accidentally removing useful information. In general, _ should only be shown for types that are not interesting or useful for the user, but for cases like Ty<'a>/Box<Ty<'a>, the lifetimes aren't useful, but knowing that "I have a type, but the same type must be boxed" is easier to understand if the (short) type Ty<'_> here is shown, instead of _/Box<_> which can make people take longer than strictly necessary to understand what the compiler is saying.

@Liewyec
Copy link
Contributor

Liewyec commented Jan 23, 2024

all right it seems simple enough, I will do this

@estebank
Copy link
Contributor

Be aware that by the nature of this part of the codebase (free-form tree comparisons), the logic can be daunting. Try to understand each chunk independently, particularly by getting a high level understanding first and then digging in, to avoid getting overwhelmed. It's not hard, just a lot of state to hold in your head and can take some time to build up that context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants