Improve generic parameters handling for #[diagnostic::on_const]

[rperier]

View all comments

cc #155570

This Improves generic parameters handling for #[diagnostic::on_const]
This is done by printing generics parameters when these are concrete Adt generics args.
A lint is emitted we try to format a generic that is found anywhere.
Moreover, generics parameters that are ty infer vars are no longer displayed
"as-is", it is not clear and might be confusing, in this specific case the
name of the generic parameter will be displayed instead.

I will need some help for displaying the Zparameter correctly. From what I have understood, it is an ty::Infer(ty::TyVar(_)) that is not correctly displayed via self.tcx.short_string() (in on_unimplemented_components()), or even not correctly inferred via self.infcx.ty_to_string().

EDIT: general descrition updated

[rustbot]

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

[rustbot]

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 73 candidates
• Random selection from 20 candidates

[rperier]

cc @mejrs

[mejrs]

r? me

This is a first proposal.

Can you explain this in words? We need to stabilize this at some point so we need to have this written down. There are three places that can have generics; the impl header, the trait and the type. And what about blanket impls?

For now it would be best if these rules are explained in the unstable book entry.

I will need some help for displaying the Z parameter correctly.

The thing is that I don't know myself what I'd expect 😆. I think I don't like _, but is it right to just print Z? Or is there some syntax to indicate that it's a non inferred type? Maybe like <Z>? 🤔 . I'm very open to ideas here. It's also an option to just lint against it and not display it at all.

View changes since this review

[rperier]

The thing is that I don't know myself what I'd expect 😆. I think I don't like _, but is it right to just print Z? Or is there some syntax to indicate that it's a non inferred type? Maybe like <Z>? 🤔 . I'm very open to ideas here. It's also an option to just lint against it and not display it at all.

Lint against it is a good idea, because it's verbose, you're probably not able to display the value for this generic parameter, so explaining why makes sense. Displaying _ as value is confusing, so I would display <Z>instead + lint against it. We cannot really "not display it at all", as it is already in the note field of the directive attribute (in the worst case it won't be expansed and left as is, in place).

My problem with the lint is that we should emit it from check_diagnostic_on_const (from rustc_passes), right? At this stage, we don't have much more informations about the GenericParam (in the sense of what it is really, inferred or not etc...), don't we ?

[mejrs]

IMO just printing Z is the least unsurprising behavior. I think in practice it boils down to how people use these attributes and what the error messages end up looking like.

[rperier]

By "just printing Z" , you mean Z literrally or the value of Z ?

So the compiler output could be :

= note: Self = X<u8, &str>, Z = Z, A = u8, B = &str

or

= note: Self = X<u8, &str>, Z = _, A = u8, B = &str

plus a lint on the diagnostic attribute, highlighting the Z argument, explaining why it cannot be displayed. Without any lints, it could be confusing.

[mejrs]

By "just printing Z" , you mean Z literrally or the value of Z ?

We have been printing it literally for e.g. diagnostic::on_unimplemented if the generic isn't actually anywhere on the trait.

[rperier]

see #t-compiler/help > Ty of kind ty::Infer(ty::TyVar) from rustc_passes (#158347)

[rperier]

For now the lint part is under discussion on zulip (see my previous message).

[rperier]

I have thought about it, I am taking a step back, I think we could do :

1. Don't display Z at all, i.e remove it from the message, label and note attributes
2. Replace Z=_ with Z=<Z>
3. Leave Z=_ as is

1 and 2 are technically feasible, but are confusing for the user (he asks to display a generic parameter that is not shown). Lint against it from rustc_passes implies more work, as we don't know the right type informations at this stage. To be honest, 3. is not a problem, the type is not infered , so it is displayed as _ .

[mejrs]

In the absence of any convincing option, I think it would be best to just display Z. If someone pops up with a good argument for something else we can change it.

[rperier]

We could display Z and... emit a lint from on_unimplemented_component (rustc_trait_selection) ? because at this stage we have all informations we need (which is not the case in rustc_passes) :)

[mejrs]

We could display Z and... emit a lint from on_unimplemented_component (rustc_trait_selection) ? because at this stage we have all informations we need (which is not the case in rustc_passes) :)

on_unimplemented_component is called at error reporting time, which is when the user's code is compiled; not when the attribute is declared.

Also the user might write something like

#![feature(diagnostic_on_const, const_trait_impl)]

pub struct X<A, B> {
    field: (A, B),
}

pub const trait Y<Z> {
    fn blah(&self) {}
}

#[diagnostic::on_const(note = "Self = {Self}, Z = {Z}, A = {A}, B = {B}")]
impl<Z, A, B> Y<Z> for X<A, B> {}

const _: () = {
    Y::<u8>::blah(&X {
        field: (42_u8, "hello"),
    });
};

where there is a concrete type for Z. So we cannot know when the library (with the attribute) is compiled whether or not the generic parameter will have a concrete type or not.

[rperier]

I am opened to suggestions and willing to help if there is anything else

[mejrs]

The impl looks good to me, just some things need to be done w/r/t tests

• can you add a test that shows the OnConstMalformedFormatLiterals lint?
• the test is named report_warning_missing_format_paramaters.rs, can you rename it to something appropriate? (also, notice the typo)
• can you add revisions for current and next solver, see https://github.com/rust-lang/rust/blob/main/tests/ui/diagnostic_namespace/do_not_recommend/nested.rs for example

@rustbot author

View changes since this review