From 319c66a2a4284c682d9575fe7aad5e4b4bf89365 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 12 Jul 2016 17:36:11 +0200 Subject: [PATCH 1/4] lint on implementing `visit_string` without also implementing `visit_str` --- CHANGELOG.md | 1 + Cargo.toml | 1 + README.md | 3 +- clippy_lints/src/lib.rs | 3 ++ clippy_lints/src/serde.rs | 55 +++++++++++++++++++++++++++++++++++++ tests/compile-fail/serde.rs | 39 ++++++++++++++++++++++++++ 6 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/serde.rs create mode 100644 tests/compile-fail/serde.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index ca3c67e4e8f..e6ceb413169 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -246,6 +246,7 @@ All notable changes to this project will be documented in this file. [`result_unwrap_used`]: https://github.com/Manishearth/rust-clippy/wiki#result_unwrap_used [`reverse_range_loop`]: https://github.com/Manishearth/rust-clippy/wiki#reverse_range_loop [`search_is_some`]: https://github.com/Manishearth/rust-clippy/wiki#search_is_some +[`serde_api_misuse`]: https://github.com/Manishearth/rust-clippy/wiki#serde_api_misuse [`shadow_reuse`]: https://github.com/Manishearth/rust-clippy/wiki#shadow_reuse [`shadow_same`]: https://github.com/Manishearth/rust-clippy/wiki#shadow_same [`shadow_unrelated`]: https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated diff --git a/Cargo.toml b/Cargo.toml index 79aefdfce61..9790937a307 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,6 +34,7 @@ lazy_static = "0.1.15" regex = "0.1.71" rustc-serialize = "0.3" clippy-mini-macro-test = { version = "0.1", path = "mini-macro" } +serde = "0.7" [features] diff --git a/README.md b/README.md index f37d242e434..c62edbe03ae 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ Table of contents: ## Lints -There are 158 lints included in this crate: +There are 159 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -136,6 +136,7 @@ name [result_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#result_unwrap_used) | allow | using `Result.unwrap()`, which might be better handled [reverse_range_loop](https://github.com/Manishearth/rust-clippy/wiki#reverse_range_loop) | warn | Iterating over an empty range, such as `10..0` or `5..5` [search_is_some](https://github.com/Manishearth/rust-clippy/wiki#search_is_some) | warn | using an iterator search followed by `is_some()`, which is more succinctly expressed as a call to `any()` +[serde_api_misuse](https://github.com/Manishearth/rust-clippy/wiki#serde_api_misuse) | warn | Various things that will negatively affect your serde experience [shadow_reuse](https://github.com/Manishearth/rust-clippy/wiki#shadow_reuse) | allow | rebinding a name to an expression that re-uses the original value, e.g. `let x = x + 1` [shadow_same](https://github.com/Manishearth/rust-clippy/wiki#shadow_same) | allow | rebinding a name to itself, e.g. `let mut x = &mut x` [shadow_unrelated](https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated) | allow | The name is re-bound without even using the original value diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c873ef8cc57..4a8ecdda6f0 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -114,6 +114,7 @@ pub mod ptr_arg; pub mod ranges; pub mod regex; pub mod returns; +pub mod serde; pub mod shadow; pub mod strings; pub mod swap; @@ -167,6 +168,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { store.register_removed("string_to_string", "using `string::to_string` is common even today and specialization will likely happen soon"); // end deprecated lints, do not remove this comment, it’s used in `update_lints` + reg.register_late_lint_pass(box serde::Serde); reg.register_late_lint_pass(box types::TypePass); reg.register_late_lint_pass(box booleans::NonminimalBool); reg.register_late_lint_pass(box misc::TopLevelRefPass); @@ -399,6 +401,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { regex::TRIVIAL_REGEX, returns::LET_AND_RETURN, returns::NEEDLESS_RETURN, + serde::SERDE_API_MISUSE, strings::STRING_LIT_AS_BYTES, swap::ALMOST_SWAPPED, swap::MANUAL_SWAP, diff --git a/clippy_lints/src/serde.rs b/clippy_lints/src/serde.rs new file mode 100644 index 00000000000..c916ad3c514 --- /dev/null +++ b/clippy_lints/src/serde.rs @@ -0,0 +1,55 @@ +use rustc::lint::*; +use rustc::hir::*; +use utils::{span_lint, get_trait_def_id}; + +/// **What it does:** This lint checks for mis-uses of the serde API +/// +/// **Why is this bad?** Serde is very finnicky about how its API should be used, but the type system can't be used to enforce it (yet) +/// +/// **Known problems:** None. +/// +/// **Example:** implementing `Visitor::visit_string` but not `Visitor::visit_str` +declare_lint! { + pub SERDE_API_MISUSE, Warn, + "Various things that will negatively affect your serde experience" +} + + +#[derive(Copy, Clone)] +pub struct Serde; + +impl LintPass for Serde { + fn get_lints(&self) -> LintArray { + lint_array!(SERDE_API_MISUSE) + } +} + +impl LateLintPass for Serde { + fn check_item(&mut self, cx: &LateContext, item: &Item) { + if let ItemImpl(_, _, _, Some(ref trait_ref), _, ref items) = item.node { + let did = cx.tcx.expect_def(trait_ref.ref_id).def_id(); + if let Some(visit_did) = get_trait_def_id(cx, &["serde", "de", "Visitor"]) { + if did == visit_did { + let mut seen_str = None; + let mut seen_string = None; + for item in items { + match &*item.name.as_str() { + "visit_str" => seen_str = Some(item.span), + "visit_string" => seen_string = Some(item.span), + _ => {}, + } + } + if let Some(span) = seen_string { + if seen_str.is_none() { + span_lint(cx, + SERDE_API_MISUSE, + span, + "you should not implement `visit_string` without also implementing `visit_str`", + ); + } + } + } + } + } + } +} diff --git a/tests/compile-fail/serde.rs b/tests/compile-fail/serde.rs new file mode 100644 index 00000000000..d5099edbc0c --- /dev/null +++ b/tests/compile-fail/serde.rs @@ -0,0 +1,39 @@ +#![feature(plugin)] +#![plugin(clippy)] +#![deny(serde_api_misuse)] +#![allow(dead_code)] + +extern crate serde; + +struct A; + +impl serde::de::Visitor for A { + type Value = (); + fn visit_str(&mut self, _v: &str) -> Result + where E: serde::Error, + { + unimplemented!() + } + + fn visit_string(&mut self, _v: String) -> Result + where E: serde::Error, + { + unimplemented!() + } +} + +struct B; + +impl serde::de::Visitor for B { + type Value = (); + + fn visit_string(&mut self, _v: String) -> Result + //~^ ERROR you should not implement `visit_string` without also implementing `visit_str` + where E: serde::Error, + { + unimplemented!() + } +} + +fn main() { +} From fc54a91916a450082de6cc708af8e093671a3325 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Fri, 15 Jul 2016 18:10:28 +0200 Subject: [PATCH 2/4] add the path to the util::path module --- clippy_lints/src/serde.rs | 4 ++-- clippy_lints/src/utils/paths.rs | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/serde.rs b/clippy_lints/src/serde.rs index c916ad3c514..adfe99363d3 100644 --- a/clippy_lints/src/serde.rs +++ b/clippy_lints/src/serde.rs @@ -1,6 +1,6 @@ use rustc::lint::*; use rustc::hir::*; -use utils::{span_lint, get_trait_def_id}; +use utils::{span_lint, get_trait_def_id, paths}; /// **What it does:** This lint checks for mis-uses of the serde API /// @@ -28,7 +28,7 @@ impl LateLintPass for Serde { fn check_item(&mut self, cx: &LateContext, item: &Item) { if let ItemImpl(_, _, _, Some(ref trait_ref), _, ref items) = item.node { let did = cx.tcx.expect_def(trait_ref.ref_id).def_id(); - if let Some(visit_did) = get_trait_def_id(cx, &["serde", "de", "Visitor"]) { + if let Some(visit_did) = get_trait_def_id(cx, &paths::SERDE_DE_VISITOR) { if did == visit_did { let mut seen_str = None; let mut seen_string = None; diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 3c91578abd0..be4fa0f6203 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -55,6 +55,7 @@ pub const REGEX_NEW: [&'static str; 4] = ["regex", "re_unicode", "Regex", "new"] pub const REGEX_SET_NEW: [&'static str; 5] = ["regex", "re_set", "unicode", "RegexSet", "new"]; pub const RESULT: [&'static str; 3] = ["core", "result", "Result"]; pub const STRING: [&'static str; 3] = ["collections", "string", "String"]; +pub const SERDE_DE_VISITOR: [&'static str; 3] = ["serde", "de", "Visitor"]; pub const TRANSMUTE: [&'static str; 4] = ["core", "intrinsics", "", "transmute"]; pub const VEC: [&'static str; 3] = ["collections", "vec", "Vec"]; pub const VEC_DEQUE: [&'static str; 3] = ["collections", "vec_deque", "VecDeque"]; From 02c46f057f51762f987a94f6932fabfe98d6ef67 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Mon, 18 Jul 2016 11:19:33 +0200 Subject: [PATCH 3/4] add an internal lint that catches misordered paths --- clippy_lints/src/lib.rs | 1 + clippy_lints/src/utils/internal_lints.rs | 53 ++++++++++++++++++++++++ clippy_lints/src/utils/mod.rs | 1 + tests/dogfood.rs | 3 +- util/update_lints.py | 9 ++-- 5 files changed, 61 insertions(+), 6 deletions(-) create mode 100644 clippy_lints/src/utils/internal_lints.rs diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 4a8ecdda6f0..7c285008ea8 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -169,6 +169,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { // end deprecated lints, do not remove this comment, it’s used in `update_lints` reg.register_late_lint_pass(box serde::Serde); + reg.register_early_lint_pass(box utils::internal_lints::Clippy); reg.register_late_lint_pass(box types::TypePass); reg.register_late_lint_pass(box booleans::NonminimalBool); reg.register_late_lint_pass(box misc::TopLevelRefPass); diff --git a/clippy_lints/src/utils/internal_lints.rs b/clippy_lints/src/utils/internal_lints.rs new file mode 100644 index 00000000000..e0d32a60722 --- /dev/null +++ b/clippy_lints/src/utils/internal_lints.rs @@ -0,0 +1,53 @@ +use rustc::lint::*; +use utils::span_lint; +use syntax::parse::token::InternedString; +use syntax::ast::*; + +/// **What it does:** This lint checks for various things we like to keep tidy in clippy +/// +/// **Why is this bad?** ??? +/// +/// **Known problems:** None. +/// +/// **Example:** wrong ordering of the util::paths constants +declare_lint! { + pub CLIPPY_LINTS_INTERNAL, Allow, + "Various things that will negatively affect your clippy experience" +} + + +#[derive(Copy, Clone)] +pub struct Clippy; + +impl LintPass for Clippy { + fn get_lints(&self) -> LintArray { + lint_array!(CLIPPY_LINTS_INTERNAL) + } +} + +impl EarlyLintPass for Clippy { + fn check_crate(&mut self, cx: &EarlyContext, krate: &Crate) { + if let Some(utils) = krate.module.items.iter().find(|item| item.ident.name.as_str() == "utils") { + if let ItemKind::Mod(ref utils_mod) = utils.node { + if let Some(paths) = utils_mod.items.iter().find(|item| item.ident.name.as_str() == "paths") { + if let ItemKind::Mod(ref paths_mod) = paths.node { + let mut last_name: Option = None; + for item in &paths_mod.items { + let name = item.ident.name.as_str(); + if let Some(ref last_name) = last_name { + if **last_name > *name { + span_lint(cx, + CLIPPY_LINTS_INTERNAL, + item.span, + "this constant should be before the previous constant due to lexical ordering", + ); + } + } + last_name = Some(name); + } + } + } + } + } + } +} diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 189801c8c0b..2b49edccbbc 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -25,6 +25,7 @@ pub mod conf; mod hir; pub mod paths; pub mod sugg; +pub mod internal_lints; pub use self::hir::{SpanlessEq, SpanlessHash}; pub type MethodArgs = HirVec>; diff --git a/tests/dogfood.rs b/tests/dogfood.rs index 821279d909c..c97be3f6e6f 100644 --- a/tests/dogfood.rs +++ b/tests/dogfood.rs @@ -17,7 +17,7 @@ fn dogfood() { let mut s = String::new(); s.push_str(" -L target/debug/"); s.push_str(" -L target/debug/deps"); - s.push_str(" -Zextra-plugins=clippy -Ltarget_recur/debug -Dclippy_pedantic -Dclippy"); + s.push_str(" -Zextra-plugins=clippy -Ltarget_recur/debug -Dclippy_pedantic -Dclippy -Dclippy_lints_internal"); config.target_rustcflags = Some(s); if let Ok(name) = var("TESTNAME") { config.filter = Some(name.to_owned()) @@ -29,6 +29,7 @@ fn dogfood() { } config.mode = cfg_mode; + config.verbose = true; let files = ["src/main.rs", "src/lib.rs", "clippy_lints/src/lib.rs"]; diff --git a/util/update_lints.py b/util/update_lints.py index 35bddcbbb5c..e81f71617ff 100755 --- a/util/update_lints.py +++ b/util/update_lints.py @@ -150,11 +150,10 @@ def main(print_only=False, check=False): return # collect all lints from source files - for root, _, files in os.walk('clippy_lints/src'): - for fn in files: - if fn.endswith('.rs'): - collect(lints, deprecated_lints, restriction_lints, - os.path.join(root, fn)) + for fn in os.listdir('clippy_lints/src'): + if fn.endswith('.rs'): + collect(lints, deprecated_lints, restriction_lints, + os.path.join('clippy_lints', 'src', fn)) # determine version with open('Cargo.toml') as fp: From b4ee9115d8231c4918498e25bcdf1a835df6fe4f Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Mon, 18 Jul 2016 11:19:45 +0200 Subject: [PATCH 4/4] "fallout" --- clippy_lints/src/utils/paths.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index be4fa0f6203..51071aed7a9 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -54,8 +54,8 @@ pub const REGEX_BYTES_SET_NEW: [&'static str; 5] = ["regex", "re_set", "bytes", pub const REGEX_NEW: [&'static str; 4] = ["regex", "re_unicode", "Regex", "new"]; pub const REGEX_SET_NEW: [&'static str; 5] = ["regex", "re_set", "unicode", "RegexSet", "new"]; pub const RESULT: [&'static str; 3] = ["core", "result", "Result"]; -pub const STRING: [&'static str; 3] = ["collections", "string", "String"]; pub const SERDE_DE_VISITOR: [&'static str; 3] = ["serde", "de", "Visitor"]; +pub const STRING: [&'static str; 3] = ["collections", "string", "String"]; pub const TRANSMUTE: [&'static str; 4] = ["core", "intrinsics", "", "transmute"]; pub const VEC: [&'static str; 3] = ["collections", "vec", "Vec"]; pub const VEC_DEQUE: [&'static str; 3] = ["collections", "vec_deque", "VecDeque"];