Prohibit specialized drops #8142
Comments
|
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 |
|
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 |
|
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... |
|
This issue seems similar to issue #6971 |
|
your example |
|
Nominating, this is a pretty serious problem. |
|
accepted for "first major release", P-high. |
|
Updated text to describe our planned fix. |
|
I would perhaps like to fix this issue in the near-term by just putting |
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]
…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
|
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 |
|
P-high, not 1.0 |
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]
|
re-nominating, since |
|
This blocks removing the calling 1.0 polish unless discussion of #22196 changes my opinion. |
|
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. |
|
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 |
|
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");
}The above prints out (if you are lucky and other data corruption does not happen first): thus illustrating that the |
|
ah sweet, I think I actually figured out how to do this (thank goodness I have 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 |
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
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
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
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
Dropmeet special criteria such that theSelfparameter 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:
but this is not
nor the example below.
ORIGINAL:
The following code prints
Dropping!. This is wrong.The text was updated successfully, but these errors were encountered: