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