Discard overflow obligations in impl_may_apply#123618
Conversation
rustbot
added
S-waiting-on-review
|
r? types |
rustbot
added
the
T-types
| 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(), |
There was a problem hiding this comment.
This is a very strange way of computing generics().count().
spastorino
left a comment
spastorino
left a comment
There was a problem hiding this comment.
Left some comments but feel free to r=me as is or if you think we should add a FIXME just do it and r=me.
| // 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, |
There was a problem hiding this comment.
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?.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll do this as a follow-up actually
| .map(|(predicate, _)| { | ||
| Obligation::new(tcx, ObligationCause::dummy(), param_env, predicate) | ||
| }) | ||
| // Kinda hacky, but let's just throw away obligations that overflow. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, at that point it will be obvious but maybe worth adding a FIXME(next_solver) or something like that
|
@bors r=spastorino |
bors
added
S-waiting-on-bors
…llaumeGomez Rollup of 5 pull requests Successful merges: - rust-lang#120900 (std: use `stream_position` where applicable) - rust-lang#123373 (skip Codegen{GCC,Cranelift} when using CI rustc) - rust-lang#123618 (Discard overflow obligations in `impl_may_apply`) - rust-lang#123905 (rustdoc: check redundant explicit links with correct itemid) - rust-lang#123915 (improve documentation slightly regarding some pointer methods) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#123618 - compiler-errors:overflow-ambig, r=spastorino Discard overflow obligations in `impl_may_apply` Hacky fix for rust-lang#123493. Throws away obligations that are overflowing in `impl_may_apply` when we recompute if an impl applies, since those will lead to fatal overflow if processed during fulfillment. Something about rust-lang#114811 (I think it's the predicate reordering) caused us to evaluate predicates differently in error reporting leading to fatal overflow, though I believe the underlying overflow is possible to hit since this code was rewritten to use fulfillment. Fixes rust-lang#123493
5 participants
Hacky fix for #123493. Throws away obligations that are overflowing in
impl_may_applywhen we recompute if an impl applies, since those will lead to fatal overflow if processed during fulfillment.Something about #114811 (I think it's the predicate reordering) caused us to evaluate predicates differently in error reporting leading to fatal overflow, though I believe the underlying overflow is possible to hit since this code was rewritten to use fulfillment.
Fixes #123493