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

Conversation

compiler-errors
Copy link
Member

Hacky fix for #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 #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

@rustbot
Copy link
Collaborator

rustbot commented Apr 8, 2024

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 8, 2024
@compiler-errors
Copy link
Member Author

r? types

@rustbot rustbot added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Apr 8, 2024
@rustbot rustbot assigned spastorino and unassigned TaKO8Ki Apr 8, 2024
@@ -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().

Copy link
Member

@spastorino spastorino left a comment

Choose a reason for hiding this comment

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

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.

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

.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

@compiler-errors
Copy link
Member Author

@bors r=spastorino

@bors
Copy link
Contributor

bors commented Apr 14, 2024

📌 Commit 87a387a has been approved by spastorino

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 14, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 14, 2024
…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
@bors bors merged commit 86b791a into rust-lang:master Apr 14, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 14, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 14, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

requirement solver overflow involving proptest::sugar::NamedArguments
5 participants