[WIP] Also lint Box::new in unused_allocation

[est31]

Previously, only box syntax uses would yield in the unused_allocation lint issuing warnings. Now, equivalent unnecessary usage of Box::new also gives a warning. Furthermore, this PR adds a test of the unused_allocation lint which was previously never tested anywhere in the test suite.

With the changes from this pr, this snippet

fn main() {
    let foo = Box::new([1, 2, 3]).len(); //~ ERROR: unnecessary allocation
}

gives

error: unnecessary allocation, use `&` instead
  --> $DIR/unused-allocation.rs:
   |
LL |     let foo = Box::new([1, 2, 3]).len();
   |               ^^^^^^^^

Just like how it lints for the equivalent snippet just with box instead of Box::new.

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

[est31]

I'm wondering about the last commit, which changed assert!(a == Box::new(8)) into assert!(*a == 8). I'm not sure about the quality of the lint here, because it still suggested adding a & (it suggests & unconditionally). In most cases, it's the right advice, but in this, it's the wrong one, as assert!(a == &8) does not work.

Fundamentally the issue already exists, with & being suggested for assert!(a == box 8):

error: unnecessary allocation, use `&` instead
 --> src/main.rs:6:18
  |
6 |     assert!(a == box 8);
  |                  ^^^^^
  |

But that affects a nightly feature so it does not need to be as polished.

I wonder if the lint needs further improvements before it can be extended to Box::new.

Especially I wonder: a) does the *a trick work all the time? Maybe box/deref patterns are needed? and b) Can you somehow recognize cases where *a should be suggested instead? Is == the only place the issue occurs or are there other places too?

[petrochenkov]

Making it a diagnostic item instead of a lang item would be preferable in this case.

[est31]

I've been planning, as a followup to this PR, to also use this for the #[rustc_box] feature. Right now, #[rustc_box] foobar(foo) is lowered to box foo, irrespective of what foobar is, even if it isn't Box::new. In order to make it error if it's not Box::new, I would be needing something that gives me a def id i suppose. Is a diagnostic item a good choice for such an error as well?

To explain, in a box_syntax free world, some users might still want to use #[rustc_box] over Box::new and they should get a more polished feature.

[petrochenkov]

Right now, #[rustc_box] foobar(foo) is lowered to box foo, irrespective of what foobar is, even if it isn't Box::new.

Which seems fine to me, given that #[rustc_box] is an internal feature that is supposed to be removed in the future.

free world, some users might still want to use #[rustc_box] over Box::new and they should get a more polished feature.

If it's supposed to be used by people and be polished, then perhaps it needs a better syntax than #[rustc_box] Box::new(expr), for example box expr, oh wait.

[petrochenkov]

I didn't follow the story behind #97293 but the state in which you need to write #[rustc_box] Box::new(expr) instead of just Box::new(expr) or box expr (*) seems like the worse from both worlds.

(*) Preferably the former, because whatever made Box::new(expr) slow in #97293 can also make other much more common things like Vec::from()/String::from() slow, so it may be a good win if it's fixed.

[est31]

Box::new(foo) is not slow in most of cases, thanks to LLVM being really good, especially post version 12 (minimum LLVM is 12 now). The issue is just that switching vec![] to Box::new would also switch it for the remaining few cases where it does cause regressions. For these, the PR added #[rustc_box] as an alternative to box foo. My ultimate goal is to remove box foo support from the compiler, this PR is another preparation of it. Eventually this will need a lang team FCP and a "go forward" from them. Anyways, the general future of box syntax is not discussed here but in the tracking issue (still need to make a proposal there).

It might be better if instead of doing empty promises, I add the lang item in a separate PR first that actually needs it, and then switch this PR to it.

[petrochenkov]

I wonder if the lint needs further improvements before it can be extended to Box::new.

No opinion on this, for that you can try finding some other reviewer who cares more about this case.
@rustbot author

[est31]

I've marked this as WIP because of: #97774 (comment)

Ideally I'd convert this into a hard error (temporarily for the experiment), run a crater run, and then see what the lint complains about and how many of those cases are not beautiful complaints.

[bors]

☔ The latest upstream changes (presumably #98673) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    Checking alloc v0.0.0 (/checkout/library/alloc)
    Checking cfg-if v0.1.10
    Checking adler v0.2.3
    Checking rustc-demangle v0.1.21
error[E0522]: definition of an unknown language item: `owned_box_new`
    |
    |
210 |     #[lang = "owned_box_new"]
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^ definition of unknown language item `owned_box_new`
For more information about this error, try `rustc --explain E0522`.
error: could not compile `alloc` due to previous error
Build completed unsuccessfully in 0:01:17

[bors]

☔ The latest upstream changes (presumably #101986) made this pull request unmergeable. Please resolve the merge conflicts.

[JohnCSimon]

@est31
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive