From 23ad811b33fb716305f69e41f57e7f8a9222a65e Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Tue, 26 Nov 2019 20:37:32 +1000 Subject: [PATCH 01/15] an initial section for libs docs --- src/SUMMARY.md | 2 + src/libs/README.md | 3 + src/libs/maintaining-std.md | 132 ++++++++++++++++++++++++++++++++++++ 3 files changed, 137 insertions(+) create mode 100644 src/libs/README.md create mode 100644 src/libs/maintaining-std.md diff --git a/src/SUMMARY.md b/src/SUMMARY.md index 8867d89bf..fdf75f194 100644 --- a/src/SUMMARY.md +++ b/src/SUMMARY.md @@ -36,6 +36,8 @@ - [Language](./lang/README.md) - [RFC Merge Procedure](./lang/rfc-merge-procedure.md) - [Triage Meeting Procedure](./lang/triage-meeting-procedure.md) +- [Libs](./libs/README.md) + - [Maintaining the standard library](./libs/maintaining-std.md) - [Release](./release/README.md) - [Beta Backporting](./release/beta-backporting.md) - [Platform Support](./release/platform-support.md) diff --git a/src/libs/README.md b/src/libs/README.md new file mode 100644 index 000000000..cc1359aa0 --- /dev/null +++ b/src/libs/README.md @@ -0,0 +1,3 @@ +# Libs +This section documents meta processes by the libs team. + diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md new file mode 100644 index 000000000..4a4f0eb57 --- /dev/null +++ b/src/libs/maintaining-std.md @@ -0,0 +1,132 @@ +# Maintaining the standard library + +> Everything I wish I knew before somebody gave me `r+` + +This document is an effort to capture some of the context needed to develop and maintain the Rust standard library. It’s goal is to help members of the Libs team share the process and experience they bring to working on the standard library so other members can benefit. It’ll probably accumulate a lot of trivia that might also be interesting to members of the wider Rust community. + +This document doesn't attempt to discuss best practices or good style. For that, see the [API Guidelines]. + +## Terms + +- Libs. That's us! The team responsible for development and maintenance of the standard library (among other things). +- Pull request (PR). A regular GitHub pull request against [`rust-lang/rust`]. +- Request for Comment (RFC). A formal document created in [`rust-lang/rfcs`] that introduces new features. +- Tracking Issue. A regular issue on GitHub that’s tagged with `C-tracking-issue`. +- Final Comment Period (FCP). Coordinated by [`rfcbot`] that gives relevant teams a chance to review RFCs and PRs. + +## If you’re ever unsure… + +Maintaining the standard library can feel like a daunting responsibility! Through the automated reviewer assignment you’ll find yourself dropped into a lot of new contexts. + +Ping the `@rust-lang/libs` team on GitHub anytime. We’re all here to help! + +If you don’t think you’re the best person to review a PR then use [`bors`] to assign it to somebody else. + +## Finding reviews waiting for your input + +Please remember to regularly check https://rfcbot.rs/. Click on any occurrence of your nickname to go to a page like https://rfcbot.rs/fcp/SimonSapin that only shows the reviews that are waiting for your input. + +## Reviewing PRs + +As a member of the Libs team you’ll find yourself assigned to PRs that need reviewing, and your input requested on issues in the Rust project. + +### When is a RFC needed? + +New unstable features don’t need a RFC before they can be merged. They do need one before they can be stabilized. New impls for already stable traits don't need a RFC. If a new unstable feature is large or has a lot of design space to explore then you might want to block merging it on a RFC. + +### Is there any `unsafe`? + +Unsafe code blocks in the standard library need a comment explaining why they're [ok](https://doc.rust-lang.org/nomicon). There's a `tidy` 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] for current thinking, and consider pinging `@rust-lang/libs` and/or somebody from the WG if you're in any doubt. + +### Is that `#[inline]` right? + +Inlining is a trade-off between potential execution speed, compile time and code size. + +You should add `#[inline]`: + +- To public, small, non-generic functions. + +You shouldn’t need `#[inline]`: + +- On methods that take a generic argument. +- On methods on traits that don’t have a default implementation. +- On `const` items. + +#### 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. + +### Is there any potential breakage? + +Breaking changes should be avoided when possible. [RFC 1105] 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` run. + +For changes where the value is high and the impact is high too, there are strategies for minimizing the impact: + +- Using compiler lints to try phase out broken behavior. + +### Could this affect inference? + +New impls on public traits for public items may cause inference to break when there are generics involved. + +#### Are there `#[fundamental]` items involved? + +Blanket trait impls can't be added to `#[fundamental]` types because they have different coherence rules. That includes: + +- `Box` +- `Pin` + +Also see [RFC 1023] for details. + +### Is there specialization involved? + +We try to avoid leaning on specialization too heavily, limiting its use to optimizing specific implementations. Any use of specialization that changes how methods are dispatched for external callers should be carefully considered. + +### Does this change drop order? + +Changes to collection internals may affect the order their items are dropped in. This has been accepted in the past, but should be noted. + +### Could `mem::replace` break assumptions? + +Any value behind a `&mut` reference can be replaced with a new one. + +### Could `mem::forget` break assumptions? + +Rust doesn't guarantee destructors will run, so code should avoid relying on them for safety. Remember, [everybody poops][Everybody Poops]. + +### Is the commit log tidy? + +PRs shouldn’t have merge commits in them. If they become out of date with `master` then they need to be rebased. + +## Merging PRs + +PRs to `rust-lang/rust` aren’t merged manually using GitHub’s UI or by pushing remote branches. Everything goes through [`bors`]. + +### When you’re confident it’ll build + +Consider explicitly specifying `rollup`. + +### When there’s new public items + +If the feature is new, then a tracking issue should be opened for it. The `issue` field on `#[unstable]` attributes should be updated with the tracking issue number. + +Unstable features can be merged as normal through [`bors`] once they look ready. + +### When there’s new trait impls + +There’s no way to make a trait impl `#[unstable]`, so **any PRs that add new impls for already `#[stable]` traits must go through a FCP before merging.** + +### When a feature is being stabilized + +Features can be stabilized in a PR that replaces `#[unstable]` attributes with `#[stable]` ones. The feature needs to have an accepted RFC before stabilizing. They also need to go through a FCP before merging. + +[API Guidelines]: https://rust-lang.github.io/api-guidelines +[Unsafe Code Guidelines WG]: https://github.com/rust-lang/unsafe-code-guidelines +[`rust-lang/rust`]: https://github.com/rust-lang/rust +[`rust-lang/rfcs`]: https://github.com/rust-lang/rfcs +[`rfcbot`]: https://github.com/rust-lang/rfcbot-rs +[`bors`]: https://github.com/rust-lang/homu +[RFC 1023]: https://rust-lang.github.io/rfcs/1023-rebalancing-coherence.html +[RFC 1105]: https://rust-lang.github.io/rfcs/1105-api-evolution.html +[Everyone Poops]: http://cglab.ca/~abeinges/blah/everyone-poops From 9e1f37391b03a254e9d9639f521c55c71f8a0903 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Wed, 27 Nov 2019 07:01:01 +1000 Subject: [PATCH 02/15] Update src/libs/maintaining-std.md Co-Authored-By: Jonas Schievink --- src/libs/maintaining-std.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md index 4a4f0eb57..5c54dafca 100644 --- a/src/libs/maintaining-std.md +++ b/src/libs/maintaining-std.md @@ -50,7 +50,7 @@ You should add `#[inline]`: You shouldn’t need `#[inline]`: -- On methods that take a generic argument. +- On methods that have any generics in scope. - On methods on traits that don’t have a default implementation. - On `const` items. From 73d19e939a73b9d038f33ba0fae16ff2b84f4223 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Wed, 27 Nov 2019 07:01:08 +1000 Subject: [PATCH 03/15] Update src/libs/maintaining-std.md Co-Authored-By: Jonas Schievink --- src/libs/maintaining-std.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md index 5c54dafca..684b75e23 100644 --- a/src/libs/maintaining-std.md +++ b/src/libs/maintaining-std.md @@ -30,7 +30,7 @@ Please remember to regularly check https://rfcbot.rs/. Click on any occurrence o As a member of the Libs team you’ll find yourself assigned to PRs that need reviewing, and your input requested on issues in the Rust project. -### When is a RFC needed? +### When is an RFC needed? New unstable features don’t need a RFC before they can be merged. They do need one before they can be stabilized. New impls for already stable traits don't need a RFC. If a new unstable feature is large or has a lot of design space to explore then you might want to block merging it on a RFC. From cb6799b467e3470b3ab6284c4f7b966160fc450c Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Wed, 27 Nov 2019 07:01:15 +1000 Subject: [PATCH 04/15] Update src/libs/maintaining-std.md Co-Authored-By: Jonas Schievink --- src/libs/maintaining-std.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md index 684b75e23..f108d0b19 100644 --- a/src/libs/maintaining-std.md +++ b/src/libs/maintaining-std.md @@ -32,7 +32,7 @@ As a member of the Libs team you’ll find yourself assigned to PRs that need re ### When is an RFC needed? -New unstable features don’t need a RFC before they can be merged. They do need one before they can be stabilized. New impls for already stable traits don't need a RFC. If a new unstable feature is large or has a lot of design space to explore then you might want to block merging it on a RFC. +New unstable features don’t need an RFC before they can be merged. They do need one before they can be stabilized. New impls for already stable traits don't need an RFC. If a new unstable feature is large or has a lot of design space to explore then you might want to block merging it on an RFC. ### Is there any `unsafe`? From daa23275b137d3e01964cf2bf3a2cf22da245902 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Wed, 27 Nov 2019 07:31:34 +1000 Subject: [PATCH 05/15] Update maintaining-std.md --- src/libs/maintaining-std.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md index f108d0b19..41ea98878 100644 --- a/src/libs/maintaining-std.md +++ b/src/libs/maintaining-std.md @@ -16,11 +16,11 @@ This document doesn't attempt to discuss best practices or good style. For that, ## If you’re ever unsure… -Maintaining the standard library can feel like a daunting responsibility! Through the automated reviewer assignment you’ll find yourself dropped into a lot of new contexts. +Maintaining the standard library can feel like a daunting responsibility! Through [`highfive`], the automated reviewer assignment, you’ll find yourself dropped into a lot of new contexts. Ping the `@rust-lang/libs` team on GitHub anytime. We’re all here to help! -If you don’t think you’re the best person to review a PR then use [`bors`] to assign it to somebody else. +If you don’t think you’re the best person to review a PR then use [`highfive`] to assign it to somebody else. ## Finding reviews waiting for your input @@ -32,13 +32,13 @@ As a member of the Libs team you’ll find yourself assigned to PRs that need re ### When is an RFC needed? -New unstable features don’t need an RFC before they can be merged. They do need one before they can be stabilized. New impls for already stable traits don't need an RFC. If a new unstable feature is large or has a lot of design space to explore then you might want to block merging it on an RFC. +New unstable features don't need an RFC before they can be merged. If the feature is small, and the design space is straightforward, stabilizing it usually only requires the feature to go through FCP. Sometimes however, Libs may ask for an RFC before stabilizing. ### Is there any `unsafe`? Unsafe code blocks in the standard library need a comment explaining why they're [ok](https://doc.rust-lang.org/nomicon). There's a `tidy` 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] for current thinking, and consider pinging `@rust-lang/libs` and/or somebody from the WG if you're in any doubt. +The rules around what's sound and what's not can be subtle. See the [Unsafe Code Guidelines WG] for current thinking, and consider pinging `@rust-lang/libs`, `@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. ### Is that `#[inline]` right? @@ -52,7 +52,6 @@ You shouldn’t need `#[inline]`: - On methods that have any generics in scope. - On methods on traits that don’t have a default implementation. -- On `const` items. #### What about `#[inline(always)]`? @@ -101,7 +100,7 @@ PRs shouldn’t have merge commits in them. If they become out of date with `mas ## Merging PRs -PRs to `rust-lang/rust` aren’t merged manually using GitHub’s UI or by pushing remote branches. Everything goes through [`bors`]. +PRs to [`rust-lang/rust`] aren’t merged manually using GitHub’s UI or by pushing remote branches. Everything goes through [`bors`]. ### When you’re confident it’ll build @@ -127,6 +126,7 @@ Features can be stabilized in a PR that replaces `#[unstable]` attributes with ` [`rust-lang/rfcs`]: https://github.com/rust-lang/rfcs [`rfcbot`]: https://github.com/rust-lang/rfcbot-rs [`bors`]: https://github.com/rust-lang/homu +[`highfive`]: https://github.com/rust-lang/highfive [RFC 1023]: https://rust-lang.github.io/rfcs/1023-rebalancing-coherence.html [RFC 1105]: https://rust-lang.github.io/rfcs/1105-api-evolution.html [Everyone Poops]: http://cglab.ca/~abeinges/blah/everyone-poops From e9953355347101842a092f31abbc7bfad2335ac5 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Wed, 27 Nov 2019 07:38:15 +1000 Subject: [PATCH 06/15] Update maintaining-std.md --- src/libs/maintaining-std.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md index 41ea98878..2ec897a62 100644 --- a/src/libs/maintaining-std.md +++ b/src/libs/maintaining-std.md @@ -102,9 +102,11 @@ PRs shouldn’t have merge commits in them. If they become out of date with `mas PRs to [`rust-lang/rust`] aren’t merged manually using GitHub’s UI or by pushing remote branches. Everything goes through [`bors`]. -### When you’re confident it’ll build +### When to `rollup` -Consider explicitly specifying `rollup`. +For Libs PRs, rolling up is usually fine, in particular if it's only a new unstable addition or if it only touches docs (with the exception of intra doc links which complicates things while the feature has bugs...). + +If a submodule is affected then probably don't `rollup`. If the feature affects perf then also avoid `rollup`. ### When there’s new public items From a96dd82fa8d470a5afba573a6a90fc924ec45f6f Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Mon, 2 Dec 2019 09:05:38 +1000 Subject: [PATCH 07/15] Update maintaining-std.md --- src/libs/maintaining-std.md | 97 +++++++++++++++++++++++++++++++------ 1 file changed, 83 insertions(+), 14 deletions(-) diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md index 2ec897a62..56a10a7fc 100644 --- a/src/libs/maintaining-std.md +++ b/src/libs/maintaining-std.md @@ -59,40 +59,107 @@ You should just about never need `#[inline(always)]`. It may be beneficial for p ### Is there any potential breakage? -Breaking changes should be avoided when possible. [RFC 1105] 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` run. +Breaking changes should be avoided when possible. [RFC 1105] 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`] run. For changes where the value is high and the impact is high too, there are strategies for minimizing the impact: - Using compiler lints to try phase out broken behavior. -### Could this affect inference? +#### Inference breaks when a second generic impl is introduced -New impls on public traits for public items may cause inference to break when there are generics involved. +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: -#### Are there `#[fundamental]` items involved? +```rust +// in `std` +impl From<&str> for Arc { .. } +``` -Blanket trait impls can't be added to `#[fundamental]` types because they have different coherence rules. That includes: +```rust +// in an external `lib` +let b = Arc::from("a"); +``` +then we add: + +```diff +impl From<&str> for Arc { .. } ++ impl From<&str> for Arc { .. } +``` + +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`. + +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 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`] run should estimate the scope. + +### Are there `#[fundamental]` items involved? + +Blanket trait impls can't be added to `#[fundamental]` types because they have different coherence rules. See [RFC 1023] for details. That includes: + +- `&T` +- `&mut T` - `Box` - `Pin` -Also see [RFC 1023] for details. - -### Is there specialization involved? +### Is specialization involved? -We try to avoid leaning on specialization too heavily, limiting its use to optimizing specific implementations. Any use of specialization that changes how methods are dispatched for external callers should be carefully considered. +We try to avoid leaning on specialization too heavily, limiting its use to optimizing specific implementations. These specialized optimizations use a private trait to find the correct implementation, rather than specializing the public method itself. Any use of specialization that changes how methods are dispatched for external callers should be carefully considered. ### Does this change drop order? Changes to collection internals may affect the order their items are dropped in. This has been accepted in the past, but should be noted. -### Could `mem::replace` break assumptions? +### How could `mem` 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`. -Any value behind a `&mut` reference can be replaced with a new one. +#### `mem::forget` -### Could `mem::forget` break assumptions? +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, [everybody poops][Everybody Poops]. -Rust doesn't guarantee destructors will run, so code should avoid relying on them for safety. Remember, [everybody poops][Everybody Poops]. +It's ok not to run a destructor when a value is leaked because its storage isn't deallocated or repurposed. It's generally _not_ ok not to run a destructor before deallocating or repurposing already initialized storage because [memory may be pinned][Drop guarantee]. Having said that, there can be exceptions for skipping destructors if you can guarantee there's never pinning involved. ### Is the commit log tidy? @@ -116,7 +183,7 @@ Unstable features can be merged as normal through [`bors`] once they look ready. ### When there’s new trait impls -There’s no way to make a trait impl `#[unstable]`, so **any PRs that add new impls for already `#[stable]` traits must go through a FCP before merging.** +There’s no way to make a trait impl `#[unstable]`, so **any PRs that add new impls for already stable traits must go through a FCP before merging.** If the trait itself is unstable though, then the impl needs to be unstable too. ### When a feature is being stabilized @@ -129,6 +196,8 @@ Features can be stabilized in a PR that replaces `#[unstable]` attributes with ` [`rfcbot`]: https://github.com/rust-lang/rfcbot-rs [`bors`]: https://github.com/rust-lang/homu [`highfive`]: https://github.com/rust-lang/highfive +[`crater`]: https://github.com/rust-lang/crater +[Drop guarantee]: https://doc.rust-lang.org/nightly/std/pin/index.html#drop-guarantee [RFC 1023]: https://rust-lang.github.io/rfcs/1023-rebalancing-coherence.html [RFC 1105]: https://rust-lang.github.io/rfcs/1105-api-evolution.html [Everyone Poops]: http://cglab.ca/~abeinges/blah/everyone-poops From 56f79026a0fe499f3067b6a1c17112bbc8664af3 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Mon, 2 Dec 2019 09:20:39 +1000 Subject: [PATCH 08/15] Update maintaining-std.md --- src/libs/maintaining-std.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md index 56a10a7fc..c382ef34c 100644 --- a/src/libs/maintaining-std.md +++ b/src/libs/maintaining-std.md @@ -32,13 +32,13 @@ As a member of the Libs team you’ll find yourself assigned to PRs that need re ### When is an RFC needed? -New unstable features don't need an RFC before they can be merged. If the feature is small, and the design space is straightforward, stabilizing it usually only requires the feature to go through FCP. Sometimes however, Libs may ask for an RFC before stabilizing. +New unstable features don't need an RFC before they can be merged. If the feature is small, and the design space is straightforward, stabilizing it usually only requires the feature to go through FCP. Sometimes however, you may ask for an RFC before stabilizing. ### Is there any `unsafe`? Unsafe code blocks in the standard library need a comment explaining why they're [ok](https://doc.rust-lang.org/nomicon). There's a `tidy` 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] for current thinking, and consider pinging `@rust-lang/libs`, `@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. +The rules around what's sound and what's not can be subtle. See the [Unsafe Code Guidelines WG] for current thinking, and consider pinging `@rust-lang/libs`, `@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! ### Is that `#[inline]` right? @@ -65,6 +65,8 @@ For changes where the value is high and the impact is high too, there are strate - Using compiler lints to try phase out broken behavior. +The following sections outline some kinds of breakage that may not be obvious just from the change made to the standard library. + #### 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: @@ -161,6 +163,10 @@ Rust doesn't guarantee destructors will run when a value is leaked (which can be It's ok not to run a destructor when a value is leaked because its storage isn't deallocated or repurposed. It's generally _not_ ok not to run a destructor before deallocating or repurposing already initialized storage because [memory may be pinned][Drop guarantee]. Having said that, there can be exceptions for skipping destructors if you can guarantee there's never pinning involved. +### How is performance impacted? + +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`] run. + ### Is the commit log tidy? PRs shouldn’t have merge commits in them. If they become out of date with `master` then they need to be rebased. @@ -177,7 +183,7 @@ If a submodule is affected then probably don't `rollup`. If the feature affects ### When there’s new public items -If the feature is new, then a tracking issue should be opened for it. The `issue` field on `#[unstable]` attributes should be updated with the tracking issue number. +If the feature is new, then a tracking issue should be opened for it. Have a look at some previous [tracking issues][Libs tracking issues] to get an idea of what needs to go in there. The `issue` field on `#[unstable]` attributes should be updated with the tracking issue number. Unstable features can be merged as normal through [`bors`] once they look ready. @@ -197,6 +203,8 @@ Features can be stabilized in a PR that replaces `#[unstable]` attributes with ` [`bors`]: https://github.com/rust-lang/homu [`highfive`]: https://github.com/rust-lang/highfive [`crater`]: https://github.com/rust-lang/crater +[`rust-timer`]: https://github.com/rust-lang-nursery/rustc-perf +[Libs tracking issues]: https://github.com/rust-lang/rust/issues?q=label%3AC-tracking-issue+label%3AT-libs [Drop guarantee]: https://doc.rust-lang.org/nightly/std/pin/index.html#drop-guarantee [RFC 1023]: https://rust-lang.github.io/rfcs/1023-rebalancing-coherence.html [RFC 1105]: https://rust-lang.github.io/rfcs/1105-api-evolution.html From 987356874069512b0845dd5b723a995b4b5e88c8 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Mon, 2 Dec 2019 09:24:54 +1000 Subject: [PATCH 09/15] Update maintaining-std.md --- src/libs/maintaining-std.md | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md index c382ef34c..a1a0b3760 100644 --- a/src/libs/maintaining-std.md +++ b/src/libs/maintaining-std.md @@ -61,13 +61,23 @@ You should just about never need `#[inline(always)]`. It may be beneficial for p Breaking changes should be avoided when possible. [RFC 1105] 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`] run. -For changes where the value is high and the impact is high too, there are strategies for minimizing the impact: +#### Managing breakage + +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. -The following sections outline some kinds of breakage that may not be obvious just from the change made to the standard library. +If the impact isn't too high: + +- Looping in maintainers of broken crates and submitting PRs to fix them. + +#### Trait impls break things + +The following sections outline some kinds of breakage from new trait impls that may not be obvious just from the change made to the standard library. -#### Inference breaks when a second generic impl is introduced +##### 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: @@ -98,7 +108,7 @@ will no longer compile, because we've previously been relying on inference to fi This kind of breakage can be ok, but a [`crater`] run should estimate the scope. -#### Deref coercion breaks when a new impl is introduced +##### 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: From 6eb68c3207c70143f5f01f598cd4df98c737e062 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Tue, 3 Dec 2019 08:15:26 +1000 Subject: [PATCH 10/15] Update maintaining-std.md --- src/libs/maintaining-std.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md index a1a0b3760..98c2531c3 100644 --- a/src/libs/maintaining-std.md +++ b/src/libs/maintaining-std.md @@ -169,7 +169,7 @@ Any value behind a `&mut` reference can be replaced with a new one using `mem::r #### `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, [everybody poops][Everybody Poops]. +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][Everyone Poops]. It's ok not to run a destructor when a value is leaked because its storage isn't deallocated or repurposed. It's generally _not_ ok not to run a destructor before deallocating or repurposing already initialized storage because [memory may be pinned][Drop guarantee]. Having said that, there can be exceptions for skipping destructors if you can guarantee there's never pinning involved. From 6306fa54600711084f17f7e551d328a69e4755ed Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Tue, 3 Dec 2019 08:31:14 +1000 Subject: [PATCH 11/15] Update maintaining-std.md --- src/libs/maintaining-std.md | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md index 98c2531c3..ef5377398 100644 --- a/src/libs/maintaining-std.md +++ b/src/libs/maintaining-std.md @@ -61,8 +61,6 @@ You should just about never need `#[inline(always)]`. It may be beneficial for p Breaking changes should be avoided when possible. [RFC 1105] 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`] run. -#### Managing breakage - There are strategies for mitigating breakage depending on the impact. For changes where the value is high and the impact is high too: @@ -73,11 +71,11 @@ If the impact isn't too high: - Looping in maintainers of broken crates and submitting PRs to fix them. -#### Trait impls break things +### Are there new impls for stable traits? -The following sections outline some kinds of breakage from new trait impls that may not be obvious just from the change made to the standard library. +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. -##### Inference breaks when a second generic impl is introduced +#### 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: @@ -108,7 +106,7 @@ will no longer compile, because we've previously been relying on inference to fi This kind of breakage can be ok, but a [`crater`] run should estimate the scope. -##### Deref coercion breaks when a new impl is introduced +#### 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: @@ -144,6 +142,10 @@ will no longer compile, because we won't attempt to use deref to coerce the `&St This kind of breakage can be ok, but a [`crater`] run should estimate the scope. +### Could an implementation use existing functionality? + +Types like `String` are implemented in terms of `Vec` and can use methods on `str` through deref coersion. `Vec` can use methods on `[T]` through deref coersion. When possible, methods on a wrapping type like `String` should defer to methods that already exis on their underlying storage or deref target. + ### Are there `#[fundamental]` items involved? Blanket trait impls can't be added to `#[fundamental]` types because they have different coherence rules. See [RFC 1023] for details. That includes: @@ -165,7 +167,7 @@ Changes to collection internals may affect the order their items are dropped in. #### `mem::replace` and `mem::swap` -Any value behind a `&mut` reference can be replaced with a new one using `mem::replace` or `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` @@ -199,12 +201,14 @@ Unstable features can be merged as normal through [`bors`] once they look ready. ### When there’s new trait impls -There’s no way to make a trait impl `#[unstable]`, so **any PRs that add new impls for already stable traits must go through a FCP before merging.** If the trait itself is unstable though, then the impl needs to be unstable too. +There’s no way to make a trait impl for a stable trait unstable, so **any PRs that add new impls for already stable traits must go through a FCP before merging.** If the trait itself is unstable though, then the impl needs to be unstable too. ### When a feature is being stabilized Features can be stabilized in a PR that replaces `#[unstable]` attributes with `#[stable]` ones. The feature needs to have an accepted RFC before stabilizing. They also need to go through a FCP before merging. +You can find the right version to use in the `#[stable]` attribute by checking the [Forge]. + [API Guidelines]: https://rust-lang.github.io/api-guidelines [Unsafe Code Guidelines WG]: https://github.com/rust-lang/unsafe-code-guidelines [`rust-lang/rust`]: https://github.com/rust-lang/rust @@ -216,6 +220,7 @@ Features can be stabilized in a PR that replaces `#[unstable]` attributes with ` [`rust-timer`]: https://github.com/rust-lang-nursery/rustc-perf [Libs tracking issues]: https://github.com/rust-lang/rust/issues?q=label%3AC-tracking-issue+label%3AT-libs [Drop guarantee]: https://doc.rust-lang.org/nightly/std/pin/index.html#drop-guarantee +[Forge]: https://forge.rust-lang.org/ [RFC 1023]: https://rust-lang.github.io/rfcs/1023-rebalancing-coherence.html [RFC 1105]: https://rust-lang.github.io/rfcs/1105-api-evolution.html [Everyone Poops]: http://cglab.ca/~abeinges/blah/everyone-poops From 2b321edca06b76747a5e781ec854f9936ef8c2ef Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Thu, 5 Dec 2019 09:46:22 +1000 Subject: [PATCH 12/15] fix a typo --- src/libs/maintaining-std.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md index ef5377398..5a16932c3 100644 --- a/src/libs/maintaining-std.md +++ b/src/libs/maintaining-std.md @@ -144,7 +144,7 @@ This kind of breakage can be ok, but a [`crater`] run should estimate the scope. ### Could an implementation use existing functionality? -Types like `String` are implemented in terms of `Vec` and can use methods on `str` through deref coersion. `Vec` can use methods on `[T]` through deref coersion. When possible, methods on a wrapping type like `String` should defer to methods that already exis on their underlying storage or deref target. +Types like `String` are implemented in terms of `Vec` and can use methods on `str` through deref coersion. `Vec` can use methods on `[T]` through deref coersion. When possible, methods on a wrapping type like `String` should defer to methods that already exist on their underlying storage or deref target. ### Are there `#[fundamental]` items involved? From 709d7568765c6c499cb39f049dcc707240282112 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Thu, 5 Dec 2019 09:53:00 +1000 Subject: [PATCH 13/15] Update src/libs/maintaining-std.md Co-Authored-By: Mark Rousskov --- src/libs/maintaining-std.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md index 5a16932c3..b32f69def 100644 --- a/src/libs/maintaining-std.md +++ b/src/libs/maintaining-std.md @@ -191,7 +191,7 @@ PRs to [`rust-lang/rust`] aren’t merged manually using GitHub’s UI or by pus For Libs PRs, rolling up is usually fine, in particular if it's only a new unstable addition or if it only touches docs (with the exception of intra doc links which complicates things while the feature has bugs...). -If a submodule is affected then probably don't `rollup`. If the feature affects perf then also avoid `rollup`. +If a submodule is affected then probably don't `rollup`. If the feature affects perf then also avoid `rollup` -- mark it as `rollup=never`. ### When there’s new public items From d9a3467988fb61c9dde8a58ed228da7c5ae1c412 Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Thu, 5 Dec 2019 11:00:13 +1000 Subject: [PATCH 14/15] avoid some double negatives --- src/libs/maintaining-std.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md index b32f69def..523900ba4 100644 --- a/src/libs/maintaining-std.md +++ b/src/libs/maintaining-std.md @@ -173,7 +173,7 @@ Any value behind a `&mut` reference can be replaced with a new one using `mem::r 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][Everyone Poops]. -It's ok not to run a destructor when a value is leaked because its storage isn't deallocated or repurposed. It's generally _not_ ok not to run a destructor before deallocating or repurposing already initialized storage because [memory may be pinned][Drop guarantee]. Having said that, there can be exceptions for skipping destructors if you can guarantee there's never pinning involved. +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 deallocating or repurposing then destructors need to be run first, because [memory may be pinned][Drop guarantee]. Having said that, there can still be exceptions for skipping destructors when deallocating if you can guarantee there's never pinning involved. ### How is performance impacted? From 3a317baf2bdf29fb74d34a379cc009dd5425633f Mon Sep 17 00:00:00 2001 From: Ashley Mannix Date: Thu, 5 Dec 2019 11:04:47 +1000 Subject: [PATCH 15/15] Update maintaining-std.md --- src/libs/maintaining-std.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libs/maintaining-std.md b/src/libs/maintaining-std.md index 523900ba4..a87f397e4 100644 --- a/src/libs/maintaining-std.md +++ b/src/libs/maintaining-std.md @@ -6,6 +6,10 @@ This document is an effort to capture some of the context needed to develop and This document doesn't attempt to discuss best practices or good style. For that, see the [API Guidelines]. +## Contributing + +If you spot anything that is outdated, under specified, missing, or just plain incorrect then feel free to open up a PR on the [`rust-lang/rust-forge`] repository! + ## Terms - Libs. That's us! The team responsible for development and maintenance of the standard library (among other things). @@ -213,6 +217,7 @@ You can find the right version to use in the `#[stable]` attribute by checking t [Unsafe Code Guidelines WG]: https://github.com/rust-lang/unsafe-code-guidelines [`rust-lang/rust`]: https://github.com/rust-lang/rust [`rust-lang/rfcs`]: https://github.com/rust-lang/rfcs +[`rust-lang/rust-forge`]: https://github.com/rust-lang/rust-forge [`rfcbot`]: https://github.com/rust-lang/rfcbot-rs [`bors`]: https://github.com/rust-lang/homu [`highfive`]: https://github.com/rust-lang/highfive