Introduce #[diagnostic::on_type_error(message)]

[Unique-Usman]

View all comments

[rustbot]

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

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

[rustbot]

r? @davidtwco

rustbot has assigned @davidtwco.
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 69 candidates
• Random selection from 12 candidates

[Unique-Usman]

r? estebank

[mejrs]

Before you sink more time and effort into this you should explain what you want on_type_error to do.

One expectation I have is that it can specialize for different types. For example if you put this attribute on struct A but the typeerror finds a B or C instead you'll want to emit different messages for them, similar to how the filtering in #[rustc_on_unimplemented] works. But implementing that properly is actually a lot of work and effort, and requires significant changes to rustc.

I'm not convinced this attribute can be implemented in a satisfying way at this time. I'm open to being convinced otherwise though, and am happy to hear your thoughts.

[estebank]

@mejrs two things: I agree that we want filtering in the same way as rustc_on_unimplemented, and we need the same kind of filtering for diagnostic::on_unimplemented that we don't have today. It should work exactly the same way. I believe that we can get away with a similar (but less powerful) version of what the rustc_ attr provides. Minimally, for now, I think we can get away with supporting the following:

#[diagnostic::on_type_error(
    note = "text",
)]
struct S<T>(T);

with an eye for something along the lines of

#[diagnostic::on_type_error(
    on(expected="Self", found="crate::K", T = "i32", note = "a"),
    on(expected="crate::K", found="Self", note = "b"),
    note = "c",
)]
struct S<T>(T);

some time later.

I believe that having the minimal functionality at least lets crate authors include the "filtering information" in the text itself ("if this is the found type, then..." or "if type parameter T is blah, ..."), and that there are already several cases in the std of unconditional addition of notes during error reporting for given types, so I think the minimal functionality is already adding value.

For the filtering I would like to share the same parser between on_type_error and on_unimplemented, as it is effectively the same functionality.

[estebank]

View changes since the review

as discussed we shouldn't do this now, only deal with the simple case that can be gleaned from only the expected/found mismatch.

[mejrs]

Did you two have a discussion about this beforehand? I'd like to read it.

that there are already several cases in the std of unconditional addition of notes during error reporting for given types

I presume you are talking about this?

rust/compiler/rustc_hir_typeck/src/method/suggest.rs

Lines 3156 to 3345 in e8e4541

	fn suggest_unwrapping_inner_self(
	&self,
	err: &mut Diag<'_>,
	source: SelfSource<'tcx>,
	actual: Ty<'tcx>,
	item_name: Ident,
	) {
	let tcx = self.tcx;
	let SelfSource::MethodCall(expr) = source else {
	return;
	};
	let call_expr = tcx.hir_expect_expr(tcx.parent_hir_id(expr.hir_id));
	let ty::Adt(kind, args) = actual.kind() else {
	return;
	};
	match kind.adt_kind() {
	ty::AdtKind::Enum => {
	let matching_variants: Vec<_> = kind
	.variants()
	.iter()
	.flat_map(|variant| {
	let [field] = &variant.fields.raw[..] else {
	return None;
	};
	let field_ty = field.ty(tcx, args);
	// Skip `_`, since that'll just lead to ambiguity.
	if self.resolve_vars_if_possible(field_ty).is_ty_var() {
	return None;
	}
	self.lookup_probe_for_diagnostic(
	item_name,
	field_ty,
	call_expr,
	ProbeScope::TraitsInScope,
	None,
	)
	.ok()
	.map(|pick| (variant, field, pick))
	})
	.collect();
	let ret_ty_matches = |diagnostic_item| {
	if let Some(ret_ty) = self
	.ret_coercion
	.as_ref()
	.map(|c| self.resolve_vars_if_possible(c.borrow().expected_ty()))
	&& let ty::Adt(kind, _) = ret_ty.kind()
	&& tcx.get_diagnostic_item(diagnostic_item) == Some(kind.did())
	{
	true
	} else {
	false
	}
	};
	match &matching_variants[..] {
	[(_, field, pick)] => {
	let self_ty = field.ty(tcx, args);
	err.span_note(
	tcx.def_span(pick.item.def_id),
	format!("the method `{item_name}` exists on the type `{self_ty}`"),
	);
	let (article, kind, variant, question) = if tcx.is_diagnostic_item(sym::Result, kind.did())
	// Do not suggest `.expect()` in const context where it's not available. rust-lang/rust#149316
	&& !tcx.hir_is_inside_const_context(expr.hir_id)
	{
	("a", "Result", "Err", ret_ty_matches(sym::Result))
	} else if tcx.is_diagnostic_item(sym::Option, kind.did()) {
	("an", "Option", "None", ret_ty_matches(sym::Option))
	} else {
	return;
	};
	if question {
	err.span_suggestion_verbose(
	expr.span.shrink_to_hi(),
	format!(
	"use the `?` operator to extract the `{self_ty}` value, propagating \
	{article} `{kind}::{variant}` value to the caller"
	),
	"?",
	Applicability::MachineApplicable,
	);
	} else {
	err.span_suggestion_verbose(
	expr.span.shrink_to_hi(),
	format!(
	"consider using `{kind}::expect` to unwrap the `{self_ty}` value, \
	panicking if the value is {article} `{kind}::{variant}`"
	),
	".expect(\"REASON\")",
	Applicability::HasPlaceholders,
	);
	}
	}
	// FIXME(compiler-errors): Support suggestions for other matching enum variants
	_ => {}
	}
	}
	// Target wrapper types - types that wrap or pretend to wrap another type,
	// perhaps this inner type is meant to be called?
	ty::AdtKind::Struct | ty::AdtKind::Union => {
	let [first] = ***args else {
	return;
	};
	let ty::GenericArgKind::Type(ty) = first.kind() else {
	return;
	};
	let Ok(pick) = self.lookup_probe_for_diagnostic(
	item_name,
	ty,
	call_expr,
	ProbeScope::TraitsInScope,
	None,
	) else {
	return;
	};
	let name = self.ty_to_value_string(actual);
	let inner_id = kind.did();
	let mutable = if let Some(AutorefOrPtrAdjustment::Autoref { mutbl, .. }) =
	pick.autoref_or_ptr_adjustment
	{
	Some(mutbl)
	} else {
	None
	};
	if tcx.is_diagnostic_item(sym::LocalKey, inner_id) {
	err.help("use `with` or `try_with` to access thread local storage");
	} else if tcx.is_lang_item(kind.did(), LangItem::MaybeUninit) {
	err.help(format!(
	"if this `{name}` has been initialized, \
	use one of the `assume_init` methods to access the inner value"
	));
	} else if tcx.is_diagnostic_item(sym::RefCell, inner_id) {
	let (suggestion, borrow_kind, panic_if) = match mutable {
	Some(Mutability::Not) => (".borrow()", "borrow", "a mutable borrow exists"),
	Some(Mutability::Mut) => {
	(".borrow_mut()", "mutably borrow", "any borrows exist")
	}
	None => return,
	};
	err.span_suggestion_verbose(
	expr.span.shrink_to_hi(),
	format!(
	"use `{suggestion}` to {borrow_kind} the `{ty}`, \
	panicking if {panic_if}"
	),
	suggestion,
	Applicability::MaybeIncorrect,
	);
	} else if tcx.is_diagnostic_item(sym::Mutex, inner_id) {
	err.span_suggestion_verbose(
	expr.span.shrink_to_hi(),
	format!(
	"use `.lock().unwrap()` to borrow the `{ty}`, \
	blocking the current thread until it can be acquired"
	),
	".lock().unwrap()",
	Applicability::MaybeIncorrect,
	);
	} else if tcx.is_diagnostic_item(sym::RwLock, inner_id) {
	let (suggestion, borrow_kind) = match mutable {
	Some(Mutability::Not) => (".read().unwrap()", "borrow"),
	Some(Mutability::Mut) => (".write().unwrap()", "mutably borrow"),
	None => return,
	};
	err.span_suggestion_verbose(
	expr.span.shrink_to_hi(),
	format!(
	"use `{suggestion}` to {borrow_kind} the `{ty}`, \
	blocking the current thread until it can be acquired"
	),
	suggestion,
	Applicability::MaybeIncorrect,
	);
	} else {
	return;
	};
	err.span_note(
	tcx.def_span(pick.item.def_id),
	format!("the method `{item_name}` exists on the type `{ty}`"),
	);
	}
	}
	}

The bit about unwrapping Option/Result is actually really impactful for learners and the way you propose the attribute it should be able to do that sort of thing (minus the inline code suggestions, obviously).

So 💯 from me.

Let's go for a minimal version for now, like estebank said:

#[diagnostic::on_type_error(
    note = "text",
)]
struct S<T>(T);

I'm a bit worried about this thing being too powerful and showing up in places where authors didn't quite expect or intend. There are after all quite a few ways and places to get type errors.

I think the following semantics would be reasonably useful but also restrictive enough to not spook me or the lang people.

• only note is supported (not message or label)
• no filtering (on) of any kind, that's a whole design space we should get into elsewhere
• the annotated ADT must have exactly one generic type parameter
• if the annotated type (here, S) is found but the type parameter type (here, T) is expected, then the note is displayed.
• let's also do that for "S has no method named foo but T does".

Then in the future, when we evolve the attribute forwards, we can specify something like "if you dont supply any filter then this wrapper kind of thing is all it can do".

[Unique-Usman]

Did you two have a discussion about this beforehand? I'd like to read it.

that there are already several cases in the std of unconditional addition of notes during error reporting for given types

There was a discussion initally, it was through video meet though, but, nothing much more than whatever is here actually.

I presume you are talking about this?

I think the following semantics would be reasonably useful but also restrictive enough to not spook me or the lang people.

• only note is supported (not message or label)
• no filtering (on) of any kind, that's a whole design space we should get into elsewhere
• the annotated ADT must have exactly one generic type parameter
• if the annotated type (here, S) is found but the type parameter type (here, T) is expected, then the note is displayed.
• let's also do that for "S has no method named foo but T does".

Then in the future, when we evolve the attribute forwards, we can specify something like "if you dont supply any filter then this wrapper kind of thing is all it can do".

Agreed.

[rustbot]

Some changes occurred to diagnostic attributes.

cc @mejrs

Some changes occurred in src/tools/cargo

cc @ehuss

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Unique-Usman]

should this be exactly one ?

View changes since the review

[mejrs]

Yes please :) Also can you rename the error type...to something like NotExactlyOneGeneric perhaps?

[Unique-Usman]

will do.

[Unique-Usman]

should we check both found and expected or just one or based on some condition ?

View changes since the review

[mejrs]

Let's just do

if the annotated type (here, S) is found but the type parameter type (here, T) is expected, then the note is displayed.

for now, and not the other way around. Please also add a test for this showing that it does nothing.

[Unique-Usman]

to get understand this better, this two is applicable, i.e this feature should be applied only when the annotated type is the found and not the expected which correctly apply for this two instances?

#![feature(diagnostic_on_type_error)]

#[diagnostic::on_type_error(note = "expected {Expected}, found {Found}")]
//~^ WARN unknown diagnostic attribute
#[derive(Debug)]
struct Foo<T>(T);

fn takes_foo(_: Foo<i32>) {}

fn main() {
    let foo = Foo(String::new());
    takes_foo(foo);
    //~^ERROR mismatched types
}

and this

#![feature(diagnostic_on_type_error)]

#[diagnostic::on_type_error(note = "expected {Expected}, found {Found}")]
//~^ WARN unknown diagnostic attribute
#[derive(Debug)]
struct Foo<T>(T);

fn takes_foo(_: String) {}

fn main() {
    let foo = Foo(String::new());
    takes_foo(foo);
    //~^ERROR mismatched types
}

[mejrs]

The first example doesn't really apply to this? It compares Foo<i32> and Foo<String>, we definitely don't want that (at least not now).

This goes back to what I said about:

showing up in places where authors didn't quite expect or intend

It's important that the note only goes "one way", because people will write things like this without thinking about it very much:

#[diagnostic::on_type_error(note = "call unwrap() to get the inner value")]
enum MyOption<T> {
    Some(T),
    None,
}

But users will write code like this:

fn take_myoption_u8(_: MyOption<u8>) {}

take_myoption_u8(12_u8);

With your current implementation they'll get a note saying that you can unwrap a u8 to get an option. Which is definitely wrong. That advice is only correct if you have a MyOption<u8> but you need a u8. This is why I think we should only show the note if it goes "inward".

[Unique-Usman]

I think allowing Found and Expected is a good idea thoughts ?

View changes since the review

[mejrs]

Yeah that's a good idea.

[Unique-Usman]

@estebank @mejrs

[mejrs]

General feedback regarding the approach:

• Please do not use the generics machinery for Found and Expected. Take a look at how rustc_on_unimplemented's This works; it can be parsed similarly (grep for sym::This), and you can supply it by making new field(s) in the FormatArgs struct.
• I'm on the fence on allowing {Self}. It feels like one of those things where you think you know what it does but you might be wrong. That's what I like about Found and Expected as well; it forces you to consider what you actually want. If it's useful we can also enable This to refer unambiguously to the annotated item, like for rustc_on_unimplemented. Thoughts?
• In your tests you have a lot of strings like note = "expected {Expected}, found {Found}", but at a glance it's really hard to tell if this is emitted by the compiler somewhere or if it's from this attribute. Please change these to something that unambiguously looks "testy".
• We talked about implementing this for method calls as well. It's up to you if you implement that here (can be a follow up PR), but either way please add some tests now to document the current behavior.
• More testing in general:
• • cross crate use
• • lifetime and const generics.
• • check that Expected and Found is rejected for other diagnostic attributes
• • duplicate uses of the attribute on the same item (these should coalesce).

So far I haven't looked too deep at how this hooks into the existing diagnostics machinery as I'm not familiar with it. That's more @estebank's area of expertise anyway. Maybe I'll have more review about that later.

View changes since this review

[mejrs]

Suggested change

	(Mode::DiagnosticOnTypeError, sym::message)
	| (Mode::DiagnosticOnTypeError, sym::label) => {
	malformed!()
	}

Keep in mind that the complexity of this entire match will grow over time as more attributes and options get added. Can you instead change the existing arms to match on the diagnostic attributes that do allow message and label?

With that, I mean:

        match (mode, name) {
            (Mode::DiagnosticOnConst | Mode::DiagnosticOnMove | /* etc */ , sym::message) => {
                   // ...
            }

etc.

While it is more wordy, it is easier to understand from a control flow perspective. You can also do use Mode::*; inside this function to use the enum variant names by themselves.

[mejrs]

Suggested change

tracing::info!("borrow checker");

if it's useful to you then leave it for now, but it should be removed at some point

[Unique-Usman]

it is noise, will remove it.

[mejrs]

This lint feels like an overly specific case of "you're using the attribute wrong". I guess we don't really have a specific lint for that, maybe MALFORMED_DIAGNOSTIC_ATTRIBUTES?

It also looks like it isn't going to age well when we start expanding what on_type_error can do.

I propose we drop the lint and just use MALFORMED_DIAGNOSTIC_ATTRIBUTES for now. Adding a more general "you're using it wrong" lint is out of scope of this PR and also requires lang team discussion etc, so let's not do that here.

[Unique-Usman]

Yeah, that is valid. Will remove.

[mejrs]

Suggested change

	Expected, // used for formatting for diagnostic::on_type_error for diagnostic purposes
	Expected,

There's no need to explain this, people can just grep sym::Expected if they're curious. Same for Found below.

[mejrs]

The order does matter; this will cause instability in the test suite. The stderr output will randomly change and it will greatly annoy someone.

We should pick a consistent order here. I'd just put them into a vec and check for duplicates. Also, we should pick whether we want the attribute's notes displayed above or below the compiler's. I think below makes most sense?

[Unique-Usman]

Yeah, I think there is also some data structure in the compiler that helps with ordering.

Attribute notes above could be better from the perspective that, I want the reader of the error to get the better and the specific notes which means the attribute note.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Unique-Usman]

General feedback regarding the approach:

• Please do not use the generics machinery for Found and Expected. Take a look at how rustc_on_unimplemented's This works; it can be parsed similarly (grep for sym::This), and you can supply it by making new field(s) in the FormatArgs struct.
  Noted.
• I'm on the fence on allowing {Self}. It feels like one of those things where you think you know what it does but you might be wrong. That's what I like about Found and Expected as well; it forces you to consider what you actually want. If it's useful we can also enable This to refer unambiguously to the annotated item, like for rustc_on_unimplemented. Thoughts?

Maybe we should just disallow Self as Found and Expected could refers to self but, better contextualize to self and as you said -> "it forces you to consider what you actually want."

• In your tests you have a lot of strings like note = "expected {Expected}, found {Found}", but at a glance it's really hard to tell if this is emitted by the compiler somewhere or if it's from this attribute. Please change these to something that unambiguously looks "testy".

Noted.

• We talked about implementing this for method calls as well. It's up to you if you implement that here (can be a follow up PR), but either way please add some tests now to document the current behavior.
  Yeah, I can have a followup pr for that.

• More testing in general:
• • cross crate use
• • lifetime and const generics.
• • check that Expected and Found is rejected for other diagnostic attributes
• • duplicate uses of the attribute on the same item (these should coalesce).

Noted.

So far I haven't looked too deep at how this hooks into the existing diagnostics machinery as I'm not familiar with it. That's more @estebank's area of expertise anyway. Maybe I'll have more review about that later.

View changes since this review

Noted. Thanks for the review.

[mejrs]

I didn't notice until now, but not all your tests have explicit NOTE annotations. Please add them.

[estebank]

Suggested change

	template!(List: &[r#" note = "...""#]),
	template!(List: &[r#"note = "...""#]),

View changes since the review

[rust-bors]

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