Auto merge of #6218 - korrat:master, r=ebroto

Add lint for maps with zero-sized value types

Hi, this is my first time contributing to clippy or rust in general, so I'm not sure about the details of contributing. Please excuse me and let me now if I did anything wrong. I have a couple of questions:

1. I'm not sure what category this lint should be. I've put it in "nursery" for now.
1. Should I squash commits this is reviewed/merged?

changelog: Add lint for maps with zero-sized value types

Fixes #1641

CHANGELOG.md

@@ -2171,5 +2171,6 @@ Released 2018-09-13
 [`zero_divided_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_divided_by_zero
 [`zero_prefixed_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_prefixed_literal
 [`zero_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_ptr
+[`zero_sized_map_values`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_sized_map_values
 [`zst_offset`]: https://rust-lang.github.io/rust-clippy/master/index.html#zst_offset
 <!-- end autogenerated links to lint list -->

clippy_lints/src/lib.rs

@@ -345,6 +345,7 @@ mod wildcard_dependencies;
 mod wildcard_imports;
 mod write;
 mod zero_div_zero;
+mod zero_sized_map_values;
 // end lints modules, do not remove this comment, it’s used in `update_lints`
 
 pub use crate::utils::conf::Conf;
@@ -944,6 +945,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &write::WRITE_LITERAL,
         &write::WRITE_WITH_NEWLINE,
         &zero_div_zero::ZERO_DIVIDED_BY_ZERO,
+        &zero_sized_map_values::ZERO_SIZED_MAP_VALUES,
     ]);
     // end register lints, do not remove this comment, it’s used in `update_lints`
 
@@ -1204,6 +1206,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box undropped_manually_drops::UndroppedManuallyDrops);
     store.register_late_pass(|| box strings::StrToString);
     store.register_late_pass(|| box strings::StringToString);
+    store.register_late_pass(|| box zero_sized_map_values::ZeroSizedMapValues);
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
         LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1336,6 +1339,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&unused_self::UNUSED_SELF),
         LintId::of(&wildcard_imports::ENUM_GLOB_USE),
         LintId::of(&wildcard_imports::WILDCARD_IMPORTS),
+        LintId::of(&zero_sized_map_values::ZERO_SIZED_MAP_VALUES),
     ]);
 
     #[cfg(feature = "internal-lints")]

clippy_lints/src/zero_sized_map_values.rs

@@ -0,0 +1,82 @@
+use if_chain::if_chain;
+use rustc_hir::{self as hir, HirId, ItemKind, Node};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::{Adt, Ty};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_target::abi::LayoutOf as _;
+use rustc_typeck::hir_ty_to_ty;
+
+use crate::utils::{is_type_diagnostic_item, match_type, paths, span_lint_and_help};
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for maps with zero-sized value types anywhere in the code.
+    ///
+    /// **Why is this bad?** Since there is only a single value for a zero-sized type, a map
+    /// containing zero sized values is effectively a set. Using a set in that case improves
+    /// readability and communicates intent more clearly.
+    ///
+    /// **Known problems:**
+    /// * A zero-sized type cannot be recovered later if it contains private fields.
+    /// * This lints the signature of public items
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// # use std::collections::HashMap;
+    /// fn unique_words(text: &str) -> HashMap<&str, ()> {
+    ///     todo!();
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// # use std::collections::HashSet;
+    /// fn unique_words(text: &str) -> HashSet<&str> {
+    ///     todo!();
+    /// }
+    /// ```
+    pub ZERO_SIZED_MAP_VALUES,
+    pedantic,
+    "usage of map with zero-sized value type"
+}
+
+declare_lint_pass!(ZeroSizedMapValues => [ZERO_SIZED_MAP_VALUES]);
+
+impl LateLintPass<'_> for ZeroSizedMapValues {
+    fn check_ty(&mut self, cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>) {
+        if_chain! {
+            if !hir_ty.span.from_expansion();
+            if !in_trait_impl(cx, hir_ty.hir_id);
+            let ty = ty_from_hir_ty(cx, hir_ty);
+            if is_type_diagnostic_item(cx, ty, sym!(hashmap_type)) || match_type(cx, ty, &paths::BTREEMAP);
+            if let Adt(_, ref substs) = ty.kind();
+            let ty = substs.type_at(1);
+            if let Ok(layout) = cx.layout_of(ty);
+            if layout.is_zst();
+            then {
+                span_lint_and_help(cx, ZERO_SIZED_MAP_VALUES, hir_ty.span, "map with zero-sized value type", None, "consider using a set instead");
+            }
+        }
+    }
+}
+
+fn in_trait_impl(cx: &LateContext<'_>, hir_id: HirId) -> bool {
+    let parent_id = cx.tcx.hir().get_parent_item(hir_id);
+    if let Some(Node::Item(item)) = cx.tcx.hir().find(cx.tcx.hir().get_parent_item(parent_id)) {
+        if let ItemKind::Impl { of_trait: Some(_), .. } = item.kind {
+            return true;
+        }
+    }
+    false
+}
+
+fn ty_from_hir_ty<'tcx>(cx: &LateContext<'tcx>, hir_ty: &hir::Ty<'_>) -> Ty<'tcx> {
+    cx.maybe_typeck_results()
+        .and_then(|results| {
+            if results.hir_owner == hir_ty.hir_id.owner {
+                results.node_type_opt(hir_ty.hir_id)
+            } else {
+                None
+            }
+        })
+        .unwrap_or_else(|| hir_ty_to_ty(cx.tcx, hir_ty))
+}

tests/ui/zero_sized_btreemap_values.rs

@@ -0,0 +1,68 @@
+#![warn(clippy::zero_sized_map_values)]
+use std::collections::BTreeMap;
+
+const CONST_OK: Option<BTreeMap<String, usize>> = None;
+const CONST_NOT_OK: Option<BTreeMap<String, ()>> = None;
+
+static STATIC_OK: Option<BTreeMap<String, usize>> = None;
+static STATIC_NOT_OK: Option<BTreeMap<String, ()>> = None;
+
+type OkMap = BTreeMap<String, usize>;
+type NotOkMap = BTreeMap<String, ()>;
+
+enum TestEnum {
+    Ok(BTreeMap<String, usize>),
+    NotOk(BTreeMap<String, ()>),
+}
+
+struct Test {
+    ok: BTreeMap<String, usize>,
+    not_ok: BTreeMap<String, ()>,
+
+    also_not_ok: Vec<BTreeMap<usize, ()>>,
+}
+
+trait TestTrait {
+    type Output;
+
+    fn produce_output() -> Self::Output;
+
+    fn weird_map(&self, map: BTreeMap<usize, ()>);
+}
+
+impl Test {
+    fn ok(&self) -> BTreeMap<String, usize> {
+        todo!()
+    }
+
+    fn not_ok(&self) -> BTreeMap<String, ()> {
+        todo!()
+    }
+}
+
+impl TestTrait for Test {
+    type Output = BTreeMap<String, ()>;
+
+    fn produce_output() -> Self::Output {
+        todo!();
+    }
+
+    fn weird_map(&self, map: BTreeMap<usize, ()>) {
+        todo!();
+    }
+}
+
+fn test(map: BTreeMap<String, ()>, key: &str) -> BTreeMap<String, ()> {
+    todo!();
+}
+
+fn test2(map: BTreeMap<String, usize>, key: &str) -> BTreeMap<String, usize> {
+    todo!();
+}
+
+fn main() {
+    let _: BTreeMap<String, ()> = BTreeMap::new();
+    let _: BTreeMap<String, usize> = BTreeMap::new();
+
+    let _: BTreeMap<_, _> = std::iter::empty::<(String, ())>().collect();
+}

tests/ui/zero_sized_btreemap_values.stderr

@@ -0,0 +1,107 @@
+error: map with zero-sized value type
+  --> $DIR/zero_sized_btreemap_values.rs:5:28
+   |
+LL | const CONST_NOT_OK: Option<BTreeMap<String, ()>> = None;
+   |                            ^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::zero-sized-map-values` implied by `-D warnings`
+   = help: consider using a set instead
+
+error: map with zero-sized value type
+  --> $DIR/zero_sized_btreemap_values.rs:8:30
+   |
+LL | static STATIC_NOT_OK: Option<BTreeMap<String, ()>> = None;
+   |                              ^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using a set instead
+
+error: map with zero-sized value type
+  --> $DIR/zero_sized_btreemap_values.rs:11:17
+   |
+LL | type NotOkMap = BTreeMap<String, ()>;
+   |                 ^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using a set instead
+
+error: map with zero-sized value type
+  --> $DIR/zero_sized_btreemap_values.rs:15:11
+   |
+LL |     NotOk(BTreeMap<String, ()>),
+   |           ^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using a set instead
+
+error: map with zero-sized value type
+  --> $DIR/zero_sized_btreemap_values.rs:20:13
+   |
+LL |     not_ok: BTreeMap<String, ()>,
+   |             ^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using a set instead
+
+error: map with zero-sized value type
+  --> $DIR/zero_sized_btreemap_values.rs:22:22
+   |
+LL |     also_not_ok: Vec<BTreeMap<usize, ()>>,
+   |                      ^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using a set instead
+
+error: map with zero-sized value type
+  --> $DIR/zero_sized_btreemap_values.rs:30:30
+   |
+LL |     fn weird_map(&self, map: BTreeMap<usize, ()>);
+   |                              ^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using a set instead
+
+error: map with zero-sized value type
+  --> $DIR/zero_sized_btreemap_values.rs:38:25
+   |
+LL |     fn not_ok(&self) -> BTreeMap<String, ()> {
+   |                         ^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using a set instead
+
+error: map with zero-sized value type
+  --> $DIR/zero_sized_btreemap_values.rs:55:14
+   |
+LL | fn test(map: BTreeMap<String, ()>, key: &str) -> BTreeMap<String, ()> {
+   |              ^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using a set instead
+
+error: map with zero-sized value type
+  --> $DIR/zero_sized_btreemap_values.rs:55:50
+   |
+LL | fn test(map: BTreeMap<String, ()>, key: &str) -> BTreeMap<String, ()> {
+   |                                                  ^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using a set instead
+
+error: map with zero-sized value type
+  --> $DIR/zero_sized_btreemap_values.rs:64:35
+   |
+LL |     let _: BTreeMap<String, ()> = BTreeMap::new();
+   |                                   ^^^^^^^^^^^^^
+   |
+   = help: consider using a set instead
+
+error: map with zero-sized value type
+  --> $DIR/zero_sized_btreemap_values.rs:64:12
+   |
+LL |     let _: BTreeMap<String, ()> = BTreeMap::new();
+   |            ^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using a set instead
+
+error: map with zero-sized value type
+  --> $DIR/zero_sized_btreemap_values.rs:67:12
+   |
+LL |     let _: BTreeMap<_, _> = std::iter::empty::<(String, ())>().collect();
+   |            ^^^^^^^^^^^^^^
+   |
+   = help: consider using a set instead
+
+error: aborting due to 13 previous errors
+

tests/ui/zero_sized_hashmap_values.rs

@@ -0,0 +1,68 @@
+#![warn(clippy::zero_sized_map_values)]
+use std::collections::HashMap;
+
+const CONST_OK: Option<HashMap<String, usize>> = None;
+const CONST_NOT_OK: Option<HashMap<String, ()>> = None;
+
+static STATIC_OK: Option<HashMap<String, usize>> = None;
+static STATIC_NOT_OK: Option<HashMap<String, ()>> = None;
+
+type OkMap = HashMap<String, usize>;
+type NotOkMap = HashMap<String, ()>;
+
+enum TestEnum {
+    Ok(HashMap<String, usize>),
+    NotOk(HashMap<String, ()>),
+}
+
+struct Test {
+    ok: HashMap<String, usize>,
+    not_ok: HashMap<String, ()>,
+
+    also_not_ok: Vec<HashMap<usize, ()>>,
+}
+
+trait TestTrait {
+    type Output;
+
+    fn produce_output() -> Self::Output;
+
+    fn weird_map(&self, map: HashMap<usize, ()>);
+}
+
+impl Test {
+    fn ok(&self) -> HashMap<String, usize> {
+        todo!()
+    }
+
+    fn not_ok(&self) -> HashMap<String, ()> {
+        todo!()
+    }
+}
+
+impl TestTrait for Test {
+    type Output = HashMap<String, ()>;
+
+    fn produce_output() -> Self::Output {
+        todo!();
+    }
+
+    fn weird_map(&self, map: HashMap<usize, ()>) {
+        todo!();
+    }
+}
+
+fn test(map: HashMap<String, ()>, key: &str) -> HashMap<String, ()> {
+    todo!();
+}
+
+fn test2(map: HashMap<String, usize>, key: &str) -> HashMap<String, usize> {
+    todo!();
+}
+
+fn main() {
+    let _: HashMap<String, ()> = HashMap::new();
+    let _: HashMap<String, usize> = HashMap::new();
+
+    let _: HashMap<_, _> = std::iter::empty::<(String, ())>().collect();
+}