]>
Commit | Line | Data |
---|---|---|
a1dfa0c6 XL |
1 | # Walkthrough: a typical contribution |
2 | ||
6a06907d XL |
3 | <!-- toc --> |
4 | ||
a1dfa0c6 XL |
5 | There are _a lot_ of ways to contribute to the rust compiler, including fixing |
6 | bugs, improving performance, helping design features, providing feedback on | |
7 | existing features, etc. This chapter does not claim to scratch the surface. | |
8 | Instead, it walks through the design and implementation of a new feature. Not | |
9 | all of the steps and processes described here are needed for every | |
10 | contribution, and I will try to point those out as they arise. | |
11 | ||
12 | In general, if you are interested in making a contribution and aren't sure | |
13 | where to start, please feel free to ask! | |
14 | ||
15 | ## Overview | |
16 | ||
17 | The feature I will discuss in this chapter is the `?` Kleene operator for | |
18 | macros. Basically, we want to be able to write something like this: | |
19 | ||
20 | ```rust,ignore | |
21 | macro_rules! foo { | |
22 | ($arg:ident $(, $optional_arg:ident)?) => { | |
23 | println!("{}", $arg); | |
24 | ||
25 | $( | |
26 | println!("{}", $optional_arg); | |
27 | )? | |
28 | } | |
29 | } | |
30 | ||
31 | fn main() { | |
32 | let x = 0; | |
33 | foo!(x); // ok! prints "0" | |
34 | foo!(x, x); // ok! prints "0 0" | |
35 | } | |
36 | ``` | |
37 | ||
38 | So basically, the `$(pat)?` matcher in the macro means "this pattern can occur | |
39 | 0 or 1 times", similar to other regex syntaxes. | |
40 | ||
41 | There were a number of steps to go from an idea to stable rust feature. Here is | |
42 | a quick list. We will go through each of these in order below. As I mentioned | |
43 | before, not all of these are needed for every type of contribution. | |
44 | ||
45 | - **Idea discussion/Pre-RFC** A Pre-RFC is an early draft or design discussion | |
46 | of a feature. This stage is intended to flesh out the design space a bit and | |
47 | get a grasp on the different merits and problems with an idea. It's a great | |
48 | way to get early feedback on your idea before presenting it the wider | |
49 | audience. You can find the original discussion [here][prerfc]. | |
50 | - **RFC** This is when you formally present your idea to the community for | |
51 | consideration. You can find the RFC [here][rfc]. | |
60c5eb7d | 52 | - **Implementation** Implement your idea unstably in the compiler. You can |
a1dfa0c6 XL |
53 | find the original implementation [here][impl1]. |
54 | - **Possibly iterate/refine** As the community gets experience with your | |
6a06907d | 55 | feature on the nightly compiler and in `std`, there may be additional |
a1dfa0c6 XL |
56 | feedback about design choice that might be adjusted. This particular feature |
57 | went [through][impl2] a [number][impl3] of [iterations][impl4]. | |
58 | - **Stabilization** When your feature has baked enough, a rust team member may | |
59 | [propose to stabilize it][merge]. If there is consensus, this is done. | |
60 | - **Relax** Your feature is now a stable rust feature! | |
61 | ||
62 | [prerfc]: https://internals.rust-lang.org/t/pre-rfc-at-most-one-repetition-macro-patterns/6557 | |
63 | [rfc]: https://github.com/rust-lang/rfcs/pull/2298 | |
64 | [impl1]: https://github.com/rust-lang/rust/pull/47752 | |
65 | [impl2]: https://github.com/rust-lang/rust/pull/49719 | |
66 | [impl3]: https://github.com/rust-lang/rust/pull/51336 | |
67 | [impl4]: https://github.com/rust-lang/rust/pull/51587 | |
68 | [merge]: https://github.com/rust-lang/rust/issues/48075#issuecomment-433177613 | |
69 | ||
70 | ## Pre-RFC and RFC | |
71 | ||
72 | > NOTE: In general, if you are not proposing a _new_ feature or substantial | |
73 | > change to rust or the ecosystem, you don't need to follow the RFC process. | |
74 | > Instead, you can just jump to [implementation](#impl). | |
75 | > | |
76 | > You can find the official guidelines for when to open an RFC [here][rfcwhen]. | |
77 | ||
78 | [rfcwhen]: https://github.com/rust-lang/rfcs#when-you-need-to-follow-this-process | |
79 | ||
80 | An RFC is a document that describes the feature or change you are proposing in | |
81 | detail. Anyone can write an RFC; the process is the same for everyone, | |
82 | including rust team members. | |
83 | ||
84 | To open an RFC, open a PR on the | |
85 | [rust-lang/rfcs](https://github.com/rust-lang/rfcs) repo on GitHub. You can | |
86 | find detailed instructions in the | |
87 | [README](https://github.com/rust-lang/rfcs#what-the-process-is). | |
88 | ||
89 | Before opening an RFC, you should do the research to "flesh out" your idea. | |
90 | Hastily-proposed RFCs tend not to be accepted. You should generally have a good | |
91 | description of the motivation, impact, disadvantages, and potential | |
92 | interactions with other features. | |
93 | ||
94 | If that sounds like a lot of work, it's because it is. But no fear! Even if | |
95 | you're not a compiler hacker, you can get great feedback by doing a _pre-RFC_. | |
96 | This is an _informal_ discussion of the idea. The best place to do this is | |
97 | internals.rust-lang.org. Your post doesn't have to follow any particular | |
98 | structure. It doesn't even need to be a cohesive idea. Generally, you will get | |
99 | tons of feedback that you can integrate back to produce a good RFC. | |
100 | ||
101 | (Another pro-tip: try searching the RFCs repo and internals for prior related | |
102 | ideas. A lot of times an idea has already been considered and was either | |
103 | rejected or postponed to be tried again later. This can save you and everybody | |
104 | else some time) | |
105 | ||
106 | In the case of our example, a participant in the pre-RFC thread pointed out a | |
107 | syntax ambiguity and a potential resolution. Also, the overall feedback seemed | |
108 | positive. In this case, the discussion converged pretty quickly, but for some | |
109 | ideas, a lot more discussion can happen (e.g. see [this RFC][nonascii] which | |
110 | received a whopping 684 comments!). If that happens, don't be discouraged; it | |
111 | means the community is interested in your idea, but it perhaps needs some | |
112 | adjustments. | |
113 | ||
114 | [nonascii]: https://github.com/rust-lang/rfcs/pull/2457 | |
115 | ||
116 | The RFC for our `?` macro feature did receive some discussion on the RFC thread | |
117 | too. As with most RFCs, there were a few questions that we couldn't answer by | |
118 | discussion: we needed experience using the feature to decide. Such questions | |
119 | are listed in the "Unresolved Questions" section of the RFC. Also, over the | |
120 | course of the RFC discussion, you will probably want to update the RFC document | |
121 | itself to reflect the course of the discussion (e.g. new alternatives or prior | |
122 | work may be added or you may decide to change parts of the proposal itself). | |
123 | ||
124 | In the end, when the discussion seems to reach a consensus and die down a bit, | |
60c5eb7d XL |
125 | a rust team member may propose to move to "final comment period" (FCP) with one |
126 | of three possible dispositions. This means that they want the other members of | |
127 | the appropriate teams to review and comment on the RFC. More discussion may | |
128 | ensue, which may result in more changes or unresolved questions being added. At | |
129 | some point, when everyone is satisfied, the RFC enters the FCP, which is the | |
130 | last chance for people to bring up objections. When the FCP is over, the | |
131 | disposition is adopted. Here are the three possible dispositions: | |
a1dfa0c6 XL |
132 | |
133 | - _Merge_: accept the feature. Here is the proposal to merge for our [`?` macro | |
134 | feature][rfcmerge]. | |
135 | - _Close_: this feature in its current form is not a good fit for rust. Don't | |
136 | be discouraged if this happens to your RFC, and don't take it personally. | |
137 | This is not a reflection on you, but rather a community decision that rust | |
138 | will go a different direction. | |
139 | - _Postpone_: there is interest in going this direction but not at the moment. | |
140 | This happens most often because the appropriate rust team doesn't have the | |
141 | bandwidth to shepherd the feature through the process to stabilization. Often | |
142 | this is the case when the feature doesn't fit into the team's roadmap. | |
143 | Postponed ideas may be revisited later. | |
144 | ||
145 | [rfcmerge]: https://github.com/rust-lang/rfcs/pull/2298#issuecomment-360582667 | |
146 | ||
147 | When an RFC is merged, the PR is merged into the RFCs repo. A new _tracking | |
148 | issue_ is created in the [rust-lang/rust] repo to track progress on the feature | |
149 | and discuss unresolved questions, implementation progress and blockers, etc. | |
150 | Here is the tracking issue on for our [`?` macro feature][tracking]. | |
151 | ||
152 | [tracking]: https://github.com/rust-lang/rust/issues/48075 | |
153 | ||
154 | <a name="impl"></a> | |
155 | ||
156 | ## Implementation | |
157 | ||
158 | To make a change to the compiler, open a PR against the [rust-lang/rust] repo. | |
159 | ||
160 | [rust-lang/rust]: https://github.com/rust-lang/rust | |
161 | ||
162 | Depending on the feature/change/bug fix/improvement, implementation may be | |
163 | relatively-straightforward or it may be a major undertaking. You can always ask | |
164 | for help or mentorship from more experienced compiler devs. Also, you don't | |
165 | have to be the one to implement your feature; but keep in mind that if you | |
166 | don't it might be a while before someone else does. | |
167 | ||
168 | For the `?` macro feature, I needed to go understand the relevant parts of | |
169 | macro expansion in the compiler. Personally, I find that [improving the | |
170 | comments][comments] in the code is a helpful way of making sure I understand | |
171 | it, but you don't have to do that if you don't want to. | |
172 | ||
173 | [comments]: https://github.com/rust-lang/rust/pull/47732 | |
174 | ||
175 | I then [implemented][impl1] the original feature, as described in the RFC. When | |
176 | a new feature is implemented, it goes behind a _feature gate_, which means that | |
177 | you have to use `#![feature(my_feature_name)]` to use the feature. The feature | |
178 | gate is removed when the feature is stabilized. | |
179 | ||
180 | **Most bug fixes and improvements** don't require a feature gate. You can just | |
181 | make your changes/improvements. | |
182 | ||
183 | When you open a PR on the [rust-lang/rust], a bot will assign your PR to a | |
184 | review. If there is a particular rust team member you are working with, you can | |
185 | request that reviewer by leaving a comment on the thread with `r? | |
186 | @reviewer-github-id` (e.g. `r? @eddyb`). If you don't know who to request, | |
187 | don't request anyone; the bot will assign someone automatically. | |
188 | ||
189 | The reviewer may request changes before they approve your PR. Feel free to ask | |
190 | questions or discuss things you don't understand or disagree with. However, | |
191 | recognize that the PR won't be merged unless someone on the rust team approves | |
192 | it. | |
193 | ||
6a06907d XL |
194 | When your reviewer approves the PR, it will go into a queue for yet another bot |
195 | called `@bors`. `@bors` manages the CI build/merge queue. When your PR reaches | |
a1dfa0c6 | 196 | the head of the `@bors` queue, `@bors` will test out the merge by running all |
6a06907d | 197 | tests against your PR on GitHub Actions. This takes a lot of time to |
ba9703b0 | 198 | finish. If all tests pass, the PR is merged and becomes part of the next |
a1dfa0c6 XL |
199 | nightly compiler! |
200 | ||
201 | There are a couple of things that may happen for some PRs during the review process | |
202 | ||
203 | - If the change is substantial enough, the reviewer may request an FCP on | |
204 | the PR. This gives all members of the appropriate team a chance to review the | |
205 | changes. | |
206 | - If the change may cause breakage, the reviewer may request a [crater] run. | |
207 | This compiles the compiler with your changes and then attempts to compile all | |
208 | crates on crates.io with your modified compiler. This is a great smoke test | |
209 | to check if you introduced a change to compiler behavior that affects a large | |
210 | portion of the ecosystem. | |
211 | - If the diff of your PR is large or the reviewer is busy, your PR may have | |
212 | some merge conflicts with other PRs that happen to get merged first. You | |
213 | should fix these merge conflicts using the normal git procedures. | |
214 | ||
215 | [crater]: ./tests/intro.html#crater | |
216 | ||
217 | If you are not doing a new feature or something like that (e.g. if you are | |
218 | fixing a bug), then that's it! Thanks for your contribution :) | |
219 | ||
220 | ## Refining your implementation | |
221 | ||
222 | As people get experience with your new feature on nightly, slight changes may | |
223 | be proposed and unresolved questions may become resolved. Updates/changes go | |
224 | through the same process for implementing any other changes, as described | |
225 | above (i.e. submit a PR, go through review, wait for `@bors`, etc). | |
226 | ||
227 | Some changes may be major enough to require an FCP and some review by rust team | |
228 | members. | |
229 | ||
230 | For the `?` macro feature, we went through a few different iterations after the | |
231 | original implementation: [1][impl2], [2][impl3], [3][impl4]. | |
232 | ||
233 | Along the way, we decided that `?` should not take a separator, which was | |
234 | previously an unresolved question listed in the RFC. We also changed the | |
235 | disambiguation strategy: we decided to remove the ability to use `?` as a | |
236 | separator token for other repetition operators (e.g. `+` or `*`). However, | |
237 | since this was a breaking change, we decided to do it over an edition boundary. | |
238 | Thus, the new feature can be enabled only in edition 2018. These deviations | |
239 | from the original RFC required [another | |
240 | FCP](https://github.com/rust-lang/rust/issues/51934). | |
241 | ||
242 | ## Stabilization | |
243 | ||
244 | Finally, after the feature had baked for a while on nightly, a language team member | |
245 | [moved to stabilize it][stabilizefcp]. | |
246 | ||
247 | [stabilizefcp]: https://github.com/rust-lang/rust/issues/48075#issuecomment-433177613 | |
248 | ||
249 | A _stabilization report_ needs to be written that includes | |
250 | ||
251 | - brief description of the behavior and any deviations from the RFC | |
252 | - which edition(s) are affected and how | |
253 | - links to a few tests to show the interesting aspects | |
254 | ||
255 | The stabilization report for our feature is [here][stabrep]. | |
256 | ||
257 | [stabrep]: https://github.com/rust-lang/rust/issues/48075#issuecomment-433243048 | |
258 | ||
259 | After this, [a PR is made][stab] to remove the feature gate, enabling the feature by | |
260 | default (on the 2018 edition). A note is added to the [Release notes][relnotes] | |
261 | about the feature. | |
262 | ||
48663c56 XL |
263 | [stab]: https://github.com/rust-lang/rust/pull/56245 |
264 | ||
532ac7d7 | 265 | Steps to stabilize the feature can be found at [Stabilizing Features](./stabilization_guide.md). |
a1dfa0c6 XL |
266 | |
267 | [relnotes]: https://github.com/rust-lang/rust/blob/master/RELEASES.md |