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.

[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

[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

[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.

[Unique-Usman]

Hello @estebank and @mejrs I started a t-lang zulip thread on this.

I already tagged you two on the thread. Do take a look.

https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/RFC.20diagnostic.3A.3Aon_type_error.20filtering.20semantics.20and.20syntax/with/590349794

Thanks

[mejrs]

@estebank, @weiznich and I discussed this at the All Hands.

We decided that we'd like to go ahead with @estebank's proposed semantics, so the diagnostic is displayed "both ways".

Our general thoughts are along these lines:

• We are concerned about ambiguity, so we want an explicit mechanism for declaring the "direction" which is blocked on diagnostic filters.
• In the meantime we'd like to have this "out there" on nightly but not stabilize it until diagnostic filters are available (which is what I am working on).
• Once we have filters, we change the attribute to always require a filter.

[rustbot]

Some changes occurred to diagnostic attributes.

cc @mejrs

[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.

[mejrs]

Thanks for getting back to this. I'm quite looking forward to it.

I have some comments on the attribute part of this. I've asked @estebank to do a review of how it hooks into the error reporting, so he should get back to you for the rest.

View changes since this review

[mejrs]

you can delete this arm, they should just fallthrough to the _other arm at the bottom

[Unique-Usman]

True. Will do.

[mejrs]

Suggested change

	#[diagnostic::on_type_error(note = "custom on_type_error note: test coelesce")]
	#[diagnostic::on_type_error(note = "custom on_type_error note: test coalesce")]

Same for the rest of this file. Also these messages get de duplicated, so you can't tell whether they coalesced and were de duplicated, or whether one got through and the other got ignored. You'd have to make the messages different to test that.

[mejrs]

You have to target check here, otherwise you can't warn if someone uses this on a macro invocation. Like this:

rust/compiler/rustc_attr_parsing/src/attributes/diagnostic/on_move.rs

Lines 30 to 32 in f3ef3bd

	if !matches!(cx.target, Target::Enum | Target::Struct | Target::Union) {
	cx.emit_lint(MISPLACED_DIAGNOSTIC_ATTRIBUTES, DiagnosticOnMoveOnlyForAdt, span);
	return;

The generic param check has to stay in check_attr, it can't be done here.

[Unique-Usman]

@mejrs, thanks for the review. I got it made the changes.