lint on implementing visit_string without also implementing visit_str

Author: oli-obk

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,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,

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`",
+                            );
+                        }
+                    }
+                }
+            }
+        }
+    }
+}

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() {
+}