Restrict #[rustc_box] to Box::new calls

[clubby789]

Currently, #[rustc_box] can be applied to any call expression with a single argument. This PR only allows it to be applied to calls to Box::new

[rustbot]

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

[jhpratt]

Other than how Box::new is detected, this looks good.

[jhpratt]

This would allow any Box, not just the standard library's.

As Box is already a lang item (owned_box), you should be able to restrict it to methods of the type without any additional lang items. Alternatively, you could make Box::new a lang item and check for that specifically, which would limit it to only the one method.

[compiler-errors]

Alternatively, you can get the Box lang item, get its tcx.associated_items, and then just use https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/assoc/struct.AssocItems.html#method.find_by_name_and_kind to grab the new item.

[jhpratt]

Sure, that would work as well. I wasn't sure on what was preferred, as I know effort is taken to avoid hard-coding names (even if that name cannot ever change).

[est31]

Alternatively, you could make Box::new a lang item and check for that specifically, which would limit it to only the one method.

Or a diagnostic item, which #104363 is adding for Box::new.

[compiler-errors]

I don't think it's the best idea to use diagnostic items for a hard error like this. The compiler should be able to operate totally fine with all of the diagnostic items cfg'd out.

[compiler-errors]

Huh -- we also could do an expr macro... rustc_box!(expr)

[clubby789]

Potentially, but I was hoping to remove ExprKind::Box from the AST with #108471

[compiler-errors]

Hm... ideally we'd remove box syntax from the parser, but there's nothing necessarily wrong with keeping it around in AST, like we have with type ascription via a macro.

[est31]

Regarding the suggestion to move the error to later in the compiler, it might be possible to move the lowering to HIR->THIR lowering in rustc_mir_build, but I'm not sure one can emit errors at that point. It's certainly worth a try tho.

Basically one would add a test here whether there are any attributes, and if there is #[rustc_box] then one would create a THIR ExprKind::Box as done down below in the same method. As an example for code that checks for attributes, you can look for the #[custom_mir] attribute where the attr check is here.

Regarding rustc_box!(expr), I'm not a fan of using the privileged access that builtin-macros have for such purposes, but it seems to be the status quo (for now, unless we get builtin#). I think that the proposals to replace placement new go in the direction of using syntax that is very similar to function calls though.

[compiler-errors]

Regarding the suggestion to move the error to later in the compiler, it might be possible to move the lowering to HIR->THIR lowering in rustc_mir_build, but I'm not sure one can emit errors at that point. It's certainly worth a try tho.

Yeah, we definitely have errors emitted at and past this point. Notably pattern exhaustiveness errors, but others too.

[compiler-errors]

r? @compiler-errors

[compiler-errors]

@rustbot author

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[clubby789]

As suggested, moved this to check the attribute and call target at HIR->THIR lowering. A clippy lint needed to be updated since it used hir::ExprKind::Box to detect a builtin macro.

Since hir::ExprKind::Box is now no longer created, it may be worth it to remove it entirely, which would require some changes to tools which exhaustively match on HIR exprs - unless builtin# becomes a thing and needs to re-add it?

@rustbot ready

[est31]

it may be worth it to remove it entirely

That will require both this PR and #108471. This one should be quick to merge, so if we remove it, it should be done in #108471 after it is rebased to a merged version of this PR. Or alternatively, a follow-up.

unless builtin# becomes a thing and needs to re-add it?

Yeah if we want builtin#box, we have to re-add it. We could by then also have changed the feature so much that we have breakage anyways: of the, as of #108476 merged, two current users of #[rustc_box], vec![] is using it in more of a hacky way.

As a first step after box syntax removal, I'd suggest a #[rustc_ptr_write] ptr::write(...) feature, taking in the second argument in-place: right now the desugaring code constructs an allocation, writes into it in-place, and then returns it, we could move the allocation construction into explicit code.

A builtin# equivalent would be builtin#ptr_write, and the AST/HIR node would take two items instead of one, so it would generate some churn anyways I guess.

Such a feature would allow us to build box!(), ptr_write!(), etc macros that expose placement on stable, but IMO for 99% of users, it would be nicer if the optimization just turned on with normal calls to Box::new and ptr::write, without needing any changes to user code. But this is more inside RFC territory I think. I'm open to co-authors, contact me on zulip.

Anyways, I'm getting offtopic. I think removing the ExprKind should be okay, but not in this PR.

[compiler-errors]

@bors r+

[bors]

📌 Commit d845769 has been approved by compiler-errors

It is now in the queue for this repository.