turn rustc_box into an intrinsic

[RalfJung]

I am not entirely sure why this was made a special magic attribute, but an intrinsic seems like a more natural way to add magic expressions to the language.

[rustbot]

r? @lcnr

rustbot has assigned @lcnr.
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

[kpreid]

This seems like a circular and unhelpful definition, because #[rustc_box] is what is left of the box <expr> syntax (and now this intrinsic will be, instead) — box <expr> itself isn't part of the language any more. Wouldn’t it be better to say something like

Constructs a Box<T> value <...specify properties it has that Box::new doesn't...>

[RalfJung]

That's what the next line already does, doesn't it?

[kpreid]

I'm saying that the phrase “a box <expr> expression” doesn't formally mean anything any more, and should therefore be replaced with something else.

[compiler-errors]

You could probably call it a thir::ExprKind::Box, or ShallowInitBox + Move in MIR, perhaps?

[RalfJung]

I think it is quite clear what "a box expression" means. It's the thing being generated by this intrinsic.

The doc comment now reads:

/// Constructs a `Box<T>` by calling the `exchange_malloc` lang item and moving the argument into
/// the newly allocated memory.
///
/// This is the surface syntax for `box <expr>` expressions.

[compiler-errors]

Anyways, I think other than the fact that this is presumably now the only "THIR" intrinsic, this does clean up the code well

side-note: It would be nice to have some way to make intrinsics more self-documenting about where what stage they're handled in the compiler, since some are lowered in MIR, some are handled by codegen backends, and one now seems to be intercepted THIR 😸.

[compiler-errors]

r=me after blessing

[RalfJung]

side-note: It would be nice to have some way to make intrinsics more self-documenting about where what stage they're handled in the compiler, since some are lowered in MIR, some are handled by codegen backends, and one now seems to be intercepted THIR 😸.

I don't disagree but I don't think this should be in the library docs. That seems more like something that should be somewhere in the compiler docs. Also, it seems hard to keep up-to-date...

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[RalfJung]

@bors r=compiler-errors

[bors]

📌 Commit ac9cb90 has been approved by compiler-errors

It is now in the queue for this repository.

[Kobzol]

This likely had negative results for max RSS for deep-vector, with slightly positive icount results.

I assume that the deep-vector effect was expected, since I saw it being discussed somewhere, but it would be nice to run perf. for anything related to the box intrinsic, as it's quite performance sensitive.

[RalfJung]

I didn't expect this to change anything TBH, this is just a small change in how the intrinsic is represented but the generated MIR should be exactly the same... but fair point, apparently even that can have perf impact somehow.

Is it worth investigating the RSS regression? I don't know how I'd even start doing that.

[Kobzol]

Ah, the max RSS regression was discussed here, on a different PR, I thought that this PR is related to that other one, but probably not.

Given that the regression was only in https://github.com/rust-lang/rustc-perf/blob/master/collector/compile-benchmarks/deep-vector/src/main.rs, which doesn't seem super realistic, and the regression wasn't that big (like the 80% figure that we saw in the other PR), I'd say that we should only investigate if someone finds a problem with this change in real code.