Skip to content

Add rebind_fn_arg_as_mut lint #6245

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1906,6 +1906,7 @@ Released 2018-09-13
[`range_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_step_by_zero
[`range_zip_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_zip_with_len
[`rc_buffer`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer
[`rebind_fn_arg_as_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#rebind_fn_arg_as_mut
[`redundant_allocation`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_allocation
[`redundant_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone
[`redundant_closure`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure
Expand Down
5 changes: 5 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,7 @@ mod ptr_eq;
mod ptr_offset_with_cast;
mod question_mark;
mod ranges;
mod rebind_fn_arg_as_mut;
mod redundant_clone;
mod redundant_closure_call;
mod redundant_field_names;
Expand Down Expand Up @@ -798,6 +799,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&ranges::RANGE_PLUS_ONE,
&ranges::RANGE_ZIP_WITH_LEN,
&ranges::REVERSED_EMPTY_RANGES,
&rebind_fn_arg_as_mut::REBIND_FN_ARG_AS_MUT,
&redundant_clone::REDUNDANT_CLONE,
&redundant_closure_call::REDUNDANT_CLOSURE_CALL,
&redundant_field_names::REDUNDANT_FIELD_NAMES,
Expand Down Expand Up @@ -959,6 +961,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|| box lifetimes::Lifetimes);
store.register_late_pass(|| box entry::HashMapPass);
store.register_late_pass(|| box ranges::Ranges);
store.register_late_pass(|| box rebind_fn_arg_as_mut::RebindFnArgAsMut);
store.register_late_pass(|| box types::Casts);
let type_complexity_threshold = conf.type_complexity_threshold;
store.register_late_pass(move || box types::TypeComplexity::new(type_complexity_threshold));
Expand Down Expand Up @@ -1489,6 +1492,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&ranges::MANUAL_RANGE_CONTAINS),
LintId::of(&ranges::RANGE_ZIP_WITH_LEN),
LintId::of(&ranges::REVERSED_EMPTY_RANGES),
LintId::of(&rebind_fn_arg_as_mut::REBIND_FN_ARG_AS_MUT),
LintId::of(&redundant_clone::REDUNDANT_CLONE),
LintId::of(&redundant_closure_call::REDUNDANT_CLOSURE_CALL),
LintId::of(&redundant_field_names::REDUNDANT_FIELD_NAMES),
Expand Down Expand Up @@ -1646,6 +1650,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&ptr_eq::PTR_EQ),
LintId::of(&question_mark::QUESTION_MARK),
LintId::of(&ranges::MANUAL_RANGE_CONTAINS),
LintId::of(&rebind_fn_arg_as_mut::REBIND_FN_ARG_AS_MUT),
LintId::of(&redundant_field_names::REDUNDANT_FIELD_NAMES),
LintId::of(&redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES),
LintId::of(&regex::TRIVIAL_REGEX),
Expand Down
92 changes: 92 additions & 0 deletions clippy_lints/src/rebind_fn_arg_as_mut.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
use crate::utils::{snippet, span_lint_and_then};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::{
BindingAnnotation, ExprKind, ImplItem, ImplItemKind, Item, ItemKind, Local, Node, PatKind, QPath, TraitFn,
TraitItem, TraitItemKind,
};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::lint::in_external_macro;
use rustc_session::{declare_lint_pass, declare_tool_lint};

declare_clippy_lint! {
/// **What it does:** Checks for function arguments declared as not mutable and
/// later rebound as mutable.
///
/// **Why is this bad?** It can be easily improved by just declaring the function
/// argument as mutable and removing the unnecessary assignment.
///
/// **Known problems:** The function argument might have been shadowed by another
/// value before the assignment.
///
/// **Example:**
///
/// ```rust
/// fn foo_bad(bar: Vec<i32>) -> Vec<i32> {
/// let mut bar = bar;
/// bar.push(42);
/// bar
/// }
/// ```
/// Use instead:
/// ```rust
/// fn foo(mut bar: Vec<i32>) -> Vec<i32> {
/// bar.push(42);
/// bar
/// }
/// ```
pub REBIND_FN_ARG_AS_MUT,
style,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ebroto what do you think of the lint level for this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be a style lint, but IMO we should:

  • Check that the scope of the linted binding is the same as the scope of the parameter (so, the whole function body)
  • Fix the known problem

If we want to leave these cases for a follow-up, I would add the lint to nursery for the time being.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I can address your first suggestion this weekend.

As to the second, if by fixing you mean generating an applicable suggestion, I only didn't do it because the output looked awful: a suggestion to remove an entire line just prints out a blank that might confuse the user if they don't pay attention to the line number. If it's possible to make it better than that, let me know!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not being precise. I meant to avoid the false positive when the parameter has been shadowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. But actually, I think that is fixed by my new impl. I'm comparing the resolution id for the variable (path) with the HIR id of the parameter's pattern. The thing is, I'm starting to think we should expand this lint to cover not only function parameters but any binding. Is there some case in which let mut x = x; is actually desired?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that is already covered by the shadow_same lint.

"non-mutable function argument rebound as mutable"
}

declare_lint_pass!(RebindFnArgAsMut => [REBIND_FN_ARG_AS_MUT]);

impl LateLintPass<'_> for RebindFnArgAsMut {
fn check_local(&mut self, cx: &LateContext<'tcx>, local: &Local<'tcx>) {
if_chain! {
if !in_external_macro(cx.tcx.sess, local.span);

// LHS
if let PatKind::Binding(BindingAnnotation::Mutable, _, name, None) = local.pat.kind;

// RHS
if let Some(init) = local.init;
if let ExprKind::Path(QPath::Resolved(_, path)) = init.kind;
if path.segments.len() == 1;
if path.segments[0].ident == name;

if let rustc_hir::def::Res::Local(id) = path.res;

// Fn
let parent_id = cx.tcx.hir().get_parent_item(id);

if let Node::Item(&Item { kind: ItemKind::Fn(_, _, body_id), .. })
| Node::ImplItem(&ImplItem { kind: ImplItemKind::Fn(_, body_id), .. })
| Node::TraitItem(&TraitItem { kind: TraitItemKind::Fn(_, TraitFn::Provided(body_id)), .. })
= cx.tcx.hir().get(parent_id);

let body = cx.tcx.hir().body(body_id);
if let Some(param) = body.params.iter().find(|param| param.pat.hir_id == id);
if let PatKind::Binding(BindingAnnotation::Unannotated, ..) = param.pat.kind;

then {
span_lint_and_then(
cx,
REBIND_FN_ARG_AS_MUT,
param.pat.span,
&format!("argument `{}` is declared as not mutable, and later rebound as mutable", name),
|diag| {
diag.span_suggestion(
param.pat.span,
"consider just declaring as mutable",
format!("mut {}", snippet(cx, param.pat.span, "_")),
Applicability::MaybeIncorrect,
);
diag.span_help(local.span, "and remove this");
},
);
}
}
}
}
7 changes: 7 additions & 0 deletions src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1936,6 +1936,13 @@ vec![
deprecation: None,
module: "types",
},
Lint {
name: "rebind_fn_arg_as_mut",
group: "style",
desc: "non-mutable function argument rebound as mutable",
deprecation: None,
module: "rebind_fn_arg_as_mut",
},
Lint {
name: "redundant_allocation",
group: "perf",
Expand Down
36 changes: 36 additions & 0 deletions tests/ui/rebind_fn_arg_as_mut.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// edition:2018

#![warn(clippy::rebind_fn_arg_as_mut)]

fn f(x: bool) {
let mut x = x;
}

trait T {
fn tm1(x: bool) {
let mut x = x;
}
fn tm2(x: bool);
}

struct S;

impl S {
fn m(x: bool) {
let mut x = x;
}
}

impl T for S {
fn tm2(x: bool) {
let mut x = x;
}
}

fn f_no_lint(mut x: bool) {
let mut x = x;
}

async fn expansion<T>(_: T) {}

fn main() {}
51 changes: 51 additions & 0 deletions tests/ui/rebind_fn_arg_as_mut.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
error: argument `x` is declared as not mutable, and later rebound as mutable
--> $DIR/rebind_fn_arg_as_mut.rs:5:6
|
LL | fn f(x: bool) {
| ^ help: consider just declaring as mutable: `mut x`
|
= note: `-D clippy::rebind-fn-arg-as-mut` implied by `-D warnings`
help: and remove this
--> $DIR/rebind_fn_arg_as_mut.rs:6:5
|
LL | let mut x = x;
| ^^^^^^^^^^^^^^

error: argument `x` is declared as not mutable, and later rebound as mutable
--> $DIR/rebind_fn_arg_as_mut.rs:10:12
|
LL | fn tm1(x: bool) {
| ^ help: consider just declaring as mutable: `mut x`
|
help: and remove this
--> $DIR/rebind_fn_arg_as_mut.rs:11:9
|
LL | let mut x = x;
| ^^^^^^^^^^^^^^

error: argument `x` is declared as not mutable, and later rebound as mutable
--> $DIR/rebind_fn_arg_as_mut.rs:19:10
|
LL | fn m(x: bool) {
| ^ help: consider just declaring as mutable: `mut x`
|
help: and remove this
--> $DIR/rebind_fn_arg_as_mut.rs:20:9
|
LL | let mut x = x;
| ^^^^^^^^^^^^^^

error: argument `x` is declared as not mutable, and later rebound as mutable
--> $DIR/rebind_fn_arg_as_mut.rs:25:12
|
LL | fn tm2(x: bool) {
| ^ help: consider just declaring as mutable: `mut x`
|
help: and remove this
--> $DIR/rebind_fn_arg_as_mut.rs:26:9
|
LL | let mut x = x;
| ^^^^^^^^^^^^^^

error: aborting due to 4 previous errors