Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove box_syntax #108471

Merged
merged 4 commits into from
Mar 13, 2023
Merged

Remove box_syntax #108471

merged 4 commits into from
Mar 13, 2023

Conversation

clubby789
Copy link
Contributor

@clubby789 clubby789 commented Feb 25, 2023

r? @Nilstrieb

This removes the feature box_syntax, which allows the use of box <expr> to create a Box, and finalises removing use of the feature from the compiler. box_patterns (allowing the use of box <pat> in a pattern) is unaffected.
It also removes ast::ExprKind::Box - the only way to create a 'box' expression now is with the rustc-internal #[rustc_box] attribute.
As a temporary measure to help users move away, box <expr> now parses the inner expression, and emits a MachineApplicable lint to replace it with Box::new

Closes #49733

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 25, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 25, 2023

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also adjust rustc_hir_pretty to use #[rustc_box] instead?

compiler/rustc_lint/src/unused.rs Outdated Show resolved Hide resolved
@Noratrieb
Copy link
Member

Deleting it sounds great (totally not me talking about removing it being the reason for you doing it) but I'd like to get some visibility for this anyways, cc @rust-lang/lang @rust-lang/compiler

@clubby789
Copy link
Contributor Author

Updated rustc_ast|hir_pretty to use #[rustc_box]. I restored ExprKind::Box for now since Clippy and R-A use it but it isn't constructed any more

@rust-log-analyzer

This comment has been minimized.

@Noratrieb Noratrieb closed this Feb 25, 2023
@Noratrieb Noratrieb reopened this Feb 25, 2023
@Noratrieb
Copy link
Member

oops, misclick
If the changes to clippy and ra are very small you can just make them here - if they aren't you can PR it on their repos and block this PR on that. I think it's not worth it to have two PRs here.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Feb 26, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@est31
Copy link
Member

est31 commented Feb 26, 2023

The eventual removal of box syntax has been my long term goal (#87781, #88316, #97293, #97494, #99066, #99577, #100194, etc all worked in that direction), but I've been avoiding doing the last step. It's great to see this being proposed.

I think the presence of box syntax causes confusion among people in the sense that they think it is going to be stabilized, which it won't.

The last triage comment from the lang team has said that the lang team waits for stabilization-track features (from my reading those are features that are more likely going to be stabilized than box will) that overlaps with box syntax before box syntax is removed.

I think given boxing is sort-of available on stable, this is mostly resolved. Yes, there is no guarantee for vec to be optimized in such a fashion, but on the other hand, removing the optimization will have performance impacts on user code that are too large. It might be a good idea to talk to potential users of placement new if their needs are met by this (code link).

Also note that the functionality itself is not deleted, just the box syntax for constructing boxes. #[rustc_box] is still around. I have waited with the box_syntax removal step because I wanted my MCP on builtin# to be seconded and implemented before proposing this, but I don't think it's a hard blocker for removal, it would just have been nicer to point users towards builtin#box(foo) instead as it wouldn't have needed two feature gates.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Feb 26, 2023

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/rust-analyzer

cc @rust-lang/wg-rls-2

@rust-log-analyzer

This comment has been minimized.

@clubby789
Copy link
Contributor Author

Not sure about the mir-opt test failures, I blessed them locally but CI has different results. I might try #[rustc_box]ing them here to restore the old behaviour

@rust-log-analyzer

This comment has been minimized.

@est31
Copy link
Member

est31 commented Feb 26, 2023

Some comments on the PR itself:

  • As implemented right now, this change makes the language accepted by the AST parsing less tolerant. In other words, it could cause breakage for macro users where the attribute macro does something with the box to make it disappear, or it could cause issues for people who have put box-syntax using stuff behind #[cfg(false)] or similar. I don't think this is happening, and I doubt going the forward compat lint route is needed, but it has caused breakages in the past, see Pre-expansion gate some more things #64672 and Partially revert the early feature-gatings added in #65742. #66004. I wonder if a crater run would be a good idea or not. In the worst case, one could move the error to AST->HIR lowering and emit a forward compat lint, but I doubt it's needed. Edit: PR Reenable disabled early syntax gates as future-incompatibility lints #99935 has added a forward compat lint for the #[cfg(FALSE)] fn foo() { box 0; } case, so I'd say this issue is resolved.

  • box syntax might be quite used outside of rustc. It might make sense, at least for a few release cycles, to emit an actual box-syntax specific error with a rustfix-able suggestion to use Box::new. That would give users an automated way to migrate away. The usage of box syntax in the wild is non zero, and it's not trivial to build a regex that converts box FooExpr(FooBar{ hi: hello(), }) to the Box::new equivalent.

@leonardo-m
Copy link

@est31 thank you, sorry for bothering you people, you're doing good work... I will try the alternatives.

@saethlin
Copy link
Member

@leonardo-m I have a fair bit of experience with this stuff. I and the comments elsewhere in this PR are suggesting you use vec! to construct a boxed slice, not that you use a Vec<T> instead of Box<[T; N]>. I thought you were saying this suggestion causes an unacceptable performance regression. If it does, we should have an issue to track that.

@mindstorm38
Copy link

I always wondered why Box::new isn't based on Box::new_in with just the global allocator given (yes, I know that GlobalAlloc is not Allocator). Do you know where I can find enlightening discussion on this?

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Mar 23, 2023
flip1995 pushed a commit to flip1995/rust that referenced this pull request Mar 24, 2023
…ieb,est31

Remove `box_syntax`

r? `@Nilstrieb`

This removes the feature `box_syntax`, which allows the use of `box <expr>` to create a Box, and finalises removing use of the feature from the compiler. `box_patterns` (allowing the use of `box <pat>` in a pattern) is unaffected.
It also removes `ast::ExprKind::Box` - the only way to create a 'box' expression now is with the rustc-internal `#[rustc_box]` attribute.
As a temporary measure to help users move away, `box <expr>` now parses the inner expression, and emits a `MachineApplicable` lint to replace it with `Box::new`

Closes rust-lang#49733
flip1995 pushed a commit to flip1995/rust that referenced this pull request Mar 24, 2023
…r-errors

Remove box expressions from HIR

After rust-lang#108516, `#[rustc_box]` is used at HIR->THIR lowering and this is no longer emitted, so it can be removed.

This is based on top of rust-lang#108471 to help with conflicts, so 43490488ccacd1a822e9c621f5ed6fca99959a0b is the only relevant commit (sorry for all the duplicated pings!)

````@rustbot```` label +S-blocked
seeplusplus added a commit to seeplusplus/turbo that referenced this pull request Apr 21, 2023
Box syntax removed in
rust-lang/rust#108471

Removed all `#![features(box_syntax)]`
and ran `cargo fix --broken-code` to
replace all previous box_patterns uses
of `box` with `Box::new()`.
jridgewell pushed a commit to vercel/turborepo that referenced this pull request Apr 26, 2023
### Description

Box syntax removed in rust-lang/rust#108471

Removed all `#![features(box_syntax)]` and ran `cargo fix --broken-code`
to replace all previous box_patterns uses of `box` with `Box::new()`.

### Testing Instructions

No testing needed.
tomtau pushed a commit to pest-parser/pest that referenced this pull request May 6, 2023
CardboardTurkey added a commit to CardboardTurkey/simplexpr that referenced this pull request Jun 24, 2023
`box_syntax` has been removed from rust:
rust-lang/rust#108471. Use `Box::new` instead.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 20, 2024
…ilstrieb

Remove special handling of `box` expressions from parser

rust-lang#108471 added a temporary hack to parse `box expr`. It's been almost a year since then, so I think it's safe to remove the special handling.

As a drive-by cleanup, move `parser/removed-syntax*` tests to their own directory.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 20, 2024
…ilstrieb

Remove special handling of `box` expressions from parser

rust-lang#108471 added a temporary hack to parse `box expr`. It's been almost a year since then, so I think it's safe to remove the special handling.

As a drive-by cleanup, move `parser/removed-syntax*` tests to their own directory.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2024
Rollup merge of rust-lang#120063 - clubby789:remove-box-handling, r=Nilstrieb

Remove special handling of `box` expressions from parser

rust-lang#108471 added a temporary hack to parse `box expr`. It's been almost a year since then, so I think it's safe to remove the special handling.

As a drive-by cleanup, move `parser/removed-syntax*` tests to their own directory.
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 25, 2024
### Description

Box syntax removed in rust-lang/rust#108471

Removed all `#![features(box_syntax)]` and ran `cargo fix --broken-code`
to replace all previous box_patterns uses of `box` with `Box::new()`.

### Testing Instructions

No testing needed.
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
### Description

Box syntax removed in rust-lang/rust#108471

Removed all `#![features(box_syntax)]` and ran `cargo fix --broken-code`
to replace all previous box_patterns uses of `box` with `Box::new()`.

### Testing Instructions

No testing needed.
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 1, 2024
### Description

Box syntax removed in rust-lang/rust#108471

Removed all `#![features(box_syntax)]` and ran `cargo fix --broken-code`
to replace all previous box_patterns uses of `box` with `Box::new()`.

### Testing Instructions

No testing needed.
@nico-abram
Copy link
Contributor

@saethlin
I'm a bit late, but I just reached this issue after running into this error while updating one of my projects.

Having the size information in the Box can help eliminate bounds checking, which is why I intentionally used a boxed array in my specific use case (An emulator, performing lots of unpredictable accesses on a u8 array).

Here's a godbolt example of what I mean, notice how the second function has no bounds checking: https://godbolt.org/z/xsE89de5M

I haven't found an adequate replacement for this usecase of box_syntax yet, I might end up using unsafe (get_unchecked)

@scottmcm
Copy link
Member

@nico-abram If you you start by building a vector, you can convert it into a boxed array with https://doc.rust-lang.org/std/vec/struct.Vec.html#impl-TryFrom%3CVec%3CT%3E%3E-for-Box%3C%5BT;+N%5D%3E -- and then you still get all the improved bounds checking elimination you were talking about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for box_syntax