Derive Hash instead of implementing it by hand

The caching mechanism of the CLI (ruff_cli::cache) relies on ruff::settings::Settings implementing the Hash trait. The ruff::settings::Settings struct previously couldn't automatically derive the Hash implementation via the #[derive(Hash)] macro attribute since some of its field types intentionally[1][2] don't implement Hash (namely regex::Regex, globset::GlobMatcher and globset::GlobSet and HashMap and HashSet from the standard library). The code therefore previously implemented the Hash trait by hand for the whole struct. Implementing Hash by hand for structs that are subject to change is a bad idea since it's very easy to forget to update the Hash implementation when adding a new field to the struct. And the Hash implementation indeed was already incorrect by omitting several fields from the hash. This commit introduces wrapper types for Regex, GlobMatcher, GlobSet, HashSet & HashMap that implement Hash so that we can still add #[derive(Hash)] to the Settings struct, guaranteeing a correct hash implementation. [1]: rust-lang/regex#364 (comment) [2]: The standard library doesn't impl<T: Hash + Ord> Hash for HashSet<T> presumably since sorted() requires an allocation and Hash implementations are generally expected to work without allocations.
astral-sh · Jan 16, 2023 · 41c5d8a · 41c5d8a
1 parent 3a3a5fc
commit 41c5d8a
Show file tree

Hide file tree

Showing 12 changed files with 298 additions and 162 deletions.
diff --git a/src/fs.rs b/src/fs.rs
@@ -4,11 +4,11 @@ use std::io::{BufReader, Read};
 use std::path::{Path, PathBuf};
 
 use anyhow::{anyhow, Result};
-use globset::GlobMatcher;
 use path_absolutize::{path_dedot, Absolutize};
 use rustc_hash::FxHashSet;
 
 use crate::registry::RuleCode;
+use crate::settings::hashable::{HashableGlobMatcher, HashableHashSet};
 
 /// Extract the absolute path and basename (as strings) from a Path.
 pub fn extract_path_names(path: &Path) -> Result<(&str, &str)> {
@@ -26,15 +26,19 @@ pub fn extract_path_names(path: &Path) -> Result<(&str, &str)> {
 /// Create a set with codes matching the pattern/code pairs.
 pub(crate) fn ignores_from_path<'a>(
     path: &Path,
-    pattern_code_pairs: &'a [(GlobMatcher, GlobMatcher, FxHashSet<RuleCode>)],
+    pattern_code_pairs: &'a [(
+        HashableGlobMatcher,
+        HashableGlobMatcher,
+        HashableHashSet<RuleCode>,
+    )],
 ) -> Result<FxHashSet<&'a RuleCode>> {
     let (file_path, file_basename) = extract_path_names(path)?;
     Ok(pattern_code_pairs
         .iter()
         .filter(|(absolute, basename, _)| {
             basename.is_match(file_basename) || absolute.is_match(file_path)
         })
-        .flat_map(|(_, _, codes)| codes)
+        .flat_map(|(_, _, codes)| codes.iter())
         .collect())
 }
 

diff --git a/src/noqa.rs b/src/noqa.rs
@@ -9,6 +9,7 @@ use regex::Regex;
 use rustc_hash::{FxHashMap, FxHashSet};
 
 use crate::registry::{Diagnostic, RuleCode, CODE_REDIRECTS};
+use crate::settings::hashable::HashableHashSet;
 use crate::source_code::LineEnding;
 
 static NOQA_LINE_REGEX: Lazy<Regex> = Lazy::new(|| {
@@ -84,7 +85,7 @@ pub fn add_noqa(
     diagnostics: &[Diagnostic],
     contents: &str,
     noqa_line_for: &IntMap<usize, usize>,
-    external: &FxHashSet<String>,
+    external: &HashableHashSet<String>,
     line_ending: &LineEnding,
 ) -> Result<usize> {
     let (count, output) =
@@ -97,7 +98,7 @@ fn add_noqa_inner(
     diagnostics: &[Diagnostic],
     contents: &str,
     noqa_line_for: &IntMap<usize, usize>,
-    external: &FxHashSet<String>,
+    external: &HashableHashSet<String>,
     line_ending: &LineEnding,
 ) -> (usize, String) {
     let mut matches_by_line: FxHashMap<usize, FxHashSet<&RuleCode>> = FxHashMap::default();
@@ -208,12 +209,12 @@ fn add_noqa_inner(
 #[cfg(test)]
 mod tests {
     use nohash_hasher::IntMap;
-    use rustc_hash::FxHashSet;
     use rustpython_parser::ast::Location;
 
     use crate::ast::types::Range;
     use crate::noqa::{add_noqa_inner, NOQA_LINE_REGEX};
     use crate::registry::Diagnostic;
+    use crate::settings::hashable::HashableHashSet;
     use crate::source_code::LineEnding;
     use crate::violations;
 
@@ -236,7 +237,7 @@ mod tests {
         let diagnostics = vec![];
         let contents = "x = 1";
         let noqa_line_for = IntMap::default();
-        let external = FxHashSet::default();
+        let external = HashableHashSet::default();
         let (count, output) = add_noqa_inner(
             &diagnostics,
             contents,
@@ -253,7 +254,7 @@ mod tests {
         )];
         let contents = "x = 1";
         let noqa_line_for = IntMap::default();
-        let external = FxHashSet::default();
+        let external = HashableHashSet::default();
         let (count, output) = add_noqa_inner(
             &diagnostics,
             contents,
@@ -276,7 +277,7 @@ mod tests {
         ];
         let contents = "x = 1  # noqa: E741\n";
         let noqa_line_for = IntMap::default();
-        let external = FxHashSet::default();
+        let external = HashableHashSet::default();
         let (count, output) = add_noqa_inner(
             &diagnostics,
             contents,
@@ -299,7 +300,7 @@ mod tests {
         ];
         let contents = "x = 1  # noqa";
         let noqa_line_for = IntMap::default();
-        let external = FxHashSet::default();
+        let external = HashableHashSet::default();
         let (count, output) = add_noqa_inner(
             &diagnostics,
             contents,

diff --git a/src/rules/flake8_import_conventions/mod.rs b/src/rules/flake8_import_conventions/mod.rs
@@ -29,10 +29,13 @@ mod tests {
             &Settings {
                 flake8_import_conventions: super::settings::Options {
                     aliases: None,
-                    extend_aliases: Some(FxHashMap::from_iter([
-                        ("dask.array".to_string(), "da".to_string()),
-                        ("dask.dataframe".to_string(), "dd".to_string()),
-                    ])),
+                    extend_aliases: Some(
+                        FxHashMap::from_iter([
+                            ("dask.array".to_string(), "da".to_string()),
+                            ("dask.dataframe".to_string(), "dd".to_string()),
+                        ])
+                        .into(),
+                    ),
                 }
                 .into(),
                 ..Settings::for_rule(RuleCode::ICN001)
@@ -48,12 +51,15 @@ mod tests {
             Path::new("./resources/test/fixtures/flake8_import_conventions/remove_default.py"),
             &Settings {
                 flake8_import_conventions: super::settings::Options {
-                    aliases: Some(FxHashMap::from_iter([
-                        ("altair".to_string(), "alt".to_string()),
-                        ("matplotlib.pyplot".to_string(), "plt".to_string()),
-                        ("pandas".to_string(), "pd".to_string()),
-                        ("seaborn".to_string(), "sns".to_string()),
-                    ])),
+                    aliases: Some(
+                        FxHashMap::from_iter([
+                            ("altair".to_string(), "alt".to_string()),
+                            ("matplotlib.pyplot".to_string(), "plt".to_string()),
+                            ("pandas".to_string(), "pd".to_string()),
+                            ("seaborn".to_string(), "sns".to_string()),
+                        ])
+                        .into(),
+                    ),
                     extend_aliases: None,
                 }
                 .into(),
@@ -71,10 +77,9 @@ mod tests {
             &Settings {
                 flake8_import_conventions: super::settings::Options {
                     aliases: None,
-                    extend_aliases: Some(FxHashMap::from_iter([(
-                        "numpy".to_string(),
-                        "nmp".to_string(),
-                    )])),
+                    extend_aliases: Some(
+                        FxHashMap::from_iter([("numpy".to_string(), "nmp".to_string())]).into(),
+                    ),
                 }
                 .into(),
                 ..Settings::for_rule(RuleCode::ICN001)

diff --git a/src/rules/flake8_import_conventions/settings.rs b/src/rules/flake8_import_conventions/settings.rs
@@ -1,13 +1,14 @@
 //! Settings for import conventions.
 
-use std::hash::{Hash, Hasher};
+use std::hash::Hash;
 
-use itertools::Itertools;
 use ruff_macros::ConfigurationOptions;
 use rustc_hash::FxHashMap;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 
+use crate::settings::hashable::HashableHashMap;
+
 const CONVENTIONAL_ALIASES: &[(&str, &str)] = &[
     ("altair", "alt"),
     ("matplotlib.pyplot", "plt"),
@@ -55,17 +56,9 @@ pub struct Options {
     pub extend_aliases: Option<FxHashMap<String, String>>,
 }
 
-#[derive(Debug)]
+#[derive(Debug, Hash)]
 pub struct Settings {
-    pub aliases: FxHashMap<String, String>,
-}
-
-impl Hash for Settings {
-    fn hash<H: Hasher>(&self, state: &mut H) {
-        for value in self.aliases.iter().sorted() {
-            value.hash(state);
-        }
-    }
+    pub aliases: HashableHashMap<String, String>,
 }
 
 fn default_aliases() -> FxHashMap<String, String> {
@@ -89,23 +82,23 @@ fn resolve_aliases(options: Options) -> FxHashMap<String, String> {
 impl Default for Settings {
     fn default() -> Self {
         Self {
-            aliases: default_aliases(),
+            aliases: default_aliases().into(),
         }
     }
 }
 
 impl From<Options> for Settings {
     fn from(options: Options) -> Self {
         Self {
-            aliases: resolve_aliases(options),
+            aliases: resolve_aliases(options).into(),
         }
     }
 }
 
 impl From<Settings> for Options {
     fn from(settings: Settings) -> Self {
         Self {
-            aliases: Some(settings.aliases),
+            aliases: Some(settings.aliases.into()),
             extend_aliases: None,
         }
     }

diff --git a/src/rules/flake8_tidy_imports/mod.rs b/src/rules/flake8_tidy_imports/mod.rs
@@ -64,7 +64,8 @@ mod tests {
                                 msg: "Use typing_extensions.TypedDict instead.".to_string(),
                             },
                         ),
-                    ]),
+                    ])
+                    .into(),
                     ..Default::default()
                 },
                 ..Settings::for_rules(vec![RuleCode::TID251])

diff --git a/src/rules/flake8_tidy_imports/rules.rs b/src/rules/flake8_tidy_imports/rules.rs
@@ -1,10 +1,10 @@
-use rustc_hash::FxHashMap;
 use rustpython_ast::{Alias, Expr, Located, Stmt};
 
 use super::settings::{BannedApi, Strictness};
 use crate::ast::types::Range;
 use crate::checkers::ast::Checker;
 use crate::registry::Diagnostic;
+use crate::settings::hashable::HashableHashMap;
 use crate::violations;
 
 /// TID252
@@ -31,7 +31,7 @@ pub fn banned_relative_import(
 pub fn name_is_banned(
     module: &str,
     name: &Alias,
-    banned_apis: &FxHashMap<String, BannedApi>,
+    banned_apis: &HashableHashMap<String, BannedApi>,
 ) -> Option<Diagnostic> {
     let full_name = format!("{module}.{}", &name.node.name);
     if let Some(ban) = banned_apis.get(&full_name) {
@@ -50,7 +50,7 @@ pub fn name_is_banned(
 pub fn name_or_parent_is_banned<T>(
     located: &Located<T>,
     name: &str,
-    banned_apis: &FxHashMap<String, BannedApi>,
+    banned_apis: &HashableHashMap<String, BannedApi>,
 ) -> Option<Diagnostic> {
     let mut name = name;
     loop {
@@ -75,7 +75,7 @@ pub fn name_or_parent_is_banned<T>(
 /// TID251
 pub fn banned_attribute_access(checker: &mut Checker, expr: &Expr) {
     if let Some(call_path) = checker.resolve_call_path(expr) {
-        for (banned_path, ban) in &checker.settings.flake8_tidy_imports.banned_api {
+        for (banned_path, ban) in checker.settings.flake8_tidy_imports.banned_api.iter() {
             if call_path == banned_path.split('.').collect::<Vec<_>>() {
                 checker.diagnostics.push(Diagnostic::new(
                     violations::BannedApi {

diff --git a/src/rules/flake8_tidy_imports/settings.rs b/src/rules/flake8_tidy_imports/settings.rs
@@ -1,13 +1,14 @@
 //! Settings for the `flake8-tidy-imports` plugin.
 
-use std::hash::{Hash, Hasher};
+use std::hash::Hash;
 
-use itertools::Itertools;
 use ruff_macros::ConfigurationOptions;
 use rustc_hash::FxHashMap;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 
+use crate::settings::hashable::HashableHashMap;
+
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Hash, JsonSchema)]
 #[serde(deny_unknown_fields, rename_all = "kebab-case")]
 pub enum Strictness {
@@ -59,17 +60,17 @@ pub struct Options {
     pub banned_api: Option<FxHashMap<String, BannedApi>>,
 }
 
-#[derive(Debug)]
+#[derive(Debug, Hash)]
 pub struct Settings {
     pub ban_relative_imports: Strictness,
-    pub banned_api: FxHashMap<String, BannedApi>,
+    pub banned_api: HashableHashMap<String, BannedApi>,
 }
 
 impl Default for Settings {
     fn default() -> Self {
         Self {
             ban_relative_imports: Strictness::Parents,
-            banned_api: FxHashMap::default(),
+            banned_api: HashableHashMap::default(),
         }
     }
 }
@@ -78,7 +79,7 @@ impl From<Options> for Settings {
     fn from(options: Options) -> Self {
         Self {
             ban_relative_imports: options.ban_relative_imports.unwrap_or(Strictness::Parents),
-            banned_api: options.banned_api.unwrap_or_default(),
+            banned_api: options.banned_api.unwrap_or_default().into(),
         }
     }
 }
@@ -87,17 +88,7 @@ impl From<Settings> for Options {
     fn from(settings: Settings) -> Self {
         Self {
             ban_relative_imports: Some(settings.ban_relative_imports),
-            banned_api: Some(settings.banned_api),
-        }
-    }
-}
-
-impl Hash for Settings {
-    fn hash<H: Hasher>(&self, state: &mut H) {
-        self.ban_relative_imports.hash(state);
-        for key in self.banned_api.keys().sorted() {
-            key.hash(state);
-            self.banned_api[key].hash(state);
+            banned_api: Some(settings.banned_api.into()),
         }
     }
 }
diff --git a/src/rules/pyflakes/mod.rs b/src/rules/pyflakes/mod.rs
@@ -121,7 +121,7 @@ mod tests {
         let diagnostics = test_path(
             Path::new("./resources/test/fixtures/pyflakes/F841_0.py"),
             &settings::Settings {
-                dummy_variable_rgx: Regex::new(r"^z$").unwrap(),
+                dummy_variable_rgx: Regex::new(r"^z$").unwrap().into(),
                 ..settings::Settings::for_rule(RuleCode::F841)
             },
         )?;

diff --git a/src/rules/ruff/mod.rs b/src/rules/ruff/mod.rs
@@ -31,7 +31,7 @@ mod tests {
         let diagnostics = test_path(
             Path::new("./resources/test/fixtures/ruff/confusables.py"),
             &settings::Settings {
-                allowed_confusables: FxHashSet::from_iter(['−', 'ρ', '∗']),
+                allowed_confusables: FxHashSet::from_iter(['−', 'ρ', '∗']).into(),
                 ..settings::Settings::for_rules(vec![
                     RuleCode::RUF001,
                     RuleCode::RUF002,