Implicit hasher lint #2120

sinkuu · 2017-10-09T10:12:50Z

Looks for HashMap<K, V> (without 3rd arg.) and HashSet<T> (without 2nd arg.) in impl Trait for <here> and function arguments.
Suggests to add hasher argument and trait bound BuildHasher. Plus Default if non-generic constructor found.
Looks for non-generic constructors (which can be used only with the default hasher) like HashMap::new or HashMap::with_capacity, and suggests to replace them with generic alternatives.

oli-obk

I ❤️ this lint a lot! I'd have no qualms merging as is, so if you think any changes I suggested are too involved, feel free to leave them to be fixed in the future.

oli-obk · 2017-10-10T08:15:40Z

clippy_lints/src/types.rs

+/// used with them.
+///
+/// **Known problems:** Suggestions for replacing constructors are not always
+/// accurate.


In what way are they inaccurate? Something a human can trivially fix? Or might there be cases that can't be resolved at all due to constraints by external crates?

My thought was that there can be a HashMap which has the same types as Self but not exposed to outside.

impl Foo for HashMap<K, V> { fn method(&self) { let used_internally = HashMap::<K, V>::new(); // suggestion for this can be unwanted } }

But yes constraints by external crates are more problematic.

oli-obk · 2017-10-10T08:22:35Z

clippy_lints/src/types.rs

+                let generics_span = generics.span.substitute_dummy({
+                    let pos = snippet_opt(cx, item.span.until(target.span()))
+                        .and_then(|snip| {
+                            Some(item.span.lo() + ::syntax_pos::BytePos(snip.find("impl")? as u32 + 4))


This scares me a little, can you add some tests where the entire thing is produced by macros, just the type is and some cases in between?

oli-obk · 2017-10-10T08:24:04Z

clippy_lints/src/types.rs

+
+            for target in vis.found {
+                let generics_snip = snippet(cx, generics.span, "");
+                let generics_snip_trimmed = if generics_snip.len() == 0 {


you can skip this check if you make the last argument of snippet be "<>"

snippet(cx, generics.span, "<>") seems to return an empty string when there is no generics.

oh... right... it's not when the span is missing, it's when the generics are empty. Don't mind me then.

oli-obk · 2017-10-10T08:25:19Z

clippy_lints/src/types.rs

+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImplicitHasher {
+    #[allow(cast_possible_truncation)]
+    fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx Item) {
+        if let ItemImpl(_, _, _, ref generics, _, ref ty, ref items) = item.node {


Can you make this a match on item.node, so both the arms are in one match? It'll probably be easier if you move the arm bodies into their own methods, too.

oli-obk · 2017-10-10T08:25:57Z

clippy_lints/src/types.rs

+
+                for target in vis.found {
+                    let generics_snip = snippet(cx, generics.span, "");
+                    let generics_snip_trimmed = if generics_snip.len() == 0 {


lots of repetition between this arm and the other, can you split out the duplicate code into a method?

oli-obk · 2017-10-10T08:28:58Z

clippy_lints/src/types.rs

+/// Looks for default-hasher-dependent constructors like `HashMap::new`.
+struct ImplicitHasherConstructorVisitor<'a, 'tcx: 'a> {
+    cx: &'a LateContext<'a, 'tcx>,
+    body: Option<BodyId>,


You could store the body_tables here instead of the optional body. That way you get rid of the option and save yourself a lot of lookups.

oli-obk · 2017-10-10T08:29:32Z

clippy_lints/src/types.rs

+    fn new(cx: &'a LateContext<'a, 'tcx>, target: ImplicitHasherType<'tcx>) -> Self {
+        Self {
+            cx,
+            body: None,


you'd just use cx.tables here in case you make the body_tables change.

oli-obk · 2017-10-10T08:33:11Z

tests/ui/implicit_hasher.stderr

+   |          ^^^^^^^^^^^^^^^^^^
+help: ...and use generic constructor here
+   |
+17 |         (HashMap::new(), HashMap::with_capacity_and_hasher(10, Default::default()))


You can use https://github.com/rust-lang-nursery/rust-clippy/blob/73a1dd8e7f10f1c6cba618b60e3fed690a72a8d7/clippy_lints/src/utils/mod.rs#L667 to merge all suggestions into one. That should merge this one and the above one into a single one.

sinkuu · 2017-10-11T13:30:32Z

clippy_lints/src/types.rs

+            let params = &path.segments.last().as_ref()?.parameters.as_ref()?.types;
+            let params_len = params.len();
+
+            let ty = cx.tcx.type_of(opt_def_id(path.def)?);


This returns the type of definition (like HashMap<K, V, S>), not what is used in the code.

Can I get the ty::Ty that corresponds to a hir::Ty? Or is there any way to test if a ty::Ty equals to a hir::Ty?

Have you tried cx.tables.node_id_to_ty(hir_ty.hir_id)?

No. I tried now but it panicked (node_id_to_type: no type for node).

rustc_typeck::hir_ty_to_ty seems to work. However its doc page says "quasi-deprecated":(

If it ever gets deprecated, we can move it to the utils module:

// In case there are any projections etc, find the "environment" // def-id that will be used to determine the traits/predicates in // scope. This is derived from the enclosing item-like thing. let env_node_id = tcx.hir.get_parent(hir_ty.id); let env_def_id = tcx.hir.local_def_id(env_node_id); let item_cx = self::collect::ItemCtxt::new(tcx, env_def_id); item_cx.to_ty(hir_ty)

sinkuu · 2017-10-17T12:24:54Z

Oops, I forgot to ping last time. Any suggestions?

oli-obk · 2017-10-17T12:29:47Z

tests/ui/implicit_hasher.stderr

+   |
+11 | impl<K: Hash + Eq, V, S: ::std::hash::BuildHasher + Default> Foo<i8> for HashMap<K, V> {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+help: ...and change the type to


This is still made up of two separate suggestions. I think it'll be less confusing if it were also merged like you did with the constructors.

oli-obk · 2017-10-17T12:30:11Z

Just one more thing ;)

sinkuu · 2017-10-17T13:23:41Z

Passed tests.

oli-obk · 2017-10-17T13:24:40Z

Thanks for the high quality lint!

oli-obk suggested changes Oct 10, 2017

View reviewed changes

sinkuu added 2 commits October 11, 2017 12:10

Add implicit_hasher lint (rust-lang#2101)

159cc84

Apply suggestions

5a61d88

sinkuu force-pushed the implicit_hasher branch from 78f6690 to 5a61d88 Compare October 11, 2017 13:18

sinkuu changed the title ~~Implicit hasher lint~~ [DO NOT MERGE] Implicit hasher lint Oct 11, 2017

sinkuu commented Oct 11, 2017

View reviewed changes

Use rustc_typeck::hir_ty_to_ty

31f16b8

sinkuu changed the title ~~[DO NOT MERGE] Implicit hasher lint~~ Implicit hasher lint Oct 11, 2017

Use cx.access_levels.exported() instead of visibility

a4f45e8

oli-obk reviewed Oct 17, 2017

View reviewed changes

Type parameter change and type change are now in a multispan suggestion

eea3077

oli-obk approved these changes Oct 17, 2017

View reviewed changes

oli-obk merged commit 343e438 into rust-lang:master Oct 17, 2017

Implicit hasher lint #2120

Implicit hasher lint #2120

Uh oh!

Conversation

sinkuu commented Oct 9, 2017

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sinkuu commented Oct 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Oct 17, 2017

Uh oh!

sinkuu commented Oct 17, 2017

Uh oh!

oli-obk commented Oct 17, 2017

Uh oh!

Uh oh!