src/doc/rustc-dev-guide/src/bug-fix-procedure.md

   1 # Rustc Bug Fix Procedure
   2
   3 <!-- toc -->
   4
   5 This page defines the best practices procedure for making bug fixes or soundness
   6 corrections in the compiler that can cause existing code to stop compiling. This
   7 text is based on
   8 [RFC 1589](https://github.com/rust-lang/rfcs/blob/master/text/1589-rustc-bug-fix-procedure.md).
   9
  10 # Motivation
  11
  12 [motivation]: #motivation
  13
  14 From time to time, we encounter the need to make a bug fix, soundness
  15 correction, or other change in the compiler which will cause existing code to
  16 stop compiling. When this happens, it is important that we handle the change in
  17 a way that gives users of Rust a smooth transition. What we want to avoid is
  18 that existing programs suddenly stop compiling with opaque error messages: we
  19 would prefer to have a gradual period of warnings, with clear guidance as to
  20 what the problem is, how to fix it, and why the change was made. This RFC
  21 describes the procedure that we have been developing for handling breaking
  22 changes that aims to achieve that kind of smooth transition.
  23
  24 One of the key points of this policy is that (a) warnings should be issued
  25 initially rather than hard errors if at all possible and (b) every change that
  26 causes existing code to stop compiling will have an associated tracking issue.
  27 This issue provides a point to collect feedback on the results of that change.
  28 Sometimes changes have unexpectedly large consequences or there may be a way to
  29 avoid the change that was not considered. In those cases, we may decide to
  30 change course and roll back the change, or find another solution (if warnings
  31 are being used, this is particularly easy to do).
  32
  33 ### What qualifies as a bug fix?
  34
  35 Note that this RFC does not try to define when a breaking change is permitted.
  36 That is already covered under [RFC 1122][]. This document assumes that the
  37 change being made is in accordance with those policies. Here is a summary of the
  38 conditions from RFC 1122:
  39
  40 - **Soundness changes:** Fixes to holes uncovered in the type system.
  41 - **Compiler bugs:** Places where the compiler is not implementing the specified
  42   semantics found in an RFC or lang-team decision.
  43 - **Underspecified language semantics:** Clarifications to grey areas where the
  44   compiler behaves inconsistently and no formal behavior had been previously
  45   decided.
  46
  47 Please see [the RFC][rfc 1122] for full details!
  48
  49 # Detailed design
  50
  51 [design]: #detailed-design
  52
  53 The procedure for making a breaking change is as follows (each of these steps is
  54 described in more detail below):
  55
  56 1. Do a **crater run** to assess the impact of the change.
  57 2. Make a **special tracking issue** dedicated to the change.
  58 3. Do not report an error right away. Instead, **issue forwards-compatibility
  59    lint warnings**.
  60    - Sometimes this is not straightforward. See the text below for suggestions
  61      on different techniques we have employed in the past.
  62    - For cases where warnings are infeasible:
  63      - Report errors, but make every effort to give a targeted error message
  64        that directs users to the tracking issue
  65      - Submit PRs to all known affected crates that fix the issue
  66        - or, at minimum, alert the owners of those crates to the problem and
  67          direct them to the tracking issue
  68 4. Once the change has been in the wild for at least one cycle, we can
  69    **stabilize the change**, converting those warnings into errors.
  70
  71 Finally, for changes to `rustc_ast` that will affect plugins, the general policy
  72 is to batch these changes. That is discussed below in more detail.
  73
  74 ### Tracking issue
  75
  76 Every breaking change should be accompanied by a **dedicated tracking issue**
  77 for that change. The main text of this issue should describe the change being
  78 made, with a focus on what users must do to fix their code. The issue should be
  79 approachable and practical; it may make sense to direct users to an RFC or some
  80 other issue for the full details. The issue also serves as a place where users
  81 can comment with questions or other concerns.
  82
  83 A template for these breaking-change tracking issues can be found below. An
  84 example of how such an issue should look can be [found
  85 here][breaking-change-issue].
  86
  87 The issue should be tagged with (at least) `B-unstable` and `T-compiler`.
  88
  89 ### Tracking issue template
  90
  91 This is a template to use for tracking issues:
  92
  93 ```
  94 This is the **summary issue** for the `YOUR_LINT_NAME_HERE`
  95 future-compatibility warning and other related errors. The goal of
  96 this page is describe why this change was made and how you can fix
  97 code that is affected by it. It also provides a place to ask questions
  98 or register a complaint if you feel the change should not be made. For
  99 more information on the policy around future-compatibility warnings,
 100 see our [breaking change policy guidelines][guidelines].
 101
 102 [guidelines]: LINK_TO_THIS_RFC
 103
 104 #### What is the warning for?
 105
 106 *Describe the conditions that trigger the warning and how they can be
 107 fixed. Also explain why the change was made.**
 108
 109 #### When will this warning become a hard error?
 110
 111 At the beginning of each 6-week release cycle, the Rust compiler team
 112 will review the set of outstanding future compatibility warnings and
 113 nominate some of them for **Final Comment Period**. Toward the end of
 114 the cycle, we will review any comments and make a final determination
 115 whether to convert the warning into a hard error or remove it
 116 entirely.
 117 ```
 118
 119 ### Issuing future compatibility warnings
 120
 121 The best way to handle a breaking change is to begin by issuing
 122 future-compatibility warnings. These are a special category of lint warning.
 123 Adding a new future-compatibility warning can be done as follows.
 124
 125 ```rust
 126 // 1. Define the lint in `compiler/rustc_middle/src/lint/builtin.rs`:
 127 declare_lint! {
 128     pub YOUR_ERROR_HERE,
 129     Warn,
 130     "illegal use of foo bar baz"
 131 }
 132
 133 // 2. Add to the list of HardwiredLints in the same file:
 134 impl LintPass for HardwiredLints {
 135     fn get_lints(&self) -> LintArray {
 136         lint_array!(
 137             ..,
 138             YOUR_ERROR_HERE
 139         )
 140     }
 141 }
 142
 143 // 3. Register the lint in `compiler/rustc_lint/src/lib.rs`:
 144 store.register_future_incompatible(sess, vec![
 145     ...,
 146     FutureIncompatibleInfo {
 147         id: LintId::of(YOUR_ERROR_HERE),
 148         reference: "issue #1234", // your tracking issue here!
 149     },
 150 ]);
 151
 152 // 4. Report the lint:
 153 tcx.lint_node(
 154     lint::builtin::YOUR_ERROR_HERE,
 155     path_id,
 156     binding.span,
 157     format!("some helper message here"));
 158 ```
 159
 160 #### Helpful techniques
 161
 162 It can often be challenging to filter out new warnings from older, pre-existing
 163 errors. One technique that has been used in the past is to run the older code
 164 unchanged and collect the errors it would have reported. You can then issue
 165 warnings for any errors you would give which do not appear in that original set.
 166 Another option is to abort compilation after the original code completes if
 167 errors are reported: then you know that your new code will only execute when
 168 there were no errors before.
 169
 170 #### Crater and crates.io
 171
 172 We should always do a crater run to assess impact. It is polite and considerate
 173 to at least notify the authors of affected crates the breaking change. If we can
 174 submit PRs to fix the problem, so much the better.
 175
 176 #### Is it ever acceptable to go directly to issuing errors?
 177
 178 Changes that are believed to have negligible impact can go directly to issuing
 179 an error. One rule of thumb would be to check against `crates.io`: if fewer than
 180 10 **total** affected projects are found (**not** root errors), we can move
 181 straight to an error. In such cases, we should still make the "breaking change"
 182 page as before, and we should ensure that the error directs users to this page.
 183 In other words, everything should be the same except that users are getting an
 184 error, and not a warning. Moreover, we should submit PRs to the affected
 185 projects (ideally before the PR implementing the change lands in rustc).
 186
 187 If the impact is not believed to be negligible (e.g., more than 10 crates are
 188 affected), then warnings are required (unless the compiler team agrees to grant
 189 a special exemption in some particular case). If implementing warnings is not
 190 feasible, then we should make an aggressive strategy of migrating crates before
 191 we land the change so as to lower the number of affected crates. Here are some
 192 techniques for approaching this scenario:
 193
 194 1. Issue warnings for subparts of the problem, and reserve the new errors for
 195    the smallest set of cases you can.
 196 2. Try to give a very precise error message that suggests how to fix the problem
 197    and directs users to the tracking issue.
 198 3. It may also make sense to layer the fix:
 199    - First, add warnings where possible and let those land before proceeding to
 200      issue errors.
 201    - Work with authors of affected crates to ensure that corrected versions are
 202      available _before_ the fix lands, so that downstream users can use them.
 203
 204 ### Stabilization
 205
 206 After a change is made, we will **stabilize** the change using the same process
 207 that we use for unstable features:
 208
 209 - After a new release is made, we will go through the outstanding tracking
 210   issues corresponding to breaking changes and nominate some of them for **final
 211   comment period** (FCP).
 212 - The FCP for such issues lasts for one cycle. In the final week or two of the
 213   cycle, we will review comments and make a final determination:
 214
 215   - Convert to error: the change should be made into a hard error.
 216   - Revert: we should remove the warning and continue to allow the older code to
 217     compile.
 218   - Defer: can't decide yet, wait longer, or try other strategies.
 219
 220 Ideally, breaking changes should have landed on the **stable branch** of the
 221 compiler before they are finalized.
 222
 223 <a name="guide">
 224
 225 ### Removing a lint
 226
 227 Once we have decided to make a "future warning" into a hard error, we need a PR
 228 that removes the custom lint. As an example, here are the steps required to
 229 remove the `overlapping_inherent_impls` compatibility lint. First, convert the
 230 name of the lint to uppercase (`OVERLAPPING_INHERENT_IMPLS`) ripgrep through the
 231 source for that string. We will basically by converting each place where this
 232 lint name is mentioned (in the compiler, we use the upper-case name, and a macro
 233 automatically generates the lower-case string; so searching for
 234 `overlapping_inherent_impls` would not find much).
 235
 236 > NOTE: these exact files don't exist anymore, but the procedure is still the same.
 237
 238 #### Remove the lint.
 239
 240 The first reference you will likely find is the lint definition [in
 241 `rustc_session/src/lint/builtin.rs` that resembles this][defsource]:
 242
 243 [defsource]: https://github.com/rust-lang/rust/blob/085d71c3efe453863739c1fb68fd9bd1beff214f/src/librustc/lint/builtin.rs#L171-L175
 244
 245 ```rust
 246 declare_lint! {
 247     pub OVERLAPPING_INHERENT_IMPLS,
 248     Deny, // this may also say Warning
 249     "two overlapping inherent impls define an item with the same name were erroneously allowed"
 250 }
 251 ```
 252
 253 This `declare_lint!` macro creates the relevant data structures. Remove it. You
 254 will also find that there is a mention of `OVERLAPPING_INHERENT_IMPLS` later in
 255 the file as [part of a `lint_array!`][lintarraysource]; remove it too.
 256
 257 [lintarraysource]: https://github.com/rust-lang/rust/blob/085d71c3efe453863739c1fb68fd9bd1beff214f/src/librustc/lint/builtin.rs#L252-L290
 258
 259 Next, you see [a reference to `OVERLAPPING_INHERENT_IMPLS` in
 260 `rustc_lint/src/lib.rs`][futuresource]. This is defining the lint as a "future
 261 compatibility lint":
 262
 263 ```rust
 264 FutureIncompatibleInfo {
 265     id: LintId::of(OVERLAPPING_INHERENT_IMPLS),
 266     reference: "issue #36889 <https://github.com/rust-lang/rust/issues/36889>",
 267 },
 268 ```
 269
 270 Remove this too.
 271
 272 #### Add the lint to the list of removed lists.
 273
 274 In `compiler/rustc_lint/src/lib.rs` there is a list of "renamed and removed lints".
 275 You can add this lint to the list:
 276
 277 ```rust
 278 store.register_removed("overlapping_inherent_impls", "converted into hard error, see #36889");
 279 ```
 280
 281 where `#36889` is the tracking issue for your lint.
 282
 283 #### Update the places that issue the lint
 284
 285 Finally, the last class of references you will see are the places that actually
 286 **trigger** the lint itself (i.e., what causes the warnings to appear). These
 287 you do not want to delete. Instead, you want to convert them into errors. In
 288 this case, the [`add_lint` call][addlintsource] looks like this:
 289
 290 ```rust
 291 self.tcx.sess.add_lint(lint::builtin::OVERLAPPING_INHERENT_IMPLS,
 292                        node_id,
 293                        self.tcx.span_of_impl(item1).unwrap(),
 294                        msg);
 295 ```
 296
 297 We want to convert this into an error. In some cases, there may be an
 298 existing error for this scenario. In others, we will need to allocate a
 299 fresh diagnostic code.  [Instructions for allocating a fresh diagnostic
 300 code can be found here.](./diagnostics/diagnostic-codes.md) You may want
 301 to mention in the extended description that the compiler behavior
 302 changed on this point, and include a reference to the tracking issue for
 303 the change.
 304
 305 Let's say that we've adopted `E0592` as our code. Then we can change the
 306 `add_lint()` call above to something like:
 307
 308 ```rust
 309 struct_span_err!(self.tcx.sess, self.tcx.span_of_impl(item1).unwrap(), msg)
 310     .emit();
 311 ```
 312
 313 #### Update tests
 314
 315 Finally, run the test suite. These should be some tests that used to reference
 316 the `overlapping_inherent_impls` lint, those will need to be updated. In
 317 general, if the test used to have `#[deny(overlapping_inherent_impls)]`, that
 318 can just be removed.
 319
 320 ```
 321 ./x.py test
 322 ```
 323
 324 #### All done!
 325
 326 Open a PR. =)
 327
 328 [addlintsource]: https://github.com/rust-lang/rust/blob/085d71c3efe453863739c1fb68fd9bd1beff214f/src/librustc_typeck/coherence/inherent.rs#L300-L303
 329 [futuresource]: https://github.com/rust-lang/rust/blob/085d71c3efe453863739c1fb68fd9bd1beff214f/src/librustc_lint/lib.rs#L202-L205
 330
 331 <!-- -Links--------------------------------------------------------------------- -->
 332
 333 [rfc 1122]: https://github.com/rust-lang/rfcs/blob/master/text/1122-language-semver.md
 334 [breaking-change-issue]: https://gist.github.com/nikomatsakis/631ec8b4af9a18b5d062d9d9b7d3d967