Skip to content

don't calculate dtors if the self ty has impossible bounds - #162715

Open
sjwang05 wants to merge 1 commit into
rust-lang:mainfrom
sjwang05:no-impossible-dtors
Open

sjwang05 wants to merge 1 commit into
rust-lang:mainfrom
sjwang05:no-impossible-dtors

Conversation

@sjwang05

Copy link
Copy Markdown
Contributor

If we have multiple drop impls and a self ty that all have the same impossible bounds, we end up with duplicate drops with different DefIds, which causes us to ICE with a delayed bug in calculate_dtor. Coherence accepts these impls since they have impossible bounds, and nothing else in check_drop_impl checks this, leading to us accepting the code and subsequently ICEing from the delayed bug.

We now skip considering the current drop impl as a dtor candidate if the self ty and impl(s) have the same impossible bounds, since if the self ty is unnameable, it's impossible to construct an instance in the first place. Checking only the self ty is sufficient here, as check_drop_impl checks the case where the impossible bounds exist only on the drop impl, and we're not allowed in general to have a struct whose bounds are more restrictive than its impl's, so the only case we're currently missing is the case where the self ty and the Drop impls have the exact same impossible bounds.

My reasoning for accepting such code instead of emitting an error is that, in general, we allow users to write code with impossible or trivial bounds, so long as they never try to actually name or run that code. This PR makes Drop behave the same way.

This results in overlapping impossible drop impls behaving like other traits: for instance, the following compiles on nightly today:

pub trait Foo {
    fn foo(&self) {}
}

pub struct Thing<T>(T)
where
    [T]: Sized;

impl<T> Foo for Thing<T> where [T]: Sized {}
impl<T> Foo for Thing<T> where [T]: Sized {} // removing this impl is ok as well

Although this ICE only seems to be reachable for sync drop, not making the same change for async drop makes the following program, which currently compiles on nightly, stop compiling:

#![feature(async_drop)]
use core::future::AsyncDrop;
use core::pin::Pin;

pub struct Thing<T>(T) where [T]: Sized;

impl<T> Drop for Thing<T> where [T]: Sized {
    fn drop(&mut self) {}
}
impl<T> AsyncDrop for Thing<T> where [T]: Sized {
    async fn drop(self: Pin<&mut Self>) {}
}

which feels like something we don't want.

cc #159118 (comment)

fixes #153947
fixes #150387

@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 Sep 13, 2026
@rustbot

rustbot commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator

r? @oli-obk

rustbot has assigned @oli-obk.
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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler, types
  • compiler, types expanded to 76 candidates
  • Random selection from 18 candidates

///
/// We already emit errors for the case where the impossible bound exists only on the self ty, or
/// only on the impl(s).
pub(crate) fn is_impossible_self_ty(tcx: TyCtxt<'_>, adt_did: LocalDefId) -> bool {

@sjwang05 sjwang05 Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I basically lifted this logic wholesale from the is_impossible_associated_item query, though I couldn't really find a nice way to make them into a single shared thing, since is_impossible_associated_item filters the obligations to those that only mention the parent item's generics before registering them with the ocx.

View changes since the review

@rust-log-analyzer

This comment has been minimized.

@rustbot

rustbot commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

@oli-obk

oli-obk commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

#150387 may keep existing. Can it be reproed with just specialization and no impossible bounds?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ICE]: ICE with multiple Drop impls with impossible bound and Self: Drop ICE from specializing Drop impl with impossible bound

4 participants