Tracking issue for RFC 2397, "#[do_not_recommend]"

[Centril]

This is a tracking issue for the RFC "Introduce #[do_not_recommend] to control errors for trait impls" (rust-lang/rfcs#2397).

Steps:

• Implement the RFC (cc @rust-lang/compiler -- can anyone write up mentoring instructions?)
• Adjust documentation (see instructions on forge)
• Stabilization PR (see instructions on forge)

Unresolved questions:

• What other names could we go with besides #[do_not_recommend]? (See RFC for some suggestions)

implementation history:

• Actually use the #[do_not_recommend] attribute if present #124708
• Refactor #[diagnostic::do_not_recommend] support #125717
• Move #[do_not_recommend] to the #[diagnostic] namespace #125326
• Allows #[diagnostic::do_not_recommend] to supress trait impls in suggestions as well #127598

[nagisa]

Minor question. Presuming we implement this, and later change diagnostics in ways that make this attribute unnecessary/harmful, what would the plan of action be then?

Does this attribute essentially encode some particular format of these specific errors?

[Centril]

@nagisa in that hypothetical situation, deprecate the attribute perhaps?

[scottmcm]

This came up in T-lang's backlog bonanza today. With no progress on this for years, it's not clear to us that this is still the right way forward here.

@estebank, do you have any thoughts on this one? Perhaps it would be better rolled into the idea you brought up in https://internals.rust-lang.org/t/stable-diagnostic-affecting-attribute-with-unstable-api/16368?u=scottmcm?

[c410-f3r]

@rustbot claim

[estebank]

If the proposal for #[diagnostics:*] is indeed accepted (which I'm confident now it will be), I strongly believe this should live under that namespace.

[weiznich]

@estebank It should be fine to implement this as unstable feature without waiting for the #[diagnostics] RFC to be accepted. I would guess that the attribute can be moved afterwards easily, as long as it is not part of any stable release yet?

[estebank]

@weiznich that's my understanding too.

[alice-i-cecile]

Now that #119888 is merged, what's the next step forward for this?

[c410-f3r]

This feature had to be temporarily paused due to concerns around possible refactors and the new solver. https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.E2.9C.94.20RFC-2397

If @lcnr thinks that the new solver is in a good state, then I can resume my activities with or without the diagnostic namespace.

[lcnr]

Sorry for taking so long to reply to this issue.

The solver is not yet in a good enough state to support work on that attribute. However, looking at the old solver again, we already track where nested obligations come from:

rust/compiler/rustc_trait_selection/src/traits/select/mod.rs

Lines 2755 to 2783 in 8d490e3

	for (index, (predicate, span)) in predicates.into_iter().enumerate() {
	let cause =
	if Some(parent_trait_pred.def_id()) == tcx.lang_items().coerce_unsized_trait() {
	cause.clone()
	} else {
	cause.clone().derived_cause(parent_trait_pred, |derived| {
	ImplDerivedObligation(Box::new(ImplDerivedObligationCause {
	derived,
	impl_or_alias_def_id: def_id,
	impl_def_predicate_index: Some(index),
	span,
	}))
	})
	};
	let clause = normalize_with_depth_to(
	self,
	param_env,
	cause.clone(),
	recursion_depth,
	predicate,
	&mut obligations,
	);
	obligations.push(Obligation {
	cause,
	recursion_depth,
	param_env,
	predicate: clause.as_predicate(),
	});
	}

This info could be used to implement it in the old solver without requiring any additional tracking: if the cause of an FUlfillmentError contains a impl_or_alias_def_id with the #[diagnostics::do_not_recommend] attribute, simply use the root obligation. This isn't perfect, as #[do_not_recommend] hides non-root parent goals we may actually have wanted to recommend.

It will take at least another few months until the "proof tree visitor" in the new solver is at a state where it can be used for diagnostics. Right now we still have to change that code to fix the happy path behavior and I don't want to slow down progress on this by having to migrate diagnostics code each time we do so.

[c410-f3r]

Thank you @lcnr