From eb75d4ee6295fdb34a87b1b9eb5494e7eaf5712d Mon Sep 17 00:00:00 2001 From: mcarton Date: Sun, 10 Jul 2016 14:07:13 +0200 Subject: [PATCH] Fix suggestions for `NEW_WITHOUT_DEFAULT` --- clippy_lints/src/new_without_default.rs | 24 +++--- clippy_lints/src/utils/sugg.rs | 97 +++++++++++++++++++++-- tests/compile-fail/new_without_default.rs | 22 ++++- 3 files changed, 124 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/new_without_default.rs b/clippy_lints/src/new_without_default.rs index 20c0d282bd8f..053e423336ef 100644 --- a/clippy_lints/src/new_without_default.rs +++ b/clippy_lints/src/new_without_default.rs @@ -7,6 +7,7 @@ use syntax::ast; use syntax::codemap::Span; use utils::paths; use utils::{get_trait_def_id, implements_trait, in_external_macro, return_ty, same_tys, span_lint_and_then}; +use utils::sugg::DiagnosticBuilderExt; /// **What it does:** This lints about type with a `fn new() -> Self` method /// and no implementation of @@ -14,8 +15,7 @@ use utils::{get_trait_def_id, implements_trait, in_external_macro, return_ty, sa /// /// **Why is this bad?** User might expect to be able to use /// [`Default`](https://doc.rust-lang.org/std/default/trait.Default.html) -/// as the type can be -/// constructed without arguments. +/// as the type can be constructed without arguments. /// /// **Known problems:** Hopefully none. /// @@ -118,8 +118,8 @@ impl LateLintPass for NewWithoutDefault { `Default` implementation for `{}`", self_ty), |db| { - db.span_suggestion(span, "try this", "#[derive(Default)]".into()); - }); + db.suggest_item_with_attr(cx, span, "try this", "#[derive(Default)]"); + }); } else { span_lint_and_then(cx, NEW_WITHOUT_DEFAULT, span, @@ -127,11 +127,17 @@ impl LateLintPass for NewWithoutDefault { `Default` implementation for `{}`", self_ty), |db| { - db.span_suggestion(span, - "try this", - format!("impl Default for {} {{ fn default() -> \ - Self {{ {}::new() }} }}", self_ty, self_ty)); - }); + db.suggest_prepend_item(cx, + span, + "try this", + &format!( +"impl Default for {} {{ + fn default() -> Self {{ + Self::new() + }} +}}", + self_ty)); + }); } }} } diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs index 624c030cdd44..b6ed9071c0a0 100644 --- a/clippy_lints/src/utils/sugg.rs +++ b/clippy_lints/src/utils/sugg.rs @@ -1,11 +1,14 @@ use rustc::hir; -use rustc::lint::{EarlyContext, LateContext}; +use rustc::lint::{EarlyContext, LateContext, LintContext}; +use rustc_errors; use std::borrow::Cow; +use std::fmt::Display; use std; -use syntax::ast; -use syntax::util::parser::AssocOp; -use utils::{higher, snippet, snippet_opt}; +use syntax::codemap::{CharPos, Span}; use syntax::print::pprust::binop_to_string; +use syntax::util::parser::AssocOp; +use syntax::ast; +use utils::{higher, snippet, snippet_opt}; /// A helper type to build suggestion correctly handling parenthesis. pub enum Sugg<'a> { @@ -20,7 +23,7 @@ pub enum Sugg<'a> { /// Literal constant `1`, for convenience. pub const ONE: Sugg<'static> = Sugg::NonParen(Cow::Borrowed("1")); -impl<'a> std::fmt::Display for Sugg<'a> { +impl<'a> 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) => { @@ -126,7 +129,7 @@ impl<'a> Sugg<'a> { } /// Convenience method to create the ` as ` suggestion. - pub fn as_ty(self, rhs: R) -> Sugg<'static> { + pub fn as_ty(self, rhs: R) -> Sugg<'static> { make_assoc(AssocOp::As, &self, &Sugg::NonParen(rhs.to_string().into())) } @@ -198,7 +201,7 @@ impl ParenHelper { } } -impl std::fmt::Display for ParenHelper { +impl Display for ParenHelper { fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> { if self.paren { write!(f, "({})", self.wrapped) @@ -354,3 +357,83 @@ fn astbinop2assignop(op: ast::BinOp) -> AssocOp { And | Eq | Ge | Gt | Le | Lt | Ne | Or => panic!("This operator does not exist"), }) } + +/// Return the indentation before `span` if there are nothing but `[ \t]` before it on its line. +fn indentation(cx: &T, span: Span) -> Option { + let lo = cx.sess().codemap().lookup_char_pos(span.lo); + if let Some(line) = lo.file.get_line(lo.line - 1 /* line numbers in `Loc` are 1-based */) { + if let Some((pos, _)) = line.char_indices().find(|&(_, c)| c != ' ' && c != '\t') { + // we can mix char and byte positions here because we only consider `[ \t]` + if lo.col == CharPos(pos) { + Some(line[..pos].into()) + } else { + None + } + } else { + None + } + } else { + None + } +} + +pub trait DiagnosticBuilderExt { + /// Suggests to add an attribute to an item. + /// + /// Correctly handles indentation of the attribute and item. + /// + /// # Example + /// + /// ```rust + /// db.suggest_item_with_attr(cx, item, "#[derive(Default)]"); + /// ``` + fn suggest_item_with_attr(&mut self, cx: &T, item: Span, msg: &str, attr: &D); + + /// Suggest to add an item before another. + /// + /// The item should not be indented (expect for inner indentation). + /// + /// # Example + /// + /// ```rust + /// db.suggest_prepend_item(cx, item, + /// "fn foo() { + /// bar(); + /// }"); + /// ``` + fn suggest_prepend_item(&mut self, cx: &T, item: Span, msg: &str, new_item: &str); +} + +impl<'a, 'b, T: LintContext> DiagnosticBuilderExt for rustc_errors::DiagnosticBuilder<'b> { + fn suggest_item_with_attr(&mut self, cx: &T, item: Span, msg: &str, attr: &D) { + if let Some(indent) = indentation(cx, item) { + let span = Span { + hi: item.lo, + ..item + }; + + self.span_suggestion(span, msg, format!("{}\n{}", attr, indent)); + } + } + + fn suggest_prepend_item(&mut self, cx: &T, item: Span, msg: &str, new_item: &str) { + if let Some(indent) = indentation(cx, item) { + let span = Span { + hi: item.lo, + ..item + }; + + let mut first = true; + let new_item = new_item.lines().map(|l| { + if first { + first = false; + format!("{}\n", l) + } else { + format!("{}{}\n", indent, l) + } + }).collect::(); + + self.span_suggestion(span, msg, format!("{}\n{}", new_item, indent)); + } + } +} diff --git a/tests/compile-fail/new_without_default.rs b/tests/compile-fail/new_without_default.rs index e3a8024dde15..cad675275db9 100644 --- a/tests/compile-fail/new_without_default.rs +++ b/tests/compile-fail/new_without_default.rs @@ -7,13 +7,21 @@ pub struct Foo; impl Foo { - pub fn new() -> Foo { Foo } //~ERROR: you should consider deriving a `Default` implementation for `Foo` + pub fn new() -> Foo { Foo } + //~^ERROR: you should consider deriving a `Default` implementation for `Foo` + //~|HELP try this + //~^^^SUGGESTION #[derive(Default)] + //~^^^SUGGESTION pub fn new } pub struct Bar; impl Bar { - pub fn new() -> Self { Bar } //~ERROR: you should consider deriving a `Default` implementation for `Bar` + pub fn new() -> Self { Bar } + //~^ERROR: you should consider deriving a `Default` implementation for `Bar` + //~|HELP try this + //~^^^SUGGESTION #[derive(Default)] + //~^^^SUGGESTION pub fn new } pub struct Ok; @@ -61,7 +69,15 @@ pub struct LtKo<'a> { } impl<'c> LtKo<'c> { - pub fn new() -> LtKo<'c> { unimplemented!() } //~ERROR: you should consider adding a `Default` implementation for + pub fn new() -> LtKo<'c> { unimplemented!() } + //~^ERROR: you should consider adding a `Default` implementation for + //~^^HELP try + //~^^^SUGGESTION impl Default for LtKo<'c> { + //~^^^SUGGESTION fn default() -> Self { + //~^^^SUGGESTION Self::new() + //~^^^SUGGESTION } + //~^^^SUGGESTION } + // FIXME: that suggestion is missing lifetimes } struct Private;