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

Prohibit specialized drops #8142

Closed
mstewartgallus opened this issue Jul 31, 2013 · 18 comments · Fixed by #14988 or #23638
Closed

Prohibit specialized drops #8142

mstewartgallus opened this issue Jul 31, 2013 · 18 comments · Fixed by #14988 or #23638
Assignees
Labels
A-destructors Area: Destructors (`Drop`, …) P-medium Medium priority
Milestone

Comments

@mstewartgallus
Copy link
Contributor

UPDATE:

The plan for drops, as I recall, was to prohibit "specialized" drops, but this never got implemented. The idea is to require that impls of Drop meet special criteria such that the Selfparameter must (a) be a nominal type (struct or enum); (b) have fresh type parameters for each type parameter of the type with (c) no additional bounds beyond those declared in the type declaration. Note that we will have to permit type bounds in type declarations, as well, which is to some extent a separate issue (also required for DST).

For example, this is legal:

struct Foo<T> { ... }

impl<T> Drop for Foo<T> { ... }

but this is not

impl<T:Send> Drop for Foo<T> { ... }

nor the example below.

ORIGINAL:

The following code prints Dropping!. This is wrong.

struct Foo<T>;

#[unsafe_destructor]
impl Drop for Foo<bool> {
    fn drop(&self) {
        println("Dropping!")
    }
}

fn main() {
    let _foo: Foo<()> = Foo;
}
@glaebhoerl
Copy link
Contributor

Is there a use case for this? C++ doesn't give you a way to do it without going to the trouble of template specializing the whole type (and I've never missed it). You could just prohibit writing a Drop impl that isn't of the form type-constructor-applied-to-zero-or-more-type-variables. (Further prior art is that standard Haskell enforces this restriction for /all/ type class instances, and requires the FlexibleInstances language extension to lift it.)

@mstewartgallus
Copy link
Contributor Author

I'd be okay if this would be prohibited because this can always be worked around by adding flag variables to the value although this would be a neat feature. One use case that I was trying to use this for was to have a drop instance for an Attribute<Enabled> type that disables it, and not for the Attribute<Disabled> type (where Attribute refers to an OpenGL vertex attribute, and Enabled, and Disabled are phantom types.

@glaebhoerl
Copy link
Contributor

Interesting, if it's not useless it might be worth keeping. :) There was also a discussion somewhere about what it means to have trait bounds on a Drop impl which seems vaguely related, but I can't find it...

@mstewartgallus
Copy link
Contributor Author

This issue seems similar to issue #6971

@bluss
Copy link
Member

bluss commented Aug 22, 2013

your example Attribute<Disabled> makes me think of c_vec which has two modes, either it owns the buffer or it doesn't.

@alexcrichton
Copy link
Member

Nominating, this is a pretty serious problem.

@pnkfelix
Copy link
Member

accepted for "first major release", P-high.

@nikomatsakis
Copy link
Contributor

Updated text to describe our planned fix.

@pcwalton
Copy link
Contributor

I would perhaps like to fix this issue in the near-term by just putting #[unsafe_destructor] behind a feature gate.

pcwalton added a commit to pcwalton/rust that referenced this issue Jun 20, 2014
Closes rust-lang#8142.

This is not the semantics we want long-term. You can continue to use
`#[unsafe_destructor]`, but you'll need to add
`#![feature(unsafe_destructor)]` to the crate attributes.

[breaking-change]
bors added a commit that referenced this issue Jun 20, 2014
…r=alexcrichton

Closes #8142.

This is not the semantics we want long-term. You can continue to use
`#[unsafe_destructor]`, but you'll need to add
`#![feature(unsafe_destructor)]` to the crate attributes.

[breaking-change]

r? @alexcrichton
@pnkfelix
Copy link
Member

Reopening, since the actual plan as described in the ticket has not been implemented.

Nominating, to be recategorized as P-high, not 1.0 milestone (and not P-backcompat-lang), since the aforementioned addition of the unsafe_destructor feature-gate takes it off the hot-path.

@pnkfelix
Copy link
Member

P-high, not 1.0

@pnkfelix pnkfelix removed this from the 1.0 milestone Aug 14, 2014
nrc pushed a commit to nrc/rust that referenced this issue Aug 22, 2014
Closes rust-lang#8142.

This is not the semantics we want long-term. You can continue to use
`#[unsafe_destructor]`, but you'll need to add
`#![feature(unsafe_destructor)]` to the crate attributes.

[breaking-change]
@steveklabnik
Copy link
Member

re-nominating, since unsafe_destructor may go away.

@pnkfelix
Copy link
Member

Ah, this appears to be a more thorough version of #21201.

(Subtask of #8861)

@pnkfelix
Copy link
Member

This blocks removing the unsafe_destructor feature gate.

calling 1.0 polish unless discussion of #22196 changes my opinion.

@pnkfelix
Copy link
Member

okay I think this is the last thing I think is blocking #22196, so while it is categorized as "1.0 polish", it really would be good to get it in for the beta. Looking at it now.

@pnkfelix
Copy link
Member

I have something plausible put together; the main thing remaining is dealing with the fallout of either adding or removing the appropriate bounds everywhere in libstd in order to placate the pass.

@pnkfelix
Copy link
Member

The pass I have put together seems to handle type-constraints properly, but it does not yet properly deal with region constraints.

For a little while I was not sure if this would be a problem, but I did eventually come up with a test case illustrating why one must ensure region constraints are indeed dealt with properly:

#![feature(unsafe_destructor)]

use std::cell::Cell;

struct InvalidateOnDrop {
    orig_value: &'static str,
    value: &'static str
}

// Note: Definition has no constraint relating 'b and 'a ...
struct P<'c, 'b:'c, 'a> {
    x: &'c Cell<&'b InvalidateOnDrop>,
    y: &'a InvalidateOnDrop
}

// ... but the Drop impl says 'a outlives 'b, and thus is
// able to copy `self.y` into `self.x`.  Havoc ensues below.
#[unsafe_destructor]
impl<'c, 'b:'c, 'a:'b> Drop for P<'c, 'b, 'a> {
    fn drop(&mut self) {
        self.x.set(self.y);
    }
}

#[allow(non_snake_case)]
fn InvalidateOnDrop(s: &'static str) -> InvalidateOnDrop {
    InvalidateOnDrop {
        orig_value: s,
        value: s,
    }
}

impl Drop for InvalidateOnDrop {
    fn drop(&mut self) {
        self.value = "invalidated";
    }
}

struct Loud<'l1, 'l2:'l1> {
    name: &'static str,
    value: &'l1 Cell<&'l2 InvalidateOnDrop>
}

#[unsafe_destructor]
impl<'l1, 'l2> Drop for Loud<'l1, 'l2> {
    fn drop(&mut self) {
        let orig = self.value.get().orig_value;
        let val = self.value.get().value;
        if orig == val {
            println!("dropping Loud {} pointing to {}", self.name, val);
        } else {
            println!("dropping Loud {} pointing to {} (orig: {})", self.name, val, orig);

        }
    }
}



fn main() {
    let b = InvalidateOnDrop("b");
    let c = Cell::new(&b);
    let _l1 = Loud { name: "l1", value: &c };
    let a = InvalidateOnDrop("a");
    let _p = P { x: &c, y: &a };

    let _l2 = Loud { name: "l2", value: &c };
    println!("Hello World");
}

playpen

The above prints out (if you are lucky and other data corruption does not happen first):

Hello World
dropping Loud l2 pointing to b
dropping Loud l1 pointing to invalidated (orig: a)

thus illustrating that the Loud destructor observed a value in a post-drop state. badness.

@pnkfelix
Copy link
Member

ah sweet, I think I actually figured out how to do this (thank goodness I have compare_method.rs to look at).

And it took me way less time that it did for me to come up with above example -- I just needed the motivation!

(Update: spoke too soon: My check is rejecting too many programs. Time to read compare_method.rs more carefully.)

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 24, 2015
Reject specialized Drop impls.

See Issue rust-lang#8142 for discussion.

This makes it illegal for a Drop impl to be more specialized than the original item.

So for example, all of the following are now rejected (when they would have been blindly accepted before):

```rust
struct S<A> { ... };
impl Drop for S<i8> { ... } // error: specialized to concrete type

struct T<'a> { ... };
impl Drop for T<'static> { ... } // error: specialized to concrete region

struct U<A> { ... };
impl<A:Clone> Drop for U<A> { ... } // error: added extra type requirement

struct V<'a,'b>;
impl<'a,'b:a> Drop for V<'a,'b> { ... } // error: added extra region requirement
```

Due to examples like the above, this is a [breaking-change].

(The fix is to either remove the specialization from the `Drop` impl, or to transcribe the requirements into the struct/enum definition; examples of both are shown in the PR's fixed to `libstd`.)

----

This is likely to be the last thing blocking the removal of the `#[unsafe_destructor]` attribute.

Fix rust-lang#8142
Fix rust-lang#23584
flip1995 pushed a commit to flip1995/rust that referenced this issue Jan 13, 2022
The `wrong_self_convention` lint uses a `SelfKind` type to decide
whether a method has the right kind of "self" for its name, or whether
the kind of "self" it has makes its name confusable for a method in
a common trait.  One possibility is `SelfKind::No`, which is supposed
to mean "No `self`".

Previously, SelfKind::No matched everything _except_ Self, including
references to Self.  This patch changes it to match Self, &Self, &mut
Self, Box<Self>, and so on.

For example, this kind of method was allowed before:

```
impl S {
    // Should trigger the lint, because
    // "methods called `is_*` usually take `self` by reference or no `self`"
    fn is_foo(&mut self) -> bool { todo!() }
}
```

But since SelfKind::No matched "&mut self", no lint was triggered
(see rust-lang#8142).

With this patch, the code above now gives a lint as expected.

Fixes rust-lang#8142

changelog: [`wrong_self_convention`] rejects `self` references in more cases
flip1995 pushed a commit to flip1995/rust that referenced this issue Jan 13, 2022
wrong_self_convention: Match `SelfKind::No` more restrictively

The `wrong_self_convention` lint uses a `SelfKind` type to decide
whether a method has the right kind of "self" for its name, or whether
the kind of "self" it has makes its name confusable for a method in
a common trait.  One possibility is `SelfKind::No`, which is supposed
to mean "No `self`".

Previously, SelfKind::No matched everything _except_ Self, including
references to Self.  This patch changes it to match Self, &Self, &mut
Self, Box<Self>, and so on.

For example, this kind of method was allowed before:

```
impl S {
    // Should trigger the lint, because
    // "methods called `is_*` usually take `self` by reference or no `self`"
    fn is_foo(&mut self) -> bool { todo!() }
}
```

But since SelfKind::No matched "&mut self", no lint was triggered
(see rust-lang#8142).

With this patch, the code above now gives a lint as expected.

fixes rust-lang#8142

changelog: [`wrong_self_convention`] rejects `self` references in more cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-destructors Area: Destructors (`Drop`, …) P-medium Medium priority
Projects
None yet
8 participants