Merge pull request #5 from rust-lang/feat/code-considerations

Initial code considerations and other sections

Author: KodrAus

.travis.yml

@@ -8,7 +8,6 @@ cache:
   - book/linkcheck/
 before_install:
 - shopt -s globstar
-- MAX_LINE_LENGTH=100 bash ci/check_line_lengths.sh src/**/*.md
 install:
 - source ~/.cargo/env || true
 - cargo install mdbook --version '^0.4.5'

src/SUMMARY.md

@@ -1,3 +1,41 @@
 # Summary
 
 [About this guide](./about-this-guide.md)
+
+[Getting started](./getting-started.md)
+
+[Reviewer checklist](./reviewer-checklist.md)
+
+---
+
+- [The feature lifecycle](./feature-lifecycle/summary.md)
+    - [Landing new features](./feature-lifecycle/new-unstable-features.md)
+    - [Using tracking issues](./feature-lifecycle/tracking-issues.md)
+    - [Stabilizing features](./feature-lifecycle/stabilization.md)
+    - [Deprecating features](./feature-lifecycle/deprecation.md)
+
+---
+
+- [Code considerations](./code-considerations/summary.md)
+    - [Design](./code-considerations/design/summary.md)
+        - [Public APIs](./code-considerations/design/public-apis.md)
+    - [Breaking changes](./code-considerations/breaking-changes/summary.md)
+        - [Breakage from changing behavior](./code-considerations/breaking-changes/behavior.md)
+        - [Breakage from new trait impls](./code-considerations/breaking-changes/new-trait-impls.md)
+        - [`#[fundamental]` types](./code-considerations/breaking-changes/fundamental.md)
+    - [Safety and soundness](./code-considerations/safety-and-soundness/summary.md)
+        - [Generics and unsafe](./code-considerations/safety-and-soundness/generics-and-unsafe.md)
+        - [Drop and `#[may_dangle]`](./code-considerations/safety-and-soundness/may-dangle.md)
+        - [`std::mem` and exclusive references](./code-considerations/safety-and-soundness/mem-and-exclusive-refs.md)
+    - [Using unstable language features](./code-considerations/using-unstable-lang/summary.md)
+        - [Const generics](./code-considerations/using-unstable-lang/const-generics.md)
+        - [Specialization](./code-considerations/using-unstable-lang/specialization.md)
+    - [Performance](./code-considerations/performance/summary.md)
+        - [When to `#[inline]`](./code-considerations/performance/inline.md)
+
+---
+
+- [Tools and bots](./tools-and-bots/summary.md)
+    - [`@bors`](./tools-and-bots/bors.md)
+    - [`@rust-timer`](./tools-and-bots/timer.md)
+    - [`@craterbot`](./tools-and-bots/crater.md)

src/about-this-guide.md

@@ -1,5 +1,7 @@
 # About this guide
 
+**Status:** Stub
+
 This guide is for contributors and reviewers to Rust's standard library.
 
 ## Other places to find information

src/code-considerations/breaking-changes/behavior.md

@@ -0,0 +1,9 @@
+# Breakage from changing behavior
+
+Breaking changes aren't just limited to compilation failures. Behavioral changes to stable functions generally can't be accepted. See [the `home_dir` issue](https://github.com/rust-lang/rust/pull/46799) for an example.
+
+An exception is when a behavior is specified in an RFC (such as IETF specifications for IP addresses). If a behavioral change fixes non-conformance then it can be considered a bug fix. In these cases, `@rust-lang/libs` should still be pinged for input.
+
+## For reviewers
+
+Look out for changes in existing implementations for stable functions, especially if assertions in test cases have been changed.

src/code-considerations/breaking-changes/fundamental.md

@@ -0,0 +1,31 @@
+# `#[fundamental]` types
+
+**Status:** Stub
+
+Type annotated with the `#[fundamental]` attribute have different coherence rules. See [RFC 1023](https://rust-lang.github.io/rfcs/1023-rebalancing-coherence.html) for details. That includes:
+
+- `&T`
+- `&mut T`
+- `Box<T>`
+- `Pin<T>`
+
+Typically, the scope of [breakage in new trait impls](./reviewing-code/breakage/new-trait-impls.md) is limited to inference and deref-coercion. New trait impls on `#[fundamental]` types may overlap with downstream impls and cause other kinds of breakage.
+
+[RFC 1023]: https://rust-lang.github.io/rfcs/1023-rebalancing-coherence.html
+
+## For reviewers
+
+Look out for blanket trait implementations for fundamental types, like:
+
+```rust
+impl<'a, T> PublicTrait for &'a T
+where
+    T: SomeBound,
+{
+
+}
+```
+
+unless the blanket implementation is being stabilized along with `PublicTrait`. In cases where we really want to do this, a [crater] run can help estimate the scope of the breakage.
+
+[crater]: ../../tools-and-bots/crater.md

src/code-considerations/breaking-changes/new-trait-impls.md

@@ -0,0 +1,76 @@
+# Breakage from new trait impls
+
+A lot of PRs to the standard library are adding new impls for already stable traits, which can break consumers in many weird and wonderful ways. The following sections gives some examples of breakage from new trait impls that may not be obvious just from the change made to the standard library.
+
+Also see [`#[fundamental]` types](./fundamental.md) for special considerations for types like `&T`, `&mut T`, `Box<T>`, and other core smart pointers.
+
+## Inference breaks when a second generic impl is introduced
+
+Rust will use the fact that there's only a single impl for a generic trait during inference. This breaks once a second impl makes the type of that generic ambiguous. Say we have:
+
+```rust
+// in `std`
+impl From<&str> for Arc<str> { .. }
+```
+
+```rust
+// in an external `lib`
+let b = Arc::from("a");
+```
+
+then we add:
+
+```diff
+impl From<&str> for Arc<str> { .. }
++ impl From<&str> for Arc<String> { .. }
+```
+
+then
+
+```rust
+let b = Arc::from("a");
+```
+
+will no longer compile, because we've previously been relying on inference to figure out the `T` in `Box<T>`.
+
+This kind of breakage can be ok, but a [crater] run should estimate the scope.
+
+## Deref coercion breaks when a new impl is introduced
+
+Rust will use deref coercion to find a valid trait impl if the arguments don't type check directly. This only seems to occur if there's a single impl so introducing a new one may break consumers relying on deref coercion. Say we have:
+
+```rust
+// in `std`
+impl Add<&str> for String { .. }
+
+impl Deref for String { type Target = str; .. }
+```
+
+```rust
+// in an external `lib`
+let a = String::from("a");
+let b = String::from("b");
+
+let c = a + &b;
+```
+
+then we add:
+
+```diff
+impl Add<&str> for String { .. }
++ impl Add<char> for String { .. }
+```
+
+then
+
+```rust
+let c = a + &b;
+```
+
+will no longer compile, because we won't attempt to use deref to coerce the `&String` into `&str`.
+
+This kind of breakage can be ok, but a [crater](../../tools-and-bots/crater.md) run should estimate the scope.
+
+## For reviewers
+
+Look out for new `#[stable]` trait implementations for existing stable traits.

src/code-considerations/breaking-changes/summary.md

@@ -0,0 +1,17 @@
+# Breaking changes
+
+Breaking changes should be avoided when possible. [RFC 1105](https://rust-lang.github.io/rfcs/1105-api-evolution.html) lays the foundations for what constitutes a breaking change. Breakage may be deemed acceptable or not based on its actual impact, which can be approximated with a [crater](../../tools-and-bots/crater.md) run.
+
+There are strategies for mitigating breakage depending on the impact.
+
+For changes where the value is high and the impact is high too:
+
+- Using compiler lints to try phase out broken behavior.
+
+If the impact isn't too high:
+
+- Looping in maintainers of broken crates and submitting PRs to fix them.
+
+## For reviewers
+
+Look out for changes to documented behavior and new trait impls for existing stable traits.

src/code-considerations/design/public-apis.md

@@ -0,0 +1,9 @@
+# Public API design
+
+**Status:** Stub
+
+Standard library APIs typically follow the [API Guidelines](https://rust-lang.github.io/api-guidelines/), which were originally spawned from the standard library itself.
+
+## For reviewers
+
+For new unstable features, look for any prior discussion of the proposed API to see what options and tradeoffs have already been considered. If in doubt, ping `@rust-lang/libs` for input.

src/code-considerations/design/summary.md

@@ -0,0 +1,9 @@
+# Design
+
+**Status:** Stub
+
+Most of the considerations in this guide are quality in some sense. This section has some general advice on maintaining code quality in the standard library.
+
+## For reviewers
+
+Think about how you would implement a feature and whether your approach would differ from what's being proposed. What trade-offs are being made? Is the weighting of those trade-offs the most appropriate?

src/code-considerations/performance/inline.md

@@ -0,0 +1,24 @@
+# When to `#[inline]`
+
+Inlining is a trade-off between potential execution speed, compile time and code size. There's some discussion about it in [this PR to the `hashbrown` crate](https://github.com/rust-lang/hashbrown/pull/119). From the thread:
+
+> `#[inline]` is very different than simply just an inline hint. As I mentioned before, there's no equivalent in C++ for what `#[inline]` does. In debug mode rustc basically ignores `#[inline]`, pretending you didn't even write it. In release mode the compiler will, by default, codegen an `#[inline]` function into every single referencing codegen unit, and then it will also add `inlinehint`. This means that if you have 16 CGUs and they all reference an item, every single one is getting the entire item's implementation inlined into it.
+
+You can add `#[inline]`:
+
+- To public, small, non-generic functions.
+
+You shouldn't need `#[inline]`:
+
+- On methods that have any generics in scope.
+- On methods on traits that don't have a default implementation.
+
+`#[inline]` can always be introduced later, so if you're in doubt they can just be removed.
+
+## What about `#[inline(always)]`?
+
+You should just about never need `#[inline(always)]`. It may be beneficial for private helper methods that are used in a limited number of places or for trivial operators. A micro benchmark should justify the attribute.
+
+## For reviewers
+
+`#[inline]` can always be added later, so if there's any debate about whether it's appropriate feel free to defer it by removing the annotations for a start.

src/code-considerations/performance/summary.md

@@ -0,0 +1,9 @@
+# Performance
+
+**Status:** Stub
+
+Changes to hot code might impact performance in consumers, for better or for worse. Appropriate benchmarks should give an idea of how performance characteristics change. For changes that affect `rustc` itself, you can also do a [`rust-timer`](../../tools-and-bots/timer.md) run.
+
+## For reviewers
+
+If a PR is focused on performance then try get some idea of what the impact is. Also consider marking the PR as `rollup=never`.

src/code-considerations/safety-and-soundness/generics-and-unsafe.md

@@ -0,0 +1,31 @@
+# Generics and unsafe
+
+Be careful of generic types that interact with unsafe code. Unless the generic type is bounded by an unsafe trait that specifies its contract, we can't rely on the results of generic types being reliable or correct.
+
+A place where this commonly comes up is with the `RangeBounds` trait. You might assume that the start and end bounds given by a `RangeBounds` implementation will remain the same since it works through shared references. That's not necessarily the case though, an adversarial implementation may change the bounds between calls:
+
+```rust
+struct EvilRange(Cell<bool>);
+
+impl RangeBounds<usize> for EvilRange {
+    fn start_bound(&self) -> Bound<&usize> {
+        Bound::Included(if self.0.get() {
+            &1
+        } else {
+            self.0.set(true);
+            &0
+        })
+    }
+    fn end_bound(&self) -> Bound<&usize> {
+        Bound::Unbounded
+    }
+}
+```
+
+This has [caused problems in the past](https://github.com/rust-lang/rust/issues/81138) for code making safety assumptions based on bounds without asserting they stay the same.
+
+Code using generic types to interact with unsafe should try convert them into known types first, then work with those instead of the generic. For our example with `RangeBounds`, this may mean converting into a concrete `Range`, or a tuple of `(Bound, Bound)`.
+
+## For reviewers
+
+Look out for generic functions that also contain unsafe blocks and consider how adversarial implementations of those generics could violate safety.

src/code-considerations/safety-and-soundness/may-dangle.md

@@ -0,0 +1,41 @@
+# Drop and `#[may_dangle]`
+
+A generic `Type<T>` that manually implements `Drop` should consider whether a `#[may_dangle]` attribute is appropriate on `T`. The [Nomicon](https://doc.rust-lang.org/nomicon/dropck.html) has some details on what `#[may_dangle]` is all about.
+
+If a generic `Type<T>` has a manual drop implementation that may also involve dropping `T` then dropck needs to know about it. If `Type<T>`'s ownership of `T` is expressed through types that don't drop `T` themselves such as `ManuallyDrop<T>`, `*mut T`, or `MaybeUninit<T>` then `Type<T>` also [needs a `PhantomData<T>` field](https://rust-lang.github.io/rfcs/0769-sound-generic-drop.html#phantom-data) to tell dropck that `T` may be dropped. Types in the standard library that use the internal `Unique<T>` pointer type don't need a `PhantomData<T>` marker field. That's taken care of for them by `Unique<T>`.
+
+As a real-world example of where this can go wrong, consider an `OptionCell<T>` that looks something like this:
+
+```rust
+struct OptionCell<T> {
+    is_init: bool,
+    value: MaybeUninit<T>,
+}
+
+impl<T> Drop for OptionCell<T> {
+    fn drop(&mut self) {
+        if self.is_init {
+            // Safety: `value` is guaranteed to be fully initialized when `is_init` is true.
+            // Safety: The cell is being dropped, so it can't be accessed again.
+            unsafe { self.value.assume_init_drop() };
+        }
+    }
+}
+```
+
+Adding a `#[may_dangle]` attribute to this `OptionCell<T>` that didn't have a `PhantomData<T>` marker field opened up [a soundness hole](https://github.com/rust-lang/rust/issues/76367) for `T`'s that didn't strictly outlive the `OptionCell<T>`, and so could be accessed after being dropped in their own `Drop` implementations. The correct application of `#[may_dangle]` also required a `PhantomData<T>` field:
+
+```diff
+struct OptionCell<T> {
+    is_init: bool,
+    value: MaybeUninit<T>,
++   _marker: PhantomData<T>,
+}
+
+- impl<T> Drop for OptionCell<T> {
++ unsafe impl<#[may_dangle] T> Drop for OptionCell<T> {
+```
+
+## For reviewers
+
+If there's a manual `Drop` implementation, consider whether `#[may_dangle]` is appropriate. If it is, make sure there's a `PhantomData<T>` too either through `Unique<T>` or as a field directly.

src/code-considerations/safety-and-soundness/mem-and-exclusive-refs.md

@@ -0,0 +1,15 @@
+# Using `mem` to break assumptions
+
+## `mem::replace` and `mem::swap`
+
+Any value behind a `&mut` reference can be replaced with a new one using `mem::replace` or `mem::swap`, so code shouldn't assume any reachable mutable references can't have their internals changed by replacing.
+
+## `mem::forget`
+
+Rust doesn't guarantee destructors will run when a value is leaked (which can be done with `mem::forget`), so code should avoid relying on them for maintaining safety. Remember, [everyone poops](http://cglab.ca/~abeinges/blah/everyone-poops).
+
+It's ok not to run a destructor when a value is leaked because its storage isn't deallocated or repurposed. If the storage is initialized and is being deallocated or repurposed then destructors need to be run first, because [memory may be pinned](https://doc.rust-lang.org/nightly/std/pin/index.html#drop-guarantee). Having said that, there can still be exceptions for skipping destructors when deallocating if you can guarantee there's never pinning involved.
+
+## For reviewers
+
+If there's a `Drop` impl involved, look out for possible soundness issues that could come from that destructor never running.

src/code-considerations/safety-and-soundness/summary.md

@@ -0,0 +1,13 @@
+# Safety and soundness
+
+**Status:** Stub
+
+Unsafe code blocks in the standard library need a comment explaining why they're [ok](https://doc.rust-lang.org/nomicon). There's a lint that checks this. The unsafe code also needs to actually be ok.
+
+The rules around what's sound and what's not can be subtle. See the [Unsafe Code Guidelines WG](https://github.com/rust-lang/unsafe-code-guidelines) for current thinking, and consider pinging `@rust-lang/libs-impl`, `@rust-lang/lang`, and/or somebody from the WG if you're in _any_ doubt. We love debating the soundness of unsafe code, and the more eyes on it the better!
+
+## For reviewers
+
+Look out for any unsafe blocks. If they're optimizations consider whether they're actually necessary. If the unsafe code is necessary then always feel free to ping somebody to help review it.
+
+Look at the level of test coverage for the new unsafe code. Tests do catch bugs!

src/code-considerations/summary.md

@@ -0,0 +1,11 @@
+# Code considerations
+
+Code considerations capture our experiences working on the standard library for all contributors. If you come across something new or unexpected then a code consideration is a great place to record it. Then other contributors and reviewers can find it by searching the guide.
+
+## How to write a code consideration
+
+Code considerations are a bit like guidelines. They should try make concrete recommendations that reviewers and contributors can refer to in discussions. A link to a real case where this was discussed or tripped us up is good to include.
+
+Code considerations should also try include a _For reviewers_ section. These can call out specific things to look out for in reviews that could suggest the consideration applies. They can also include advice on how to apply it.
+
+It's more important that we capture these experiences _somehow_ though, so don't be afraid to drop some sketchy notes in and debate the details later!

src/code-considerations/using-unstable-lang/const-generics.md

@@ -0,0 +1,25 @@
+# Using const generics
+
+**Status:** Stub
+
+Complete const generics are currently unstable. You can track their progress [here](https://github.com/rust-lang/rust/issues/44580).
+
+Const generics are ok to use in public APIs, so long as they fit in the [`min_const_generics` subset](https://github.com/rust-lang/rust/issues/74878).
+
+## For reviewers
+
+Look out for const operations on const generics in public APIs like:
+
+```rust
+pub fn extend_array<T, const N: usize, const M: usize>(arr: [T; N]) -> [T; N + 1] {
+    ..
+}
+```
+
+or for const generics that aren't integers, bools, or chars:
+
+```rust
+pub fn tag<const S: &'static str>() {
+    ..
+}
+```