Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor #[diagnostic::do_not_recommend] support #125717

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,6 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
ty::PredicateKind::Clause(ty::ClauseKind::Trait(trait_predicate)) => {
let trait_predicate = bound_predicate.rebind(trait_predicate);
let trait_predicate = self.resolve_vars_if_possible(trait_predicate);
let trait_predicate = self.apply_do_not_recommend(trait_predicate, &mut obligation);

// Let's use the root obligation as the main message, when we care about the
// most general case ("X doesn't implement Pattern<'_>") over the case that
Expand Down Expand Up @@ -996,12 +995,9 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
err.emit()
}

fn apply_do_not_recommend(
&self,
mut trait_predicate: ty::Binder<'tcx, ty::TraitPredicate<'tcx>>,
obligation: &'_ mut PredicateObligation<'tcx>,
) -> ty::Binder<'tcx, ty::TraitPredicate<'tcx>> {
fn apply_do_not_recommend(&self, obligation: &mut PredicateObligation<'tcx>) -> bool {
let mut base_cause = obligation.cause.code().clone();
let mut applied_do_not_recommend = false;
loop {
if let ObligationCauseCode::ImplDerived(ref c) = base_cause {
if self.tcx.has_attrs_with_path(
Expand All @@ -1011,7 +1007,7 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
let code = (*c.derived.parent_code).clone();
obligation.cause.map_code(|_| code);
obligation.predicate = c.derived.parent_trait_pred.upcast(self.tcx);
trait_predicate = c.derived.parent_trait_pred.clone();
applied_do_not_recommend = true;
}
}
if let Some((parent_cause, _parent_pred)) = base_cause.parent() {
Expand All @@ -1021,7 +1017,7 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
}
}

trait_predicate
applied_do_not_recommend
}

fn emit_specialized_closure_kind_error(
Expand Down Expand Up @@ -1521,6 +1517,20 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {

#[instrument(skip(self), level = "debug")]
fn report_fulfillment_error(&self, error: &FulfillmentError<'tcx>) -> ErrorGuaranteed {
let mut error = FulfillmentError {
obligation: error.obligation.clone(),
code: error.code.clone(),
root_obligation: error.root_obligation.clone(),
};
if matches!(
error.code,
FulfillmentErrorCode::Select(crate::traits::SelectionError::Unimplemented)
| FulfillmentErrorCode::Project(_)
) && self.apply_do_not_recommend(&mut error.obligation)
{
error.code = FulfillmentErrorCode::Select(SelectionError::Unimplemented);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct Select(Unimplemented) is only meant for trait goals, and only if the goal is truly unimplemented (e.g. not ambiguity, not overflow, not a cycle).

I think we need to look at the obligation to re-derive the FulfillmentErrorCode -- perhaps look at how the FulfillmentErrorCode is created in the first place in the old solver?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, I haven't looked deep enough to see if this is possible to turn into a weird diagnostic (or an ICE), so you may need to do a bit of research yourself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to look at the obligation to re-derive the FulfillmentErrorCode -- perhaps look at how the FulfillmentErrorCode is created in the first place in the old solver?

I'm not sure if that is easily possible with the old solver. As far as I understand these FulfillmentErrorCode are produced somewhat ad hoc in the code here that does the actual solving:

fn process_projection_obligation(
&mut self,
obligation: &PredicateObligation<'tcx>,
project_obligation: PolyProjectionObligation<'tcx>,
stalled_on: &mut Vec<TyOrConstInferVar>,
) -> ProcessResult<PendingPredicateObligation<'tcx>, FulfillmentErrorCode<'tcx>> {

At this point we know the following things about the passed obligation:

  • If it changes it does contain a unimplemented trait requirement somewhere in the stack as #[do_not_recommend] an only applied to trait impls (That's asserted at other places).
  • I think we can only ever end up with changing anything here for FulfillmentErrorCode::Project or FulfillmentErrorCode::Project. In both cases it will change it to the a trait requirement not implemented obligation due to where do_not_recommend can be placed.

Maybe it's a good idea to just explicitly check the FulfillmentErrorCode before if its FulfillmentErrorCode::Select(Unimplementd) or FulfillmentErrorCode::Project so that it always matches that assumptions?

For the record, I haven't looked deep enough to see if this is possible to turn into a weird diagnostic (or an ICE), so you may need to do a bit of research yourself.

At least all of the compiler test suite (via ./x.py test) runs fine with this change. That said I will try to produce a few edge cases with the #[do_not_recommend] attribute added and add them as well to the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried to think about potential edge cases in the last few days, but I did not came up with something interesting with the current set of restrictions.
We only walk the tree now for FulfillmentErrorCode::Select(Unimplementd) or FulfillmentErrorCode::Project. We only change anything if we hit a trait impl that is marked as #[do_not_recommend] at all. As these are enforced to be on impls I cannot see a different case than just having an Obligation that just says that a certain trait is not implemented.

@rustbot ready

}

match error.code {
FulfillmentErrorCode::Select(ref selection_error) => self.report_selection_error(
error.obligation.clone(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error[E0277]: Very important message!
--> $DIR/type_mismatch.rs:25:14
|
LL | verify::<u8>();
| ^^ the trait `TheImportantOne` is not implemented for `u8`
|
note: required by a bound in `verify`
--> $DIR/type_mismatch.rs:22:14
|
LL | fn verify<T: TheImportantOne>() {}
| ^^^^^^^^^^^^^^^ required by this bound in `verify`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0277`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error[E0277]: Very important message!
--> $DIR/type_mismatch.rs:25:14
|
LL | verify::<u8>();
| ^^ the trait `TheImportantOne` is not implemented for `u8`
|
note: required by a bound in `verify`
--> $DIR/type_mismatch.rs:22:14
|
LL | fn verify<T: TheImportantOne>() {}
| ^^^^^^^^^^^^^^^ required by this bound in `verify`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0277`.
27 changes: 27 additions & 0 deletions tests/ui/diagnostic_namespace/do_not_recommend/type_mismatch.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//@ revisions: current next
//@ ignore-compare-mode-next-solver (explicit revisions)
//@[next] compile-flags: -Znext-solver

#![feature(do_not_recommend)]

#[diagnostic::on_unimplemented(message = "Very important message!")]
trait TheImportantOne {}

trait ImplementationDetail {
type Restriction;
}

#[diagnostic::do_not_recommend]
impl<T: ImplementationDetail<Restriction = ()>> TheImportantOne for T {}

// Comment out this `impl` to show the expected error message.
impl ImplementationDetail for u8 {
type Restriction = u8;
}

fn verify<T: TheImportantOne>() {}

pub fn main() {
verify::<u8>();
//~^ERROR: Very important message! [E0277]
}
Loading