enable do_not_recommend attr for method call errors in current solver

[mejrs]

This should help with #146381 (comment)

The logic is mostly copied from the apply_do_not_recommend function.

r? @khyperia :3

[Kivooeo]

i'd chain this let

View changes since the review

[Kivooeo]

i think it should be possible to use while let instead of loop { if let }

View changes since the review

[khyperia]

Hmm... the method you're editing, report_no_match_method_error, is called by both solvers, not just the current solver. The difference seems to be that the caller, check_expr_method_call, calls self.lookup_method, and Err is returned, but the contents of the Err are different depending on the solver:

the old solver returns a bunch of stuff:

NoMatch(NoMatchData { static_candidates: [], unsatisfied_predicates: [(Binder { value: TraitPredicate(<u8 as DoNotMentionThis>, polarity:Positive), bound_vars: [] }, Some(Binder { value: TraitPredicate(<LocalType<u8> as std::clone::Clone>, polarity:Positive), bound_vars: [] }), Some(ObligationCause { span: tests/ui/diagnostic_namespace/do_not_recommend/method_call.rs:23:19: 23:24 (#0), body_id: DefId(0:8 ~ method_call[38bf]::main), code: ImplDerived(ImplDerivedCause { derived: DerivedCause { parent_trait_pred: Binder { value: TraitPredicate(<_ as std::clone::Clone>, polarity:Positive), bound_vars: [] }, parent_code: Misc }, impl_or_alias_def_id: DefId(0:9 ~ method_call[38bf]::{impl#0}), impl_def_predicate_index: Some(1), span: tests/ui/diagnostic_namespace/do_not_recommend/method_call.rs:11:9: 11:25 (#0) }) }))], out_of_scope_traits: [], similar_candidate: None, mode: MethodCall })

the new solver does not:

NoMatch(NoMatchData { static_candidates: [], unsatisfied_predicates: [], out_of_scope_traits: [], similar_candidate: None, mode: MethodCall })

figuring out where exactly the difference is is a bit annoying due to lack of tracing output related to errors - my guess is that the new solver is calling TypeErrCtxt::apply_do_not_recommend (or some other caller of TyCtxt::do_not_recommend_impl) somewhere and the old solver probably should too, but that's just a guess.

IMO, either both solvers should return #[diagnostic::do_not_recommend] things in unsatisfied_predicates, or neither should, and the error output should be the same across both solvers. I don't quite understand the context here though, so feel free to let me know if I'm missing something!

View changes since this review

[rustbot]

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

[mejrs]

figuring out where exactly the difference is is a bit annoying due to lack of tracing output related to errors - my guess is that the new solver is calling TypeErrCtxt::apply_do_not_recommend (or some other caller of TyCtxt::do_not_recommend_impl) somewhere and the old solver probably should too, but that's just a guess.

It appears the compiler handles UFCS and method call syntax differently, in the latterTypeErrCtxt::apply_do_not_recommend is never called, in neither solver, for the test added here.

It might be related to (see the revision output) that the next solver talkes about LocalType<T> but the current solver talks about LocalType<u8> 🤷‍♂️

I'm not sure what I can or should change about that, that seems like a much more involved change.

@rustbot ready

[khyperia]

in the latterTypeErrCtxt::apply_do_not_recommend is never called, in neither solver, for the test added here

Apologies for not being more specific! When I said

my guess is that the new solver is calling TypeErrCtxt::apply_do_not_recommend (or some other caller of TyCtxt::do_not_recommend_impl)

it was indeed some other caller of TyCtxt::do_not_recommend_impl that is being called in the new solver. Specifically, this one:

rust/compiler/rustc_trait_selection/src/solve/fulfill/derive_errors.rs

Lines 439 to 448 in 5e91de6

	// Don't walk into impls that have `do_not_recommend`.
	if let inspect::ProbeKind::TraitCandidate {
	source: CandidateSource::Impl(impl_def_id),
	result: _,
	} = candidate.kind()
	&& tcx.do_not_recommend_impl(impl_def_id)
	{
	trace!("#[diagnostic::do_not_recommend] -> exit");
	return ControlFlow::Break(self.obligation.clone());
	}

We want to replicate that logic in the old solver: filter out impls that have #[diagnostic::do_not_recommend] when reporting unsatisfied_predicates. Specifically, my guess would be that we want to filter them out here:

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

Lines 2086 to 2097 in 5e91de6

	result = ProbeResult::NoMatch;
	if let Ok(Some(candidate)) = self.select_trait_candidate(trait_ref) {
	for nested_obligation in candidate.nested_obligations() {
	if !self.infcx.predicate_may_hold(&nested_obligation) {
	possibly_unsatisfied_predicates.push((
	self.resolve_vars_if_possible(nested_obligation.predicate),
	Some(self.resolve_vars_if_possible(obligation.predicate)),
	Some(nested_obligation.cause),
	));
	}
	}
	}

On a tangent, the only other caller of select_trait_candidate is here, which is also used in diagnostics, and my guess is that we almost certainly want to filter out #[diagnostic::do_not_recommend] impls here too:

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

Lines 1918 to 1925 in 5e91de6

	match self.select_trait_candidate(trait_ref) {
	Ok(Some(traits::ImplSource::UserDefined(ref impl_data))) => {
	// If only a single impl matches, make the error message point
	// to that impl.
	CandidateSource::Impl(impl_data.impl_def_id)
	}
	_ => CandidateSource::Trait(candidate.item.container_id(self.tcx)),
	}

As an example WIP, changing your PR to be this functional change instead:

diff --git a/compiler/rustc_hir_typeck/src/method/probe.rs b/compiler/rustc_hir_typeck/src/method/probe.rs
index dfb1dae10ae..f0798f484e3 100644
--- a/compiler/rustc_hir_typeck/src/method/probe.rs
+++ b/compiler/rustc_hir_typeck/src/method/probe.rs
@@ -1887,7 +1887,14 @@ fn select_trait_candidate(
     ) -> traits::SelectionResult<'tcx, traits::Selection<'tcx>> {
         let obligation =
             traits::Obligation::new(self.tcx, self.misc(self.span), self.param_env, trait_ref);
-        traits::SelectionContext::new(self).select(&obligation)
+        let candidate = traits::SelectionContext::new(self).select(&obligation);
+        if let Ok(Some(traits::ImplSource::UserDefined(ref impl_source_user_defined_data))) =
+            candidate
+            && self.infcx.tcx.do_not_recommend_impl(impl_source_user_defined_data.impl_def_id)
+        {
+            return Err(traits::SelectionError::Unimplemented);
+        }
+        candidate
     }

     /// Used for ambiguous method call error reporting. Uses probing that throws away the result internally,

makes your test pass (if you stop branching the output by changing the old solver output to be the same as the new solver output).

however, select_trait_candidate kinda looks like it's intended to be used in "real" codepaths rather than just diagnostics, even though it's only called by diagnostics code today - fixing that up, I leave to you. I'd be happy with:

• either changing just the nested_obligations codepath, because that's what this PR is originally for
• or, change both the nested_obligations codepath and the single-impl error message redirection, by changing select_trait_candidate and documenting it as being for error reporting only

---

This is all predicated on that we're making the old solver behave like the new solver. If we instead want to make the new solver behave like the old solver, we probably want to change something about that first line of code I linked. I haven't investigated that plan of action though, no idea what that might entail :3

I'm so sorry I'm being so nitpicky! this got way more involved than I expected, haha 💔

@rustbot author

[khyperia]

actually wait, looking into the original issue, we might want to continue calling nested_obligations on #[diagnostic::do_not_recommend] impls but filtering out result obligations that mention the #[diagnostic::do_not_recommend] impl. or something like that (I'm not 100% sure).

since the original issue does have a nested obligation (T: Clone) that we do want to report, it's just CloneToUninit that we don't want to mention... or something like that, again, not 100% sure, sorry, haha.

but yeah, probably something to do with this area of code, rather than after the two solvers codepaths have already merged

[mejrs]

however, select_trait_candidate kinda looks like it's intended to be used in "real" codepaths rather than just diagnostics, even though it's only called by diagnostics code today

You cannot delayed_bug there without crashing the compiler, so it very much looks like it's used outside of diagnostics.

[khyperia]

so it very much looks like it's used outside of diagnostics

How so? There are two callers:

•   /// Used for ambiguous method call error reporting. Uses probing that throws away the result internally,
    /// so do not use to make a decision that may lead to a successful compilation.
    fn candidate_source(&self, candidate: &Candidate<'tcx>, self_ty: Ty<'tcx>) -> CandidateSource {

  i.e. "Used for ambiguous method call error reporting"
• and then one in consider_probe with result = ProbeResult::NoMatch just before it, and the result is only used to append to possibly_unsatisfied_predicates.

that seems like "only used in diagnostics" to me. Where do we disagree? My guess on where the misunderstanding is, is that the function may be called when compiling code that does not error, but my hypothesis is that the result is only ever used in error reporting. So a delayed_bug will explode, because the function is called, but it's still a diagnostics-only function because the result is unused.

Let me know if you discover otherwise, if the result is actually used in compilation results.

[mejrs]

Ah, I think you're right. Still, that makes me somewhat uneasy.

[khyperia]

fair! four options I see:

• keep select_trait_candidate, add a select_trait_candidate_for_diagnostics wrapper that does the filtering, then call select_trait_candidate_for_diagnostics in its two callsites
• similar as above, but only have select_trait_candidate_for_diagnostics, delete the original, since it is only used in diagnostics (which AFAIK is what you're uneasy with)
• do the filtering in both callsites of select_trait_candidate inline, don't extract the filtering into a helper
• only do the filtering in the nested_obligations callsite, don't edit the single-impl-check callsite (the single-impl-check would likely be an outstanding bug then, my guess is that we're able to mention a #[diagnostic::do_not_recommend] impl due to that code right now, but that probably-exists-bug is unrelated to your bug)
  • you mmmight need to do this anyway for your original bug rather than just the test case in this PR, filtering out nested obligations that mention the do_not_recommend impl, rather than nuking the entire impl from diagnostics. unsure. I'm leaving this up to you to check!
• or a secret fifth option if you figure something cool out~

[mejrs]

I've just renamed the method, I think that's the best option other than some big refactoring that I don't have the stomach for rn.

@rustbot ready

[khyperia]

@bors r+ rollup

[rust-bors]

📌 Commit 77bce37 has been approved by khyperia

It is now in the queue for this repository.