diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index b8415fa3af1..19c46476263 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -838,6 +838,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &unwrap::UNNECESSARY_UNWRAP, &use_self::USE_SELF, &utils::internal_lints::CLIPPY_LINTS_INTERNAL, + &utils::internal_lints::COLLAPSIBLE_SPAN_LINT_CALLS, &utils::internal_lints::COMPILER_LINT_FUNCTIONS, &utils::internal_lints::DEFAULT_LINT, &utils::internal_lints::LINT_WITHOUT_LINT_PASS, @@ -1051,6 +1052,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box unnamed_address::UnnamedAddress); store.register_late_pass(|| box dereference::Dereferencing); store.register_late_pass(|| box future_not_send::FutureNotSend); + store.register_late_pass(|| box utils::internal_lints::CollapsibleCalls); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(&arithmetic::FLOAT_ARITHMETIC), @@ -1162,6 +1164,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_group(true, "clippy::internal", Some("clippy_internal"), vec![ LintId::of(&utils::internal_lints::CLIPPY_LINTS_INTERNAL), + LintId::of(&utils::internal_lints::COLLAPSIBLE_SPAN_LINT_CALLS), LintId::of(&utils::internal_lints::COMPILER_LINT_FUNCTIONS), LintId::of(&utils::internal_lints::DEFAULT_LINT), LintId::of(&utils::internal_lints::LINT_WITHOUT_LINT_PASS), diff --git a/clippy_lints/src/utils/diagnostics.rs b/clippy_lints/src/utils/diagnostics.rs index 4ad6689f3e0..3dff710bd85 100644 --- a/clippy_lints/src/utils/diagnostics.rs +++ b/clippy_lints/src/utils/diagnostics.rs @@ -166,6 +166,7 @@ pub fn span_lint_hir_and_then( /// | /// = note: `-D fold-any` implied by `-D warnings` /// ``` +#[allow(clippy::collapsible_span_lint_calls)] pub fn span_lint_and_sugg<'a, T: LintContext>( cx: &'a T, lint: &'static Lint, diff --git a/clippy_lints/src/utils/internal_lints.rs b/clippy_lints/src/utils/internal_lints.rs index bc2200800de..a2101234318 100644 --- a/clippy_lints/src/utils/internal_lints.rs +++ b/clippy_lints/src/utils/internal_lints.rs @@ -1,6 +1,7 @@ +use crate::utils::SpanlessEq; use crate::utils::{ - is_expn_of, match_def_path, match_type, method_calls, paths, span_lint, span_lint_and_help, span_lint_and_sugg, - walk_ptrs_ty, + is_expn_of, match_def_path, match_qpath, match_type, method_calls, paths, snippet, span_lint, span_lint_and_help, + span_lint_and_sugg, walk_ptrs_ty, }; use if_chain::if_chain; use rustc_ast::ast::{Crate as AstCrate, ItemKind, LitKind, Name, NodeId}; @@ -10,13 +11,15 @@ use rustc_hir as hir; use rustc_hir::def::{DefKind, Res}; use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; -use rustc_hir::{Crate, Expr, ExprKind, HirId, Item, MutTy, Mutability, Path, Ty, TyKind}; +use rustc_hir::{Crate, Expr, ExprKind, HirId, Item, MutTy, Mutability, Path, StmtKind, Ty, TyKind}; use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass}; use rustc_middle::hir::map::Map; use rustc_session::{declare_lint_pass, declare_tool_lint, impl_lint_pass}; use rustc_span::source_map::{Span, Spanned}; use rustc_span::symbol::SymbolStr; +use std::borrow::{Borrow, Cow}; + declare_clippy_lint! { /// **What it does:** Checks for various things we like to keep tidy in clippy. /// @@ -142,6 +145,67 @@ "found 'default lint description' in a lint declaration" } +declare_clippy_lint! { + /// **What it does:** Lints `span_lint_and_then` function calls, where the + /// closure argument has only one statement and that statement is a method + /// call to `span_suggestion`, `span_help`, `span_note` (using the same + /// span), `help` or `note`. + /// + /// These usages of `span_lint_and_then` should be replaced with one of the + /// wrapper functions `span_lint_and_sugg`, span_lint_and_help`, or + /// `span_lint_and_note`. + /// + /// **Why is this bad?** Using the wrapper `span_lint_and_*` functions, is more + /// convenient, readable and less error prone. + /// + /// **Known problems:** None + /// + /// *Example:** + /// Bad: + /// ```rust,ignore + /// span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { + /// db.span_suggestion( + /// expr.span, + /// help_msg, + /// sugg.to_string(), + /// Applicability::MachineApplicable, + /// ); + /// }); + /// span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { + /// db.span_help(expr.span, help_msg); + /// }); + /// span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { + /// db.help(help_msg); + /// }); + /// span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { + /// db.span_note(expr.span, note_msg); + /// }); + /// span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { + /// db.note(note_msg); + /// }); + /// ``` + /// + /// Good: + /// ```rust,ignore + /// span_lint_and_sugg( + /// cx, + /// TEST_LINT, + /// expr.span, + /// lint_msg, + /// help_msg, + /// sugg.to_string(), + /// Applicability::MachineApplicable, + /// ); + /// span_lint_and_help(cx, TEST_LINT, expr.span, lint_msg, help_msg); + /// span_lint_and_help(cx, TEST_LINT, expr.span, lint_msg, help_msg); + /// span_lint_and_note(cx, TEST_LINT, expr.span, lint_msg, expr.span, note_msg); + /// span_lint_and_note(cx, TEST_LINT, expr.span, lint_msg, expr.span, note_msg); + /// ``` + pub COLLAPSIBLE_SPAN_LINT_CALLS, + internal, + "found collapsible `span_lint_and_then` calls" +} + declare_lint_pass!(ClippyLintsInternal => [CLIPPY_LINTS_INTERNAL]); impl EarlyLintPass for ClippyLintsInternal { @@ -165,7 +229,7 @@ fn check_crate(&mut self, cx: &EarlyContext<'_>, krate: &AstCrate) { CLIPPY_LINTS_INTERNAL, item.span, "this constant should be before the previous constant due to lexical \ - ordering", + ordering", ); } } @@ -195,8 +259,8 @@ fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx Item<'_>) { if let ExprKind::AddrOf(_, _, ref inner_exp) = expr.kind; if let ExprKind::Struct(_, ref fields, _) = inner_exp.kind; let field = fields.iter() - .find(|f| f.ident.as_str() == "desc") - .expect("lints must have a description field"); + .find(|f| f.ident.as_str() == "desc") + .expect("lints must have a description field"); if let ExprKind::Lit(Spanned { node: LitKind::Str(ref sym, _), .. @@ -332,7 +396,7 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) { if let Some(sugg) = self.map.get(&*fn_name.as_str()); let ty = walk_ptrs_ty(cx.tables.expr_ty(&args[0])); if match_type(cx, ty, &paths::EARLY_CONTEXT) - || match_type(cx, ty, &paths::LATE_CONTEXT); + || match_type(cx, ty, &paths::LATE_CONTEXT); then { span_lint_and_help( cx, @@ -391,3 +455,155 @@ fn is_trigger_fn(fn_kind: FnKind<'_>) -> bool { FnKind::Closure(..) => false, } } + +declare_lint_pass!(CollapsibleCalls => [COLLAPSIBLE_SPAN_LINT_CALLS]); + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for CollapsibleCalls { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr<'_>) { + if_chain! { + if let ExprKind::Call(ref func, ref and_then_args) = expr.kind; + if let ExprKind::Path(ref path) = func.kind; + if match_qpath(path, &["span_lint_and_then"]); + if and_then_args.len() == 5; + if let ExprKind::Closure(_, _, body_id, _, _) = &and_then_args[4].kind; + let body = cx.tcx.hir().body(*body_id); + if let ExprKind::Block(block, _) = &body.value.kind; + let stmts = &block.stmts; + if stmts.len() == 1 && block.expr.is_none(); + if let StmtKind::Semi(only_expr) = &stmts[0].kind; + if let ExprKind::MethodCall(ref ps, _, ref span_call_args) = &only_expr.kind; + let and_then_snippets = get_and_then_snippets(cx, and_then_args); + let mut sle = SpanlessEq::new(cx).ignore_fn(); + then { + match &*ps.ident.as_str() { + "span_suggestion" if sle.eq_expr(&and_then_args[2], &span_call_args[1]) => { + suggest_suggestion(cx, expr, &and_then_snippets, &span_suggestion_snippets(cx, span_call_args)); + }, + "span_help" if sle.eq_expr(&and_then_args[2], &span_call_args[1]) => { + let help_snippet = snippet(cx, span_call_args[2].span, r#""...""#); + suggest_help(cx, expr, &and_then_snippets, help_snippet.borrow()); + }, + "span_note" if sle.eq_expr(&and_then_args[2], &span_call_args[1]) => { + let note_snippet = snippet(cx, span_call_args[2].span, r#""...""#); + suggest_note(cx, expr, &and_then_snippets, note_snippet.borrow()); + }, + "help" => { + let help_snippet = snippet(cx, span_call_args[1].span, r#""...""#); + suggest_help(cx, expr, &and_then_snippets, help_snippet.borrow()); + } + "note" => { + let note_snippet = snippet(cx, span_call_args[1].span, r#""...""#); + suggest_note(cx, expr, &and_then_snippets, note_snippet.borrow()); + } + _ => (), + } + } + } + } +} + +struct AndThenSnippets<'a> { + cx: Cow<'a, str>, + lint: Cow<'a, str>, + span: Cow<'a, str>, + msg: Cow<'a, str>, +} + +fn get_and_then_snippets<'a, 'hir>( + cx: &LateContext<'_, '_>, + and_then_snippets: &'hir [Expr<'hir>], +) -> AndThenSnippets<'a> { + let cx_snippet = snippet(cx, and_then_snippets[0].span, "cx"); + let lint_snippet = snippet(cx, and_then_snippets[1].span, ".."); + let span_snippet = snippet(cx, and_then_snippets[2].span, "span"); + let msg_snippet = snippet(cx, and_then_snippets[3].span, r#""...""#); + + AndThenSnippets { + cx: cx_snippet, + lint: lint_snippet, + span: span_snippet, + msg: msg_snippet, + } +} + +struct SpanSuggestionSnippets<'a> { + help: Cow<'a, str>, + sugg: Cow<'a, str>, + applicability: Cow<'a, str>, +} + +fn span_suggestion_snippets<'a, 'hir>( + cx: &LateContext<'_, '_>, + span_call_args: &'hir [Expr<'hir>], +) -> SpanSuggestionSnippets<'a> { + let help_snippet = snippet(cx, span_call_args[2].span, r#""...""#); + let sugg_snippet = snippet(cx, span_call_args[3].span, ".."); + let applicability_snippet = snippet(cx, span_call_args[4].span, "Applicability::MachineApplicable"); + + SpanSuggestionSnippets { + help: help_snippet, + sugg: sugg_snippet, + applicability: applicability_snippet, + } +} + +fn suggest_suggestion( + cx: &LateContext<'_, '_>, + expr: &Expr<'_>, + and_then_snippets: &AndThenSnippets<'_>, + span_suggestion_snippets: &SpanSuggestionSnippets<'_>, +) { + span_lint_and_sugg( + cx, + COLLAPSIBLE_SPAN_LINT_CALLS, + expr.span, + "this call is collapsible", + "collapse into", + format!( + "span_lint_and_sugg({}, {}, {}, {}, {}, {}, {})", + and_then_snippets.cx, + and_then_snippets.lint, + and_then_snippets.span, + and_then_snippets.msg, + span_suggestion_snippets.help, + span_suggestion_snippets.sugg, + span_suggestion_snippets.applicability + ), + Applicability::MachineApplicable, + ); +} + +fn suggest_help(cx: &LateContext<'_, '_>, expr: &Expr<'_>, and_then_snippets: &AndThenSnippets<'_>, help: &str) { + span_lint_and_sugg( + cx, + COLLAPSIBLE_SPAN_LINT_CALLS, + expr.span, + "this call is collapsible", + "collapse into", + format!( + "span_lint_and_help({}, {}, {}, {}, {})", + and_then_snippets.cx, and_then_snippets.lint, and_then_snippets.span, and_then_snippets.msg, help + ), + Applicability::MachineApplicable, + ); +} + +fn suggest_note(cx: &LateContext<'_, '_>, expr: &Expr<'_>, and_then_snippets: &AndThenSnippets<'_>, note: &str) { + span_lint_and_sugg( + cx, + COLLAPSIBLE_SPAN_LINT_CALLS, + expr.span, + "this call is collspible", + "collapse into", + format!( + "span_lint_and_note({}, {}, {}, {}, {}, {})", + and_then_snippets.cx, + and_then_snippets.lint, + and_then_snippets.span, + and_then_snippets.msg, + and_then_snippets.span, + note + ), + Applicability::MachineApplicable, + ); +} diff --git a/tests/ui/collapsible_span_lint_calls.fixed b/tests/ui/collapsible_span_lint_calls.fixed new file mode 100644 index 00000000000..19ff69ec667 --- /dev/null +++ b/tests/ui/collapsible_span_lint_calls.fixed @@ -0,0 +1,83 @@ +// run-rustfix +#![deny(clippy::internal)] +#![feature(rustc_private)] + +extern crate rustc_ast; +extern crate rustc_errors; +extern crate rustc_lint; +extern crate rustc_session; +extern crate rustc_span; + +use rustc_ast::ast::Expr; +use rustc_errors::{Applicability, DiagnosticBuilder}; +use rustc_lint::{EarlyContext, EarlyLintPass, Lint, LintContext}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::source_map::Span; + +#[allow(unused_variables)] +pub fn span_lint_and_then<'a, T: LintContext, F>(cx: &'a T, lint: &'static Lint, sp: Span, msg: &str, f: F) +where + F: for<'b> FnOnce(&mut DiagnosticBuilder<'b>), +{ +} + +#[allow(unused_variables)] +fn span_lint_and_help<'a, T: LintContext>(cx: &'a T, lint: &'static Lint, span: Span, msg: &str, help: &str) {} + +#[allow(unused_variables)] +fn span_lint_and_note<'a, T: LintContext>( + cx: &'a T, + lint: &'static Lint, + span: Span, + msg: &str, + note_span: Span, + note: &str, +) { +} + +#[allow(unused_variables)] +fn span_lint_and_sugg<'a, T: LintContext>( + cx: &'a T, + lint: &'static Lint, + sp: Span, + msg: &str, + help: &str, + sugg: String, + applicability: Applicability, +) { +} + +declare_tool_lint! { + pub clippy::TEST_LINT, + Warn, + "", + report_in_external_macro: true +} + +declare_lint_pass!(Pass => [TEST_LINT]); + +impl EarlyLintPass for Pass { + fn check_expr(&mut self, cx: &EarlyContext, expr: &Expr) { + let lint_msg = "lint message"; + let help_msg = "help message"; + let note_msg = "note message"; + let sugg = "new_call()"; + let predicate = true; + + span_lint_and_sugg(cx, TEST_LINT, expr.span, lint_msg, help_msg, sugg.to_string(), Applicability::MachineApplicable); + span_lint_and_help(cx, TEST_LINT, expr.span, lint_msg, help_msg); + span_lint_and_help(cx, TEST_LINT, expr.span, lint_msg, help_msg); + span_lint_and_note(cx, TEST_LINT, expr.span, lint_msg, expr.span, note_msg); + span_lint_and_note(cx, TEST_LINT, expr.span, lint_msg, expr.span, note_msg); + + // This expr shouldn't trigger this lint. + span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { + db.note(note_msg); + if predicate { + db.note(note_msg); + } + }) + } +} + +fn main() {} diff --git a/tests/ui/collapsible_span_lint_calls.rs b/tests/ui/collapsible_span_lint_calls.rs new file mode 100644 index 00000000000..af263fcbea1 --- /dev/null +++ b/tests/ui/collapsible_span_lint_calls.rs @@ -0,0 +1,93 @@ +// run-rustfix +#![deny(clippy::internal)] +#![feature(rustc_private)] + +extern crate rustc_ast; +extern crate rustc_errors; +extern crate rustc_lint; +extern crate rustc_session; +extern crate rustc_span; + +use rustc_ast::ast::Expr; +use rustc_errors::{Applicability, DiagnosticBuilder}; +use rustc_lint::{EarlyContext, EarlyLintPass, Lint, LintContext}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::source_map::Span; + +#[allow(unused_variables)] +pub fn span_lint_and_then<'a, T: LintContext, F>(cx: &'a T, lint: &'static Lint, sp: Span, msg: &str, f: F) +where + F: for<'b> FnOnce(&mut DiagnosticBuilder<'b>), +{ +} + +#[allow(unused_variables)] +fn span_lint_and_help<'a, T: LintContext>(cx: &'a T, lint: &'static Lint, span: Span, msg: &str, help: &str) {} + +#[allow(unused_variables)] +fn span_lint_and_note<'a, T: LintContext>( + cx: &'a T, + lint: &'static Lint, + span: Span, + msg: &str, + note_span: Span, + note: &str, +) { +} + +#[allow(unused_variables)] +fn span_lint_and_sugg<'a, T: LintContext>( + cx: &'a T, + lint: &'static Lint, + sp: Span, + msg: &str, + help: &str, + sugg: String, + applicability: Applicability, +) { +} + +declare_tool_lint! { + pub clippy::TEST_LINT, + Warn, + "", + report_in_external_macro: true +} + +declare_lint_pass!(Pass => [TEST_LINT]); + +impl EarlyLintPass for Pass { + fn check_expr(&mut self, cx: &EarlyContext, expr: &Expr) { + let lint_msg = "lint message"; + let help_msg = "help message"; + let note_msg = "note message"; + let sugg = "new_call()"; + let predicate = true; + + span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { + db.span_suggestion(expr.span, help_msg, sugg.to_string(), Applicability::MachineApplicable); + }); + span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { + db.span_help(expr.span, help_msg); + }); + span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { + db.help(help_msg); + }); + span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { + db.span_note(expr.span, note_msg); + }); + span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { + db.note(note_msg); + }); + + // This expr shouldn't trigger this lint. + span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { + db.note(note_msg); + if predicate { + db.note(note_msg); + } + }) + } +} + +fn main() {} diff --git a/tests/ui/collapsible_span_lint_calls.stderr b/tests/ui/collapsible_span_lint_calls.stderr new file mode 100644 index 00000000000..fa40c82c983 --- /dev/null +++ b/tests/ui/collapsible_span_lint_calls.stderr @@ -0,0 +1,49 @@ +error: this call is collapsible + --> $DIR/collapsible_span_lint_calls.rs:67:9 + | +LL | / span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { +LL | | db.span_suggestion(expr.span, help_msg, sugg.to_string(), Applicability::MachineApplicable); +LL | | }); + | |__________^ help: collapse into: `span_lint_and_sugg(cx, TEST_LINT, expr.span, lint_msg, help_msg, sugg.to_string(), Applicability::MachineApplicable)` + | +note: the lint level is defined here + --> $DIR/collapsible_span_lint_calls.rs:2:9 + | +LL | #![deny(clippy::internal)] + | ^^^^^^^^^^^^^^^^ + = note: `#[deny(clippy::collapsible_span_lint_calls)]` implied by `#[deny(clippy::internal)]` + +error: this call is collapsible + --> $DIR/collapsible_span_lint_calls.rs:70:9 + | +LL | / span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { +LL | | db.span_help(expr.span, help_msg); +LL | | }); + | |__________^ help: collapse into: `span_lint_and_help(cx, TEST_LINT, expr.span, lint_msg, help_msg)` + +error: this call is collapsible + --> $DIR/collapsible_span_lint_calls.rs:73:9 + | +LL | / span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { +LL | | db.help(help_msg); +LL | | }); + | |__________^ help: collapse into: `span_lint_and_help(cx, TEST_LINT, expr.span, lint_msg, help_msg)` + +error: this call is collspible + --> $DIR/collapsible_span_lint_calls.rs:76:9 + | +LL | / span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { +LL | | db.span_note(expr.span, note_msg); +LL | | }); + | |__________^ help: collapse into: `span_lint_and_note(cx, TEST_LINT, expr.span, lint_msg, expr.span, note_msg)` + +error: this call is collspible + --> $DIR/collapsible_span_lint_calls.rs:79:9 + | +LL | / span_lint_and_then(cx, TEST_LINT, expr.span, lint_msg, |db| { +LL | | db.note(note_msg); +LL | | }); + | |__________^ help: collapse into: `span_lint_and_note(cx, TEST_LINT, expr.span, lint_msg, expr.span, note_msg)` + +error: aborting due to 5 previous errors +