Auto merge of rust-lang#7565 - Jarcho:manual_split_once, r=llogiq

New lint `manual_split_once`

This is a WIP because it still needs to recognize more patterns. Currently handles:

```rust
s.splitn(2, ' ').next();
s.splitn(2, ' ').nth(0)
s.splitn(2, ' ').nth(1);
s.splitn(2, ' ').skip(0).next();
s.splitn(2, ' ').skip(1).next();
s.splitn(2, ' ').next_tuple(); // from itertools

// as well as `unwrap()` and `?` forms
```

Still to do:

```rust
let mut iter = s.splitn(2, ' ');
(iter.next().unwrap(), iter.next()?)

let mut iter = s.splitn(2, ' ');
let key = iter.next().unwrap();
let value = iter.next()?;
```

Suggestions on other patterns to check for would be useful. I've done a search on github for uses of `splitn`. Still have yet to actually look through the results.

There's also the question of whether the lint shouold trigger on all uses of `splitn` with two values, or only on recognized usages. The former could have false positives where it couldn't be replaced, but I'm not sure how common that would be.

changelog: Add lint `manual_split_once`

Author: bors

CHANGELOG.md

@@ -2754,6 +2754,7 @@ Released 2018-09-13
 [`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or
 [`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
 [`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic
+[`manual_split_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_split_once
 [`manual_str_repeat`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_str_repeat
 [`manual_strip`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_strip
 [`manual_swap`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_swap

clippy_lints/src/bytecount.rs

@@ -1,7 +1,7 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::ty::match_type;
-use clippy_utils::visitors::LocalUsedVisitor;
+use clippy_utils::visitors::is_local_used;
 use clippy_utils::{path_to_local_id, paths, peel_ref_operators, remove_blocks, strip_pat_refs};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
@@ -65,7 +65,7 @@ impl<'tcx> LateLintPass<'tcx> for ByteCount {
                 return;
             };
             if ty::Uint(UintTy::U8) == *cx.typeck_results().expr_ty(needle).peel_refs().kind();
-            if !LocalUsedVisitor::new(cx, arg_id).check_expr(needle);
+            if !is_local_used(cx, needle, arg_id);
             then {
                 let haystack = if let ExprKind::MethodCall(path, _, args, _) =
                         filter_recv.kind {

clippy_lints/src/collapsible_match.rs

@@ -1,5 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::visitors::LocalUsedVisitor;
+use clippy_utils::visitors::is_local_used;
 use clippy_utils::{is_lang_ctor, path_to_local, peel_ref_operators, SpanlessEq};
 use if_chain::if_chain;
 use rustc_hir::LangItem::OptionNone;
@@ -83,13 +83,12 @@ fn check_arm<'tcx>(arm: &Arm<'tcx>, wild_outer_arm: &Arm<'tcx>, cx: &LateContext
         // the "wild-like" branches must be equal
         if SpanlessEq::new(cx).eq_expr(wild_inner_arm.body, wild_outer_arm.body);
         // the binding must not be used in the if guard
-        let mut used_visitor = LocalUsedVisitor::new(cx, binding_id);
         if match arm.guard {
             None => true,
-            Some(Guard::If(expr) | Guard::IfLet(_, expr)) => !used_visitor.check_expr(expr),
+            Some(Guard::If(expr) | Guard::IfLet(_, expr)) => !is_local_used(cx, expr, binding_id),
         };
         // ...or anywhere in the inner match
-        if !arms_inner.iter().any(|arm| used_visitor.check_arm(arm));
+        if !arms_inner.iter().any(|arm| is_local_used(cx, arm, binding_id));
         then {
             span_lint_and_then(
                 cx,

clippy_lints/src/let_if_seq.rs

@@ -1,6 +1,6 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::source::snippet;
-use clippy_utils::{path_to_local_id, visitors::LocalUsedVisitor};
+use clippy_utils::{path_to_local_id, visitors::is_local_used};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
@@ -65,11 +65,10 @@ impl<'tcx> LateLintPass<'tcx> for LetIfSeq {
                 if let hir::PatKind::Binding(mode, canonical_id, ident, None) = local.pat.kind;
                 if let hir::StmtKind::Expr(if_) = expr.kind;
                 if let hir::ExprKind::If(cond, then, ref else_) = if_.kind;
-                let mut used_visitor = LocalUsedVisitor::new(cx, canonical_id);
-                if !used_visitor.check_expr(cond);
+                if !is_local_used(cx, cond, canonical_id);
                 if let hir::ExprKind::Block(then, _) = then.kind;
                 if let Some(value) = check_assign(cx, canonical_id, &*then);
-                if !used_visitor.check_expr(value);
+                if !is_local_used(cx, value, canonical_id);
                 then {
                     let span = stmt.span.to(if_.span);
 
@@ -148,15 +147,13 @@ fn check_assign<'tcx>(
         if let hir::ExprKind::Assign(var, value, _) = expr.kind;
         if path_to_local_id(var, decl);
         then {
-            let mut v = LocalUsedVisitor::new(cx, decl);
-
-            if block.stmts.iter().take(block.stmts.len()-1).any(|stmt| v.check_stmt(stmt)) {
-                return None;
+            if block.stmts.iter().take(block.stmts.len()-1).any(|stmt| is_local_used(cx, stmt, decl)) {
+                None
+            } else {
+                Some(value)
             }
-
-            return Some(value);
+        } else {
+            None
         }
     }
-
-    None
 }

clippy_lints/src/lib.rs

@@ -773,6 +773,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         methods::MANUAL_FILTER_MAP,
         methods::MANUAL_FIND_MAP,
         methods::MANUAL_SATURATING_ARITHMETIC,
+        methods::MANUAL_SPLIT_ONCE,
         methods::MANUAL_STR_REPEAT,
         methods::MAP_COLLECT_RESULT_UNIT,
         methods::MAP_FLATTEN,
@@ -1319,6 +1320,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(methods::MANUAL_FILTER_MAP),
         LintId::of(methods::MANUAL_FIND_MAP),
         LintId::of(methods::MANUAL_SATURATING_ARITHMETIC),
+        LintId::of(methods::MANUAL_SPLIT_ONCE),
         LintId::of(methods::MANUAL_STR_REPEAT),
         LintId::of(methods::MAP_COLLECT_RESULT_UNIT),
         LintId::of(methods::MAP_IDENTITY),
@@ -1617,6 +1619,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(methods::ITER_COUNT),
         LintId::of(methods::MANUAL_FILTER_MAP),
         LintId::of(methods::MANUAL_FIND_MAP),
+        LintId::of(methods::MANUAL_SPLIT_ONCE),
         LintId::of(methods::MAP_IDENTITY),
         LintId::of(methods::OPTION_AS_REF_DEREF),
         LintId::of(methods::OPTION_FILTER_MAP),

clippy_lints/src/loops/for_kv_map.rs

@@ -3,7 +3,7 @@ use clippy_utils::diagnostics::{multispan_sugg, span_lint_and_then};
 use clippy_utils::source::snippet;
 use clippy_utils::sugg;
 use clippy_utils::ty::is_type_diagnostic_item;
-use clippy_utils::visitors::LocalUsedVisitor;
+use clippy_utils::visitors::is_local_used;
 use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability, Pat, PatKind};
 use rustc_lint::LateContext;
 use rustc_middle::ty;
@@ -66,9 +66,7 @@ pub(super) fn check<'tcx>(
 fn pat_is_wild<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx PatKind<'_>, body: &'tcx Expr<'_>) -> bool {
     match *pat {
         PatKind::Wild => true,
-        PatKind::Binding(_, id, ident, None) if ident.as_str().starts_with('_') => {
-            !LocalUsedVisitor::new(cx, id).check_expr(body)
-        },
+        PatKind::Binding(_, id, ident, None) if ident.as_str().starts_with('_') => !is_local_used(cx, body, id),
         _ => false,
     }
 }

clippy_lints/src/loops/manual_flatten.rs

@@ -1,7 +1,7 @@
 use super::utils::make_iterator_snippet;
 use super::MANUAL_FLATTEN;
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::visitors::LocalUsedVisitor;
+use clippy_utils::visitors::is_local_used;
 use clippy_utils::{is_lang_ctor, path_to_local_id};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
@@ -49,7 +49,7 @@ pub(super) fn check<'tcx>(
             let ok_ctor = is_lang_ctor(cx, qpath, ResultOk);
             if some_ctor || ok_ctor;
             // Ensure epxr in `if let` is not used afterwards
-            if !LocalUsedVisitor::new(cx, pat_hir_id).check_arm(true_arm);
+            if !is_local_used(cx, true_arm, pat_hir_id);
             then {
                 let if_let_type = if some_ctor { "Some" } else { "Ok" };
                 // Prepare the error message

clippy_lints/src/loops/needless_range_loop.rs

@@ -2,10 +2,8 @@ use super::NEEDLESS_RANGE_LOOP;
 use clippy_utils::diagnostics::{multispan_sugg, span_lint_and_then};
 use clippy_utils::source::snippet;
 use clippy_utils::ty::has_iter_method;
-use clippy_utils::visitors::LocalUsedVisitor;
-use clippy_utils::{
-    contains_name, higher, is_integer_const, match_trait_method, path_to_local_id, paths, sugg, SpanlessEq,
-};
+use clippy_utils::visitors::is_local_used;
+use clippy_utils::{contains_name, higher, is_integer_const, match_trait_method, paths, sugg, SpanlessEq};
 use if_chain::if_chain;
 use rustc_ast::ast;
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
@@ -256,43 +254,36 @@ impl<'a, 'tcx> VarVisitor<'a, 'tcx> {
             if let ExprKind::Path(ref seqpath) = seqexpr.kind;
             if let QPath::Resolved(None, seqvar) = *seqpath;
             if seqvar.segments.len() == 1;
-            let index_used_directly = path_to_local_id(idx, self.var);
-            let indexed_indirectly = {
-                let mut used_visitor = LocalUsedVisitor::new(self.cx, self.var);
-                walk_expr(&mut used_visitor, idx);
-                used_visitor.used
-            };
-            if indexed_indirectly || index_used_directly;
+            if is_local_used(self.cx, idx, self.var);
             then {
                 if self.prefer_mutable {
                     self.indexed_mut.insert(seqvar.segments[0].ident.name);
                 }
+                let index_used_directly = matches!(idx.kind, ExprKind::Path(_));
                 let res = self.cx.qpath_res(seqpath, seqexpr.hir_id);
                 match res {
                     Res::Local(hir_id) => {
                         let parent_id = self.cx.tcx.hir().get_parent_item(expr.hir_id);
                         let parent_def_id = self.cx.tcx.hir().local_def_id(parent_id);
                         let extent = self.cx.tcx.region_scope_tree(parent_def_id).var_scope(hir_id.local_id);
-                        if indexed_indirectly {
-                            self.indexed_indirectly.insert(seqvar.segments[0].ident.name, Some(extent));
-                        }
                         if index_used_directly {
                             self.indexed_directly.insert(
                                 seqvar.segments[0].ident.name,
                                 (Some(extent), self.cx.typeck_results().node_type(seqexpr.hir_id)),
                             );
+                        } else {
+                            self.indexed_indirectly.insert(seqvar.segments[0].ident.name, Some(extent));
                         }
                         return false;  // no need to walk further *on the variable*
                     }
                     Res::Def(DefKind::Static | DefKind::Const, ..) => {
-                        if indexed_indirectly {
-                            self.indexed_indirectly.insert(seqvar.segments[0].ident.name, None);
-                        }
                         if index_used_directly {
                             self.indexed_directly.insert(
                                 seqvar.segments[0].ident.name,
                                 (None, self.cx.typeck_results().node_type(seqexpr.hir_id)),
                             );
+                        } else {
+                            self.indexed_indirectly.insert(seqvar.segments[0].ident.name, None);
                         }
                         return false;  // no need to walk further *on the variable*
                     }

clippy_lints/src/matches.rs

@@ -5,7 +5,7 @@ use clippy_utils::diagnostics::{
 use clippy_utils::source::{expr_block, indent_of, snippet, snippet_block, snippet_opt, snippet_with_applicability};
 use clippy_utils::sugg::Sugg;
 use clippy_utils::ty::{implements_trait, is_type_diagnostic_item, match_type, peel_mid_ty_refs};
-use clippy_utils::visitors::LocalUsedVisitor;
+use clippy_utils::visitors::is_local_used;
 use clippy_utils::{
     get_parent_expr, in_macro, is_expn_of, is_lang_ctor, is_lint_allowed, is_refutable, is_wild, meets_msrv, msrvs,
     path_to_local, path_to_local_id, peel_hir_pat_refs, peel_n_hir_expr_refs, recurse_or_patterns, remove_blocks,
@@ -953,9 +953,7 @@ fn check_wild_err_arm<'tcx>(cx: &LateContext<'tcx>, ex: &Expr<'tcx>, arms: &[Arm
                         // Looking for unused bindings (i.e.: `_e`)
                         for pat in inner.iter() {
                             if let PatKind::Binding(_, id, ident, None) = pat.kind {
-                                if ident.as_str().starts_with('_')
-                                    && !LocalUsedVisitor::new(cx, id).check_expr(arm.body)
-                                {
+                                if ident.as_str().starts_with('_') && !is_local_used(cx, arm.body, id) {
                                     ident_bind_name = (&ident.name.as_str()).to_string();
                                     matching_wild = true;
                                 }