Auto merge of rust-lang#5134 - flip1995:snippet_block, r=phansch

Make it possible to correctly indent snippet_block snippets

This adds a `indent_relative_to` arg to the `{snippet,expr}_block` functions. This makes it possible to keep the correct indentation of block like suggestions.

In addition, this makes the `trim_multiline` function private and adds a `indent_of` function, to get the indentation of the first line of a span.

The suggestion of `needless_continue` cannot be made auto applicable, since it would be also necessary to remove code following the linted expression. (Well, maybe it is possible, but I don't know how to do it. Expanding the suggestion span to the last expression, that should be removed didn't work)

changelog: Improve suggestions, when blocks of code are involved

Author: bors

clippy_lints/src/attrs.rs

@@ -2,7 +2,7 @@
 
 use crate::reexport::*;
 use crate::utils::{
-    is_present_in_source, last_line_of_span, match_def_path, paths, snippet_opt, span_lint, span_lint_and_sugg,
+    first_line_of_span, is_present_in_source, match_def_path, paths, snippet_opt, span_lint, span_lint_and_sugg,
     span_lint_and_then, without_block_comments,
 };
 use if_chain::if_chain;
@@ -261,7 +261,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Attributes {
                                             _ => {},
                                         }
                                     }
-                                    let line_span = last_line_of_span(cx, attr.span);
+                                    let line_span = first_line_of_span(cx, attr.span);
 
                                     if let Some(mut sugg) = snippet_opt(cx, line_span) {
                                         if sugg.contains("#[") {

clippy_lints/src/block_in_if_condition.rs

@@ -2,6 +2,7 @@ use crate::utils::*;
 use matches::matches;
 use rustc::hir::map::Map;
 use rustc::lint::in_external_macro;
+use rustc_errors::Applicability;
 use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
 use rustc_hir::*;
 use rustc_lint::{LateContext, LateLintPass, LintContext};
@@ -79,8 +80,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for BlockInIfCondition {
         if in_external_macro(cx.sess(), expr.span) {
             return;
         }
-        if let Some((check, then, _)) = higher::if_block(&expr) {
-            if let ExprKind::Block(block, _) = &check.kind {
+        if let Some((cond, _, _)) = higher::if_block(&expr) {
+            if let ExprKind::Block(block, _) = &cond.kind {
                 if block.rules == BlockCheckMode::DefaultBlock {
                     if block.stmts.is_empty() {
                         if let Some(ex) = &block.expr {
@@ -89,16 +90,24 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for BlockInIfCondition {
                             if expr.span.from_expansion() || differing_macro_contexts(expr.span, ex.span) {
                                 return;
                             }
-                            span_lint_and_help(
+                            let mut applicability = Applicability::MachineApplicable;
+                            span_lint_and_sugg(
                                 cx,
                                 BLOCK_IN_IF_CONDITION_EXPR,
-                                check.span,
+                                cond.span,
                                 BRACED_EXPR_MESSAGE,
-                                &format!(
-                                    "try\nif {} {} ... ",
-                                    snippet_block(cx, ex.span, ".."),
-                                    snippet_block(cx, then.span, "..")
+                                "try",
+                                format!(
+                                    "{}",
+                                    snippet_block_with_applicability(
+                                        cx,
+                                        ex.span,
+                                        "..",
+                                        Some(expr.span),
+                                        &mut applicability
+                                    )
                                 ),
+                                applicability,
                             );
                         }
                     } else {
@@ -107,22 +116,30 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for BlockInIfCondition {
                             return;
                         }
                         // move block higher
-                        span_lint_and_help(
+                        let mut applicability = Applicability::MachineApplicable;
+                        span_lint_and_sugg(
                             cx,
                             BLOCK_IN_IF_CONDITION_STMT,
-                            check.span,
+                            expr.span.with_hi(cond.span.hi()),
                             COMPLEX_BLOCK_MESSAGE,
-                            &format!(
-                                "try\nlet res = {};\nif res {} ... ",
-                                snippet_block(cx, block.span, ".."),
-                                snippet_block(cx, then.span, "..")
+                            "try",
+                            format!(
+                                "let res = {}; if res",
+                                snippet_block_with_applicability(
+                                    cx,
+                                    block.span,
+                                    "..",
+                                    Some(expr.span),
+                                    &mut applicability
+                                ),
                             ),
+                            applicability,
                         );
                     }
                 }
             } else {
                 let mut visitor = ExVisitor { found_block: None, cx };
-                walk_expr(&mut visitor, check);
+                walk_expr(&mut visitor, cond);
                 if let Some(block) = visitor.found_block {
                     span_lint(cx, BLOCK_IN_IF_CONDITION_STMT, block.span, COMPLEX_BLOCK_MESSAGE);
                 }

clippy_lints/src/collapsible_if.rs

@@ -95,7 +95,7 @@ fn check_if(cx: &EarlyContext<'_>, expr: &ast::Expr) {
 
 fn block_starts_with_comment(cx: &EarlyContext<'_>, expr: &ast::Block) -> bool {
     // We trim all opening braces and whitespaces and then check if the next string is a comment.
-    let trimmed_block_text = snippet_block(cx, expr.span, "..")
+    let trimmed_block_text = snippet_block(cx, expr.span, "..", None)
         .trim_start_matches(|c: char| c.is_whitespace() || c == '{')
         .to_owned();
     trimmed_block_text.starts_with("//") || trimmed_block_text.starts_with("/*")
@@ -116,7 +116,7 @@ fn check_collapsible_maybe_if_let(cx: &EarlyContext<'_>, else_: &ast::Expr) {
                 block.span,
                 "this `else { if .. }` block can be collapsed",
                 "try",
-                snippet_block_with_applicability(cx, else_.span, "..", &mut applicability).into_owned(),
+                snippet_block_with_applicability(cx, else_.span, "..", Some(block.span), &mut applicability).into_owned(),
                 applicability,
             );
         }
@@ -146,7 +146,7 @@ fn check_collapsible_no_if_let(cx: &EarlyContext<'_>, expr: &ast::Expr, check: &
                     format!(
                         "if {} {}",
                         lhs.and(&rhs),
-                        snippet_block(cx, content.span, ".."),
+                        snippet_block(cx, content.span, "..", Some(expr.span)),
                     ),
                     Applicability::MachineApplicable, // snippet
                 );

clippy_lints/src/matches.rs

@@ -3,9 +3,9 @@ use crate::utils::paths;
 use crate::utils::sugg::Sugg;
 use crate::utils::usage::is_unused;
 use crate::utils::{
-    expr_block, get_arg_name, in_macro, is_allowed, is_expn_of, is_refutable, is_wild, match_qpath, match_type,
-    match_var, multispan_sugg, remove_blocks, snippet, snippet_block, snippet_with_applicability, span_lint_and_help,
-    span_lint_and_note, span_lint_and_sugg, span_lint_and_then, walk_ptrs_ty,
+    expr_block, get_arg_name, in_macro, indent_of, is_allowed, is_expn_of, is_refutable, is_wild, match_qpath,
+    match_type, match_var, multispan_sugg, remove_blocks, snippet, snippet_block, snippet_with_applicability,
+    span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then, walk_ptrs_ty,
 };
 use if_chain::if_chain;
 use rustc::lint::in_external_macro;
@@ -434,7 +434,7 @@ fn report_single_match_single_pattern(
 ) {
     let lint = if els.is_some() { SINGLE_MATCH_ELSE } else { SINGLE_MATCH };
     let els_str = els.map_or(String::new(), |els| {
-        format!(" else {}", expr_block(cx, els, None, ".."))
+        format!(" else {}", expr_block(cx, els, None, "..", Some(expr.span)))
     });
     span_lint_and_sugg(
         cx,
@@ -447,7 +447,7 @@ fn report_single_match_single_pattern(
             "if let {} = {} {}{}",
             snippet(cx, arms[0].pat.span, ".."),
             snippet(cx, ex.span, ".."),
-            expr_block(cx, &arms[0].body, None, ".."),
+            expr_block(cx, &arms[0].body, None, "..", Some(expr.span)),
             els_str,
         ),
         Applicability::HasPlaceholders,
@@ -523,17 +523,21 @@ fn check_match_bool(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_>], e
                             (false, false) => Some(format!(
                                 "if {} {} else {}",
                                 snippet(cx, ex.span, "b"),
-                                expr_block(cx, true_expr, None, ".."),
-                                expr_block(cx, false_expr, None, "..")
+                                expr_block(cx, true_expr, None, "..", Some(expr.span)),
+                                expr_block(cx, false_expr, None, "..", Some(expr.span))
                             )),
                             (false, true) => Some(format!(
                                 "if {} {}",
                                 snippet(cx, ex.span, "b"),
-                                expr_block(cx, true_expr, None, "..")
+                                expr_block(cx, true_expr, None, "..", Some(expr.span))
                             )),
                             (true, false) => {
                                 let test = Sugg::hir(cx, ex, "..");
-                                Some(format!("if {} {}", !test, expr_block(cx, false_expr, None, "..")))
+                                Some(format!(
+                                    "if {} {}",
+                                    !test,
+                                    expr_block(cx, false_expr, None, "..", Some(expr.span))
+                                ))
                             },
                             (true, true) => None,
                         };
@@ -832,7 +836,7 @@ fn check_match_single_binding(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[A
     let mut snippet_body = if match_body.span.from_expansion() {
         Sugg::hir_with_macro_callsite(cx, match_body, "..").to_string()
     } else {
-        snippet_block(cx, match_body.span, "..").to_owned().to_string()
+        snippet_block(cx, match_body.span, "..", Some(expr.span)).to_string()
     };
 
     // Do we need to add ';' to suggestion ?
@@ -861,10 +865,11 @@ fn check_match_single_binding(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[A
                 "this match could be written as a `let` statement",
                 "consider using `let` statement",
                 format!(
-                    "let {} = {};\n{}",
+                    "let {} = {};\n{}{}",
                     snippet_with_applicability(cx, bind_names, "..", &mut applicability),
                     snippet_with_applicability(cx, matched_vars, "..", &mut applicability),
-                    snippet_body
+                    " ".repeat(indent_of(cx, expr.span).unwrap_or(0)),
+                    snippet_body,
                 ),
                 applicability,
             );