From 1f81dcbebd3af4d448f0304d632c0952852652e7 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Thu, 19 Oct 2017 09:27:58 -0700 Subject: [PATCH 1/3] Pass null borrow context to EUV --- clippy_lints/src/escape.rs | 2 +- clippy_lints/src/loops.rs | 2 +- clippy_lints/src/needless_pass_by_value.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/escape.rs b/clippy_lints/src/escape.rs index beb96f333..2038a5913 100644 --- a/clippy_lints/src/escape.rs +++ b/clippy_lints/src/escape.rs @@ -71,7 +71,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { }; let region_scope_tree = &cx.tcx.region_scope_tree(fn_def_id); - ExprUseVisitor::new(&mut v, cx.tcx, cx.param_env, region_scope_tree, cx.tables).consume_body(body); + ExprUseVisitor::new(&mut v, cx.tcx, cx.param_env, region_scope_tree, cx.tables, None).consume_body(body); for node in v.set { span_lint( diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 97925b0bd..6ea48c5ea 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1390,7 +1390,7 @@ fn check_for_mutation(cx: &LateContext, body: &Expr, bound_ids: &[Option let mut delegate = MutateDelegate { node_id_low: bound_ids[0], node_id_high: bound_ids[1], span_low: None, span_high: None }; let def_id = def_id::DefId::local(body.hir_id.owner); let region_scope_tree = &cx.tcx.region_scope_tree(def_id); - ExprUseVisitor::new(&mut delegate, cx.tcx, cx.param_env, region_scope_tree, cx.tables).walk_expr(body); + ExprUseVisitor::new(&mut delegate, cx.tcx, cx.param_env, region_scope_tree, cx.tables, None).walk_expr(body); delegate.mutation_span() } diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index 3c1f6ef2c..60509c1aa 100644 --- a/clippy_lints/src/needless_pass_by_value.rs +++ b/clippy_lints/src/needless_pass_by_value.rs @@ -108,7 +108,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { } = { let mut ctx = MovedVariablesCtxt::new(cx); let region_scope_tree = &cx.tcx.region_scope_tree(fn_def_id); - euv::ExprUseVisitor::new(&mut ctx, cx.tcx, cx.param_env, region_scope_tree, cx.tables).consume_body(body); + euv::ExprUseVisitor::new(&mut ctx, cx.tcx, cx.param_env, region_scope_tree, cx.tables, None).consume_body(body); ctx }; From 3e108b71907b8da09176cac271e9e8ae8b46f352 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Thu, 19 Oct 2017 10:16:03 -0700 Subject: [PATCH 2/3] Fix constant promotion stuff --- clippy_lints/src/methods.rs | 28 +++++++++++-------- tests/ui/methods.stderr | 56 +------------------------------------ 2 files changed, 17 insertions(+), 67 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index d986a5485..38e700965 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -821,18 +821,6 @@ fn lint_or_fun_call(cx: &LateContext, expr: &hir::Expr, name: &str, args: &[hir: or_has_args: bool, span: Span, ) { - // don't lint for constant values - // FIXME: can we `expect` here instead of match? - let promotable = cx.tcx - .rvalue_promotable_to_static - .borrow() - .get(&arg.id) - .cloned() - .unwrap_or(true); - if promotable { - return; - } - // (path, fn_has_argument, methods, suffix) let know_types: &[(&[_], _, &[_], _)] = &[ (&paths::BTREEMAP_ENTRY, false, &["or_insert"], "with"), @@ -841,6 +829,22 @@ fn lint_or_fun_call(cx: &LateContext, expr: &hir::Expr, name: &str, args: &[hir: (&paths::RESULT, true, &["or", "unwrap_or"], "else"), ]; + // early check if the name is one we care about + if know_types.iter().all(|k| !k.2.contains(&name)) { + return; + } + + // don't lint for constant values + // FIXME: can we `expect` here instead of match? + let owner = cx.tcx.hir.get_parent(arg.id); + let owner_def = cx.tcx.hir.local_def_id(owner); + let promotable = cx.tcx + .rvalue_promotable_map(owner_def) + .contains_key(&arg.hir_id.local_id); + if promotable { + return; + } + let self_ty = cx.tables.expr_ty(self_expr); let (fn_has_arguments, poss, suffix) = if let Some(&(_, fn_has_arguments, poss, suffix)) = diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 97e8c25ad..068cbbcd1 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -318,37 +318,13 @@ error: unnecessary structure name repetition 263 | fn new() -> Foo { Foo } | ^^^ help: use the applicable keyword: `Self` -error: use of `unwrap_or` followed by a function call - --> $DIR/methods.rs:281:5 - | -281 | with_constructor.unwrap_or(make()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_constructor.unwrap_or_else(make)` - | - = note: `-D or-fun-call` implied by `-D warnings` - error: use of `unwrap_or` followed by a call to `new` --> $DIR/methods.rs:284:5 | 284 | with_new.unwrap_or(Vec::new()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_new.unwrap_or_default()` - -error: use of `unwrap_or` followed by a function call - --> $DIR/methods.rs:287:5 | -287 | with_const_args.unwrap_or(Vec::with_capacity(12)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_const_args.unwrap_or_else(|| Vec::with_capacity(12))` - -error: use of `unwrap_or` followed by a function call - --> $DIR/methods.rs:290:5 - | -290 | with_err.unwrap_or(make()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_err.unwrap_or_else(|_| make())` - -error: use of `unwrap_or` followed by a function call - --> $DIR/methods.rs:293:5 - | -293 | with_err_args.unwrap_or(Vec::with_capacity(12)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_err_args.unwrap_or_else(|_| Vec::with_capacity(12))` + = note: `-D or-fun-call` implied by `-D warnings` error: use of `unwrap_or` followed by a call to `default` --> $DIR/methods.rs:296:5 @@ -362,36 +338,6 @@ error: use of `unwrap_or` followed by a call to `default` 299 | with_default_type.unwrap_or(u64::default()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_default_type.unwrap_or_default()` -error: use of `unwrap_or` followed by a function call - --> $DIR/methods.rs:302:5 - | -302 | with_vec.unwrap_or(vec![]); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_vec.unwrap_or_else(|| < [ _ ] > :: into_vec ( box [ $ ( $ x ) , * ] ))` - -error: use of `unwrap_or` followed by a function call - --> $DIR/methods.rs:307:5 - | -307 | without_default.unwrap_or(Foo::new()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `without_default.unwrap_or_else(Foo::new)` - -error: use of `or_insert` followed by a function call - --> $DIR/methods.rs:310:5 - | -310 | map.entry(42).or_insert(String::new()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `map.entry(42).or_insert_with(String::new)` - -error: use of `or_insert` followed by a function call - --> $DIR/methods.rs:313:5 - | -313 | btree.entry(42).or_insert(String::new()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `btree.entry(42).or_insert_with(String::new)` - -error: use of `unwrap_or` followed by a function call - --> $DIR/methods.rs:316:13 - | -316 | let _ = stringy.unwrap_or("".to_owned()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `stringy.unwrap_or_else(|| "".to_owned())` - error: called `.iter().nth()` on a Vec. Calling `.get()` is both faster and more readable --> $DIR/methods.rs:327:23 | From 35b2669219002529411fda269b193cd9d89b605a Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Fri, 20 Oct 2017 09:02:32 +0200 Subject: [PATCH 3/3] Check the map for promotable instead for existance of a node (which is always the case) --- clippy_lints/src/methods.rs | 5 ++-- tests/ui/methods.stderr | 56 ++++++++++++++++++++++++++++++++++++- 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 38e700965..d8309ff6f 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -836,11 +836,10 @@ fn lint_or_fun_call(cx: &LateContext, expr: &hir::Expr, name: &str, args: &[hir: // don't lint for constant values // FIXME: can we `expect` here instead of match? - let owner = cx.tcx.hir.get_parent(arg.id); - let owner_def = cx.tcx.hir.local_def_id(owner); + let owner_def = cx.tcx.hir.get_parent_did(arg.id); let promotable = cx.tcx .rvalue_promotable_map(owner_def) - .contains_key(&arg.hir_id.local_id); + [&arg.hir_id.local_id]; if promotable { return; } diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 068cbbcd1..97e8c25ad 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -318,13 +318,37 @@ error: unnecessary structure name repetition 263 | fn new() -> Foo { Foo } | ^^^ help: use the applicable keyword: `Self` +error: use of `unwrap_or` followed by a function call + --> $DIR/methods.rs:281:5 + | +281 | with_constructor.unwrap_or(make()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_constructor.unwrap_or_else(make)` + | + = note: `-D or-fun-call` implied by `-D warnings` + error: use of `unwrap_or` followed by a call to `new` --> $DIR/methods.rs:284:5 | 284 | with_new.unwrap_or(Vec::new()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_new.unwrap_or_default()` + +error: use of `unwrap_or` followed by a function call + --> $DIR/methods.rs:287:5 | - = note: `-D or-fun-call` implied by `-D warnings` +287 | with_const_args.unwrap_or(Vec::with_capacity(12)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_const_args.unwrap_or_else(|| Vec::with_capacity(12))` + +error: use of `unwrap_or` followed by a function call + --> $DIR/methods.rs:290:5 + | +290 | with_err.unwrap_or(make()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_err.unwrap_or_else(|_| make())` + +error: use of `unwrap_or` followed by a function call + --> $DIR/methods.rs:293:5 + | +293 | with_err_args.unwrap_or(Vec::with_capacity(12)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_err_args.unwrap_or_else(|_| Vec::with_capacity(12))` error: use of `unwrap_or` followed by a call to `default` --> $DIR/methods.rs:296:5 @@ -338,6 +362,36 @@ error: use of `unwrap_or` followed by a call to `default` 299 | with_default_type.unwrap_or(u64::default()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_default_type.unwrap_or_default()` +error: use of `unwrap_or` followed by a function call + --> $DIR/methods.rs:302:5 + | +302 | with_vec.unwrap_or(vec![]); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `with_vec.unwrap_or_else(|| < [ _ ] > :: into_vec ( box [ $ ( $ x ) , * ] ))` + +error: use of `unwrap_or` followed by a function call + --> $DIR/methods.rs:307:5 + | +307 | without_default.unwrap_or(Foo::new()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `without_default.unwrap_or_else(Foo::new)` + +error: use of `or_insert` followed by a function call + --> $DIR/methods.rs:310:5 + | +310 | map.entry(42).or_insert(String::new()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `map.entry(42).or_insert_with(String::new)` + +error: use of `or_insert` followed by a function call + --> $DIR/methods.rs:313:5 + | +313 | btree.entry(42).or_insert(String::new()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `btree.entry(42).or_insert_with(String::new)` + +error: use of `unwrap_or` followed by a function call + --> $DIR/methods.rs:316:13 + | +316 | let _ = stringy.unwrap_or("".to_owned()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `stringy.unwrap_or_else(|| "".to_owned())` + error: called `.iter().nth()` on a Vec. Calling `.get()` is both faster and more readable --> $DIR/methods.rs:327:23 |