Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove MAX_SUGGESTION_HIGHLIGHT_LINES #98261

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions compiler/rustc_errors/src/emitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -656,11 +656,6 @@ impl Emitter for SilentEmitter {
}
}

/// Maximum number of lines we will print for a multiline suggestion; arbitrary.
///
/// This should be replaced with a more involved mechanism to output multiline suggestions that
/// more closely mimics the regular diagnostic output, where irrelevant code lines are elided.
pub const MAX_SUGGESTION_HIGHLIGHT_LINES: usize = 6;
/// Maximum number of suggestions to be shown
///
/// Arbitrary, but taken from trait import suggestion limit
Expand Down
11 changes: 4 additions & 7 deletions src/tools/clippy/clippy_lints/src/methods/map_flatten.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use clippy_utils::diagnostics::span_lint_and_sugg_for_edges;
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::is_trait_method;
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::ty::is_type_diagnostic_item;
Expand All @@ -14,17 +14,14 @@ use super::MAP_FLATTEN;
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(
span_lint_and_sugg(
cx,
MAP_FLATTEN,
expr.span.with_lo(map_span.lo()),
&format!("called `map(..).flatten()` on `{}`", caller_ty_name),
&help_msgs,
&format!("try replacing `map` with `{}` and remove the `.flatten()`", method_to_use),
format!("{}({})", method_to_use, closure_snippet),
applicability,
);
Expand Down
13 changes: 3 additions & 10 deletions src/tools/clippy/clippy_lints/src/methods/or_fun_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use clippy_utils::source::{snippet, snippet_with_applicability, snippet_with_mac
use clippy_utils::ty::{implements_trait, match_type};
use clippy_utils::{contains_return, is_trait_item, last_path_segment, paths};
use if_chain::if_chain;
use rustc_errors::emitter::MAX_SUGGESTION_HIGHLIGHT_LINES;
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_lint::LateContext;
Expand Down Expand Up @@ -33,7 +32,6 @@ pub(super) fn check<'tcx>(
arg: &hir::Expr<'_>,
or_has_args: bool,
span: Span,
method_span: Span,
) -> bool {
let is_default_default = || is_trait_item(cx, fun, sym::Default);

Expand All @@ -56,19 +54,14 @@ pub(super) fn check<'tcx>(
then {
let mut applicability = Applicability::MachineApplicable;
let hint = "unwrap_or_default()";
let mut sugg_span = span;
let sugg_span = span;

let mut sugg: String = format!(
let sugg: String = format!(
"{}.{}",
snippet_with_applicability(cx, self_expr.span, "..", &mut applicability),
hint
);

if sugg.lines().count() > MAX_SUGGESTION_HIGHLIGHT_LINES {
sugg_span = method_span.with_hi(span.hi());
sugg = hint.to_string();
}

span_lint_and_sugg(
cx,
OR_FUN_CALL,
Expand Down Expand Up @@ -178,7 +171,7 @@ pub(super) fn check<'tcx>(
match inner_arg.kind {
hir::ExprKind::Call(fun, or_args) => {
let or_has_args = !or_args.is_empty();
if !check_unwrap_or_default(cx, name, fun, self_arg, arg, or_has_args, expr.span, method_span) {
if !check_unwrap_or_default(cx, name, fun, self_arg, arg, or_has_args, expr.span) {
let fun_span = if or_has_args { None } else { Some(fun.span) };
check_general_case(cx, name, method_span, self_arg, arg, expr.span, fun_span);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ const LINT_EMISSION_FUNCTIONS: [&[&str]; 8] = [
&["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),
Expand Down
91 changes: 1 addition & 90 deletions src/tools/clippy/clippy_utils/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
//! Thank you!
//! ~The `INTERNAL_METADATA_COLLECTOR` lint

use rustc_errors::{emitter::MAX_SUGGESTION_HIGHLIGHT_LINES, Applicability, Diagnostic, MultiSpan};
use rustc_errors::{Applicability, Diagnostic, MultiSpan};
use rustc_hir::HirId;
use rustc_lint::{LateContext, Lint, LintContext};
use rustc_span::source_map::Span;
Expand Down Expand Up @@ -219,95 +219,6 @@ 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(|lines| lines[line_upper.line + split_idx])),
'\n',
);
let span_bottom = sp.with_lo(line_bottom.sf.lines(|lines| 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
Expand Down
25 changes: 9 additions & 16 deletions src/tools/clippy/tests/ui/map_flatten.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,12 @@ LL | | .flatten();
| |__________________^
|
= note: `-D clippy::map-flatten` implied by `-D warnings`
help: try replacing `map` with `and_then`
help: try replacing `map` with `and_then` and remove the `.flatten()`
|
LL ~ .and_then(|x| {
LL + if x <= 5 {
LL + Some(x)
|
help: and remove the `.flatten()`
|
LL + } else {
LL + None
LL + }
LL ~ });
Expand All @@ -38,14 +36,12 @@ LL | | })
LL | | .flatten();
| |__________________^
|
help: try replacing `map` with `and_then`
help: try replacing `map` with `and_then` and remove the `.flatten()`
|
LL ~ .and_then(|x| {
LL + if x == 1 {
LL + Ok(x)
|
help: and remove the `.flatten()`
|
LL + } else {
LL + Err(0)
LL + }
LL ~ });
Expand All @@ -64,14 +60,13 @@ LL | | })
LL | | .flatten();
| |__________________^
|
help: try replacing `map` with `and_then`
help: try replacing `map` with `and_then` and remove the `.flatten()`
|
LL ~ .and_then(|res| {
LL + if res > 0 {
LL + do_something();
|
help: and remove the `.flatten()`
|
LL + Ok(res)
LL + } else {
LL + Err(0)
LL + }
LL ~ });
Expand All @@ -90,14 +85,12 @@ LL | | })
LL | | .flatten()
| |__________________^
|
help: try replacing `map` with `filter_map`
help: try replacing `map` with `filter_map` and remove the `.flatten()`
|
LL ~ .filter_map(|some_value| {
LL + if some_value > 3 {
LL + Some(some_value)
|
help: and remove the `.flatten()`
|
LL + } else {
LL + None
LL + }
LL + })
Expand Down
2 changes: 0 additions & 2 deletions src/tools/clippy/tests/ui/map_flatten_fixable.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,6 @@ fn issue8878() {
.and_then(|_| {
// we need some newlines
// so that the span is big enough
// we need some newlines
// so that the span is big enough
Comment on lines -62 to -63
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those lines are probably there in order to test that also with more than 6 lines the message is displayed correctly. Can you add those back an re-bless the tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's weird, because I didn't actually change anything here, the original file doesn't have these too:

fn issue8878() {
std::collections::HashMap::<u32, u32>::new()
.get(&0)
.map(|_| {
// we need some newlines
// so that the span is big enough
// for a splitted output of the diagnostic
Some("")
// whitespace beforehand is important as well
})
.flatten();
}

These were removed by x test clippy --bless

Copy link
Member Author

@WaffleLapkin WaffleLapkin Jun 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit that added this test, rust-lang/rust-clippy@22673bc for some reason has more comments in the .fixed file, than in the .rs file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh so this was a bug before, but this PR now fixed that bug. Awesome! Thanks for pointing this out.

// for a splitted output of the diagnostic
Some("")
// whitespace beforehand is important as well
Expand Down
54 changes: 9 additions & 45 deletions src/tools/clippy/tests/ui/map_flatten_fixable.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,79 +2,45 @@ error: called `map(..).flatten()` on `Iterator`
--> $DIR/map_flatten_fixable.rs:18:47
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id).flatten().collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try replacing `map` with `filter_map` and remove the `.flatten()`: `filter_map(option_id)`
|
= note: `-D clippy::map-flatten` implied by `-D warnings`
help: try replacing `map` with `filter_map`, and remove the `.flatten()`
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().filter_map(option_id).collect();
| ~~~~~~~~~~~~~~~~~~~~~

error: called `map(..).flatten()` on `Iterator`
--> $DIR/map_flatten_fixable.rs:19:47
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id_ref).flatten().collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: try replacing `map` with `filter_map`, and remove the `.flatten()`
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().filter_map(option_id_ref).collect();
| ~~~~~~~~~~~~~~~~~~~~~~~~~
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try replacing `map` with `filter_map` and remove the `.flatten()`: `filter_map(option_id_ref)`

error: called `map(..).flatten()` on `Iterator`
--> $DIR/map_flatten_fixable.rs:20:47
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id_closure).flatten().collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: try replacing `map` with `filter_map`, and remove the `.flatten()`
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().filter_map(option_id_closure).collect();
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try replacing `map` with `filter_map` and remove the `.flatten()`: `filter_map(option_id_closure)`

error: called `map(..).flatten()` on `Iterator`
--> $DIR/map_flatten_fixable.rs:21:47
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| x.checked_add(1)).flatten().collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: try replacing `map` with `filter_map`, and remove the `.flatten()`
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().filter_map(|x| x.checked_add(1)).collect();
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try replacing `map` with `filter_map` and remove the `.flatten()`: `filter_map(|x| x.checked_add(1))`

error: called `map(..).flatten()` on `Iterator`
--> $DIR/map_flatten_fixable.rs:24:47
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect();
| ^^^^^^^^^^^^^^^^^^^^^^^
|
help: try replacing `map` with `flat_map`, and remove the `.flatten()`
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().flat_map(|x| 0..x).collect();
| ~~~~~~~~~~~~~~~~~~
| ^^^^^^^^^^^^^^^^^^^^^^^ help: try replacing `map` with `flat_map` and remove the `.flatten()`: `flat_map(|x| 0..x)`

error: called `map(..).flatten()` on `Option`
--> $DIR/map_flatten_fixable.rs:27:40
|
LL | let _: Option<_> = (Some(Some(1))).map(|x| x).flatten();
| ^^^^^^^^^^^^^^^^^^^^
|
help: try replacing `map` with `and_then`, and remove the `.flatten()`
|
LL | let _: Option<_> = (Some(Some(1))).and_then(|x| x);
| ~~~~~~~~~~~~~~~
| ^^^^^^^^^^^^^^^^^^^^ help: try replacing `map` with `and_then` and remove the `.flatten()`: `and_then(|x| x)`

error: called `map(..).flatten()` on `Result`
--> $DIR/map_flatten_fixable.rs:30:42
|
LL | let _: Result<_, &str> = (Ok(Ok(1))).map(|x| x).flatten();
| ^^^^^^^^^^^^^^^^^^^^
|
help: try replacing `map` with `and_then`, and remove the `.flatten()`
|
LL | let _: Result<_, &str> = (Ok(Ok(1))).and_then(|x| x);
| ~~~~~~~~~~~~~~~
| ^^^^^^^^^^^^^^^^^^^^ help: try replacing `map` with `and_then` and remove the `.flatten()`: `and_then(|x| x)`

error: called `map(..).flatten()` on `Option`
--> $DIR/map_flatten_fixable.rs:59:10
Expand All @@ -89,14 +55,12 @@ LL | | })
LL | | .flatten();
| |__________________^
|
help: try replacing `map` with `and_then`
help: try replacing `map` with `and_then` and remove the `.flatten()`
|
LL ~ .and_then(|_| {
LL + // we need some newlines
LL + // so that the span is big enough
|
help: and remove the `.flatten()`
|
LL + // for a splitted output of the diagnostic
LL + Some("")
LL + // whitespace beforehand is important as well
LL ~ });
Expand Down
Loading