Best practices for bug fixes in the compiler

text/0000-rustc-bug-fix-procedure.md

@@ -0,0 +1,285 @@
+- Feature Name: N/A
+- Start Date: 2016-04-22
+- RFC PR: (leave this empty)
+- Rust Issue: (leave this empty)
+
+# Summary
+[summary]: #summary
+
+Defines a best practices procedure for making bug fixes or soundness
+corrections in the compiler that can cause existing code to stop
+compiling.
+
+# Motivation
+[motivation]: #motivation
+
+From time to time, we encounter the need to make a bug fix, soundness
+correction, or other change in the compiler which will cause existing
+code to stop compiling. When this happens, it is important that we
+handle the change in a way that gives users of Rust a smooth
+transition. What we want to avoid is that existing programs suddenly
+stop compiling with opaque error messages: we would prefer to have a
+gradual period of warnings, with clear guidance as to what the problem
+is, how to fix it, and why the change was made. This RFC describes the
+procedure that we have been developing for handling breaking changes
+that aims to achieve that kind of smooth transition.
+
+One of the key points of this policy is that (a) warnings should be
+issued initially rather than hard errors if at all possible and (b)
+every change that causes existing code to stop compiling will have an
+associated tracking issue. This issue provides a point to collect
+feedback on the results of that change. Sometimes changes have
+unexpectedly large consequences or there may be a way to avoid the
+change that was not considered.  In those cases, we may decide to
+change course and roll back the change, or find another solution (if
+warnings are being used, this is particularly easy to do).
+
+### What qualifies as a bug fix? 
+
+Note that this RFC does not try to define when a breaking change is
+permitted. That is already covered under [RFC 1122][]. This document
+assumes that the change being made is in accordance with those
+policies. Here is a summary of the conditions from RFC 1122:
+
+- **Soundness changes:** Fixes to holes uncovered in the type system.
+- **Compiler bugs:** Places where the compiler is not implementing the
+  specified semantics found in an RFC or lang-team decision.
+- **Underspecified language semantics:** Clarifications to grey areas
+  where the compiler behaves inconsistently and no formal behavior had
+  been previously decided.
+
+Please see [the RFC][RFC 1122] for full details!
+
+# Detailed design
+[design]: #detailed-design
+
+The procedure for making a breaking change is as follows (each of
+these steps is described in more detail below):
+
+0. Do a **crater run** to assess the impact of the change.
+1. Make a **special tracking issue** dedicated to the change.
+2. Do not report an error right away. Instead, **issue
+   forwards-compatibility lint warnings**.
+   - Sometimes this is not straightforward. See the text below for
+     suggestions on different techniques we have employed in the past.
+   - For cases where warnings are infeasible:
+     - Report errors, but make every effort to give a targeted error
+       message that directs users to the tracking issue
+     - Submit PRs to all known affected crates that fix the issue
+       - or, at minimum, alert the owners of those crates to the problem
+         and direct them to the tracking issue
+3. Once the change has been in the wild for at least one cycle, we can
+   **stabilize the change**, converting those warnings into errors.
+
+Finally, for changes to libsyntax that will affect plugins, the
+general policy is to batch these changes. That is discussed below in
+more detail.
+
+### Tracking issue
+
+Every breaking change should be accompanied by a **dedicated tracking
+issue** for that change. The main text of this issue should describe
+the change being made, with a focus on what users must do to fix their
+code. The issue should be approachable and practical; it may make
+sense to direct users to an RFC or some other issue for the full
+details. The issue also serves as a place where users can comment with
+questions or other concerns.
+
+A template for these breaking-change tracking issues can be found
+below. An example of how such an issue should look can be
+[found here][breaking-change-issue].
+
+The issue should be tagged with (at least) `B-unstable` and
+`T-compiler`.
+
+### Tracking issue template
+
+What follows is a template for tracking issues.
+
+---------------------------------------------------------------------------
+
+This is the **summary issue** for the `YOUR_LINT_NAME_HERE`
+future-compatibility warning and other related errors. The goal of
+this page is describe why this change was made and how you can fix
+code that is affected by it. It also provides a place to ask questions
+or register a complaint if you feel the change should not be made. For
+more information on the policy around future-compatibility warnings,
+see our [breaking change policy guidelines][guidelines].
+
+[guidelines]: LINK_TO_THIS_RFC
+
+#### What is the warning for?
+
+*Describe the conditions that trigger the warning and how they can be
+fixed. Also explain why the change was made.**
+
+#### When will this warning become a hard error?
+
+At the beginning of each 6-week release cycle, the Rust compiler team
+will review the set of outstanding future compatibility warnings and
+nominate some of them for **Final Comment Period**. Toward the end of
+the cycle, we will review any comments and make a final determination
+whether to convert the warning into a hard error or remove it
+entirely.
+
+---------------------------------------------------------------------------
+
+### Issuing future compatibility warnings
+
+The best way to handle a breaking change is to begin by issuing
+future-compatibility warnings. These are a special category of lint
+warning. Adding a new future-compatibility warning can be done as
+follows.
+
+```rust
+// 1. Define the lint in `src/librustc/lint/builtin.rs`:
+declare_lint! {
+    pub YOUR_ERROR_HERE,
+    Warn,
+    "illegal use of foo bar baz"
+}
+
+// 2. Add to the list of HardwiredLints in the same file:
+impl LintPass for HardwiredLints {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(
+            ..,
+            YOUR_ERROR_HERE
+        )
+    }
+}
+
+// 3. Register the lint in `src/librustc_lint/lib.rs`:
+store.register_future_incompatible(sess, vec![
+    ...,
+    FutureIncompatibleInfo {
+        id: LintId::of(YOUR_ERROR_HERE),
+        reference: "issue #1234", // your tracking issue here!
+    },
+]);
+
+// 4. Report the lint:
+tcx.sess.add_lint(
+    lint::builtin::YOUR_ERROR_HERE,
+    path_id,
+    binding.span,
+    format!("some helper message here"));
+```
+
+#### Helpful techniques
+
+It can often be challenging to filter out new warnings from older,
+pre-existing errors. One technique that has been used in the past is
+to run the older code unchanged and collect the errors it would have
+reported. You can then issue warnings for any errors you would give
+which do not appear in that original set. Another option is to abort
+compilation after the original code completes if errors are reported:
+then you know that your new code will only execute when there were no
+errors before.
+
+#### Crater and crates.io
+
+We should always do a crater run to assess impact. It is polite and
+considerate to at least notify the authors of affected crates the
+breaking change.  If we can submit PRs to fix the problem, so much the
+better.
+
+#### Is it ever acceptable to go directly to issuing errors?
+
+Changes that are believed to have negligible impact can go directly to
+issuing an error. One rule of thumb would be to check against
+`crates.io`: if fewer than 10 **total** affected projects are found
+(**not** root errors), we can move straight to an error. In such
+cases, we should still make the "breaking change" page as before, and
+we should ensure that the error directs users to this page. In other
+words, everything should be the same except that users are getting an
+error, and not a warning. Moreover, we should submit PRs to the
+affected projects (ideally before the PR implementing the change lands
+in rustc).
+
+If the impact is not believed to be negligible (e.g., more than 10
+crates are affected), then warnings are required (unless the compiler
+team agrees to grant a special exemption in some particular case). If
+implementing warnings is not feasible, then we should make an
+aggressive strategy of migrating crates before we land the change so
+as to lower the number of affected crates. Here are some techniques
+for approaching this scenario:
+
+1. Issue warnings for subparts of the problem, and reserve the new
+   errors for the smallest set of cases you can.
+2. Try to give a very precise error message that suggests how to fix
+   the problem and directs users to the tracking issue.
+3. It may also make sense to layer the fix:
+   - First, add warnings where possible and let those land before proceeding
+     to issue errors.
+   - Work with authors of affected crates to ensure that corrected
+     versions are available *before* the fix lands, so that downstream
+     users can use them.
+
+
+### Stabilization
+
+After a change is made, we will **stabilize** the change using the same
+process that we use for unstable features:
+
+- After a new release is made, we will go through the outstanding tracking
+  issues corresponding to breaking changes and nominate some of them for
+  **final comment period** (FCP).
+- The FCP for such issues lasts for one cycle. In the final week or two of the cycle,
+  we will review comments and make a final determination:
+  - Convert to error: the change should be made into a hard error.
+  - Revert: we should remove the warning and continue to allow the older code to compile.
+  - Defer: can't decide yet, wait longer, or try other strategies.
+
+Ideally, breaking changes should have landed on the **stable branch**
+of the compiler before they are finalized.
+
+### Batching breaking changes to libsyntax
+
+Due to the lack of stable plugins, making changes to libsyntax can
+currently be quite disruptive to the ecosystem that relies on plugins.
+In an effort to ease this pain, we generally try to batch up such
+changes so that they occur all at once, rather than occuring in a
+piecemeal fashion. In practice, this means that you should add:
+
+    cc #31645 @Manishearth
+
+to the PR and avoid directly merging it. In the future we may develop
+a more polished procedure here, but the hope is that this is a
+relatively temporary state of affairs.
+
+# Drawbacks
+[drawbacks]: #drawbacks
+
+Following this policy can require substantial effort and slows the
+time it takes for a change to become final. However, this is far
+outweighed by the benefits of avoiding sharp disruptions in the
+ecosystem.
+
+# Alternatives
+[alternatives]: #alternatives
+
+There are obviously many points that we could tweak in this policy:
+
+- Eliminate the tracking issue.
+- Change the stabilization schedule.
+- 
+
+Two other obvious (and rather extreme) alternatives are not having a
+policy and not making any sort of breaking change at all:
+
+- Not having a policy at all (as is the case today) encourages
+  inconsistent treatment of issues.
+- Not making any sorts of breaking changes would mean that Rust simply
+  has to stop evolving, or else would issue new major versions quite
+  frequently, causing undue disruption.
+
+# Unresolved questions
+[unresolved]: #unresolved-questions
+
+N/A
+
+<!-- -Links--------------------------------------------------------------------- -->
+
+[RFC 1122]: https://github.com/rust-lang/rfcs/blob/master/text/1122-language-semver.md
+[breaking-change-issue]: https://gist.github.com/nikomatsakis/631ec8b4af9a18b5d062d9d9b7d3d967