remove box_patterns

[cyrgani]

View all comments

This PR removes the old feature box_patterns, tracked in #29641. As explained in #29641 (comment), this feature will not be stabilized since deref_patterns (#87121) is a better and more general version of this feature.

It was not possible to remove this feature until recently since the compiler itself still depended on this feature. This was changed with rust-lang/compiler-team#984.

This PR currently breaks crates that rely on box patterns using the incorrect pre-expansion feature gating (see #154045 for more details). Therefore, this needs a crater run.

Closes #29641.
Closes #105099.
Closes #156110.

[rustbot]

The parser was modified, potentially altering the grammar of (stable) Rust
which would be a breaking change.

cc @fmease

The Rustfmt subtree was changed

cc @rust-lang/rustfmt

The Clippy subtree was changed

cc @rust-lang/clippy

[rustbot]

r? @Kivooeo

rustbot has assigned @Kivooeo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 73 candidates
• Random selection from 20 candidates

[Kivooeo]

Therefore, this needs a crater run

Which one? would check-only be sufficient for this?

[tmandry]

I don't expect the crater breakage to be significant; if it is, we should discuss again.

@rfcbot merge lang

[rust-rfcbot]

@tmandry has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

Concerns:

• crater resolved by remove box_patterns #156749 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

Looks good to me pending crater results; thanks @cyrgani.

@rfcbot reviewed
@rfcbot concern crater

[Kivooeo]

About to run crater. In my opinion check-only should be sufficient for this, do folks have other opinion?

[fmease]

About to run crater. In my opinion check-only should be sufficient for this, do folks have other opinion?

Yes, it's absolutely sufficient. This PR rejects previously valid code in the frontend of the compiler, it removes syntax. The only things that don't show up in check builds are monomorphization and codegen changes.

IINM one thing which you miss out on in check-only crater runs are potential doctest regressions but that's just something you have to accept if you want to have "faster" crater runs, otherwise craterbot check would never be an option for anyone. In any case, the beta crater runs will run in build-and-test mode and exercise doctests anyway, so we'd still catch such regressions eventually and still have a bit of time to either revert the change or to send (more) downstream patches. Anyway, it's not super likely that anybody is using box patterns in doctests (without also using them in the main crater), I'd say.

[fmease]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 1639375 (163937523761cc0def32f1ba9ed63fdf65ad3478, parent: dc375db7d8df0aa450e622c529147c95eee756f5)

[Kivooeo]

@craterbot run mode=check-only

[traviscross]

As I understand it, the way we'd see breakage in stable rust is if a macro was relying on box $pat or $patpath { box $ident } being parsed as a pattern, since we weren't feature-gating box_patterns pre-expansion. As far as I could tell at least, nothing like that turned up in this crater report.

Based on this understanding, we can proceed.

@rfcbot resolve crater

[rust-rfcbot]

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

[rust-rfcbot]

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

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[fmease]

AFAICT this only needs a rebase and a formal "r=" blessing from @Kivooeo, then this can be shipped.

Seems like @cyrgani has been inactive lately, so we could rebase for them.

[cyrgani]

I rebased this, but right now this is still blocked on removing box pattern usage from clippy (rust-lang/rust-clippy#17008). Should I just include these changes in this PR as well?

[fmease]

I rebased this, but right now this is still blocked on removing box pattern usage from clippy (rust-lang/rust-clippy#17008). Should I just include these changes in this PR as well?

Ah, I missed that.

Yes, feel free to update the clippy subtree in this PR given that it's but a mechanical change that has to happen to make our CI pass -- the changes will get sync'ed anyway.

[cyrgani]

@rustbot ready

[Kivooeo]

#156749 (comment) is still unresolved. We should decide whether to tackle it in a follow-up or fix it in this PR