Add an internal lint for FxHashMap/FxHashSet #3026

dwijnand · 2018-08-10T17:59:43Z

My first lint, so feedback and guidance welcome! Thank you.

llogiq · 2018-08-10T19:30:26Z

clippy_lints/src/utils/internal_lints.rs

+}
+
+impl EarlyLintPass for PreferFxHash {
+    fn check_ident(&mut self, cx: &EarlyContext, ident: Ident) {


This seems to be the first EarlyLintPass Herr, so you want to use syntax::ast::*;.

llogiq · 2018-08-10T19:32:29Z

clippy_lints/src/utils/internal_lints.rs

+
+impl EarlyLintPass for PreferFxHash {
+    fn check_ident(&mut self, cx: &EarlyContext, ident: Ident) {
+        if ident.to_string().contains("HashMap") {


if is an expression in Rust, so you can just use it to decide on the message without all the other code duplicated.

mati865 · 2018-08-10T23:49:22Z

clippy_lints/src/utils/internal_lints.rs

+
+impl EarlyLintPass for PreferFxHash {
+    fn check_ident(&mut self, cx: &EarlyContext<'_>, ident: Ident) {
+        if let Some(prefer) = {


Unused variable prefer?

llogiq · 2018-08-11T06:26:11Z

clippy_lints/src/utils/internal_lints.rs

+
+impl EarlyLintPass for PreferFxHash {
+    fn check_ident(&mut self, cx: &EarlyContext<'_>, ident: Ident) {
+        if let Some(msg) = {


You can get rid of the if let and the Some(_) if you use an early return in the else case.

Like: let msg = if .. { .. } else if .. { .. } else { return; }

Oh neat. Will do.

ghost · 2018-08-11T06:46:39Z

The name should describe the flaw the lint is catching so that it makes sense when you allow/warn/deny it. #[allow(prefer_fx_hash)] does the opposite of how it reads. This lint should be called something like plain_hash_sets or something like that. Naming is hard. See #2845.

ghost · 2018-08-11T07:41:04Z

clippy_lints/src/utils/internal_lints.rs

+impl EarlyLintPass for PreferFxHash {
+    fn check_ident(&mut self, cx: &EarlyContext<'_>, ident: Ident) {
+        if let Some(msg) = {
+            if ident.to_string().contains("HashMap") {


Won't this lint FxHashMap as well? Shouldn't it be == instead of contains? 🤔

Yeah you're right. I don't know Ident, so I'm looking for help on this one..

Went the easy way..

dwijnand · 2018-08-11T08:04:44Z

Thanks @mikerite for the feedback. Pushed my attempts to resolving them.

flip1995 · 2018-08-11T08:22:28Z

clippy_lints/src/utils/internal_lints.rs

+
+impl LintPass for DefaultHashTypes {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(PREFER_FX_HASH)


Still the old lint name here

flip1995 · 2018-08-11T08:29:34Z

clippy_lints/src/utils/internal_lints.rs

+    fn check_ident(&mut self, cx: &EarlyContext<'_>, ident: Ident) {
+        let msg = {
+            let ident: String = ident.to_string();
+            if ident.contains("HashMap") && !ident.contains("FxHashMap") {


Another solution to consider for this would be to have a FxHashMap with the keys being the disallowed idents (strings) and the value the wanted replacement.

Then you can just

let msg = if let Some(replace) = map.get(ident_string) { format!("Prefer {} over {}", replace, ident_string); } else { return }

This would also be easy to extend, if needed.

What's the to_string of an Ident that .contains("HashMap")? Is it "HashMap"? Or is it something more fully-qualified?

It should just be "HashMap" .

dwijnand · 2018-08-11T19:05:26Z

Even nicer, @flip1995. Thank you!

flip1995 · 2018-08-11T19:26:35Z

clippy_lints/src/utils/internal_lints.rs

+
+impl EarlyLintPass for DefaultHashTypes {
+    fn check_ident(&mut self, cx: &EarlyContext<'_>, ident: Ident) {
+        let mut map = HashMap::default();


You should do this in a new() function on creating the Lint. Doing this here will create a new HashMap everytime you check an ident. This happens pretty often and is bad for the performance.

Oh and you're using HashMap instead of FxHashMap. 😄

flip1995 · 2018-08-11T22:23:22Z

LGTM! Could you add a few test cases with #[warn(default_hash_types]?

dwijnand · 2018-08-12T06:59:46Z

Sure, I'll have to look into how to do that.

flip1995 · 2018-08-12T14:10:41Z

Thanks! Waiting for the new nightly and a CI rerun. Could you squash some of the commits?

flip1995 · 2018-08-12T14:17:03Z

Just tested it locally:
You have to update the *.stderr file after afe684c:

stderr diff

 error: Prefer FxHashMap over HashMap, it has better performance and we don't need any collision prevention in clippy
- --> $DIR/fxhash.rs:7:24
+ --> $DIR/fxhash.rs:6:24
   |
-7 | use std::collections::{HashMap, HashSet};
+6 | use std::collections::{HashMap, HashSet};
   |                        ^^^^^^^
   |
   = note: `-D default-hash-types` implied by `-D warnings`

 error: Prefer FxHashSet over HashSet, it has better performance and we don't need any collision prevention in clippy
- --> $DIR/fxhash.rs:7:33
+ --> $DIR/fxhash.rs:6:33
   |
-7 | use std::collections::{HashMap, HashSet};
+6 | use std::collections::{HashMap, HashSet};
   |                                 ^^^^^^^

 error: Prefer FxHashMap over HashMap, it has better performance and we don't need any collision prevention in clippy
-  --> $DIR/fxhash.rs:12:15
+  --> $DIR/fxhash.rs:10:15
    |
-12 |     let _map: HashMap<String, String> = HashMap::default();
+10 |     let _map: HashMap<String, String> = HashMap::default();
    |               ^^^^^^^

 error: Prefer FxHashMap over HashMap, it has better performance and we don't need any collision prevention in clippy
-  --> $DIR/fxhash.rs:12:41
+  --> $DIR/fxhash.rs:10:41
    |
-12 |     let _map: HashMap<String, String> = HashMap::default();
+10 |     let _map: HashMap<String, String> = HashMap::default();
    |                                         ^^^^^^^

 error: Prefer FxHashSet over HashSet, it has better performance and we don't need any collision prevention in clippy
-  --> $DIR/fxhash.rs:13:15
+  --> $DIR/fxhash.rs:11:15
    |
-13 |     let _set: HashSet<String> = HashSet::default();
+11 |     let _set: HashSet<String> = HashSet::default();
    |               ^^^^^^^

 error: Prefer FxHashSet over HashSet, it has better performance and we don't need any collision prevention in clippy
-  --> $DIR/fxhash.rs:13:33
+  --> $DIR/fxhash.rs:11:33
    |
-13 |     let _set: HashSet<String> = HashSet::default();
+11 |     let _set: HashSet<String> = HashSet::default();
    |                                 ^^^^^^^

 error: aborting due to 6 previous errors

flip1995 · 2018-08-12T23:09:00Z

clippy_lints/src/utils/internal_lints.rs

+        let ident_string = ident.to_string();
+        if let Some(replace) = self.map.get(&ident_string) {
+            let msg = format!("Prefer {} over {}, it has better performance and we don't need any collision prevention in clippy", replace, ident_string);
+            cx.span_lint(DEFAULT_HASH_TYPES, ident.span, &msg);


I think using the utils::span_lint_and_sugg function here would be even nicer.
https://github.com/rust-lang-nursery/rust-clippy/blob/e6d92f95c6d6964628d75c25fb3d1a58a9f25e5d/clippy_lints/src/utils/mod.rs#L580-L587
help: "use"
sugg: replace

You should always use the span_lint functions from the utils module for Clippy lints, because they add the link to the doc to the warning messages.

Sorry I missed that in the previous review.

flip1995 · 2018-08-13T10:42:01Z

stderr will need an update

dwijnand · 2018-08-13T10:50:44Z

Thanks for all the review comments @flip1995: it's been silly mistake after silly mistake from me. 🙂

flip1995

Always happy to help!

llogiq reviewed Aug 10, 2018

View reviewed changes

mati865 reviewed Aug 10, 2018

View reviewed changes

llogiq reviewed Aug 11, 2018

View reviewed changes

ghost reviewed Aug 11, 2018

View reviewed changes

flip1995 reviewed Aug 11, 2018

View reviewed changes

dwijnand changed the title ~~Add an internal PreferFxHash lint~~ Add an internal lint for FxHashMap/FxHashSet Aug 11, 2018

flip1995 reviewed Aug 11, 2018

View reviewed changes

Add an internal lint for FxHashMap/FxHashSet

8fc425b

flip1995 reviewed Aug 12, 2018

View reviewed changes

Use utils::span_lint_and_sugg in default_hash_types

1812707

flip1995 approved these changes Aug 13, 2018

View reviewed changes

flip1995 closed this Aug 14, 2018

flip1995 reopened this Aug 14, 2018

Avoid new_without_default_derive in DefaultHashTypes

22ff5a3

flip1995 merged commit 84aa499 into rust-lang:master Aug 14, 2018

dwijnand deleted the fxhash branch August 14, 2018 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an internal lint for FxHashMap/FxHashSet #3026

Add an internal lint for FxHashMap/FxHashSet #3026

dwijnand commented Aug 10, 2018

llogiq Aug 10, 2018

llogiq Aug 10, 2018

mati865 Aug 10, 2018

llogiq Aug 11, 2018 •

edited

Loading

dwijnand Aug 11, 2018

ghost commented Aug 11, 2018

ghost Aug 11, 2018

dwijnand Aug 11, 2018

dwijnand Aug 11, 2018

dwijnand commented Aug 11, 2018

flip1995 Aug 11, 2018

flip1995 Aug 11, 2018 •

edited

Loading

dwijnand Aug 11, 2018

flip1995 Aug 11, 2018

dwijnand commented Aug 11, 2018

flip1995 Aug 11, 2018

flip1995 Aug 11, 2018

flip1995 commented Aug 11, 2018

dwijnand commented Aug 12, 2018

flip1995 commented Aug 12, 2018

flip1995 commented Aug 12, 2018

flip1995 Aug 12, 2018

flip1995 commented Aug 13, 2018

dwijnand commented Aug 13, 2018

flip1995 left a comment

Add an internal lint for FxHashMap/FxHashSet #3026

Add an internal lint for FxHashMap/FxHashSet #3026

Conversation

dwijnand commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llogiq Aug 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwijnand commented Aug 11, 2018

Choose a reason for hiding this comment

flip1995 Aug 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwijnand commented Aug 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flip1995 commented Aug 11, 2018

dwijnand commented Aug 12, 2018

flip1995 commented Aug 12, 2018

flip1995 commented Aug 12, 2018

Choose a reason for hiding this comment

flip1995 commented Aug 13, 2018

dwijnand commented Aug 13, 2018

flip1995 left a comment

Choose a reason for hiding this comment

llogiq Aug 11, 2018 •

edited

Loading

flip1995 Aug 11, 2018 •

edited

Loading