Best practices for bug fixes in the compiler #1589

Open
wants to merge 3 commits into from

5 participants

@nikomatsakis
nikomatsakis commented Apr 22, 2016 edited

Defines a "best practices" procedure for making bug fixes or soundness corrections in the compiler that can cause existing code to stop compiling.

Rendered view.

@nikomatsakis nikomatsakis self-assigned this Apr 22, 2016
@sgrif

One case that this leaves unaddressed is if a security vulnerability were to occur that required a breaking change to address. Presumably in that case there would not be a forwards compatibility warning?

@nagisa

@sgrif what is a security vulnerability in a compiler or a language? I can’t think of any potential issues which could be related to “security” but aren’t related to any of the soundness bugs.

@jethrogb
jethrogb commented Apr 23, 2016 edited

@nagisa e.g. a miscompilation that almost always results in an exploitable buffer overflow. Like rust-lang/rust#31234 but worse.

@nagisa

@jethrogb I’d classify any “miscompilation” as a “Compiler bug”.

@nagisa nagisa commented on the diff Apr 23, 2016
text/0000-rustc-bug-fix-procedure.md
+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.
@nagisa
nagisa added a note Apr 23, 2016

We do not have an RFC for the whole language as it was before RFC process was introduced. Perhaps this should also include things like the rust reference?

@nikomatsakis
nikomatsakis added a note Apr 27, 2016 edited

@nagisa rust-lang/rfcs#1122 specifically addressed this point, under the section on "underspecified language semantics". I do not consider the rust reference to be normative (though I have done sweeps at various points to try and find inaccuracies and so forth). I would say it's comparable to the official rust grammar -- a work in progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jethrogb
jethrogb commented Apr 23, 2016 edited

I’d classify any “miscompilation” as a “Compiler bug”.

I can agree with that, but that still doesn't answer @sgrif's question. There are many different soundness bugs some of which might be more serious than others. I'm thinking an edge case in the type system that almost no one is encountering vs. failing to emit bounds checks.

@sgrif

It doesn't matter if it's a "Compiler bug" or not. My point is that this RFC declares that all cases should have a forwards compatibility warning, when there are classes that probably shouldn't. I was simply pointing out that I think this should likely be addressed.

@jethrogb
jethrogb commented Apr 23, 2016 edited

I whole-heartedly agree. What's missing—not necessarily in this RFC, but in the current release process in general—is the ability to do bugfix-only point releases.¹ We have been getting away with not doing those so far but it's completely possible that at some point in the future there will be a huge bug (security-related or not) that might need immediate fixing in stable. A seperate question—one we might address here—is, what if the fix for that huge bug is not backwards-compatible?

¹ RFC 507: "Provisions for stable point releases will be made at a future time."

@nikomatsakis

@sgrif

One case that this leaves unaddressed is if a security vulnerability were to occur that required a breaking change to address. Presumably in that case there would not be a forwards compatibility warning?

Interesting point. I can add some text saying that we may want to consider foregoing a warning period in particularly dire cases. That said, I am not sure what such a case would be just now. For example, we generally try to patch soundness holes in the type system with warnings first, because in practice most code that is relying on them is not in fact going to crash. (Note also that users can -- and probably should -- add things like #[deny(forwards_incompatible)] to get stricter checks locally.) The example of foregoing bounds checks failures is something I imagine we would simply fix, since fixing it will not cause compilation errors (in general there is no good way to issue a "warning period" for pure runtime patches). Nonetheless, never say never: I'm sure we'll eventually encounter a scenario like the one you describe.

@nikomatsakis

I wrote this in my last comment:

I can add some text saying that we may want to consider foregoing a warning period in particularly dire cases.

But I just want to clarify one thing. The RFC doesn't say you must issue warnings even now. It just says that if you plan to forego them, there are had better be a good reason (for example, issuing warnings is infeasible).

@nikomatsakis

I've been contemplating a loosening of this policy. Basically I think we should convert the "infeasible" clause into a sort of exemption for PRs with particularly small impact. Specifically:

"If a crater run reveals fewer than 10 total affected projects (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 always ensure that the error directs users to this page. In other words, everything should be the same except that they 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 more than 10 crates are affected, warnings are mandatory -- if implementing warnings is not feasible, then we have to make an aggressive strategy of migrating crates before we land the change so as to lower the number of affected crates."

That said, I hope that we would prefer warnings even when very few crates are affected. I want us to make a point of pride on ensuring that all users have the best transition experience possible -- and warnings are strictly better than errors. Moreover, crater runs remain a very imperfect measurement of affected change. (Note though that we are now also gating on certain key crates, such as cargo or winapi, so those crates will never break no matter what -- if they would be affected, then they must be fixed first.)

But at the same time I want to balance the total engineering effort and make sure that we can still make forward progress without getting stuck implementing a lot of warning scaffolding for changes with little real-world impact.

@nikomatsakis

OK, I have finally adjusted the wording as I wanted to do. Nominating for FCP.

@pnkfelix
The Rust Programming Language member

This RFC is entering its 🔔 final comment period 🔔

@michaelwoerister michaelwoerister commented on an outdated diff Jul 26, 2016
text/0000-rustc-bug-fix-procedure.md
+# 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.
+-

This - sign here messes up formatting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment