rust-lang
Added the `[unnecessary_box_returns]` lint
fixes #5
I'm not confident in the name of this lint. Let me know if you can think of something better
changelog: Added the [`unnecessary_box_returns`] lint to warn when the return type of a function is Box<T> where T implements Sized.
r? @dswij
(rust-highfive has picked a reviewer for you, use r? to override)
:umbrella: The latest upstream changes (presumably #9105) made this pull request unmergeable. Please resolve the merge conflicts.
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
:umbrella: The latest upstream changes (presumably #9103) made this pull request unmergeable. Please resolve the merge conflicts.
@dswij I like unnecessary_box_return. I think an 's' at the end is unnecessary.
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)]
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.
:umbrella: The latest upstream changes (presumably #9243) made this pull request unmergeable. Please resolve the merge conflicts.
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 callingfoo()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.
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
@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.
:umbrella: The latest upstream changes (presumably #9264) made this pull request unmergeable. Please resolve the merge conflicts.
...
---> 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
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.
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?
:umbrella: The latest upstream changes (presumably #10206) made this pull request unmergeable. Please resolve the merge conflicts.
Hey @botahamec, this is a ping from triage. There hasn't been any updates in a long time, are you stuck on an issue or need help with anything?
@xFrednet Oh not at all. I just assumed for a while that @dswij was working on the comments. But the comments seem like things I can resolve if @dswij is ok with that.
Ahh, sorry, I missed that @dswij took over, thank you! @dswij do you need help with anything? :)
:umbrella: The latest upstream changes (presumably #10415) made this pull request unmergeable. Please resolve the merge conflicts.
:umbrella: The latest upstream changes (presumably #10362) made this pull request unmergeable. Please resolve the merge conflicts.
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 :)
@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.
Alright, no worries, just wanted to make sure you're not stuck on anything :). Enjoy your week!
Alright. I'm done squashing. That was surprisingly convoluted, but it's down to just four commits now.
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+