Auto merge of rust-lang#8520 - J-ZhengLi:issue8506, r=xFrednet

fix suggestion on `[map_flatten]` being cropped causing possible information loss

fixes rust-lang#8506

Multi-line suggestion given by the lint is missing its bottom part, which could potentially contains useful information about the fix.

---

changelog: [`map_flatten`]: Long suggestions will now be splitup into two help messages

Author: bors

clippy_lints/src/methods/map_flatten.rs

@@ -1,83 +1,73 @@
-use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::diagnostics::span_lint_and_sugg_for_edges;
 use clippy_utils::is_trait_method;
-use clippy_utils::source::snippet;
+use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::ty::is_type_diagnostic_item;
 use rustc_errors::Applicability;
-use rustc_hir as hir;
+use rustc_hir::Expr;
 use rustc_lint::LateContext;
 use rustc_middle::ty;
-use rustc_span::symbol::sym;
+use rustc_span::{symbol::sym, Span};
 
 use super::MAP_FLATTEN;
 
 /// lint use of `map().flatten()` for `Iterators` and 'Options'
-pub(super) fn check<'tcx>(
-    cx: &LateContext<'tcx>,
-    expr: &'tcx hir::Expr<'_>,
-    recv: &'tcx hir::Expr<'_>,
-    map_arg: &'tcx hir::Expr<'_>,
-) {
-    // lint if caller of `.map().flatten()` is an Iterator
-    if is_trait_method(cx, expr, sym::Iterator) {
-        let map_closure_ty = cx.typeck_results().expr_ty(map_arg);
-        let is_map_to_option = match map_closure_ty.kind() {
-            ty::Closure(_, _) | ty::FnDef(_, _) | ty::FnPtr(_) => {
-                let map_closure_sig = match map_closure_ty.kind() {
-                    ty::Closure(_, substs) => substs.as_closure().sig(),
-                    _ => map_closure_ty.fn_sig(cx.tcx),
-                };
-                let map_closure_return_ty = cx.tcx.erase_late_bound_regions(map_closure_sig.output());
-                is_type_diagnostic_item(cx, map_closure_return_ty, sym::Option)
-            },
-            _ => false,
-        };
-
-        let method_to_use = if is_map_to_option {
-            // `(...).map(...)` has type `impl Iterator<Item=Option<...>>
-            "filter_map"
-        } else {
-            // `(...).map(...)` has type `impl Iterator<Item=impl Iterator<...>>
-            "flat_map"
-        };
-        let func_snippet = snippet(cx, map_arg.span, "..");
-        let hint = format!(".{0}({1})", method_to_use, func_snippet);
-        span_lint_and_sugg(
+pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, map_arg: &Expr<'_>, map_span: Span) {
+    if let Some((caller_ty_name, method_to_use)) = try_get_caller_ty_name_and_method_name(cx, expr, recv, map_arg) {
+        let mut applicability = Applicability::MachineApplicable;
+        let help_msgs = [
+            &format!("try replacing `map` with `{}`", method_to_use),
+            "and remove the `.flatten()`",
+        ];
+        let closure_snippet = snippet_with_applicability(cx, map_arg.span, "..", &mut applicability);
+        span_lint_and_sugg_for_edges(
             cx,
             MAP_FLATTEN,
-            expr.span.with_lo(recv.span.hi()),
-            "called `map(..).flatten()` on an `Iterator`",
-            &format!("try using `{}` instead", method_to_use),
-            hint,
-            Applicability::MachineApplicable,
+            expr.span.with_lo(map_span.lo()),
+            &format!("called `map(..).flatten()` on `{}`", caller_ty_name),
+            &help_msgs,
+            format!("{}({})", method_to_use, closure_snippet),
+            applicability,
         );
     }
+}
 
-    // lint if caller of `.map().flatten()` is an Option or Result
-    let caller_type = match cx.typeck_results().expr_ty(recv).kind() {
-        ty::Adt(adt, _) => {
+fn try_get_caller_ty_name_and_method_name(
+    cx: &LateContext<'_>,
+    expr: &Expr<'_>,
+    caller_expr: &Expr<'_>,
+    map_arg: &Expr<'_>,
+) -> Option<(&'static str, &'static str)> {
+    if is_trait_method(cx, expr, sym::Iterator) {
+        if is_map_to_option(cx, map_arg) {
+            // `(...).map(...)` has type `impl Iterator<Item=Option<...>>
+            Some(("Iterator", "filter_map"))
+        } else {
+            // `(...).map(...)` has type `impl Iterator<Item=impl Iterator<...>>
+            Some(("Iterator", "flat_map"))
+        }
+    } else {
+        if let ty::Adt(adt, _) = cx.typeck_results().expr_ty(caller_expr).kind() {
             if cx.tcx.is_diagnostic_item(sym::Option, adt.did()) {
-                "Option"
+                return Some(("Option", "and_then"));
             } else if cx.tcx.is_diagnostic_item(sym::Result, adt.did()) {
-                "Result"
-            } else {
-                return;
+                return Some(("Result", "and_then"));
             }
-        },
-        _ => {
-            return;
-        },
-    };
+        }
+        None
+    }
+}
 
-    let func_snippet = snippet(cx, map_arg.span, "..");
-    let hint = format!(".and_then({})", func_snippet);
-    let lint_info = format!("called `map(..).flatten()` on an `{}`", caller_type);
-    span_lint_and_sugg(
-        cx,
-        MAP_FLATTEN,
-        expr.span.with_lo(recv.span.hi()),
-        &lint_info,
-        "try using `and_then` instead",
-        hint,
-        Applicability::MachineApplicable,
-    );
+fn is_map_to_option(cx: &LateContext<'_>, map_arg: &Expr<'_>) -> bool {
+    let map_closure_ty = cx.typeck_results().expr_ty(map_arg);
+    match map_closure_ty.kind() {
+        ty::Closure(_, _) | ty::FnDef(_, _) | ty::FnPtr(_) => {
+            let map_closure_sig = match map_closure_ty.kind() {
+                ty::Closure(_, substs) => substs.as_closure().sig(),
+                _ => map_closure_ty.fn_sig(cx.tcx),
+            };
+            let map_closure_return_ty = cx.tcx.erase_late_bound_regions(map_closure_sig.output());
+            is_type_diagnostic_item(cx, map_closure_return_ty, sym::Option)
+        },
+        _ => false,
+    }
 }

clippy_lints/src/methods/mod.rs

@@ -2377,7 +2377,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
                 flat_map_option::check(cx, expr, arg, span);
             },
             (name @ "flatten", args @ []) => match method_call(recv) {
-                Some(("map", [recv, map_arg], _)) => map_flatten::check(cx, expr, recv, map_arg),
+                Some(("map", [recv, map_arg], map_span)) => map_flatten::check(cx, expr, recv, map_arg, map_span),
                 Some(("cloned", [recv2], _)) => iter_overeager_cloned::check(cx, expr, recv2, name, args),
                 _ => {},
             },

clippy_lints/src/utils/internal_lints/metadata_collector.rs

@@ -85,14 +85,15 @@ macro_rules! CONFIGURATION_VALUE_TEMPLATE {
     };
 }
 
-const LINT_EMISSION_FUNCTIONS: [&[&str]; 7] = [
+const LINT_EMISSION_FUNCTIONS: [&[&str]; 8] = [
     &["clippy_utils", "diagnostics", "span_lint"],
     &["clippy_utils", "diagnostics", "span_lint_and_help"],
     &["clippy_utils", "diagnostics", "span_lint_and_note"],
     &["clippy_utils", "diagnostics", "span_lint_hir"],
     &["clippy_utils", "diagnostics", "span_lint_and_sugg"],
     &["clippy_utils", "diagnostics", "span_lint_and_then"],
     &["clippy_utils", "diagnostics", "span_lint_hir_and_then"],
+    &["clippy_utils", "diagnostics", "span_lint_and_sugg_for_edges"],
 ];
 const SUGGESTION_DIAGNOSTIC_BUILDER_METHODS: [(&str, bool); 9] = [
     ("span_suggestion", false),

clippy_utils/src/diagnostics.rs

@@ -8,7 +8,7 @@
 //! Thank you!
 //! ~The `INTERNAL_METADATA_COLLECTOR` lint
 
-use rustc_errors::{Applicability, Diagnostic};
+use rustc_errors::{emitter::MAX_SUGGESTION_HIGHLIGHT_LINES, Applicability, Diagnostic};
 use rustc_hir::HirId;
 use rustc_lint::{LateContext, Lint, LintContext};
 use rustc_span::source_map::{MultiSpan, Span};
@@ -213,6 +213,90 @@ pub fn span_lint_and_sugg<'a, T: LintContext>(
     });
 }
 
+/// Like [`span_lint_and_sugg`] with a focus on the edges. The output will either
+/// emit single span or multispan suggestion depending on the number of its lines.
+///
+/// If the given suggestion string has more lines than the maximum display length defined by
+/// [`MAX_SUGGESTION_HIGHLIGHT_LINES`][`rustc_errors::emitter::MAX_SUGGESTION_HIGHLIGHT_LINES`],
+/// this function will split the suggestion and span to showcase the change for the top and
+/// bottom edge of the code. For normal suggestions, in one display window, the help message
+/// will be combined with a colon.
+///
+/// Multipart suggestions like the one being created here currently cannot be
+/// applied by rustfix (See [rustfix#141](https://github.com/rust-lang/rustfix/issues/141)).
+/// Testing rustfix with this lint emission function might require a file with
+/// suggestions that can be fixed and those that can't. See
+/// [clippy#8520](https://github.com/rust-lang/rust-clippy/pull/8520/files) for
+/// an example and of this.
+///
+/// # Example for a long suggestion
+///
+/// ```text
+/// error: called `map(..).flatten()` on `Option`
+///   --> $DIR/map_flatten.rs:8:10
+///    |
+/// LL |           .map(|x| {
+///    |  __________^
+/// LL | |             if x <= 5 {
+/// LL | |                 Some(x)
+/// LL | |             } else {
+/// ...  |
+/// LL | |         })
+/// LL | |         .flatten();
+///    | |__________________^
+///    |
+///   = note: `-D clippy::map-flatten` implied by `-D warnings`
+/// help: try replacing `map` with `and_then`
+///    |
+/// LL ~         .and_then(|x| {
+/// LL +             if x <= 5 {
+/// LL +                 Some(x)
+///    |
+/// help: and remove the `.flatten()`
+///    |
+/// LL +                 None
+/// LL +             }
+/// LL ~         });
+///    |
+/// ```
+pub fn span_lint_and_sugg_for_edges(
+    cx: &LateContext<'_>,
+    lint: &'static Lint,
+    sp: Span,
+    msg: &str,
+    helps: &[&str; 2],
+    sugg: String,
+    applicability: Applicability,
+) {
+    span_lint_and_then(cx, lint, sp, msg, |diag| {
+        let sugg_lines_count = sugg.lines().count();
+        if sugg_lines_count > MAX_SUGGESTION_HIGHLIGHT_LINES {
+            let sm = cx.sess().source_map();
+            if let (Ok(line_upper), Ok(line_bottom)) = (sm.lookup_line(sp.lo()), sm.lookup_line(sp.hi())) {
+                let split_idx = MAX_SUGGESTION_HIGHLIGHT_LINES / 2;
+                let span_upper = sm.span_until_char(sp.with_hi(line_upper.sf.lines[line_upper.line + split_idx]), '\n');
+                let span_bottom = sp.with_lo(line_bottom.sf.lines[line_bottom.line - split_idx]);
+
+                let sugg_lines_vec = sugg.lines().collect::<Vec<&str>>();
+                let sugg_upper = sugg_lines_vec[..split_idx].join("\n");
+                let sugg_bottom = sugg_lines_vec[sugg_lines_count - split_idx..].join("\n");
+
+                diag.span_suggestion(span_upper, helps[0], sugg_upper, applicability);
+                diag.span_suggestion(span_bottom, helps[1], sugg_bottom, applicability);
+
+                return;
+            }
+        }
+        diag.span_suggestion_with_style(
+            sp,
+            &helps.join(", "),
+            sugg,
+            applicability,
+            rustc_errors::SuggestionStyle::ShowAlways,
+        );
+    });
+}
+
 /// Create a suggestion made from several `span → replacement`.
 ///
 /// Note: in the JSON format (used by `compiletest_rs`), the help message will

tests/ui/map_flatten.rs

@@ -1,31 +1,55 @@
-// run-rustfix
-
-#![warn(clippy::all, clippy::pedantic)]
-#![allow(clippy::let_underscore_drop)]
-#![allow(clippy::missing_docs_in_private_items)]
-#![allow(clippy::map_identity)]
-#![allow(clippy::redundant_closure)]
-#![allow(clippy::unnecessary_wraps)]
+#![warn(clippy::map_flatten)]
 #![feature(result_flattening)]
 
-fn main() {
-    // mapping to Option on Iterator
-    fn option_id(x: i8) -> Option<i8> {
-        Some(x)
-    }
-    let option_id_ref: fn(i8) -> Option<i8> = option_id;
-    let option_id_closure = |x| Some(x);
-    let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id).flatten().collect();
-    let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id_ref).flatten().collect();
-    let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id_closure).flatten().collect();
-    let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| x.checked_add(1)).flatten().collect();
+// issue #8506, multi-line
+#[rustfmt::skip]
+fn long_span() {
+    let _: Option<i32> = Some(1)
+        .map(|x| {
+            if x <= 5 {
+                Some(x)
+            } else {
+                None
+            }
+        })
+        .flatten();
 
-    // mapping to Iterator on Iterator
-    let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect();
+    let _: Result<i32, i32> = Ok(1)
+        .map(|x| {
+            if x == 1 {
+                Ok(x)
+            } else {
+                Err(0)
+            }
+        })
+        .flatten();
 
-    // mapping to Option on Option
-    let _: Option<_> = (Some(Some(1))).map(|x| x).flatten();
+    let result: Result<i32, i32> = Ok(2);
+    fn do_something() { }
+    let _: Result<i32, i32> = result
+        .map(|res| {
+            if res > 0 {
+                do_something();
+                Ok(res)
+            } else {
+                Err(0)
+            }
+        })
+        .flatten();
+        
+    let _: Vec<_> = vec![5_i8; 6]
+        .into_iter()
+        .map(|some_value| {
+            if some_value > 3 {
+                Some(some_value)
+            } else {
+                None
+            }
+        })
+        .flatten()
+        .collect();
+}
 
-    // mapping to Result on Result
-    let _: Result<_, &str> = (Ok(Ok(1))).map(|x| x).flatten();
+fn main() {
+    long_span();
 }