Best practices for bug fixes in the compiler

[nikomatsakis]

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]

cc @rust-lang/compiler

[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]

@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]

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]

@nagisa #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.

[jethrogb]

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]

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]

This RFC is entering its 🔔 final comment period 🔔

[michaelwoerister]

This - sign here messes up formatting.

[nikomatsakis]

Huzzah! The @rust-lang/compiler team has decided to accept this RFC. Actually, we decided a long time ago, but I kept forgetting to merge. Done!