Tracking issue for eRFC 2497, "if- and while-let-chains, take 2", edition changes

[Centril]

This is a tracking issue for the eRFC "if- and while-let-chains, take 2" (rust-lang/rfcs#2497) pertaining solely to the immediate Rust 2018 transition changes. For the main tracking issue, see #53667.

Steps:

• Implement the edition parts of the RFC (cc @rust-lang/compiler -- can anyone write up mentoring instructions?)
• Adjust documentation (see instructions on forge)
• Stabilization PR (see instructions on forge)

Unresolved questions:

There are none.

[Centril]

Nominating for discussion on whether we should try to do this for all editions or as exactly per the RFC (>=2018).

[nikomatsakis]

We discussed in the @rust-lang/lang meeting and decided that we would first try to do this for all editions, because the belief is that in practice this will not a problem — and in general it'd be nice to avoid bifurcating our grammar in this way if we can get away with it.

[Centril]

Triage:

• There isn't anything to stabilize here since we chose to implement this directly in Rust 2015+.
• The documentation still needs to be updated before closing this issue.

[SimonSapin]

As discussed in rust-lang/rfcs#2544, I feel very uncomfortable with making breaking parser changes for existing code. I’d much prefer this only applied in Rust 2018.

[durka]

If the RFC process decided on not doing a breaking change, it seems really problematic to go back on that now.

[nikomatsakis]

@rfcbot fcp merge

So, there was some discussion on Discord. First off, independent of the merits of the decision, @durka is correct that we did not really follow the proper process here. We ought not to make a decision like this without an FCP to allow for public comment.

Therefore, I am making a formal "move to merge" here to allow for this discussion. At the moment, the only thing that has landed is a "post-parse" check that issues errors in the event of the disallowed construction -- it'd be relatively easy to roll back if we decide to do that. (As to whether or not to do so, I find myself somewhat torn.)

The rationale in the meeting was that we had plenty of evidence that there would be no crates affected by this change at all:

• We had complete a crater run that showed zero regressions.
• We had also done regular-expression based tests that show similar results.
• The actual breaking pattern is awfully unlikely (if let false = foo && bar) though certainly not impossible (e.g., one could imagine macros generating something like this).

All in all, it did not seem necessary to gate this change on the Edition. There are some advantages to not doing so -- it reduces the code maintenance burden and removes the need to document two variants of the language. But these are more "tactical" advantages.

And, of course, we can never know with 100% certainty whether anyone is affected. It would be nice to know if there are any actual crates affected by this decision (whether or not they are on crates.io). @SimonSapin is correct that we should not be cavalier about making breaking changes.

[rfcbot]

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

• @Centril
• @aturon
• @cramertj
• @eddyb
• @joshtriplett
• @nikomatsakis
• @nrc
• @pnkfelix
• @scottmcm
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[SimonSapin]

All in all, it did not seem necessary to gate this change on the Edition. There are some advantages to not doing so --

These advantages seem to be only for compiler maintainers, who are far fewer than its users. Should we have a Priority of Constituencies similar to that of web standards?

In case of conflict, consider users over authors over implementors over specifiers over theoretical purity.

[phaylon]

All in all, it did not seem necessary to gate this change on the Edition. There are some advantages to not doing so -- it reduces the code maintenance burden and removes the need to document two variants of the language. But these are more "tactical" advantages.

I'd like to voice my general disagreement with this stance. Backwards-compatibility only if anyone speaks up isn't real backwards compatibility in my eyes. I agree with breakage for fixing soundness issues, or closing holes that the spec disallowed but where incorrectly permitted and such, but I don't feel syntax adjustments justify breaking compatibility.

A crater run only covers what is on crates.io. It doesn't cover:

• In-house code that isn't public
• Non-published libraries on Github
• Public code that isn't on crates.io, like Rust-built applications
• Things that generate Rust code
• Historical code in repositories one might want to run for comparisons
• Tooling code that operates on the syntax
• Probably other things I'm not thinking of

I would also say that calling for people to speak up in the comments of the tracking issue of an experimental RFC doesn't have much reach. What amount of work in following Rust development should be necessary to be able to rely on the backwards compatibility? I would assume the recommendaton is to regularly test on the beta channel. But that doesn't cover everywhere code might be. In the 2017 survey, 2793 people said they use stable, with only 139 using beta. I assume the ones testing beta on previous versions of their code is minimal.

Since this is already used as precedent in the turbofish discussion, I hope you understand where some worry about the weight of Rusts backwards compatibility guarantees comes in.

[Centril]

@SimonSapin

These advantages seem to be only for compiler maintainers [...]

It also affects people who use syn and people who do Rust related tooling.
It also affects people who read documentation.

Should we have a Priority of Constituencies similar to that of web standards?

I don't think we should and the meaning of such priorities are not clear to me when applies to Rust.
For example, does "user" refer to people who use Rust programs? (e.g. users of ripgrep) or does it refer to people who write code in Rust but don't develop Rust itself? The latter group are programmers and should be expected to be much more comfortable with dealing with technological changes.

@phaylon

A crater run only covers what is on crates.io. It doesn't cover:

This is all true; however, from crates.io + sourcegraph + the overall silliness of writing if let true = p && q { .. } as compared to if p && q { .. } we have good reasons to believe that the probability of breakage in those places you've listed are also exceedingly slim.

I would also say that calling for people to speak up in the comments of the tracking issue of an experimental RFC doesn't have much reach.

This will reach TWiR soon but I'll add a note to the RFC itself :)

That said; I do apologize for the process mistake made here.
We would have had time to FCP merge this before landing the PR that implemented the change in all editions.

[Centril]

cc @nasa42, @llogiq, and @Flavsditz -- can we sneak this into TWiR issue 252 as well as TWiR issue 253?

[SimonSapin]

the meaning of such priorities are not clear to me when applies to Rust.

I meant adopting something in the same spirit, not literally this to the letter.

[phaylon]

@Centril:

It's more about the principle. Saying that Rust is backwards-compatible except for soundness fixes will simply be no longer true. I can also easily see things like if let <pattern> = <expr> being generated with "silly" code. Same goes for turbofish. let (cond_a, cond_b) = (a < b, c > (d + e)) could be something generated.

I understand you expect the impact to be minimal. It is still the end of backwards compatibility as it currently stands. Trying to judge impact of breakage is to me exactly the opposite of a backwards compatibility guarantee. The fact that editions exist, the next one is right around the corner, but backwards compatibility is still being broken is kind of an alarm bell for me.

I mean, what would happen if these changes are stabilized and someone in the community runs into an issue? Do they need to fix their code, or will Rust and the toolset roll back these changes? How many breakages would be required for the rollback?

I would propose that "should Rust uphold backwards compatibility guarantees" should be discussed in the wider community. For a question like that even TWIR feels too small.

[aidanhs]

• Non-published libraries on Github

@phaylon minor note, crater does test these (github is scraped for Rust repos). Doesn't discount the rest of the items though.

[SimonSapin]

I agree with @phaylon that it’s more a matter of principle and of setting a precedent than evaluating how bad the breakage will be in practice. Especially when the edition mechanism exists (and was introduce exactly for this purpose!) and provides an alternative other than "never do this at all".

[Centril]

@SimonSapin The meaning of adopting such a thing is still vague and not particularly meaningful to me.

For example, the guideline includes "theoretical purity" as the least important thing.
My interpretation of "theoretical purity" is that we should be rigorously principled here and not do the 2015 breakage and that the overall benefit to people programming in Rust is to do the breakage. Interestingly, rust-lang/rfcs#2544 (comment) due to @dherman seems to agree with my interpretation above and this is from experiences due to TC39 (however, we are much more strict as compared to TC39 I think). However, I think your interpretation as applied to this particular case differs from mine. This leads me to believe having such a "Priority of Constituencies" is not useful because interpretations differ so much.

@phaylon

It's more about the principle. Saying that Rust is backwards-compatible except for soundness fixes will simply be no longer true.

Our bar for doing backwards compatibility breaks has never been soundness fixes.
We have in the past done changes given future-compatibility warning with lints and then made such changes without an edition. Also see rust-lang/rfcs#1105.

[Centril]

@nasa42 Yup; however, in this case, I would like to propose an exception to the rule to rectify the process mistake.

[Centril]

@Flavsditz

We can at most change in the repository.

That's what I'm suggesting :)

[petrochenkov]

@phaylon

It is still the end of backwards compatibility as it currently stands

The backwards compatibility you are talking about never even started in the first place, so it could end.
Changes with negligible effects on contrived code are done very regularly, it's just nobody notices them.

[nasa42]

@Centril @Flavsditz added to TWiR 252 (and next issue's draft).

[Centril]

@nasa42 Thank you! ❤️

[phaylon]

@Centril
From https://github.com/aturon/rfcs/blob/api-evolution/text/0000-api-evolution.md

The RFC covers only API issues; other issues related to language features, lints, type inference, command line arguments, Cargo, and so on are considered out of scope.

But also:

Major [Version]: must be incremented for changes that break client code.

And from https://blog.rust-lang.org/2014/10/30/Stability.html

We reserve the right to fix compiler bugs, patch safety holes, and change type inference in ways that may occasionally require new type annotations.

However, the epoch RFC has a section about "repurposing corner cases" which to me seems exactly fitting this case.

@petrochenkov

Changes with negligible effects on contrived code are done very regularly, it's just nobody notices them.

Do you have an example that isn't a bug or soundness fix? I've seen widespread belief that Rust guarantees backwards compatibility, if this is untrue we should correct that.

[Centril]

@phaylon

However, the epoch RFC has a section about "repurposing corner cases" which to me seems exactly fitting this case.

If we had found crater regressions (even a single one) I would have been inclined to do this via the edition mechanism, such as we did with modules, trait Foo { fn bar(Type) } and the await, async, try keywords. However, we didn't, and so I think it is better to do it in all editions.

Do you have an example that isn't a bug soundness fix?

This probably counts: #53851 (comment)

[phaylon]

However, we didn't, and so I think it is better to do it in all editions.

Why do you believe the things crater can test are enough to justify breaking backwards compatibility? Do you believe the things it can't reach, that live in different contexts, are less important?

This probably counts

That issue still seems to be open. Anything that got merged?

I have to say, the fact that there's a third breakage coming down the line I didn't notice or know about makes this even worse.

[phaylon]

Also,
@Centril

If we had found crater regressions (even a single one) I would have been inclined to do this via the edition mechanism

What will be the course of action if someone speaks up after this hits stable?

[phaylon]

Some more information:

The HN discussion from when the stability promise came out reads to me as if people understood it as I did.

I often see people make comments about Rust's backwards compatibility, for example these by @steveklabnik

From this HN comment

Rust does have a stable syntax, or at least, by "stable" I mean it only changes in backwards compatible ways.

So I wouldn't be surprised if more people had the same understanding as I have/had.

[Centril]

@phaylon

What will be the course of action if someone speaks up after this hits stable?

Keeping in mind that this is a hypothetical question; this depends on a few variables:

1. How many regressions were there?
2. How many crates regressed?
3. Were the regressions in crates with reverse dependencies that makes fixing it locally difficult?
4. How difficult is it to fix locally? E.g. is this a macro that is hard to rewrite?
5. What are our chances to "demote" the change to being Rust-2018-only?

What I would be quite opposed to is to be forced to do it in Rust 2021.
But if examples of actual breakage are raised in a timely manner we could make this a Rust 2018 change.

As for @steveklabnik's note, I think we should make a distinction about de jure and de facto breaking changes as @dherman aptly put it. I believe what we did here constitutes a de jure breaking change, but not a de facto breaking change because I believe no one was affected by this.

I would also note that by making this a 2015 breakage it is likely that the net breakage is lower than if we had done this on 2018 only as more crates can keep working on Rust 2015 and get to use the let_chains feature.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[nikomatsakis]

So...I continue to be sympathetic to the points being raised here. However, I also think that we have from the beginning of the project interpreted the term "breaking changes" in a practical way -- that is, we want to ensure "hassle free upgrades" and that "rustc will not break your code", but not necessarily "rustc will not break any code". In addition to the two semver RFCs (RFC 1105, RFC 1122) and the rustc bug fix procedure, there is other precedent. The most notable to me is RFC 599, which changed the defaults around default object lifetimes. In the process it explicitly overrode a prior RFC. Here is the text from the "drawbacks" section of that RFC:

A. Breaking change. This change has the potential to break some existing code, though given the statistics gathered we believe the effect will be minimal (in particular, defaults are only permitted in fn signatures today, so in most existing code explicit lifetime bounds are used).

As I said before, I think that -- procedurally -- we have not handled this very well. I wish that the RFC had originally stipulated that we make this a breaking change, for example. However, I think that making this decision via FCP on the tracking issue is also legitimate.

Seperately, I think it would be useful to open an RFC that makes this "practical definition" of breaking changes more explicit and tries to narrow down and layout the criteria when what is a "breaking change" (versus a "change that theoretically breaks") -- the rustc bug fix procedure does exactly this, but in a narrower context.

For the time being, though, we have to ask ourselves about where particular case falls. My sense remains that it pretty clearly falls on the "no practical breakage" side of the line and that we might as well clean it up.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[Centril]

Triage: Remaining work to be done is documenting the changes made. After that we can close this issue.

[Centril]

Triage: documentation to the reference was done by @davidtwco and merged.