277 lines
14 KiB
Markdown
277 lines
14 KiB
Markdown
# Walkthrough: a typical contribution
|
|
|
|
<!-- toc -->
|
|
|
|
There are _a lot_ of ways to contribute to the Rust compiler, including fixing
|
|
bugs, improving performance, helping design features, providing feedback on
|
|
existing features, etc. This chapter does not claim to scratch the surface.
|
|
Instead, it walks through the design and implementation of a new feature. Not
|
|
all of the steps and processes described here are needed for every
|
|
contribution, and I will try to point those out as they arise.
|
|
|
|
In general, if you are interested in making a contribution and aren't sure
|
|
where to start, please feel free to ask!
|
|
|
|
## Overview
|
|
|
|
The feature I will discuss in this chapter is the `?` Kleene operator for
|
|
macros. Basically, we want to be able to write something like this:
|
|
|
|
```rust,ignore
|
|
macro_rules! foo {
|
|
($arg:ident $(, $optional_arg:ident)?) => {
|
|
println!("{}", $arg);
|
|
|
|
$(
|
|
println!("{}", $optional_arg);
|
|
)?
|
|
}
|
|
}
|
|
|
|
fn main() {
|
|
let x = 0;
|
|
foo!(x); // ok! prints "0"
|
|
foo!(x, x); // ok! prints "0 0"
|
|
}
|
|
```
|
|
|
|
So basically, the `$(pat)?` matcher in the macro means "this pattern can occur
|
|
0 or 1 times", similar to other regex syntaxes.
|
|
|
|
There were a number of steps to go from an idea to stable Rust feature. Here is
|
|
a quick list. We will go through each of these in order below. As I mentioned
|
|
before, not all of these are needed for every type of contribution.
|
|
|
|
- **Idea discussion/Pre-RFC** A Pre-RFC is an early draft or design discussion
|
|
of a feature. This stage is intended to flesh out the design space a bit and
|
|
get a grasp on the different merits and problems with an idea. It's a great
|
|
way to get early feedback on your idea before presenting it the wider
|
|
audience. You can find the original discussion [here][prerfc].
|
|
- **RFC** This is when you formally present your idea to the community for
|
|
consideration. You can find the RFC [here][rfc].
|
|
- **Implementation** Implement your idea unstably in the compiler. You can
|
|
find the original implementation [here][impl1].
|
|
- **Possibly iterate/refine** As the community gets experience with your
|
|
feature on the nightly compiler and in `std`, there may be additional
|
|
feedback about design choice that might be adjusted. This particular feature
|
|
went [through][impl2] a [number][impl3] of [iterations][impl4].
|
|
- **Stabilization** When your feature has baked enough, a Rust team member may
|
|
[propose to stabilize it][merge]. If there is consensus, this is done.
|
|
- **Relax** Your feature is now a stable Rust feature!
|
|
|
|
[prerfc]: https://internals.rust-lang.org/t/pre-rfc-at-most-one-repetition-macro-patterns/6557
|
|
[rfc]: https://github.com/rust-lang/rfcs/pull/2298
|
|
[impl1]: https://github.com/rust-lang/rust/pull/47752
|
|
[impl2]: https://github.com/rust-lang/rust/pull/49719
|
|
[impl3]: https://github.com/rust-lang/rust/pull/51336
|
|
[impl4]: https://github.com/rust-lang/rust/pull/51587
|
|
[merge]: https://github.com/rust-lang/rust/issues/48075#issuecomment-433177613
|
|
|
|
## Pre-RFC and RFC
|
|
|
|
> NOTE: In general, if you are not proposing a _new_ feature or substantial
|
|
> change to Rust or the ecosystem, you don't need to follow the RFC process.
|
|
> Instead, you can just jump to [implementation](#impl).
|
|
>
|
|
> You can find the official guidelines for when to open an RFC [here][rfcwhen].
|
|
|
|
[rfcwhen]: https://github.com/rust-lang/rfcs#when-you-need-to-follow-this-process
|
|
|
|
An RFC is a document that describes the feature or change you are proposing in
|
|
detail. Anyone can write an RFC; the process is the same for everyone,
|
|
including Rust team members.
|
|
|
|
To open an RFC, open a PR on the
|
|
[rust-lang/rfcs](https://github.com/rust-lang/rfcs) repo on GitHub. You can
|
|
find detailed instructions in the
|
|
[README](https://github.com/rust-lang/rfcs#what-the-process-is).
|
|
|
|
Before opening an RFC, you should do the research to "flesh out" your idea.
|
|
Hastily-proposed RFCs tend not to be accepted. You should generally have a good
|
|
description of the motivation, impact, disadvantages, and potential
|
|
interactions with other features.
|
|
|
|
If that sounds like a lot of work, it's because it is. But no fear! Even if
|
|
you're not a compiler hacker, you can get great feedback by doing a _pre-RFC_.
|
|
This is an _informal_ discussion of the idea. The best place to do this is
|
|
internals.rust-lang.org. Your post doesn't have to follow any particular
|
|
structure. It doesn't even need to be a cohesive idea. Generally, you will get
|
|
tons of feedback that you can integrate back to produce a good RFC.
|
|
|
|
(Another pro-tip: try searching the RFCs repo and internals for prior related
|
|
ideas. A lot of times an idea has already been considered and was either
|
|
rejected or postponed to be tried again later. This can save you and everybody
|
|
else some time)
|
|
|
|
In the case of our example, a participant in the pre-RFC thread pointed out a
|
|
syntax ambiguity and a potential resolution. Also, the overall feedback seemed
|
|
positive. In this case, the discussion converged pretty quickly, but for some
|
|
ideas, a lot more discussion can happen (e.g. see [this RFC][nonascii] which
|
|
received a whopping 684 comments!). If that happens, don't be discouraged; it
|
|
means the community is interested in your idea, but it perhaps needs some
|
|
adjustments.
|
|
|
|
[nonascii]: https://github.com/rust-lang/rfcs/pull/2457
|
|
|
|
The RFC for our `?` macro feature did receive some discussion on the RFC thread
|
|
too. As with most RFCs, there were a few questions that we couldn't answer by
|
|
discussion: we needed experience using the feature to decide. Such questions
|
|
are listed in the "Unresolved Questions" section of the RFC. Also, over the
|
|
course of the RFC discussion, you will probably want to update the RFC document
|
|
itself to reflect the course of the discussion (e.g. new alternatives or prior
|
|
work may be added or you may decide to change parts of the proposal itself).
|
|
|
|
In the end, when the discussion seems to reach a consensus and die down a bit,
|
|
a Rust team member may propose to move to "final comment period" (FCP) with one
|
|
of three possible dispositions. This means that they want the other members of
|
|
the appropriate teams to review and comment on the RFC. More discussion may
|
|
ensue, which may result in more changes or unresolved questions being added. At
|
|
some point, when everyone is satisfied, the RFC enters the FCP, which is the
|
|
last chance for people to bring up objections. When the FCP is over, the
|
|
disposition is adopted. Here are the three possible dispositions:
|
|
|
|
- _Merge_: accept the feature. Here is the proposal to merge for our [`?` macro
|
|
feature][rfcmerge].
|
|
- _Close_: this feature in its current form is not a good fit for rust. Don't
|
|
be discouraged if this happens to your RFC, and don't take it personally.
|
|
This is not a reflection on you, but rather a community decision that rust
|
|
will go a different direction.
|
|
- _Postpone_: there is interest in going this direction but not at the moment.
|
|
This happens most often because the appropriate Rust team doesn't have the
|
|
bandwidth to shepherd the feature through the process to stabilization. Often
|
|
this is the case when the feature doesn't fit into the team's roadmap.
|
|
Postponed ideas may be revisited later.
|
|
|
|
[rfcmerge]: https://github.com/rust-lang/rfcs/pull/2298#issuecomment-360582667
|
|
|
|
When an RFC is merged, the PR is merged into the RFCs repo. A new _tracking
|
|
issue_ is created in the [rust-lang/rust] repo to track progress on the feature
|
|
and discuss unresolved questions, implementation progress and blockers, etc.
|
|
Here is the tracking issue on for our [`?` macro feature][tracking].
|
|
|
|
[tracking]: https://github.com/rust-lang/rust/issues/48075
|
|
|
|
<a name="impl"></a>
|
|
|
|
## Implementation
|
|
|
|
To make a change to the compiler, open a PR against the [rust-lang/rust] repo.
|
|
|
|
[rust-lang/rust]: https://github.com/rust-lang/rust
|
|
|
|
Depending on the feature/change/bug fix/improvement, implementation may be
|
|
relatively-straightforward or it may be a major undertaking. You can always ask
|
|
for help or mentorship from more experienced compiler devs. Also, you don't
|
|
have to be the one to implement your feature; but keep in mind that if you
|
|
don't, it might be a while before someone else does.
|
|
|
|
For the `?` macro feature, I needed to go understand the relevant parts of
|
|
macro expansion in the compiler. Personally, I find that [improving the
|
|
comments][comments] in the code is a helpful way of making sure I understand
|
|
it, but you don't have to do that if you don't want to.
|
|
|
|
[comments]: https://github.com/rust-lang/rust/pull/47732
|
|
|
|
I then [implemented][impl1] the original feature, as described in the RFC. When
|
|
a new feature is implemented, it goes behind a _feature gate_, which means that
|
|
you have to use `#![feature(my_feature_name)]` to use the feature. The feature
|
|
gate is removed when the feature is stabilized.
|
|
|
|
**Most bug fixes and improvements** don't require a feature gate. You can just
|
|
make your changes/improvements.
|
|
|
|
When you open a PR on the [rust-lang/rust], a bot will assign your PR to a
|
|
reviewer. If there is a particular Rust team member you are working with, you can
|
|
request that reviewer by leaving a comment on the thread with `r?
|
|
@reviewer-github-id` (e.g. `r? @eddyb`). If you don't know who to request,
|
|
don't request anyone; the bot will assign someone automatically based on which files you changed.
|
|
|
|
The reviewer may request changes before they approve your PR, they may mark the PR with label
|
|
"S-waiting-on-author" after leaving comments, this means that the PR is blocked on you to make
|
|
some requested changes. When you finished iterating on the changes, you can mark the PR as
|
|
`S-waiting-on-review` again by leaving a comment with `@rustbot ready`, this will remove the
|
|
`S-waiting-on-author` label and add the `S-waiting-on-review` label.
|
|
|
|
Feel free to ask questions or discuss things you don't understand or disagree with. However,
|
|
recognize that the PR won't be merged unless someone on the Rust team approves
|
|
it. If a reviewer leave a comment like `r=me after fixing ...`, that means they approve the PR and
|
|
you can merge it with comment with `@bors r=reviwer-github-id`(e.g. `@bors r=eddyb`) to merge it
|
|
after fixing trivial issues. Note that `r=someone` requires permission and bors could say
|
|
something like "🔑 Insufficient privileges..." when commenting `r=someone`. In that case,
|
|
you have to ask the reviewer to revisit your PR.
|
|
|
|
When your reviewer approves the PR, it will go into a queue for yet another bot
|
|
called `@bors`. `@bors` manages the CI build/merge queue. When your PR reaches
|
|
the head of the `@bors` queue, `@bors` will test out the merge by running all
|
|
tests against your PR on GitHub Actions. This takes a lot of time to
|
|
finish. If all tests pass, the PR is merged and becomes part of the next
|
|
nightly compiler!
|
|
|
|
There are a couple of things that may happen for some PRs during the review process
|
|
|
|
- If the change is substantial enough, the reviewer may request an FCP on
|
|
the PR. This gives all members of the appropriate team a chance to review the
|
|
changes.
|
|
- If the change may cause breakage, the reviewer may request a [crater] run.
|
|
This compiles the compiler with your changes and then attempts to compile all
|
|
crates on crates.io with your modified compiler. This is a great smoke test
|
|
to check if you introduced a change to compiler behavior that affects a large
|
|
portion of the ecosystem.
|
|
- If the diff of your PR is large or the reviewer is busy, your PR may have
|
|
some merge conflicts with other PRs that happen to get merged first. You
|
|
should fix these merge conflicts using the normal git procedures.
|
|
|
|
[crater]: ./tests/intro.html#crater
|
|
|
|
If you are not doing a new feature or something like that (e.g. if you are
|
|
fixing a bug), then that's it! Thanks for your contribution :)
|
|
|
|
## Refining your implementation
|
|
|
|
As people get experience with your new feature on nightly, slight changes may
|
|
be proposed and unresolved questions may become resolved. Updates/changes go
|
|
through the same process for implementing any other changes, as described
|
|
above (i.e. submit a PR, go through review, wait for `@bors`, etc).
|
|
|
|
Some changes may be major enough to require an FCP and some review by Rust team
|
|
members.
|
|
|
|
For the `?` macro feature, we went through a few different iterations after the
|
|
original implementation: [1][impl2], [2][impl3], [3][impl4].
|
|
|
|
Along the way, we decided that `?` should not take a separator, which was
|
|
previously an unresolved question listed in the RFC. We also changed the
|
|
disambiguation strategy: we decided to remove the ability to use `?` as a
|
|
separator token for other repetition operators (e.g. `+` or `*`). However,
|
|
since this was a breaking change, we decided to do it over an edition boundary.
|
|
Thus, the new feature can be enabled only in edition 2018. These deviations
|
|
from the original RFC required [another
|
|
FCP](https://github.com/rust-lang/rust/issues/51934).
|
|
|
|
## Stabilization
|
|
|
|
Finally, after the feature had baked for a while on nightly, a language team member
|
|
[moved to stabilize it][stabilizefcp].
|
|
|
|
[stabilizefcp]: https://github.com/rust-lang/rust/issues/48075#issuecomment-433177613
|
|
|
|
A _stabilization report_ needs to be written that includes
|
|
|
|
- brief description of the behavior and any deviations from the RFC
|
|
- which edition(s) are affected and how
|
|
- links to a few tests to show the interesting aspects
|
|
|
|
The stabilization report for our feature is [here][stabrep].
|
|
|
|
[stabrep]: https://github.com/rust-lang/rust/issues/48075#issuecomment-433243048
|
|
|
|
After this, [a PR is made][stab] to remove the feature gate, enabling the feature by
|
|
default (on the 2018 edition). A note is added to the [Release notes][relnotes]
|
|
about the feature.
|
|
|
|
[stab]: https://github.com/rust-lang/rust/pull/56245
|
|
|
|
Steps to stabilize the feature can be found at [Stabilizing Features](./stabilization_guide.md).
|
|
|
|
[relnotes]: https://github.com/rust-lang/rust/blob/master/RELEASES.md
|