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

An initial section for libs docs #281

Merged
merged 15 commits into from
Dec 5, 2019
2 changes: 2 additions & 0 deletions src/SUMMARY.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions src/libs/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Libs
This section documents meta processes by the libs team.

132 changes: 132 additions & 0 deletions src/libs/maintaining-std.md
Original file line number Diff line number Diff line change
@@ -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 [`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 [`highfive`] 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 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.

### 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.

### 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 have any generics in scope.
- On methods on traits that don’t have a default implementation.

#### 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is limited to generics, right? Can't adding a second impl of some trait always cause inference failures, regardless of whether any of the impls is generic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I was mostly thinking of the case here where we'll optimistically type things when there's only a single trait impl. Say we have something like:

impl From<&'_ str> for Arc<str> { .. }

let b = Arc::from("a");

then we add:

impl From<&'_ str> for Arc<str> { .. }
+ impl From<&'_ str> for Arc<String> { .. }

then

let b = Arc::from("a");

will no longer compile, because we're relying on inference to figure out the T in Box<T>. I think this sort of thing comes up every now and again with types like Arc, and in new FromIterator impls.

Can we think of any other examples of real inference breakage that could make this section more useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another case to consider is deref coersion: rust-lang/rust#51916


#### Are there `#[fundamental]` items involved?

Blanket trait impls can't be added to `#[fundamental]` types because they have different coherence rules. That includes:

- `Box<T>`
- `Pin<T>`

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.
KodrAus marked this conversation as resolved.
Show resolved Hide resolved

### 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].
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @rust-lang/wg-unsafe-code-guidelines

Copy link
Member

Choose a reason for hiding this comment

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

Seems accurate in general. However, also see these UCG issues. And note that for some instances, we do guarantee things about destructors -- I am talking about the drop guarantee for pinned data. So, just because mem::forget is safe doesn't mean you can just drop arbitrary storage (like stack frames) without running the destructors. mem::forget leaks, that is okay -- but it doesn't deallocate without dropping, which is not (in general) okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks, I wasn't familiar with the drop guarantee for Pin!

The kind of undesirable invariant I was trying to catch with this section was just along the same lines as Vec::drain. Maybe we should reword this one around whether or not there are manual impls for Drop, and some of the things to keep in mind there? Like remembering that leaking is a thing?

Copy link
Member

Choose a reason for hiding this comment

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

It is definitely worth mentioning that mem::forget is a thing, and that old Vec::drain and thread::spawn are unsound. But at the same time I think we should remind the reader that everything can leak does not imply that we can just deallocate any storage without calling destructors. This is an incorrect inference I have seen several people make.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what we're saying here is that, in general, you can't guarantee an impl of Drop will get called, but you must ensure that an impl of Drop will get called if its storage is being deallocated/repourposed through something like ptr::write? And that's on the burden of the caller to ptr::write to enforce?

Copy link
Member

@RalfJung RalfJung Nov 29, 2019

Choose a reason for hiding this comment

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

Yes. Calling all relevant Drop impls is a precondition to deallocating or repurposing storage.

Or, in other words: there is no guarantee that an impl of Drop will get called. However, if the impl of Drop does not get called, then the memory backing that instance of the type must leak.

Note that this is true in general, i.e., when working with arbitrary T. Under more controlled circumstances, skipping Drop can be fine -- e.g. if you know that there is definitely nothing pinned in that storage you are repurposing.


### 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`.
KodrAus marked this conversation as resolved.
Show resolved Hide resolved

### 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
[`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