From a3e8091e875a34aa288f675f90c657fa0a86f0e6 Mon Sep 17 00:00:00 2001 From: Seo Sanghyeon Date: Wed, 25 Nov 2015 02:44:40 +0900 Subject: [PATCH 1/3] Dogfood match_ref_pats for `if let` --- src/approx_const.rs | 2 +- src/attrs.rs | 2 +- src/bit_mask.rs | 2 +- src/len_zero.rs | 2 +- src/lifetimes.rs | 2 +- src/misc.rs | 8 ++++---- src/mutex_atomic.rs | 2 +- src/ptr_arg.rs | 6 +++--- src/ranges.rs | 4 ++-- src/shadow.rs | 6 +++--- src/strings.rs | 6 +++--- 11 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/approx_const.rs b/src/approx_const.rs index 89cb5204a..9d1d51444 100644 --- a/src/approx_const.rs +++ b/src/approx_const.rs @@ -44,7 +44,7 @@ impl LintPass for ApproxConstant { impl LateLintPass for ApproxConstant { fn check_expr(&mut self, cx: &LateContext, e: &Expr) { - if let &ExprLit(ref lit) = &e.node { + if let ExprLit(ref lit) = e.node { check_lit(cx, lit, e); } } diff --git a/src/attrs.rs b/src/attrs.rs index a01016886..f4a5b4c15 100644 --- a/src/attrs.rs +++ b/src/attrs.rs @@ -42,7 +42,7 @@ impl LateLintPass for AttrPass { } fn is_relevant_item(item: &Item) -> bool { - if let &ItemFn(_, _, _, _, _, ref block) = &item.node { + if let ItemFn(_, _, _, _, _, ref block) = item.node { is_relevant_block(block) } else { false } } diff --git a/src/bit_mask.rs b/src/bit_mask.rs index c8530b92d..ab73086d0 100644 --- a/src/bit_mask.rs +++ b/src/bit_mask.rs @@ -182,7 +182,7 @@ fn check_ineffective_gt(cx: &LateContext, span: Span, m: u64, c: u64, op: &str) fn fetch_int_literal(cx: &LateContext, lit : &Expr) -> Option { match lit.node { ExprLit(ref lit_ptr) => { - if let &LitInt(value, _) = &lit_ptr.node { + if let LitInt(value, _) = lit_ptr.node { Option::Some(value) //TODO: Handle sign } else { Option::None } } diff --git a/src/len_zero.rs b/src/len_zero.rs index 25645ea87..9ac4ab1e0 100644 --- a/src/len_zero.rs +++ b/src/len_zero.rs @@ -125,7 +125,7 @@ fn check_len_zero(cx: &LateContext, span: Span, name: &Name, fn has_is_empty(cx: &LateContext, expr: &Expr) -> bool { /// get a ImplOrTraitItem and return true if it matches is_empty(self) fn is_is_empty(cx: &LateContext, id: &ImplOrTraitItemId) -> bool { - if let &MethodTraitItemId(def_id) = id { + if let MethodTraitItemId(def_id) = *id { if let ty::MethodTraitItem(ref method) = cx.tcx.impl_or_trait_item(def_id) { method.name.as_str() == "is_empty" diff --git a/src/lifetimes.rs b/src/lifetimes.rs index 9b47a4d83..acc7b0140 100644 --- a/src/lifetimes.rs +++ b/src/lifetimes.rs @@ -168,7 +168,7 @@ impl <'v, 't> RefVisitor<'v, 't> { } fn record(&mut self, lifetime: &Option) { - if let &Some(ref lt) = lifetime { + if let Some(ref lt) = *lifetime { if lt.name.as_str() == "'static" { self.lts.push(Static); } else { diff --git a/src/misc.rs b/src/misc.rs index 9a8ce74e9..9df751d49 100644 --- a/src/misc.rs +++ b/src/misc.rs @@ -84,10 +84,10 @@ impl LateLintPass for CmpNan { fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { if let ExprBinary(ref cmp, ref left, ref right) = expr.node { if is_comparison_binop(cmp.node) { - if let &ExprPath(_, ref path) = &left.node { + if let ExprPath(_, ref path) = left.node { check_nan(cx, path, expr.span); } - if let &ExprPath(_, ref path) = &right.node { + if let ExprPath(_, ref path) = right.node { check_nan(cx, path, expr.span); } } @@ -189,7 +189,7 @@ fn check_to_owned(cx: &LateContext, expr: &Expr, other_span: Span, left: bool, o } } ExprCall(ref path, ref v) if v.len() == 1 => { - if let &ExprPath(None, ref path) = &path.node { + if let ExprPath(None, ref path) = path.node { if match_path(path, &["String", "from_str"]) || match_path(path, &["String", "from"]) { snippet(cx, v[0].span, "..") @@ -235,7 +235,7 @@ impl LintPass for ModuloOne { impl LateLintPass for ModuloOne { fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { if let ExprBinary(ref cmp, _, ref right) = expr.node { - if let &Spanned {node: BinOp_::BiRem, ..} = cmp { + if let Spanned {node: BinOp_::BiRem, ..} = *cmp { if is_integer_literal(right, 1) { cx.span_lint(MODULO_ONE, expr.span, "any number modulo 1 will be 0"); } diff --git a/src/mutex_atomic.rs b/src/mutex_atomic.rs index 9c10a0624..e6d1fc8a8 100644 --- a/src/mutex_atomic.rs +++ b/src/mutex_atomic.rs @@ -34,7 +34,7 @@ pub struct MutexAtomic; impl LateLintPass for MutexAtomic { fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { let ty = cx.tcx.expr_ty(expr); - if let &ty::TyStruct(_, subst) = &ty.sty { + if let ty::TyStruct(_, subst) = ty.sty { if match_type(cx, ty, &MUTEX_PATH) { let mutex_param = &subst.types.get(ParamSpace::TypeSpace, 0).sty; if let Some(atomic_name) = get_atomic_name(mutex_param) { diff --git a/src/ptr_arg.rs b/src/ptr_arg.rs index 78be2af12..6946d0549 100644 --- a/src/ptr_arg.rs +++ b/src/ptr_arg.rs @@ -28,13 +28,13 @@ impl LintPass for PtrArg { impl LateLintPass for PtrArg { fn check_item(&mut self, cx: &LateContext, item: &Item) { - if let &ItemFn(ref decl, _, _, _, _, _) = &item.node { + if let ItemFn(ref decl, _, _, _, _, _) = item.node { check_fn(cx, decl); } } fn check_impl_item(&mut self, cx: &LateContext, item: &ImplItem) { - if let &ImplItemKind::Method(ref sig, _) = &item.node { + if let ImplItemKind::Method(ref sig, _) = item.node { if let Some(Node::NodeItem(it)) = cx.tcx.map.find(cx.tcx.map.get_parent(item.id)) { if let ItemImpl(_, _, _, Some(_), _, _) = it.node { return; // ignore trait impls @@ -45,7 +45,7 @@ impl LateLintPass for PtrArg { } fn check_trait_item(&mut self, cx: &LateContext, item: &TraitItem) { - if let &MethodTraitItem(ref sig, _) = &item.node { + if let MethodTraitItem(ref sig, _) = item.node { check_fn(cx, &sig.decl); } } diff --git a/src/ranges.rs b/src/ranges.rs index 39ff7d3cd..31bb98523 100644 --- a/src/ranges.rs +++ b/src/ranges.rs @@ -40,10 +40,10 @@ impl LateLintPass for StepByZero { if_let_chain! { [ // .iter() call - let &ExprMethodCall( Spanned { node: ref iter_name, .. }, _, ref iter_args ) = iter, + let ExprMethodCall( Spanned { node: ref iter_name, .. }, _, ref iter_args ) = *iter, iter_name.as_str() == "iter", // range expression in .zip() call: 0..x.len() - let &ExprRange(Some(ref from), Some(ref to)) = zip_arg, + let ExprRange(Some(ref from), Some(ref to)) = *zip_arg, is_integer_literal(from, 0), // .len() call let ExprMethodCall(Spanned { node: ref len_name, .. }, _, ref len_args) = to.node, diff --git a/src/shadow.rs b/src/shadow.rs index 3f7272233..5fb27f75c 100644 --- a/src/shadow.rs +++ b/src/shadow.rs @@ -63,8 +63,8 @@ fn check_decl(cx: &LateContext, decl: &Decl, bindings: &mut Vec<(Name, Span)>) { if is_from_for_desugar(decl) { return; } if let DeclLocal(ref local) = decl.node { let Local{ ref pat, ref ty, ref init, id: _, span } = **local; - if let &Some(ref t) = ty { check_ty(cx, t, bindings) } - if let &Some(ref o) = init { + if let Some(ref t) = *ty { check_ty(cx, t, bindings) } + if let Some(ref o) = *init { check_expr(cx, o, bindings); check_pat(cx, pat, &Some(o), span, bindings); } else { @@ -210,7 +210,7 @@ fn check_expr(cx: &LateContext, expr: &Expr, bindings: &mut Vec<(Name, Span)>) { ExprIf(ref cond, ref then, ref otherwise) => { check_expr(cx, cond, bindings); check_block(cx, then, bindings); - if let &Some(ref o) = otherwise { check_expr(cx, o, bindings); } + if let Some(ref o) = *otherwise { check_expr(cx, o, bindings); } } ExprWhile(ref cond, ref block, _) => { check_expr(cx, cond, bindings); diff --git a/src/strings.rs b/src/strings.rs index 082746575..3c34e188d 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -34,14 +34,14 @@ impl LintPass for StringAdd { impl LateLintPass for StringAdd { fn check_expr(&mut self, cx: &LateContext, e: &Expr) { - if let &ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) = &e.node { + if let ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) = e.node { if is_string(cx, left) { if let Allow = cx.current_level(STRING_ADD_ASSIGN) { // the string_add_assign is allow, so no duplicates } else { let parent = get_parent_expr(cx, e); if let Some(ref p) = parent { - if let &ExprAssign(ref target, _) = &p.node { + if let ExprAssign(ref target, _) = p.node { // avoid duplicate matches if is_exp_equal(cx, target, left) { return; } } @@ -51,7 +51,7 @@ impl LateLintPass for StringAdd { "you added something to a string. \ Consider using `String::push_str()` instead") } - } else if let &ExprAssign(ref target, ref src) = &e.node { + } else if let ExprAssign(ref target, ref src) = e.node { if is_string(cx, target) && is_add(cx, src, target) { span_lint(cx, STRING_ADD_ASSIGN, e.span, "you assigned the result of adding something to this string. \ From 746991572fac33ba29c4d1a83574d3b7c2776998 Mon Sep 17 00:00:00 2001 From: Seo Sanghyeon Date: Wed, 25 Nov 2015 02:47:17 +0900 Subject: [PATCH 2/3] Extend match_ref_pats to desugared matches --- src/matches.rs | 49 ++++++++++++++++++++++++----------- tests/compile-fail/matches.rs | 16 +++++++++--- 2 files changed, 47 insertions(+), 18 deletions(-) diff --git a/src/matches.rs b/src/matches.rs index eaa3e0026..55eb4fe38 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -25,9 +25,8 @@ impl LintPass for MatchPass { impl LateLintPass for MatchPass { fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { + if in_external_macro(cx, expr.span) { return; } if let ExprMatch(ref ex, ref arms, MatchSource::Normal) = expr.node { - if in_external_macro(cx, expr.span) { return; } - // check preconditions for SINGLE_MATCH // only two arms if arms.len() == 2 && @@ -53,19 +52,6 @@ impl LateLintPass for MatchPass { expr_block(cx, &arms[0].body, None, ".."))); } - // check preconditions for MATCH_REF_PATS - if has_only_ref_pats(arms) { - if let ExprAddrOf(Mutability::MutImmutable, ref inner) = ex.node { - span_lint(cx, MATCH_REF_PATS, expr.span, &format!( - "you don't need to add `&` to both the expression to match \ - and the patterns: use `match {} {{ ...`", snippet(cx, inner.span, ".."))); - } else { - span_lint(cx, MATCH_REF_PATS, expr.span, &format!( - "instead of prefixing all patterns with `&`, you can dereference the \ - expression to match: `match *{} {{ ...`", snippet(cx, ex.span, ".."))); - } - } - // check preconditions for MATCH_BOOL // type of expression == bool if cx.tcx.expr_ty(ex).sty == ty::TyBool { @@ -123,6 +109,22 @@ impl LateLintPass for MatchPass { } } } + if let ExprMatch(ref ex, ref arms, source) = expr.node { + // check preconditions for MATCH_REF_PATS + if has_only_ref_pats(arms) { + if let ExprAddrOf(Mutability::MutImmutable, ref inner) = ex.node { + let template = match_template(source, "", &snippet(cx, inner.span, "..")); + span_lint(cx, MATCH_REF_PATS, expr.span, &format!( + "you don't need to add `&` to both the expression \ + and the patterns: use `{}`", template)); + } else { + let template = match_template(source, "*", &snippet(cx, ex.span, "..")); + span_lint(cx, MATCH_REF_PATS, expr.span, &format!( + "instead of prefixing all patterns with `&`, you can dereference the \ + expression: `{}`", template)); + } + } + } } } @@ -143,3 +145,20 @@ fn has_only_ref_pats(arms: &[Arm]) -> bool { // look for Some(v) where there's at least one true element mapped.map_or(false, |v| v.iter().any(|el| *el)) } + +fn match_template(source: MatchSource, op: &str, expr: &str) -> String { + match source { + MatchSource::Normal => { + format!("match {}{} {{ ...", op, expr) + } + MatchSource::IfLetDesugar { .. } => { + format!("if let ... = {}{} {{", op, expr) + } + MatchSource::WhileLetDesugar => { + format!("while let ... = {}{} {{", op, expr) + } + MatchSource::ForLoopDesugar => { + panic!("for loop desugared to match with &-patterns!") + } + } +} diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs index 20d7552d7..ea3a48a94 100644 --- a/tests/compile-fail/matches.rs +++ b/tests/compile-fail/matches.rs @@ -78,7 +78,7 @@ fn match_bool() { fn ref_pats() { { let v = &Some(0); - match v { //~ERROR instead of prefixing all patterns with `&` + match v { //~ERROR dereference the expression: `match *v { ...` &Some(v) => println!("{:?}", v), &None => println!("none"), } @@ -88,13 +88,13 @@ fn ref_pats() { } } let tup =& (1, 2); - match tup { //~ERROR instead of prefixing all patterns with `&` + match tup { //~ERROR dereference the expression: `match *tup { ...` &(v, 1) => println!("{}", v), _ => println!("none"), } // special case: using & both in expr and pats let w = Some(0); - match &w { //~ERROR you don't need to add `&` to both + match &w { //~ERROR use `match w { ...` &Some(v) => println!("{:?}", v), &None => println!("none"), } @@ -103,6 +103,16 @@ fn ref_pats() { match w { _ => println!("none"), } + + let a = &Some(0); + if let &None = a { //~ERROR dereference the expression: `if let ... = *a {` + println!("none"); + } + + let b = Some(0); + if let &None = &b { //~ERROR use `if let ... = b {` + println!("none"); + } } fn main() { From b1a0abe404740e1425a8586c7519114578d20372 Mon Sep 17 00:00:00 2001 From: Seo Sanghyeon Date: Wed, 25 Nov 2015 13:57:50 +0900 Subject: [PATCH 3/3] Don't panic --- src/matches.rs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/matches.rs b/src/matches.rs index 55eb4fe38..ec118052f 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -2,6 +2,7 @@ use rustc::lint::*; use rustc_front::hir::*; use rustc::middle::ty; use syntax::ast::Lit_::LitBool; +use syntax::codemap::Span; use utils::{snippet, span_lint, span_help_and_lint, in_external_macro, expr_block}; @@ -113,12 +114,12 @@ impl LateLintPass for MatchPass { // check preconditions for MATCH_REF_PATS if has_only_ref_pats(arms) { if let ExprAddrOf(Mutability::MutImmutable, ref inner) = ex.node { - let template = match_template(source, "", &snippet(cx, inner.span, "..")); + let template = match_template(cx, expr.span, source, "", inner); span_lint(cx, MATCH_REF_PATS, expr.span, &format!( "you don't need to add `&` to both the expression \ and the patterns: use `{}`", template)); } else { - let template = match_template(source, "*", &snippet(cx, ex.span, "..")); + let template = match_template(cx, expr.span, source, "*", ex); span_lint(cx, MATCH_REF_PATS, expr.span, &format!( "instead of prefixing all patterns with `&`, you can dereference the \ expression: `{}`", template)); @@ -146,19 +147,24 @@ fn has_only_ref_pats(arms: &[Arm]) -> bool { mapped.map_or(false, |v| v.iter().any(|el| *el)) } -fn match_template(source: MatchSource, op: &str, expr: &str) -> String { +fn match_template(cx: &LateContext, + span: Span, + source: MatchSource, + op: &str, + expr: &Expr) -> String { + let expr_snippet = snippet(cx, expr.span, ".."); match source { MatchSource::Normal => { - format!("match {}{} {{ ...", op, expr) + format!("match {}{} {{ ...", op, expr_snippet) } MatchSource::IfLetDesugar { .. } => { - format!("if let ... = {}{} {{", op, expr) + format!("if let ... = {}{} {{", op, expr_snippet) } MatchSource::WhileLetDesugar => { - format!("while let ... = {}{} {{", op, expr) + format!("while let ... = {}{} {{", op, expr_snippet) } MatchSource::ForLoopDesugar => { - panic!("for loop desugared to match with &-patterns!") + cx.sess().span_bug(span, "for loop desugared to match with &-patterns!") } } }