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

Discard overflow obligations in impl_may_apply #123618

Merged
merged 1 commit into from Apr 14, 2024
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -5,6 +5,7 @@ use rustc_infer::traits::{Obligation, ObligationCause, PolyTraitObligation};
use rustc_middle::ty;
use rustc_span::{Span, DUMMY_SP};

use crate::traits::query::evaluate_obligation::InferCtxtExt;
use crate::traits::ObligationCtxt;

#[derive(Debug)]
Expand Down Expand Up @@ -52,10 +53,21 @@ pub fn compute_applicable_impls_for_diagnostics<'tcx>(
_ => return false,
}

let impl_predicates = tcx.predicates_of(impl_def_id).instantiate(tcx, impl_args);
ocx.register_obligations(impl_predicates.predicates.iter().map(|&predicate| {
Obligation::new(tcx, ObligationCause::dummy(), param_env, predicate)
}));
let obligations = tcx
.predicates_of(impl_def_id)
.instantiate(tcx, impl_args)
.into_iter()
.map(|(predicate, _)| {
Obligation::new(tcx, ObligationCause::dummy(), param_env, predicate)
})
// Kinda hacky, but let's just throw away obligations that overflow.
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering if we should add a FIXME here or something and if you have some kind of thought about how we could potentially get rid of the "hack" at some point.

Copy link
Member Author

Choose a reason for hiding this comment

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

We get rid of the hack when the new solver is stabilized, which at that point someone will replace all the infcx.next_solver with true and do the const-folding to remove this whole filter.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, at that point it will be obvious but maybe worth adding a FIXME(next_solver) or something like that

// This may reduce the accuracy of this check (if the obligation guides
// inference or it actually resulted in error after others are processed)
// ... but this is diagnostics code.
.filter(|obligation| {
infcx.next_trait_solver() || infcx.evaluate_obligation(obligation).is_ok()
});
ocx.register_obligations(obligations);

ocx.select_where_possible().is_empty()
})
Expand Down
Expand Up @@ -2398,12 +2398,12 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
if ambiguities.len() > 5 {
let infcx = self.infcx;
if !ambiguities.iter().all(|option| match option {
DefId(did) => infcx.fresh_args_for_item(DUMMY_SP, *did).is_empty(),
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a very strange way of computing generics().count().

DefId(did) => infcx.tcx.generics_of(*did).count() == 0,
ParamEnv(_) => true,
}) {
// If not all are blanket impls, we filter blanked impls out.
ambiguities.retain(|option| match option {
DefId(did) => infcx.fresh_args_for_item(DUMMY_SP, *did).is_empty(),
DefId(did) => infcx.tcx.generics_of(*did).count() == 0,
Copy link
Member

Choose a reason for hiding this comment

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

Just commenting to note that we do .count() == 0 in some places and .params.is_empty() in other places, those two things are different as count considers parent_count and params is owns generics.
I was wondering should we maybe add is_empty and is_own_empty methods for the sake of making these things clear?.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea. I can add those.

A lot of the places where we use .params implicitly assumes there's no parent (i.e. impls/traits) but we don't need to.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do this as a follow-up actually

ParamEnv(_) => true,
});
}
Expand Down
14 changes: 14 additions & 0 deletions tests/ui/traits/overflow-computing-ambiguity.rs
@@ -0,0 +1,14 @@
trait Hello {}

struct Foo<'a, T: ?Sized>(&'a T);

impl<'a, T: ?Sized> Hello for Foo<'a, &'a T> where Foo<'a, T>: Hello {}

impl Hello for Foo<'static, i32> {}

fn hello<T: ?Sized + Hello>() {}

fn main() {
hello();
//~^ ERROR type annotations needed
}
23 changes: 23 additions & 0 deletions tests/ui/traits/overflow-computing-ambiguity.stderr
@@ -0,0 +1,23 @@
error[E0283]: type annotations needed
--> $DIR/overflow-computing-ambiguity.rs:12:5
|
LL | hello();
| ^^^^^ cannot infer type of the type parameter `T` declared on the function `hello`
|
= note: cannot satisfy `_: Hello`
= help: the following types implement trait `Hello`:
Foo<'a, &'a T>
Foo<'static, i32>
note: required by a bound in `hello`
--> $DIR/overflow-computing-ambiguity.rs:9:22
|
LL | fn hello<T: ?Sized + Hello>() {}
| ^^^^^ required by this bound in `hello`
help: consider specifying the generic argument
|
LL | hello::<T>();
| +++++

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0283`.