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

Add PhantomData marker for dropck to BTreeMap #99413

Merged
merged 1 commit into from
Jul 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 26 additions & 4 deletions library/alloc/src/collections/btree/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,8 @@ pub struct BTreeMap<
length: usize,
/// `ManuallyDrop` to control drop order (needs to be dropped after all the nodes).
pub(super) alloc: ManuallyDrop<A>,
// For dropck; the `Box` avoids making the `Unpin` impl more strict than before
_marker: PhantomData<crate::boxed::Box<(K, V)>>,
Comment on lines +181 to +182
Copy link
Member

Choose a reason for hiding this comment

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

I think this is yet another reminder for us that we need something more intuitive than PhantomData. PhantomData is used to control Send, Sync, dropck, UnwindSafe, UnwindSafeRef, variance, and Unpin. Possibly more (in the future). It happens so often that we try to control just some of these, and accidentally end up affecting one of the others too. (Like what would've happened here if (K, V) wasn't wrapped in a Box.)

cc @rust-lang/lang

Copy link
Member

Choose a reason for hiding this comment

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

This reminds me of a crate that I wrote half a year ago. https://docs.rs/phtm/1.0.3/phtm/

}

#[stable(feature = "btree_drop", since = "1.7.0")]
Expand All @@ -187,6 +189,19 @@ unsafe impl<#[may_dangle] K, #[may_dangle] V, A: Allocator + Clone> Drop for BTr
}
}

// FIXME: This implementation is "wrong", but changing it would be a breaking change.
// (The bounds of the automatic `UnwindSafe` implementation have been like this since Rust 1.50.)
// Maybe we can fix it nonetheless with a crater run, or if the `UnwindSafe`
// traits are deprecated, or disarmed (no longer causing hard errors) in the future.
#[stable(feature = "btree_unwindsafe", since = "1.64.0")]
Comment on lines +192 to +196
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth just correcting (so, breaking) this. The damage is probably very small, if any.

@rust-lang/libs-api Opinions?

impl<K, V, A: Allocator + Clone> core::panic::UnwindSafe for BTreeMap<K, V, A>
where
A: core::panic::UnwindSafe,
K: core::panic::RefUnwindSafe,
V: core::panic::RefUnwindSafe,
{
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<K: Clone, V: Clone, A: Allocator + Clone> Clone for BTreeMap<K, V, A> {
fn clone(&self) -> BTreeMap<K, V, A> {
Expand All @@ -204,6 +219,7 @@ impl<K: Clone, V: Clone, A: Allocator + Clone> Clone for BTreeMap<K, V, A> {
root: Some(Root::new(alloc.clone())),
length: 0,
alloc: ManuallyDrop::new(alloc),
_marker: PhantomData,
};

{
Expand Down Expand Up @@ -567,7 +583,7 @@ impl<K, V> BTreeMap<K, V> {
#[rustc_const_unstable(feature = "const_btree_new", issue = "71835")]
#[must_use]
pub const fn new() -> BTreeMap<K, V> {
BTreeMap { root: None, length: 0, alloc: ManuallyDrop::new(Global) }
BTreeMap { root: None, length: 0, alloc: ManuallyDrop::new(Global), _marker: PhantomData }
}
}

Expand All @@ -593,6 +609,7 @@ impl<K, V, A: Allocator + Clone> BTreeMap<K, V, A> {
root: mem::replace(&mut self.root, None),
length: mem::replace(&mut self.length, 0),
alloc: self.alloc.clone(),
_marker: PhantomData,
});
}

Expand All @@ -615,7 +632,7 @@ impl<K, V, A: Allocator + Clone> BTreeMap<K, V, A> {
/// ```
#[unstable(feature = "btreemap_alloc", issue = "32838")]
pub fn new_in(alloc: A) -> BTreeMap<K, V, A> {
BTreeMap { root: None, length: 0, alloc: ManuallyDrop::new(alloc) }
BTreeMap { root: None, length: 0, alloc: ManuallyDrop::new(alloc), _marker: PhantomData }
}
}

Expand Down Expand Up @@ -1320,7 +1337,12 @@ impl<K, V, A: Allocator + Clone> BTreeMap<K, V, A> {
let (new_left_len, right_len) = Root::calc_split_length(total_num, &left_root, &right_root);
self.length = new_left_len;

BTreeMap { root: Some(right_root), length: right_len, alloc: self.alloc.clone() }
BTreeMap {
root: Some(right_root),
length: right_len,
alloc: self.alloc.clone(),
_marker: PhantomData,
}
}

/// Creates an iterator that visits all elements (key-value pairs) in
Expand Down Expand Up @@ -1445,7 +1467,7 @@ impl<K, V, A: Allocator + Clone> BTreeMap<K, V, A> {
let mut root = Root::new(alloc.clone());
let mut length = 0;
root.bulk_push(DedupSortedIter::new(iter.into_iter()), &mut length, alloc.clone());
BTreeMap { root: Some(root), length, alloc: ManuallyDrop::new(alloc) }
BTreeMap { root: Some(root), length, alloc: ManuallyDrop::new(alloc), _marker: PhantomData }
}
}

Expand Down
16 changes: 16 additions & 0 deletions src/test/ui/btreemap/btreemap_dropck.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
struct PrintOnDrop<'a>(&'a str);

impl Drop for PrintOnDrop<'_> {
fn drop(&mut self) {
println!("printint: {}", self.0);
}
}

use std::collections::BTreeMap;
use std::iter::FromIterator;

fn main() {
let s = String::from("Hello World!");
let _map = BTreeMap::from_iter([((), PrintOnDrop(&s))]);
drop(s); //~ ERROR cannot move out of `s` because it is borrowed
}
13 changes: 13 additions & 0 deletions src/test/ui/btreemap/btreemap_dropck.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
error[E0505]: cannot move out of `s` because it is borrowed
--> $DIR/btreemap_dropck.rs:15:10
|
LL | let _map = BTreeMap::from_iter([((), PrintOnDrop(&s))]);
| -- borrow of `s` occurs here
LL | drop(s);
| ^ move out of `s` occurs here
LL | }
| - borrow might be used here, when `_map` is dropped and runs the `Drop` code for type `BTreeMap`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0505`.
5 changes: 4 additions & 1 deletion src/test/ui/issues/issue-72554.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
use std::collections::BTreeSet;

#[derive(Hash)]
pub enum ElemDerived { //~ ERROR recursive type `ElemDerived` has infinite size
pub enum ElemDerived {
//~^ ERROR recursive type `ElemDerived` has infinite size
//~| ERROR cycle detected when computing drop-check constraints for `ElemDerived`
A(ElemDerived)
}


pub enum Elem {
Derived(ElemDerived)
}
Expand Down
19 changes: 17 additions & 2 deletions src/test/ui/issues/issue-72554.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ error[E0072]: recursive type `ElemDerived` has infinite size
|
LL | pub enum ElemDerived {
| ^^^^^^^^^^^^^^^^^^^^ recursive type has infinite size
...
LL | A(ElemDerived)
| ----------- recursive without indirection
|
Expand All @@ -11,6 +12,20 @@ help: insert some indirection (e.g., a `Box`, `Rc`, or `&`) to make `ElemDerived
LL | A(Box<ElemDerived>)
| ++++ +

error: aborting due to previous error
error[E0391]: cycle detected when computing drop-check constraints for `ElemDerived`
--> $DIR/issue-72554.rs:4:1
|
LL | pub enum ElemDerived {
| ^^^^^^^^^^^^^^^^^^^^
|
= note: ...which immediately requires computing drop-check constraints for `ElemDerived` again
note: cycle used when computing drop-check constraints for `Elem`
--> $DIR/issue-72554.rs:11:1
|
LL | pub enum Elem {
| ^^^^^^^^^^^^^

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0072`.
Some errors have detailed explanations: E0072, E0391.
For more information about an error, try `rustc --explain E0072`.