Audit #[derive]s in std for stability

[huonw]

#[derive(Foo)] currently just takes the stability of Foo, which may not be what we want for some types in std.

cc #22500

According to

$ git grep -C 5 '#\[derive' -- liballoc/ libarena/ libcollections/ libcore/ libstd/ libunicode/ | grep -C 5 '\[stable' | grep ':[0-9]\+:' | sed 's/:.*//' | sort | uniq

these files have a #[derive] attribute near a #[stable] one:

• libcollections/ (12) @huonw Audit #[derive]s in std for stability #22511 (comment)
  • binary_heap.rs
  • bit.rs
  • btree/map.rs
  • btree/set.rs
  • dlist.rs
  • string.rs
  • vec_map.rs
• libcore/ (37) @huonw Audit #[derive]s in std for stability #22511 (comment)
  • any.rs
  • atomic.rs
  • char.rs
  • cmp.rs
  • default.rs
  • fmt/mod.rs
  • fmt/rt/v1.rs
  • num/mod.rs
  • ops.rs
  • option.rs
  • result.rs
  • slice.rs
  • str/mod.rs
• libcore/iter.rs (18) @huonw Audit #[derive]s in std for stability #22511 (comment)
• libstd/ (10) @huonw Audit #[derive]s in std for stability #22511 (comment)
  • collections/hash/map.rs
  • collections/hash/set.rs
  • sync/mpsc/mod.rs
  • thread.rs

Replacing the sed ... with wc -l gives 77, i.e. there are approximately 77 #[derive]s to look at (nearly 25% of which are in core::iter, hence it being listed separately).

Comment below if you wish to handle some of these.

[huonw]

cc @aturon, @alexcrichton

std

The only dubious implementation is the #[derive(Clone)] for std::thread::Thread. I (very vaguely) recall that there was some discussion about changing the internals of Thread to not have the Arc (I could very easily be misremembering). This may make the Clone impossible in future.

[huonw]

collections

All #[derive]s seem fine. Although there are a few cases of Debug not necessarily having the output we want, long term (e.g. collections::string::FromUtf8Error will print a whole raw Vec<u8> with the [ , , , ] syntax) but its not clear what guarantees we want to give about the precise output of Debug (not many, IMO).

[huonw]

core

Why is core::fmt::Arguments Copy?

rust/src/libcore/fmt/mod.rs

Lines 212 to 233 in 1576142

	/// This structure represents a safely precompiled version of a format string
	/// and its arguments. This cannot be generated at runtime because it cannot
	/// safely be done so, so no constructors are given and the fields are private
	/// to prevent modification.
	///
	/// The `format_args!` macro will safely create an instance of this structure
	/// and pass it to a function or closure, passed as the first argument. The
	/// macro validates the format string at compile-time so usage of the `write`
	/// and `format` functions can be safely performed.
	#[stable(feature = "rust1", since = "1.0.0")]
	#[derive(Copy)]
	pub struct Arguments<'a> {
	// Format string pieces to print.
	pieces: &'a [&'a str],
	// Placeholder specs, or `None` if all specs are default (as in "{}{}").
	fmt: Option<&'a [rt::v1::Argument]>,
	// Dynamic arguments for interpolation, to be interleaved with string
	// pieces. (Every argument is preceded by a string piece.)
	args: &'a [ArgumentV1<'a>],
	}

RangeFull and RangeTo are Copy but Range and RangeFrom are not.

The Ordering/PartialOrdering of Option and Result is inconsistent (in the sense of Option<T> ≅ Result<T, ()>): None < Some(_), but Err(_) > Ok(_).

[huonw]

core::iter

All seems fine: they're essentially all just #[derive(Clone)]s.

(Although I did notice #22950 in the process.)

[SimonSapin]

The Ordering/PartialOrdering of Option and Result is inconsistent (in the sense of Option ≅ Result<T, ()>): None < Some(), but Err() > Ok(_).

Either None < Some(_) or None > Some(_) (similarly for Result) is arbitrary. Should they really implement PartialOrd at all?

[alexcrichton]

The only dubious implementation is the #[derive(Clone)] for std::thread::Thread. I (very vaguely) recall that there was some discussion about changing the internals of Thread to not have the Arc (I could very easily be misremembering). This may make the Clone impossible in future.

Hm I feel like this was some other structure we may remove the Arc from. I think that this handle must fundamentally have an Arc as it's handed out to other threads for park and unpark. That being said we could also be more conservative for now!

Why is core::fmt::Arguments Copy?

rust/src/libcore/fmt/mod.rs

Lines 212 to 233 in 1576142

	/// This structure represents a safely precompiled version of a format string
	/// and its arguments. This cannot be generated at runtime because it cannot
	/// safely be done so, so no constructors are given and the fields are private
	/// to prevent modification.
	///
	/// The `format_args!` macro will safely create an instance of this structure
	/// and pass it to a function or closure, passed as the first argument. The
	/// macro validates the format string at compile-time so usage of the `write`
	/// and `format` functions can be safely performed.
	#[stable(feature = "rust1", since = "1.0.0")]
	#[derive(Copy)]
	pub struct Arguments<'a> {
	// Format string pieces to print.
	pieces: &'a [&'a str],
	// Placeholder specs, or `None` if all specs are default (as in "{}{}").
	fmt: Option<&'a [rt::v1::Argument]>,
	// Dynamic arguments for interpolation, to be interleaved with string
	// pieces. (Every argument is preceded by a string piece.)
	args: &'a [ArgumentV1<'a>],
	}

I believe that we used to pass the structure by reference but we now pass it by value sometimes necessitating the Copy bound. I would also be fine going back to passing it by reference by convention (I believe there's also an issue about the codegen of pass-by-value which is tangentially related)

RangeFull and RangeTo are Copy but Range and RangeFrom are not.

cc #21846

[aturon]

cc me

[huonw]

Either None < Some() or None > Some() (similarly for Result) is arbitrary. Should they really implement PartialOrd at all?

I think so: it is useful for generic code (e.g. when storing things in a tree, the details of the ordering usually don't matter, as long as one exists) and #[derive], e.g. one might have

#[derive(PartialOrd)]
struct Foo {
     x: T
     y: U,
     z: Option<V>
}

The first two fields will usually completely distinguish instances of Foo and the last just serves as a tie-break. As we found with Path and #[derive(Show)] previously, being forced to write out the impl manually in cases like this is very annoying. (FWIW, Haskell's Maybe and Either both implement Ord.)

[SimonSapin]

It’s not quite the same as Option, but FWIW None in Python 2 is less than all other objects. In Python 3, it was changed to be non-comparable to other objects.

[aturon]

The only dubious implementation is the #[derive(Clone)] for std::thread::Thread. I (very vaguely) recall that there was some discussion about changing the internals of Thread to not have the Arc (I could very easily be misremembering). This may make the Clone impossible in future.

Hm I feel like this was some other structure we may remove the Arc from. I think that this handle must fundamentally have an Arc as it's handed out to other threads for park and unpark. That being said we could also be more conservative for now!

The Clone is definitely wanted here, and the Arc is intended -- as you say, you want to be able to hand out copies of the handle to place in data structures for use with park and unpark.