Merge pull request #1093 from oli-obk/serde_specific_lint

lint on implementing `visit_string` without also implementing `visit_str`

Author: llogiq

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

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]

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

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,8 @@ 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_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);
@@ -399,6 +402,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,

clippy_lints/src/serde.rs

@@ -0,0 +1,55 @@
+use rustc::lint::*;
+use rustc::hir::*;
+use utils::{span_lint, get_trait_def_id, paths};
+
+/// **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, &paths::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`",
+                            );
+                        }
+                    }
+                }
+            }
+        }
+    }
+}

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<InternedString> = 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);
+                        }
+                    }
+                }
+            }
+        }
+    }
+}

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<P<Expr>>;

clippy_lints/src/utils/paths.rs

@@ -54,6 +54,7 @@ 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 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"];

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<E>(&mut self, _v: &str) -> Result<Self::Value, E>
+        where E: serde::Error,
+    {
+        unimplemented!()
+    }
+
+    fn visit_string<E>(&mut self, _v: String) -> Result<Self::Value, E>
+        where E: serde::Error,
+    {
+        unimplemented!()
+    }
+}
+
+struct B;
+
+impl serde::de::Visitor for B {
+    type Value = ();
+
+    fn visit_string<E>(&mut self, _v: String) -> Result<Self::Value, E>
+    //~^ ERROR you should not implement `visit_string` without also implementing `visit_str`
+        where E: serde::Error,
+    {
+        unimplemented!()
+    }
+}
+
+fn main() {
+}

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"];
 

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: