Lint needless take-by-value #1556
Conversation
sinkuu
force-pushed
the
take_by_value
branch
2 times, most recently
from
7985a23 to
c08643a
Compare
sinkuu
force-pushed
the
take_by_value
branch
from
9d9040f to
d81d961
Compare
| span_lint_and_then(cx, | ||
| NEEDLESS_TAKE_BY_VALUE, | ||
| input.span, | ||
| "this function taking a value by value, but only using them by reference", |
There was a problem hiding this comment.
"this function [is] taking a value by value, but only using [it] by reference"?
| 4 | #![deny(needless_take_by_value)] | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ | ||
| help: consider taking a reference instead | ||
| | fn foo<T: Default>(v: &Vec<T>, w: Vec<T>, mut x: Vec<T>, y: Vec<T>) -> Vec<T> { |
There was a problem hiding this comment.
(Sadly, clippy will complain about this on the next run and suggest &[T]. But I guess that's for another issue.)
There was a problem hiding this comment.
This looks like a problem quite frustrating for users and I would like this lint to be aware of ptr_arg.
CHANGELOG.md
Outdated
| @@ -383,6 +383,7 @@ All notable changes to this project will be documented in this file. | |||
| [`needless_lifetimes`]: https://github.com/Manishearth/rust-clippy/wiki#needless_lifetimes | |||
| [`needless_range_loop`]: https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop | |||
| [`needless_return`]: https://github.com/Manishearth/rust-clippy/wiki#needless_return | |||
| [`needless_take_by_value`]: https://github.com/Manishearth/rust-clippy/wiki#needless_take_by_value | |||
There was a problem hiding this comment.
needless_pass_by_value sounds more idiomatic.
And fixes false-positives for generics and `match`
This `match` is not moving the `String`:
```rust
fn foo(x: Option<String>) -> i32 {
match x {
Some(_) => 1,
None => 2,
}
}
```
With this change, it will be linted and suggested to add `*` to deref it.
```rust
fn foo(x: &Option<String>) -> i32 {
match *x {
Some(_) => 1,
None => 2,
}
}
```
sinkuu
force-pushed
the
take_by_value
branch
from
effb4ab to
6c83d88
Compare
sinkuu
force-pushed
the
take_by_value
branch
from
6c83d88 to
627d24c
Compare
| } | ||
|
|
||
| // Suggests adding `*` to dereference the added reference. | ||
| if let Some(spans) = spans_need_deref.get(&defid) { |
There was a problem hiding this comment.
this isn't happening in the Vec case anymore.
It would probably be best if it (both the derefs and the main suggestion) were a single suggestion with multiple spans and replacements. Use util::multispan_sugg to create such a suggestion.
| @@ -573,10 +573,10 @@ pub fn span_lint_and_then<'a, 'tcx: 'a, T: LintContext<'tcx>, F>( | |||
| /// | |||
| /// Note: in the JSON format (used by `compiletest_rs`), the help message will appear once per | |||
| /// replacement. In human-readable format though, it only appears once before the whole suggestion. | |||
| pub fn multispan_sugg(db: &mut DiagnosticBuilder, help_msg: String, sugg: &[(Span, &str)]) { | |||
There was a problem hiding this comment.
looks like we need the opposite lint, too :D Opened an issue #1563
| @@ -146,19 +145,13 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { | |||
| format!("&{}", snippet(cx, input.span, "_"))); | |||
| } | |||
|
|
|||
| // Suggests adding `*` to dereference the added reference. | |||
| // Suggests adding `*` to dereference the added reference if needed. | |||
There was a problem hiding this comment.
The primary suggestions also need to go into the spans Vec, otherwise a tool like rustfix won't be able to know that they belong together and you could end up applying one but not the other.
|
Sorry for the churn - updated to create a single suggestion now. |
| @@ -132,27 +132,25 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { | |||
| db.span_suggestion(input.span, | |||
| &format!("consider changing the type to `{}`", slice_ty), | |||
| slice_ty); | |||
| return; | |||
| return; // `Vec` and `String` cannot be destructured - no need for `*` suggestion | |||
There was a problem hiding this comment.
Please add assertions that span_need_deref is empty as a canary for when matching on vec/string becomes possible.
No worries. This PR is already golden, we're just polishing it to a shine. |
Fixes #1551.