Merge pull request #1060 from Manishearth/sugg

Improve suggestions

clippy_lints/src/utils/higher.rs

@@ -0,0 +1,190 @@
+//! This module contains functions for retrieve the original AST from lowered `hir`.
+
+use rustc::hir;
+use rustc::lint::LateContext;
+use syntax::ast;
+use syntax::ptr::P;
+use utils::{is_expn_of, match_path, paths};
+
+/// Convert a hir binary operator to the corresponding `ast` type.
+pub fn binop(op: hir::BinOp_) -> ast::BinOpKind {
+    match op {
+        hir::BiEq => ast::BinOpKind::Eq,
+        hir::BiGe => ast::BinOpKind::Ge,
+        hir::BiGt => ast::BinOpKind::Gt,
+        hir::BiLe => ast::BinOpKind::Le,
+        hir::BiLt => ast::BinOpKind::Lt,
+        hir::BiNe => ast::BinOpKind::Ne,
+        hir::BiOr => ast::BinOpKind::Or,
+        hir::BiAdd => ast::BinOpKind::Add,
+        hir::BiAnd => ast::BinOpKind::And,
+        hir::BiBitAnd => ast::BinOpKind::BitAnd,
+        hir::BiBitOr => ast::BinOpKind::BitOr,
+        hir::BiBitXor => ast::BinOpKind::BitXor,
+        hir::BiDiv => ast::BinOpKind::Div,
+        hir::BiMul => ast::BinOpKind::Mul,
+        hir::BiRem => ast::BinOpKind::Rem,
+        hir::BiShl => ast::BinOpKind::Shl,
+        hir::BiShr => ast::BinOpKind::Shr,
+        hir::BiSub => ast::BinOpKind::Sub,
+    }
+}
+
+/// Represent a range akin to `ast::ExprKind::Range`.
+#[derive(Debug, Copy, Clone)]
+pub struct Range<'a> {
+    pub start: Option<&'a hir::Expr>,
+    pub end: Option<&'a hir::Expr>,
+    pub limits: ast::RangeLimits,
+}
+
+/// Higher a `hir` range to something similar to `ast::ExprKind::Range`.
+pub fn range(expr: &hir::Expr) -> Option<Range> {
+    // To be removed when ranges get stable.
+    fn unwrap_unstable(expr: &hir::Expr) -> &hir::Expr {
+        if let hir::ExprBlock(ref block) = expr.node {
+            if block.rules == hir::BlockCheckMode::PushUnstableBlock || block.rules == hir::BlockCheckMode::PopUnstableBlock {
+                if let Some(ref expr) = block.expr {
+                    return expr;
+                }
+            }
+        }
+
+        expr
+    }
+
+    fn get_field<'a>(name: &str, fields: &'a [hir::Field]) -> Option<&'a hir::Expr> {
+        let expr = &fields.iter()
+                          .find(|field| field.name.node.as_str() == name)
+                          .unwrap_or_else(|| panic!("missing {} field for range", name))
+                          .expr;
+
+        Some(unwrap_unstable(expr))
+    }
+
+    // The range syntax is expanded to literal paths starting with `core` or `std` depending on
+    // `#[no_std]`. Testing both instead of resolving the paths.
+
+    match unwrap_unstable(expr).node {
+        hir::ExprPath(None, ref path) => {
+            if match_path(path, &paths::RANGE_FULL_STD) || match_path(path, &paths::RANGE_FULL) {
+                Some(Range {
+                    start: None,
+                    end: None,
+                    limits: ast::RangeLimits::HalfOpen,
+                })
+            } else {
+                None
+            }
+        }
+        hir::ExprStruct(ref path, ref fields, None) => {
+            if match_path(path, &paths::RANGE_FROM_STD) || match_path(path, &paths::RANGE_FROM) {
+                Some(Range {
+                    start: get_field("start", fields),
+                    end: None,
+                    limits: ast::RangeLimits::HalfOpen,
+                })
+            } else if match_path(path, &paths::RANGE_INCLUSIVE_NON_EMPTY_STD) ||
+               match_path(path, &paths::RANGE_INCLUSIVE_NON_EMPTY) {
+                Some(Range {
+                    start: get_field("start", fields),
+                    end: get_field("end", fields),
+                    limits: ast::RangeLimits::Closed,
+                })
+            } else if match_path(path, &paths::RANGE_STD) || match_path(path, &paths::RANGE) {
+                Some(Range {
+                    start: get_field("start", fields),
+                    end: get_field("end", fields),
+                    limits: ast::RangeLimits::HalfOpen,
+                })
+            } else if match_path(path, &paths::RANGE_TO_INCLUSIVE_STD) || match_path(path, &paths::RANGE_TO_INCLUSIVE) {
+                Some(Range {
+                    start: None,
+                    end: get_field("end", fields),
+                    limits: ast::RangeLimits::Closed,
+                })
+            } else if match_path(path, &paths::RANGE_TO_STD) || match_path(path, &paths::RANGE_TO) {
+                Some(Range {
+                    start: None,
+                    end: get_field("end", fields),
+                    limits: ast::RangeLimits::HalfOpen,
+                })
+            } else {
+                None
+            }
+        }
+        _ => None,
+    }
+}
+
+/// Checks if a `let` decl is from a `for` loop desugaring.
+pub fn is_from_for_desugar(decl: &hir::Decl) -> bool {
+    if_let_chain! {[
+        let hir::DeclLocal(ref loc) = decl.node,
+        let Some(ref expr) = loc.init,
+        let hir::ExprMatch(_, _, hir::MatchSource::ForLoopDesugar) = expr.node,
+    ], {
+        return true;
+    }}
+    false
+}
+
+/// Recover the essential nodes of a desugared for loop:
+/// `for pat in arg { body }` becomes `(pat, arg, body)`.
+pub fn for_loop(expr: &hir::Expr) -> Option<(&hir::Pat, &hir::Expr, &hir::Expr)> {
+    if_let_chain! {[
+        let hir::ExprMatch(ref iterexpr, ref arms, _) = expr.node,
+        let hir::ExprCall(_, ref iterargs) = iterexpr.node,
+        iterargs.len() == 1 && arms.len() == 1 && arms[0].guard.is_none(),
+        let hir::ExprLoop(ref block, _) = arms[0].body.node,
+        block.stmts.is_empty(),
+        let Some(ref loopexpr) = block.expr,
+        let hir::ExprMatch(_, ref innerarms, hir::MatchSource::ForLoopDesugar) = loopexpr.node,
+        innerarms.len() == 2 && innerarms[0].pats.len() == 1,
+        let hir::PatKind::TupleStruct(_, ref somepats, _) = innerarms[0].pats[0].node,
+        somepats.len() == 1
+    ], {
+        return Some((&somepats[0],
+                     &iterargs[0],
+                     &innerarms[0].body));
+    }}
+    None
+}
+
+/// Represent the pre-expansion arguments of a `vec!` invocation.
+pub enum VecArgs<'a> {
+    /// `vec![elem; len]`
+    Repeat(&'a P<hir::Expr>, &'a P<hir::Expr>),
+    /// `vec![a, b, c]`
+    Vec(&'a [P<hir::Expr>]),
+}
+
+/// Returns the arguments of the `vec!` macro if this expression was expanded from `vec!`.
+pub fn vec_macro<'e>(cx: &LateContext, expr: &'e hir::Expr) -> Option<VecArgs<'e>> {
+    if_let_chain!{[
+        let hir::ExprCall(ref fun, ref args) = expr.node,
+        let hir::ExprPath(_, ref path) = fun.node,
+        is_expn_of(cx, fun.span, "vec").is_some()
+    ], {
+        return if match_path(path, &paths::VEC_FROM_ELEM) && args.len() == 2 {
+            // `vec![elem; size]` case
+            Some(VecArgs::Repeat(&args[0], &args[1]))
+        }
+        else if match_path(path, &["into_vec"]) && args.len() == 1 {
+            // `vec![a, b, c]` case
+            if_let_chain!{[
+                let hir::ExprBox(ref boxed) = args[0].node,
+                let hir::ExprVec(ref args) = boxed.node
+            ], {
+                return Some(VecArgs::Vec(&*args));
+            }}
+
+            None
+        }
+        else {
+            None
+        };
+    }}
+
+    None
+}

clippy_lints/src/utils/mod.rs

@@ -9,21 +9,23 @@ use rustc::traits::ProjectionMode;
 use rustc::traits;
 use rustc::ty::subst::Subst;
 use rustc::ty;
+use rustc_errors;
 use std::borrow::Cow;
 use std::env;
 use std::mem;
 use std::str::FromStr;
-use syntax::ast::{self, LitKind, RangeLimits};
-use syntax::codemap::{ExpnInfo, Span, ExpnFormat};
+use syntax::ast::{self, LitKind};
+use syntax::codemap::{ExpnFormat, ExpnInfo, MultiSpan, Span};
 use syntax::errors::DiagnosticBuilder;
 use syntax::ptr::P;
 
+pub mod cargo;
 pub mod comparisons;
 pub mod conf;
 mod hir;
 pub mod paths;
+pub mod sugg;
 pub use self::hir::{SpanlessEq, SpanlessHash};
-pub mod cargo;
 
 pub type MethodArgs = HirVec<P<Expr>>;
 
@@ -80,6 +82,8 @@ macro_rules! if_let_chain {
     };
 }
 
+pub mod higher;
+
 /// Returns true if the two spans come from differing expansions (i.e. one is from a macro and one
 /// isn't).
 pub fn differing_macro_contexts(lhs: Span, rhs: Span) -> bool {
@@ -317,19 +321,6 @@ pub fn get_item_name(cx: &LateContext, expr: &Expr) -> Option<Name> {
     }
 }
 
-/// Checks if a `let` decl is from a `for` loop desugaring.
-pub fn is_from_for_desugar(decl: &Decl) -> bool {
-    if_let_chain! {[
-        let DeclLocal(ref loc) = decl.node,
-        let Some(ref expr) = loc.init,
-        let ExprMatch(_, _, MatchSource::ForLoopDesugar) = expr.node
-    ], {
-        return true;
-    }}
-    false
-}
-
-
 /// Convert a span to a code snippet if available, otherwise use default.
 ///
 /// # Example
@@ -500,6 +491,25 @@ pub fn span_lint_and_then<'a, T: LintContext, F>(cx: &'a T, lint: &'static Lint,
     }
 }
 
+/// Create a suggestion made from several `span → replacement`.
+///
+/// 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)]) {
+    let sugg = rustc_errors::RenderSpan::Suggestion(rustc_errors::CodeSuggestion {
+        msp: MultiSpan::from_spans(sugg.iter().map(|&(span, _)| span).collect()),
+        substitutes: sugg.iter().map(|&(_, subs)| subs.to_owned()).collect(),
+    });
+
+    let sub = rustc_errors::SubDiagnostic {
+        level: rustc_errors::Level::Help,
+        message: help_msg,
+        span: MultiSpan::new(),
+        render_span: Some(sugg),
+    };
+    db.children.push(sub);
+}
+
 /// Return the base type for references and raw pointers.
 pub fn walk_ptrs_ty(ty: ty::Ty) -> ty::Ty {
     match ty.sty {
@@ -681,93 +691,6 @@ pub fn camel_case_from(s: &str) -> usize {
     last_i
 }
 
-/// Represent a range akin to `ast::ExprKind::Range`.
-#[derive(Debug, Copy, Clone)]
-pub struct UnsugaredRange<'a> {
-    pub start: Option<&'a Expr>,
-    pub end: Option<&'a Expr>,
-    pub limits: RangeLimits,
-}
-
-/// Unsugar a `hir` range.
-pub fn unsugar_range(expr: &Expr) -> Option<UnsugaredRange> {
-    // To be removed when ranges get stable.
-    fn unwrap_unstable(expr: &Expr) -> &Expr {
-        if let ExprBlock(ref block) = expr.node {
-            if block.rules == BlockCheckMode::PushUnstableBlock || block.rules == BlockCheckMode::PopUnstableBlock {
-                if let Some(ref expr) = block.expr {
-                    return expr;
-                }
-            }
-        }
-
-        expr
-    }
-
-    fn get_field<'a>(name: &str, fields: &'a [Field]) -> Option<&'a Expr> {
-        let expr = &fields.iter()
-                          .find(|field| field.name.node.as_str() == name)
-                          .unwrap_or_else(|| panic!("missing {} field for range", name))
-                          .expr;
-
-        Some(unwrap_unstable(expr))
-    }
-
-    // The range syntax is expanded to literal paths starting with `core` or `std` depending on
-    // `#[no_std]`. Testing both instead of resolving the paths.
-
-    match unwrap_unstable(expr).node {
-        ExprPath(None, ref path) => {
-            if match_path(path, &paths::RANGE_FULL_STD) || match_path(path, &paths::RANGE_FULL) {
-                Some(UnsugaredRange {
-                    start: None,
-                    end: None,
-                    limits: RangeLimits::HalfOpen,
-                })
-            } else {
-                None
-            }
-        }
-        ExprStruct(ref path, ref fields, None) => {
-            if match_path(path, &paths::RANGE_FROM_STD) || match_path(path, &paths::RANGE_FROM) {
-                Some(UnsugaredRange {
-                    start: get_field("start", fields),
-                    end: None,
-                    limits: RangeLimits::HalfOpen,
-                })
-            } else if match_path(path, &paths::RANGE_INCLUSIVE_NON_EMPTY_STD) ||
-               match_path(path, &paths::RANGE_INCLUSIVE_NON_EMPTY) {
-                Some(UnsugaredRange {
-                    start: get_field("start", fields),
-                    end: get_field("end", fields),
-                    limits: RangeLimits::Closed,
-                })
-            } else if match_path(path, &paths::RANGE_STD) || match_path(path, &paths::RANGE) {
-                Some(UnsugaredRange {
-                    start: get_field("start", fields),
-                    end: get_field("end", fields),
-                    limits: RangeLimits::HalfOpen,
-                })
-            } else if match_path(path, &paths::RANGE_TO_INCLUSIVE_STD) || match_path(path, &paths::RANGE_TO_INCLUSIVE) {
-                Some(UnsugaredRange {
-                    start: None,
-                    end: get_field("end", fields),
-                    limits: RangeLimits::Closed,
-                })
-            } else if match_path(path, &paths::RANGE_TO_STD) || match_path(path, &paths::RANGE_TO) {
-                Some(UnsugaredRange {
-                    start: None,
-                    end: get_field("end", fields),
-                    limits: RangeLimits::HalfOpen,
-                })
-            } else {
-                None
-            }
-        }
-        _ => None,
-    }
-}
-
 /// Convenience function to get the return type of a function or `None` if the function diverges.
 pub fn return_ty<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, fn_item: NodeId) -> Option<ty::Ty<'tcx>> {
     let parameter_env = ty::ParameterEnvironment::for_item(cx.tcx, fn_item);
@@ -792,28 +715,6 @@ pub fn same_tys<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, a: ty::Ty<'tcx>, b: ty::Ty
     })
 }
 
-/// Recover the essential nodes of a desugared for loop:
-/// `for pat in arg { body }` becomes `(pat, arg, body)`.
-pub fn recover_for_loop(expr: &Expr) -> Option<(&Pat, &Expr, &Expr)> {
-    if_let_chain! {[
-        let ExprMatch(ref iterexpr, ref arms, _) = expr.node,
-        let ExprCall(_, ref iterargs) = iterexpr.node,
-        iterargs.len() == 1 && arms.len() == 1 && arms[0].guard.is_none(),
-        let ExprLoop(ref block, _) = arms[0].body.node,
-        block.stmts.is_empty(),
-        let Some(ref loopexpr) = block.expr,
-        let ExprMatch(_, ref innerarms, MatchSource::ForLoopDesugar) = loopexpr.node,
-        innerarms.len() == 2 && innerarms[0].pats.len() == 1,
-        let PatKind::TupleStruct(_, ref somepats, _) = innerarms[0].pats[0].node,
-        somepats.len() == 1
-    ], {
-        return Some((&somepats[0],
-                     &iterargs[0],
-                     &innerarms[0].body));
-    }}
-    None
-}
-
 /// Return whether the given type is an `unsafe` function.
 pub fn type_is_unsafe_function(ty: ty::Ty) -> bool {
     match ty.sty {

clippy_lints/src/utils/sugg.rs

@@ -0,0 +1,356 @@
+use rustc::hir;
+use rustc::lint::{EarlyContext, LateContext};
+use std::borrow::Cow;
+use std;
+use syntax::ast;
+use syntax::util::parser::AssocOp;
+use utils::{higher, snippet, snippet_opt};
+use syntax::print::pprust::binop_to_string;
+
+/// A helper type to build suggestion correctly handling parenthesis.
+pub enum Sugg<'a> {
+    /// An expression that never needs parenthesis such as `1337` or `[0; 42]`.
+    NonParen(Cow<'a, str>),
+    /// An expression that does not fit in other variants.
+    MaybeParen(Cow<'a, str>),
+    /// A binary operator expression, including `as`-casts and explicit type coercion.
+    BinOp(AssocOp, Cow<'a, str>),
+}
+
+/// Literal constant `1`, for convenience.
+pub const ONE: Sugg<'static> = Sugg::NonParen(Cow::Borrowed("1"));
+
+impl<'a> std::fmt::Display for Sugg<'a> {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> {
+        match *self {
+            Sugg::NonParen(ref s) | Sugg::MaybeParen(ref s) | Sugg::BinOp(_, ref s) => {
+                s.fmt(f)
+            }
+        }
+    }
+}
+
+#[allow(wrong_self_convention)] // ok, because of the function `as_ty` method
+impl<'a> Sugg<'a> {
+    pub fn hir_opt(cx: &LateContext, expr: &hir::Expr) -> Option<Sugg<'a>> {
+        snippet_opt(cx, expr.span).map(|snippet| {
+            let snippet = Cow::Owned(snippet);
+            match expr.node {
+                hir::ExprAddrOf(..) |
+                hir::ExprBox(..) |
+                hir::ExprClosure(..) |
+                hir::ExprIf(..) |
+                hir::ExprUnary(..) |
+                hir::ExprMatch(..) => Sugg::MaybeParen(snippet),
+                hir::ExprAgain(..) |
+                hir::ExprBlock(..) |
+                hir::ExprBreak(..) |
+                hir::ExprCall(..) |
+                hir::ExprField(..) |
+                hir::ExprIndex(..) |
+                hir::ExprInlineAsm(..) |
+                hir::ExprLit(..) |
+                hir::ExprLoop(..) |
+                hir::ExprMethodCall(..) |
+                hir::ExprPath(..) |
+                hir::ExprRepeat(..) |
+                hir::ExprRet(..) |
+                hir::ExprStruct(..) |
+                hir::ExprTup(..) |
+                hir::ExprTupField(..) |
+                hir::ExprVec(..) |
+                hir::ExprWhile(..) => Sugg::NonParen(snippet),
+                hir::ExprAssign(..) => Sugg::BinOp(AssocOp::Assign, snippet),
+                hir::ExprAssignOp(op, ..) => Sugg::BinOp(hirbinop2assignop(op), snippet),
+                hir::ExprBinary(op, ..) => Sugg::BinOp(AssocOp::from_ast_binop(higher::binop(op.node)), snippet),
+                hir::ExprCast(..) => Sugg::BinOp(AssocOp::As, snippet),
+                hir::ExprType(..) => Sugg::BinOp(AssocOp::Colon, snippet),
+            }
+        })
+    }
+
+    pub fn hir(cx: &LateContext, expr: &hir::Expr, default: &'a str) -> Sugg<'a> {
+        Self::hir_opt(cx, expr).unwrap_or_else(|| Sugg::NonParen(Cow::Borrowed(default)))
+    }
+
+    pub fn ast(cx: &EarlyContext, expr: &ast::Expr, default: &'a str) -> Sugg<'a> {
+        use syntax::ast::RangeLimits;
+
+        let snippet = snippet(cx, expr.span, default);
+
+        match expr.node {
+            ast::ExprKind::AddrOf(..) |
+            ast::ExprKind::Box(..) |
+            ast::ExprKind::Closure(..) |
+            ast::ExprKind::If(..) |
+            ast::ExprKind::IfLet(..) |
+            ast::ExprKind::InPlace(..) |
+            ast::ExprKind::Unary(..) |
+            ast::ExprKind::Match(..) => Sugg::MaybeParen(snippet),
+            ast::ExprKind::Block(..) |
+            ast::ExprKind::Break(..) |
+            ast::ExprKind::Call(..) |
+            ast::ExprKind::Continue(..) |
+            ast::ExprKind::Field(..) |
+            ast::ExprKind::ForLoop(..) |
+            ast::ExprKind::Index(..) |
+            ast::ExprKind::InlineAsm(..) |
+            ast::ExprKind::Lit(..) |
+            ast::ExprKind::Loop(..) |
+            ast::ExprKind::Mac(..) |
+            ast::ExprKind::MethodCall(..) |
+            ast::ExprKind::Paren(..) |
+            ast::ExprKind::Path(..) |
+            ast::ExprKind::Repeat(..) |
+            ast::ExprKind::Ret(..) |
+            ast::ExprKind::Struct(..) |
+            ast::ExprKind::Try(..) |
+            ast::ExprKind::Tup(..) |
+            ast::ExprKind::TupField(..) |
+            ast::ExprKind::Vec(..) |
+            ast::ExprKind::While(..) |
+            ast::ExprKind::WhileLet(..) => Sugg::NonParen(snippet),
+            ast::ExprKind::Range(.., RangeLimits::HalfOpen) => Sugg::BinOp(AssocOp::DotDot, snippet),
+            ast::ExprKind::Range(.., RangeLimits::Closed) => Sugg::BinOp(AssocOp::DotDotDot, snippet),
+            ast::ExprKind::Assign(..) => Sugg::BinOp(AssocOp::Assign, snippet),
+            ast::ExprKind::AssignOp(op, ..) => Sugg::BinOp(astbinop2assignop(op), snippet),
+            ast::ExprKind::Binary(op, ..) => Sugg::BinOp(AssocOp::from_ast_binop(op.node), snippet),
+            ast::ExprKind::Cast(..) => Sugg::BinOp(AssocOp::As, snippet),
+            ast::ExprKind::Type(..) => Sugg::BinOp(AssocOp::Colon, snippet),
+        }
+    }
+
+    /// Convenience method to create the `<lhs> && <rhs>` suggestion.
+    pub fn and(self, rhs: Self) -> Sugg<'static> {
+        make_binop(ast::BinOpKind::And, &self, &rhs)
+    }
+
+    /// Convenience method to create the `<lhs> as <rhs>` suggestion.
+    pub fn as_ty<R: std::fmt::Display>(self, rhs: R) -> Sugg<'static> {
+        make_assoc(AssocOp::As, &self, &Sugg::NonParen(rhs.to_string().into()))
+    }
+
+    /// Convenience method to create the `&<expr>` suggestion.
+    pub fn addr(self) -> Sugg<'static> {
+        make_unop("&", self)
+    }
+
+    /// Convenience method to create the `&mut <expr>` suggestion.
+    pub fn mut_addr(self) -> Sugg<'static> {
+        make_unop("&mut ", self)
+    }
+
+    /// Convenience method to create the `*<expr>` suggestion.
+    pub fn deref(self) -> Sugg<'static> {
+        make_unop("*", self)
+    }
+
+    /// Convenience method to create the `<lhs>..<rhs>` or `<lhs>...<rhs>` suggestion.
+    pub fn range(self, end: Self, limit: ast::RangeLimits) -> Sugg<'static> {
+        match limit {
+            ast::RangeLimits::HalfOpen => make_assoc(AssocOp::DotDot, &self, &end),
+            ast::RangeLimits::Closed => make_assoc(AssocOp::DotDotDot, &self, &end),
+        }
+    }
+
+    /// Add parenthesis to any expression that might need them. Suitable to the `self` argument of
+    /// a method call (eg. to build `bar.foo()` or `(1 + 2).foo()`).
+    pub fn maybe_par(self) -> Self {
+        match self {
+            Sugg::NonParen(..) => self,
+            Sugg::MaybeParen(sugg) | Sugg::BinOp(_, sugg) => Sugg::NonParen(format!("({})", sugg).into()),
+        }
+    }
+}
+
+impl<'a, 'b> std::ops::Add<Sugg<'b>> for Sugg<'a> {
+    type Output = Sugg<'static>;
+    fn add(self, rhs: Sugg<'b>) -> Sugg<'static> {
+        make_binop(ast::BinOpKind::Add, &self, &rhs)
+    }
+}
+
+impl<'a, 'b> std::ops::Sub<Sugg<'b>> for Sugg<'a> {
+    type Output = Sugg<'static>;
+    fn sub(self, rhs: Sugg<'b>) -> Sugg<'static> {
+        make_binop(ast::BinOpKind::Sub, &self, &rhs)
+    }
+}
+
+impl<'a> std::ops::Not for Sugg<'a> {
+    type Output = Sugg<'static>;
+    fn not(self) -> Sugg<'static> {
+        make_unop("!", self)
+    }
+}
+
+struct ParenHelper<T> {
+    paren: bool,
+    wrapped: T,
+}
+
+impl<T> ParenHelper<T> {
+    fn new(paren: bool, wrapped: T) -> Self {
+        ParenHelper {
+            paren: paren,
+            wrapped: wrapped,
+        }
+    }
+}
+
+impl<T: std::fmt::Display> std::fmt::Display for ParenHelper<T> {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> {
+        if self.paren {
+            write!(f, "({})", self.wrapped)
+        } else {
+            self.wrapped.fmt(f)
+        }
+    }
+}
+
+/// Build the string for `<op><expr>` adding parenthesis when necessary.
+///
+/// For convenience, the operator is taken as a string because all unary operators have the same
+/// precedence.
+pub fn make_unop(op: &str, expr: Sugg) -> Sugg<'static> {
+    Sugg::MaybeParen(format!("{}{}", op, expr.maybe_par()).into())
+}
+
+/// Build the string for `<lhs> <op> <rhs>` adding parenthesis when necessary.
+///
+/// Precedence of shift operator relative to other arithmetic operation is often confusing so
+/// parenthesis will always be added for a mix of these.
+pub fn make_assoc(op: AssocOp, lhs: &Sugg, rhs: &Sugg) -> Sugg<'static> {
+    fn is_shift(op: &AssocOp) -> bool {
+        matches!(*op, AssocOp::ShiftLeft | AssocOp::ShiftRight)
+    }
+
+    fn is_arith(op: &AssocOp) -> bool {
+        matches!(*op, AssocOp::Add | AssocOp::Subtract | AssocOp::Multiply | AssocOp::Divide | AssocOp::Modulus)
+    }
+
+    fn needs_paren(op: &AssocOp, other: &AssocOp, dir: Associativity) -> bool {
+        other.precedence() < op.precedence() ||
+            (other.precedence() == op.precedence() &&
+                ((op != other && associativity(op) != dir) ||
+                 (op == other && associativity(op) != Associativity::Both))) ||
+             is_shift(op) && is_arith(other) ||
+             is_shift(other) && is_arith(op)
+    }
+
+    let lhs_paren = if let Sugg::BinOp(ref lop, _) = *lhs {
+        needs_paren(&op, lop, Associativity::Left)
+    } else {
+        false
+    };
+
+    let rhs_paren = if let Sugg::BinOp(ref rop, _) = *rhs {
+        needs_paren(&op, rop, Associativity::Right)
+    } else {
+        false
+    };
+
+    let lhs = ParenHelper::new(lhs_paren, lhs);
+    let rhs = ParenHelper::new(rhs_paren, rhs);
+    let sugg = match op {
+        AssocOp::Add |
+        AssocOp::BitAnd |
+        AssocOp::BitOr |
+        AssocOp::BitXor |
+        AssocOp::Divide |
+        AssocOp::Equal |
+        AssocOp::Greater |
+        AssocOp::GreaterEqual |
+        AssocOp::LAnd |
+        AssocOp::LOr |
+        AssocOp::Less |
+        AssocOp::LessEqual |
+        AssocOp::Modulus |
+        AssocOp::Multiply |
+        AssocOp::NotEqual |
+        AssocOp::ShiftLeft |
+        AssocOp::ShiftRight |
+        AssocOp::Subtract => format!("{} {} {}", lhs, op.to_ast_binop().expect("Those are AST ops").to_string(), rhs),
+        AssocOp::Inplace => format!("in ({}) {}", lhs, rhs),
+        AssocOp::Assign => format!("{} = {}", lhs, rhs),
+        AssocOp::AssignOp(op) => format!("{} {}= {}", lhs, binop_to_string(op), rhs),
+        AssocOp::As => format!("{} as {}", lhs, rhs),
+        AssocOp::DotDot => format!("{}..{}", lhs, rhs),
+        AssocOp::DotDotDot => format!("{}...{}", lhs, rhs),
+        AssocOp::Colon => format!("{}: {}", lhs, rhs),
+    };
+
+    Sugg::BinOp(op, sugg.into())
+}
+
+/// Convinience wrapper arround `make_assoc` and `AssocOp::from_ast_binop`.
+pub fn make_binop(op: ast::BinOpKind, lhs: &Sugg, rhs: &Sugg) -> Sugg<'static> {
+    make_assoc(AssocOp::from_ast_binop(op), lhs, rhs)
+}
+
+#[derive(PartialEq, Eq)]
+enum Associativity {
+    Both,
+    Left,
+    None,
+    Right,
+}
+
+/// Return the associativity/fixity of an operator. The difference with `AssocOp::fixity` is that
+/// an operator can be both left and right associative (such as `+`:
+/// `a + b + c == (a + b) + c == a + (b + c)`.
+///
+/// Chained `as` and explicit `:` type coercion never need inner parenthesis so they are considered
+/// associative.
+fn associativity(op: &AssocOp) -> Associativity {
+    use syntax::util::parser::AssocOp::*;
+
+    match *op {
+        Inplace | Assign | AssignOp(_) => Associativity::Right,
+        Add | BitAnd | BitOr | BitXor | LAnd | LOr | Multiply |
+        As | Colon => Associativity::Both,
+    Divide | Equal | Greater | GreaterEqual | Less | LessEqual | Modulus | NotEqual | ShiftLeft |
+        ShiftRight | Subtract => Associativity::Left,
+        DotDot | DotDotDot => Associativity::None
+    }
+}
+
+/// Convert a `hir::BinOp` to the corresponding assigning binary operator.
+fn hirbinop2assignop(op: hir::BinOp) -> AssocOp {
+    use rustc::hir::BinOp_::*;
+    use syntax::parse::token::BinOpToken::*;
+
+    AssocOp::AssignOp(match op.node {
+        BiAdd => Plus,
+        BiBitAnd => And,
+        BiBitOr => Or,
+        BiBitXor => Caret,
+        BiDiv => Slash,
+        BiMul => Star,
+        BiRem => Percent,
+        BiShl => Shl,
+        BiShr => Shr,
+        BiSub => Minus,
+        BiAnd | BiEq | BiGe | BiGt | BiLe | BiLt | BiNe | BiOr => panic!("This operator does not exist"),
+    })
+}
+
+/// Convert an `ast::BinOp` to the corresponding assigning binary operator.
+fn astbinop2assignop(op: ast::BinOp) -> AssocOp {
+    use syntax::ast::BinOpKind::*;
+    use syntax::parse::token::BinOpToken;
+
+    AssocOp::AssignOp(match op.node {
+        Add => BinOpToken::Plus,
+        BitAnd => BinOpToken::And,
+        BitOr => BinOpToken::Or,
+        BitXor => BinOpToken::Caret,
+        Div => BinOpToken::Slash,
+        Mul => BinOpToken::Star,
+        Rem => BinOpToken::Percent,
+        Shl => BinOpToken::Shl,
+        Shr => BinOpToken::Shr,
+        Sub => BinOpToken::Minus,
+        And | Eq | Ge | Gt | Le | Lt | Ne | Or => panic!("This operator does not exist"),
+    })
+}