Add useless_borrows_in_formatting lint#16523
Conversation
|
rustbot has assigned @samueltardieu. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Lintcheck changes for aaa5bd2
This comment will be updated if you push new changes |
There was a problem hiding this comment.
I would prefer if rustc would fix this to inline the generated function, but even then that wouldn't apply to older versions, and removing the useless reference is cleaner, so I think this lint is a good idea.
I'd like to see tests with macros (println! in a macro itself, arguments in a macro, etc.) to ensure this is solid, especially if this becomes a warn-by-default lint. Also, expressions with parentheses such as &(…) or blocks such as &{…}.
|
Reminder, once the PR becomes ready for a review, use |
7b3e922 to
f1d7286
Compare
This comment has been minimized.
This comment has been minimized.
|
Thx @samueltardieu, fixed @rustbot ready |
This comment has been minimized.
This comment has been minimized.
00e13af to
0e0615c
Compare
This comment has been minimized.
This comment has been minimized.
0e0615c to
a4b70d8
Compare
This comment has been minimized.
This comment has been minimized.
|
This lint has been nominated for inclusion. |
There was a problem hiding this comment.
Could you please rename the lint into useless_borrows_in_formatting, and also rename the files implementing the lint and the test files accordingly?
It would also be great if you could split this into two commits:
- The first one with the result of this lint applied to the current Clippy sources
- The second one with the lint itself
This way, the lint implementation stays isolated inside one commit (and can be reverted for example if there are any problems, or better identified in case of a bisection), and at any point the tree stays CI-green.
This comment has been minimized.
This comment has been minimized.
|
@samueltardieu thx, I updated the lint and moved the unrelated changes to #16872 |
|
If you had made it a commit in the same PR, then this PR would have been green. Can you rebase once the other PR is merged into |
minor code cleanup getting ready for the `useless_borrows_in_formatting` lint See #16523 r? @samueltardieu --- changelog: none
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
thx @samueltardieu , i misunderstood your comment - I usually squash all PRs into a single commit in all my projects, so I thought you wanted multiple PRs, not multiple commits in the same PR. Now rebased onto main, and should pass. |
|
done |
Not in the commit message (which does not require the |
Detect format macros where an argument
is passed with an explicit `&` even though the formatter already takes
references, e.g. `println!("{}", &s)` when `s: &str`.
Some original micro-benchmarks showed ~6% performance improvement when redundant refs are removed.
- Lint runs for both Display (`{}`) and Debug (`{:?}`) placeholders when
the inner type is Sized and implements the corresponding trait.
- Applies to the main value argument and to width/precision arguments
(e.g. `format!("{0:1$.2$}", &v1, &v2, &v3)`).
- Suggests removing the redundant `&` with MachineApplicable fix.
- Skip when the argument comes from expansion or a proc macro.
|
ah, right, thx, fixed |
View all comments
Fixes #10851
This is a workaround for the current compiler limitation that results in a ~6% performance degradation.
This lint detects format macros where an argument is passed with an explicit
&references, e.g.println!("{}", &s){}) and Debug ({:?}) placeholders when the inner type is Sized and implements the corresponding trait.format!("{0:1$.2$}", &v1, &v2, &v3)).&with MachineApplicable fix.changelog: [
useless_borrows_in_formatting]: detect redundant&in format macro argumentsP.S. I think this lint should go into
perfcategory once the code is reviewed