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

~const Drop bounds are broken / wrong / unsound #94803

Closed
steffahn opened this issue Mar 10, 2022 · 4 comments
Closed

~const Drop bounds are broken / wrong / unsound #94803

steffahn opened this issue Mar 10, 2022 · 4 comments
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. C-bug Category: This is a bug. F-const_trait_impl `#![feature(const_trait_impl)]` I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@steffahn
Copy link
Member

#![feature(const_trait_impl)]

fn extend_lifetime(x: &str) -> &'static str {
    f::<S>(x)
}

const fn f<'a, T: Tr<'a> + ~const Drop>(x: &'a str) -> T::Ty {
    // can call g with T: Drop bound, even though
    // T: ~const Drop works for non-Drop-implementing types
    g::<T>(x)
}

#[allow(drop_bounds)]
const fn g<T: Drop>(x: &str) -> <T as Tr<'_>>::Ty {
    x
}

struct S;
trait Tr<'a> {
    type Ty;
}

#[allow(drop_bounds)]
impl<'a, T: Drop> Tr<'a> for T {
    type Ty = &'a str;
}

impl<'a> Tr<'a> for S {
    type Ty = &'static str;
}

fn main() {
    let x = "Hello World".to_owned();
    let s = extend_lifetime(&x);
    drop(x);
    println!("{s}");
}
��)

(playground)

@rustbot label requires-nightly, I-unsound, T-compiler, F-const_trait_impl, A-const-fn

By the way, even if this worked “correctly” (i.e. soundly) I really hate the fact that this feature is using a sort of T: Drop bound; the drop_bounds warning exists for a reason, using the Drop trait in bounds is discouraged for a reason, abusing it as in the feature that allows “T: ~const Drop” makes everything just sooo much more confusing and inconsistent.

@steffahn steffahn added the C-bug Category: This is a bug. label Mar 10, 2022
@rustbot rustbot added A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. F-const_trait_impl `#![feature(const_trait_impl)]` I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 10, 2022
@compiler-errors
Copy link
Member

cc @fee1-dead @oli-obk

@oli-obk
Copy link
Contributor

oli-obk commented Mar 10, 2022

I really hate the fact that this feature is using a sort of T: Drop bound; the drop_bounds warning exists for a reason, using the Drop trait in bounds is discouraged for a reason, abusing it as in the feature that allows “T: ~const Drop” makes everything just sooo much more confusing and inconsistent.

FWIW I agree. I brought some of this up in https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/.22implicit.22.20const-if-const.20Drop

We can't automatically require all generic params to be const droppable, as that would make const fn foo<T>(T: T) -> T {t} stop being callable in const contexts with types that implement Drop, which works right now on stable.

@steffahn
Copy link
Member Author

steffahn commented Mar 10, 2022

@oli-obk Looking at the linked discussion I don’t see where this brings up the same point. Maybe I’m missing something.

In my mind, there should be a trait CanBeDropped1 (that can’t be implemented manually) that is always (non-const-)implemented by all types T: ?Sized2, but then a type also implements T: const CanBeDropped whenever all Drop implementations called in the destructor of T are const Drop implementations.

I.e. for a type with no drop glue at all, the destructor invokes no Drop implementations at all, so all of them are const Drop implementations. For a struct S with field types Foo and Bar, we get S: const CanBeDropped if and only if both:

  • S implements const Drop, or doesn’t implement Drop at all
  • Foo: const CanBeDropped and Bar: const CanBeDropped (checked recursively)

The necessary bound for dropping some type T in a const fn then would be T: ~const CanBeDropped. As mentioned, the bound T: CanBeDropped is fulfilled automatically by all types, so the bound doesn't hurt non-const function calls.

Obviously, this approach would also get rid of the limitation under the current rules that impl const Drop for ... requires all fields to be const Drop.

Footnotes

  1. actual name TBD

  2. types that don't implement it might exist if we get linear types; so it might be possible to use the same “CanBeDropped” trait for const destructors as well as linear types, but I haven't thought this through whether or not there might be any significant differences/incompatibilities

bors added a commit to rust-lang-ci/rust that referenced this issue Mar 23, 2022
Rename `~const Drop` to `~const Destruct`

r? `@oli-obk`

Completely switching to `~const Destructible` would be rather complicated, so it seems best to add it for now and wait for it to be backported to beta in the next release.

The rationale is to prevent complications such as rust-lang#92149 and rust-lang#94803 by introducing an entirely new trait. And `~const Destructible` reads a bit better than `~const Drop`. Name Bikesheddable.
@fee1-dead
Copy link
Member

Issue seems to be resolved after #94901.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. C-bug Category: This is a bug. F-const_trait_impl `#![feature(const_trait_impl)]` I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants