Added the `[unnecessary_box_returns]` lint #9102

botahamec · 2022-07-02T23:40:11Z

fixes #5

I'm not confident in the name of this lint. Let me know if you can think of something better

changelog: New lint: [`unnecessary_box_returns`]
#9102

rust-highfive · 2022-07-02T23:40:14Z

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

bors · 2022-07-03T20:23:22Z

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

dswij · 2022-07-15T10:31:45Z

Thanks for the PR! Sorry I haven't been able to take a look at this. I will get to it asap this week when I have the time

bors · 2022-07-15T12:38:47Z

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

dswij

Thanks for the PR and the patience! The lint logic itself looks great. For the naming, I think unnecessary_box_returns is slightly more descriptive. What do you think?

I'm actually curious whether this will fire a lot in other rust codebases. I will try to run this lint for some of the most popular crates.

clippy_lints/src/unused_box.rs

botahamec · 2022-07-19T19:45:16Z

@dswij I like unnecessary_box_return. I think an 's' at the end is unnecessary.

xFrednet · 2022-07-19T22:00:55Z

Hey, happy that you asked about the name and the suggestion. I would keep the 's' at the end of the name. Rustc's lint naming conventions suggest that lint names should be plural if possible. This also works better with lint attributes affecting multiple cases, like #![deny(unnecessary_box_returns)]

botahamec · 2022-07-20T01:11:39Z

@xFrednet Ah, ok. I'll make that change then

Jarcho · 2022-07-20T02:01:46Z

The lint itself is fine, but it requires placement syntax before it's generally applicable. As far as I know that's completely stalled as of now. Box::new(foo()) can't optimize out the temporary from calling foo() unless it's inlined (and not necessarily then).

As a restriction lint the downside is fine. A size limit would also be fine. Just leaving it in the nursery until placement syntax is stabilized would also work.

bors · 2022-07-25T15:39:18Z

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

dswij · 2022-07-26T09:45:57Z

The lint itself is fine, but it requires placement syntax before it's generally applicable. As far as I know that's completely stalled as of now. Box::new(foo()) can't optimize out the temporary from calling foo() unless it's inlined (and not necessarily then).

@botahamec I think we can add this to the test case, and I think it's good to be merged once the merge conflict is addressed.

dswij · 2022-07-26T10:05:19Z

So, I ran this lint on 250 crates:

Summary

Build failures

Total: 1

vsdb-0.45.2

Warnings

Total: 17

Crate	Count
bincode-1.3.3	4
clap-3.2.15	2
crossbeam-channel-0.5.6	1
crossbeam-epoch-0.9.10	1
miniz_oxide-0.5.3	2
parking_lot_core-0.9.3	1
pest-2.1.3	4
regex-1.6.0	1
structopt-0.3.26	1

There are some improvements that needs to be made. e.g. these suggestion aren't the most accurate.

---> pest-2.1.3/src/parser_state.rs:1107:57
warning: function returns `Box<parser_state::ParserState<'i, R>>` when `parser_state::ParserState<'i, R>` implements `Sized`
    --> src/parser_state.rs:1107:57
     |
1107 |     pub(crate) fn checkpoint_ok(mut self: Box<Self>) -> Box<Self> {
     |                                                         ^^^^^^^^^ help: change the return type to: `parser_state::ParserState<'i, R>`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns

---> crossbeam-channel-0.5.6/src/flavors/zero.rs:57:27
warning: function returns `Box<flavors::zero::Packet<T>>` when `flavors::zero::Packet<T>` implements `Sized`
  --> src/flavors/zero.rs:57:27
   |
57 |     fn empty_on_heap() -> Box<Packet<T>> {
   |                           ^^^^^^^^^^^^^^ help: change the return type to: `flavors::zero::Packet<T>`
   |
   = note: requested on the command line with `-W clippy::unnecessary-box-returns`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns

---> structopt-0.3.26/src/lib.rs:1218:53
warning: function returns `Box<T>` when `T` implements `Sized`
    --> src/lib.rs:1218:53
     |
1218 |     fn from_clap(matches: &clap::ArgMatches<'_>) -> Self {
     |                                                     ^^^^ help: change the return type to: `T`
     |
     = note: requested on the command line with `-W clippy::unnecessary-box-returns`
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns

We can address this in this PR or later as an improvement since this lint is going into nursery. What do you think? @botahamec

botahamec · 2022-07-26T15:26:49Z

@dswij I think it at least makes sense to take a look at the third one. I'd honestly be ok with merging on the first two examples. Let me know if there's anything else I should look at though. I don't have my desktop with me at the moment, so it might take a while.

dswij · 2022-07-26T18:46:19Z

No rush. Feel free to ping me when it's ready

bors · 2022-08-02T11:36:59Z

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

kraktus · 2022-09-14T15:59:34Z

...

---> pest-2.1.3/src/parser_state.rs:1107:57
warning: function returns `Box<parser_state::ParserState<'i, R>>` when `parser_state::ParserState<'i, R>` implements `Sized`
    --> src/parser_state.rs:1107:57
     |
1107 |     pub(crate) fn checkpoint_ok(mut self: Box<Self>) -> Box<Self> {
     |                                                         ^^^^^^^^^ help: change the return type to: `parser_state::ParserState<'i, R>`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns

---> crossbeam-channel-0.5.6/src/flavors/zero.rs:57:27
warning: function returns `Box<flavors::zero::Packet<T>>` when `flavors::zero::Packet<T>` implements `Sized`
 --> src/flavors/zero.rs:57:27
  |
57 |     fn empty_on_heap() -> Box<Packet<T>> {
  |                           ^^^^^^^^^^^^^^ help: change the return type to: `flavors::zero::Packet<T>`
  |
  = note: requested on the command line with `-W clippy::unnecessary-box-returns`
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns

---> structopt-0.3.26/src/lib.rs:1218:53
warning: function returns `Box<T>` when `T` implements `Sized`
   --> src/lib.rs:1218:53
    |
1218 |     fn from_clap(matches: &clap::ArgMatches<'_>) -> Self {
    |                                                     ^^^^ help: change the return type to: `T`
    |
    = note: requested on the command line with `-W clippy::unnecessary-box-returns`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns

Note that the first two examples would be caught back by use_self anyway

dswij · 2022-11-19T09:09:39Z

hey @botahamec, sorry to nag, but it seems like there's not a lot of activity on this PR lately. Let me know if you don't have much time, and I might be able to continue the work on this PR.

botahamec · 2022-11-19T15:08:36Z

@dswij Oh yeah, feel free.

dswij · 2023-01-07T19:17:08Z

r? @xFrednet

I've rebased this and update some outdated stuff. This should be ready to go, any improvements can be in future PRs since this is going to nursery.

Would you mind to help take a look if you have the time?

bors · 2023-03-08T09:46:31Z

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

xFrednet · 2023-03-13T23:42:22Z

Hey @botahamec, since you pushed since my last review, have you seen the comments I left? Besides those two I think this is ready for merge :). Your welcome to reach out if you need help with the rebases or have any other questions :)

botahamec · 2023-03-13T23:45:28Z

@xFrednet Yeah. I was hoping to get it done last week, but I never got around to it. I don't have my computer this week, so hopefully I'll finish it next week.

xFrednet · 2023-03-13T23:58:12Z

Alright, no worries, just wanted to make sure you're not stuck on anything :). Enjoy your week!

botahamec · 2023-03-26T22:02:51Z

Alright. I'm done squashing. That was surprisingly convoluted, but it's down to just four commits now.

xFrednet · 2023-03-30T08:10:47Z

This version looks good to me. Thank you for sticking with this PR and completing it, I highly appreciate it. The squash also looks perfect. I hope you had fun :)

@bors r+

bors · 2023-03-30T08:10:49Z

📌 Commit 76d13bb has been approved by xFrednet

It is now in the queue for this repository.

bors · 2023-03-30T08:22:19Z

⌛ Testing commit 76d13bb with merge ef3867f...

bors · 2023-03-30T08:35:31Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing ef3867f to master...

rust-highfive assigned dswij Jul 2, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 2, 2022

botahamec force-pushed the unused-box branch from d353bc6 to 59009fc Compare July 4, 2022 16:17

botahamec force-pushed the unused-box branch from 59009fc to 79bf2d0 Compare July 15, 2022 14:08

dswij reviewed Jul 19, 2022

View reviewed changes

clippy_lints/src/unused_box.rs Outdated Show resolved Hide resolved

clippy_lints/src/unused_box.rs Outdated Show resolved Hide resolved

xFrednet changed the title ~~Added the [unused_box] lint~~ Added the [unnecessary_box_returns] lint Jul 20, 2022

dswij mentioned this pull request Jul 21, 2022

lint matchable if let for pattern #9221

Closed

botahamec force-pushed the unused-box branch 2 times, most recently from 4a2a27d to 59a3ccd Compare July 30, 2022 19:55

dswij added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Aug 14, 2022

dswij force-pushed the unused-box branch from 59a3ccd to ae6496c Compare January 7, 2023 18:25

botahamec force-pushed the unused-box branch from 4db1131 to c328db2 Compare March 7, 2023 23:32

botahamec force-pushed the unused-box branch 8 times, most recently from 2e65e4a to 4d50aaa Compare March 26, 2023 21:39

botahamec added 4 commits March 26, 2023 17:49

Added the new lint with some docs and tests

022f76d

Lint on trait declarations, not implementations

1b55c81

Avoid breaking exported API

a143fb7

Don't lint functions with "box" in their name

76d13bb

botahamec force-pushed the unused-box branch from 4d50aaa to 76d13bb Compare March 26, 2023 21:50

xFrednet approved these changes Mar 30, 2023

View reviewed changes

bors merged commit ef3867f into rust-lang:master Mar 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the `[unnecessary_box_returns]` lint #9102

Added the `[unnecessary_box_returns]` lint #9102

botahamec commented Jul 2, 2022 •

edited by xFrednet

Loading

rust-highfive commented Jul 2, 2022

bors commented Jul 3, 2022

dswij commented Jul 15, 2022

bors commented Jul 15, 2022

dswij left a comment

botahamec commented Jul 19, 2022

xFrednet commented Jul 19, 2022

botahamec commented Jul 20, 2022

Jarcho commented Jul 20, 2022

bors commented Jul 25, 2022

dswij commented Jul 26, 2022

dswij commented Jul 26, 2022

botahamec commented Jul 26, 2022

dswij commented Jul 26, 2022

bors commented Aug 2, 2022

kraktus commented Sep 14, 2022 •

edited

Loading

dswij commented Nov 19, 2022

botahamec commented Nov 19, 2022

dswij commented Jan 7, 2023

bors commented Mar 8, 2023

xFrednet commented Mar 13, 2023

botahamec commented Mar 13, 2023

xFrednet commented Mar 13, 2023

botahamec commented Mar 26, 2023

xFrednet commented Mar 30, 2023

bors commented Mar 30, 2023

bors commented Mar 30, 2023

bors commented Mar 30, 2023

Added the [unnecessary_box_returns] lint #9102

Added the [unnecessary_box_returns] lint #9102

Conversation

botahamec commented Jul 2, 2022 • edited by xFrednet Loading

rust-highfive commented Jul 2, 2022

bors commented Jul 3, 2022

dswij commented Jul 15, 2022

bors commented Jul 15, 2022

dswij left a comment

Choose a reason for hiding this comment

botahamec commented Jul 19, 2022

xFrednet commented Jul 19, 2022

botahamec commented Jul 20, 2022

Jarcho commented Jul 20, 2022

bors commented Jul 25, 2022

dswij commented Jul 26, 2022

dswij commented Jul 26, 2022

Summary

Build failures

Warnings

botahamec commented Jul 26, 2022

dswij commented Jul 26, 2022

bors commented Aug 2, 2022

kraktus commented Sep 14, 2022 • edited Loading

dswij commented Nov 19, 2022

botahamec commented Nov 19, 2022

dswij commented Jan 7, 2023

bors commented Mar 8, 2023

xFrednet commented Mar 13, 2023

botahamec commented Mar 13, 2023

xFrednet commented Mar 13, 2023

botahamec commented Mar 26, 2023

xFrednet commented Mar 30, 2023

bors commented Mar 30, 2023

bors commented Mar 30, 2023

bors commented Mar 30, 2023

Added the `[unnecessary_box_returns]` lint #9102

Added the `[unnecessary_box_returns]` lint #9102

botahamec commented Jul 2, 2022 •

edited by xFrednet

Loading

kraktus commented Sep 14, 2022 •

edited

Loading