From c56fb6e15aef4338344070d7d4953ef7c30299fe Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 12 Mar 2024 12:59:38 -0700 Subject: [PATCH] Sort hash maps in `Settings` display (#10370) ## Summary We had a report of a test failure on a specific architecture, and looking into it, I think the test assumes that the hash keys are iterated in a specific order. This PR thus adds a variant to our settings display macro specifically for maps and sets. Like `CacheKey`, it sorts the keys when printing. Closes https://github.com/astral-sh/ruff/issues/10359. --- ...ow_settings__display_default_settings.snap | 17 ++++++- .../rules/flake8_import_conventions/mod.rs | 13 ++++-- .../rules/banned_import_alias.rs | 6 ++- .../flake8_import_conventions/settings.rs | 46 ++++++++++++++++--- .../src/rules/flake8_tidy_imports/settings.rs | 8 +++- crates/ruff_linter/src/rules/isort/mod.rs | 33 ++++++------- .../ruff_linter/src/rules/isort/settings.rs | 45 +++++++++--------- .../src/rules/pydocstyle/settings.rs | 4 +- .../ruff_linter/src/rules/pylint/settings.rs | 2 +- crates/ruff_linter/src/settings/mod.rs | 32 +++++++++++++ crates/ruff_workspace/src/options.rs | 15 +++--- ruff.schema.json | 11 +++-- 12 files changed, 165 insertions(+), 67 deletions(-) diff --git a/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap b/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap index 6b7d064333d18..f04f78f8a7deb 100644 --- a/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap +++ b/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap @@ -241,7 +241,22 @@ linter.flake8_gettext.functions_names = [ ngettext, ] linter.flake8_implicit_str_concat.allow_multiline = true -linter.flake8_import_conventions.aliases = {"matplotlib": "mpl", "matplotlib.pyplot": "plt", "pandas": "pd", "seaborn": "sns", "tensorflow": "tf", "networkx": "nx", "plotly.express": "px", "polars": "pl", "numpy": "np", "panel": "pn", "pyarrow": "pa", "altair": "alt", "tkinter": "tk", "holoviews": "hv"} +linter.flake8_import_conventions.aliases = { + altair = alt, + holoviews = hv, + matplotlib = mpl, + matplotlib.pyplot = plt, + networkx = nx, + numpy = np, + pandas = pd, + panel = pn, + plotly.express = px, + polars = pl, + pyarrow = pa, + seaborn = sns, + tensorflow = tf, + tkinter = tk, +} linter.flake8_import_conventions.banned_aliases = {} linter.flake8_import_conventions.banned_from = [] linter.flake8_pytest_style.fixture_parentheses = true diff --git a/crates/ruff_linter/src/rules/flake8_import_conventions/mod.rs b/crates/ruff_linter/src/rules/flake8_import_conventions/mod.rs index 3bf95be28128e..fc5c6034af93d 100644 --- a/crates/ruff_linter/src/rules/flake8_import_conventions/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_import_conventions/mod.rs @@ -11,7 +11,7 @@ mod tests { use crate::assert_messages; use crate::registry::Rule; - use crate::rules::flake8_import_conventions::settings::default_aliases; + use crate::rules::flake8_import_conventions::settings::{default_aliases, BannedAliases}; use crate::settings::LinterSettings; use crate::test::test_path; @@ -57,17 +57,20 @@ mod tests { banned_aliases: FxHashMap::from_iter([ ( "typing".to_string(), - vec!["t".to_string(), "ty".to_string()], + BannedAliases::from_iter(["t".to_string(), "ty".to_string()]), ), ( "numpy".to_string(), - vec!["nmp".to_string(), "npy".to_string()], + BannedAliases::from_iter(["nmp".to_string(), "npy".to_string()]), ), ( "tensorflow.keras.backend".to_string(), - vec!["K".to_string()], + BannedAliases::from_iter(["K".to_string()]), + ), + ( + "torch.nn.functional".to_string(), + BannedAliases::from_iter(["F".to_string()]), ), - ("torch.nn.functional".to_string(), vec!["F".to_string()]), ]), banned_from: FxHashSet::default(), }, diff --git a/crates/ruff_linter/src/rules/flake8_import_conventions/rules/banned_import_alias.rs b/crates/ruff_linter/src/rules/flake8_import_conventions/rules/banned_import_alias.rs index bf41de81a9225..071736ac3cbae 100644 --- a/crates/ruff_linter/src/rules/flake8_import_conventions/rules/banned_import_alias.rs +++ b/crates/ruff_linter/src/rules/flake8_import_conventions/rules/banned_import_alias.rs @@ -1,10 +1,12 @@ -use ruff_python_ast::Stmt; use rustc_hash::FxHashMap; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::Stmt; use ruff_text_size::Ranged; +use crate::rules::flake8_import_conventions::settings::BannedAliases; + /// ## What it does /// Checks for imports that use non-standard naming conventions, like /// `import tensorflow.keras.backend as K`. @@ -49,7 +51,7 @@ pub(crate) fn banned_import_alias( stmt: &Stmt, name: &str, asname: &str, - banned_conventions: &FxHashMap>, + banned_conventions: &FxHashMap, ) -> Option { if let Some(banned_aliases) = banned_conventions.get(name) { if banned_aliases diff --git a/crates/ruff_linter/src/rules/flake8_import_conventions/settings.rs b/crates/ruff_linter/src/rules/flake8_import_conventions/settings.rs index 70fe742a20e3e..50c5aacc67eb1 100644 --- a/crates/ruff_linter/src/rules/flake8_import_conventions/settings.rs +++ b/crates/ruff_linter/src/rules/flake8_import_conventions/settings.rs @@ -1,11 +1,14 @@ //! Settings for import conventions. -use rustc_hash::{FxHashMap, FxHashSet}; use std::fmt::{Display, Formatter}; -use crate::display_settings; +use rustc_hash::{FxHashMap, FxHashSet}; +use serde::{Deserialize, Serialize}; + use ruff_macros::CacheKey; +use crate::display_settings; + const CONVENTIONAL_ALIASES: &[(&str, &str)] = &[ ("altair", "alt"), ("matplotlib", "mpl"), @@ -23,10 +26,41 @@ const CONVENTIONAL_ALIASES: &[(&str, &str)] = &[ ("pyarrow", "pa"), ]; +#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize, CacheKey)] +#[serde(deny_unknown_fields, rename_all = "kebab-case")] +#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] +pub struct BannedAliases(Vec); + +impl Display for BannedAliases { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "[")?; + for (i, alias) in self.0.iter().enumerate() { + if i > 0 { + write!(f, ", ")?; + } + write!(f, "{alias}")?; + } + write!(f, "]") + } +} + +impl BannedAliases { + /// Returns an iterator over the banned aliases. + pub fn iter(&self) -> impl Iterator { + self.0.iter().map(String::as_str) + } +} + +impl FromIterator for BannedAliases { + fn from_iter>(iter: I) -> Self { + Self(iter.into_iter().collect()) + } +} + #[derive(Debug, CacheKey)] pub struct Settings { pub aliases: FxHashMap, - pub banned_aliases: FxHashMap>, + pub banned_aliases: FxHashMap, pub banned_from: FxHashSet, } @@ -53,9 +87,9 @@ impl Display for Settings { formatter = f, namespace = "linter.flake8_import_conventions", fields = [ - self.aliases | debug, - self.banned_aliases | debug, - self.banned_from | array, + self.aliases | map, + self.banned_aliases | map, + self.banned_from | set, ] } Ok(()) diff --git a/crates/ruff_linter/src/rules/flake8_tidy_imports/settings.rs b/crates/ruff_linter/src/rules/flake8_tidy_imports/settings.rs index 35d55e2e75b80..8f9e29ea21aac 100644 --- a/crates/ruff_linter/src/rules/flake8_tidy_imports/settings.rs +++ b/crates/ruff_linter/src/rules/flake8_tidy_imports/settings.rs @@ -13,6 +13,12 @@ pub struct ApiBan { pub msg: String, } +impl Display for ApiBan { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.msg) + } +} + #[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize, CacheKey, Default)] #[serde(deny_unknown_fields, rename_all = "kebab-case")] #[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] @@ -47,7 +53,7 @@ impl Display for Settings { namespace = "linter.flake8_tidy_imports", fields = [ self.ban_relative_imports, - self.banned_api | debug, + self.banned_api | map, self.banned_module_level_imports | array, ] } diff --git a/crates/ruff_linter/src/rules/isort/mod.rs b/crates/ruff_linter/src/rules/isort/mod.rs index 47ec3c4dc9335..8f8990680e87e 100644 --- a/crates/ruff_linter/src/rules/isort/mod.rs +++ b/crates/ruff_linter/src/rules/isort/mod.rs @@ -278,7 +278,7 @@ mod tests { use std::path::Path; use anyhow::Result; - use rustc_hash::FxHashMap; + use rustc_hash::{FxHashMap, FxHashSet}; use test_case::test_case; use ruff_text_size::Ranged; @@ -495,7 +495,7 @@ mod tests { Path::new("isort").join(path).as_path(), &LinterSettings { isort: super::settings::Settings { - force_to_top: BTreeSet::from([ + force_to_top: FxHashSet::from_iter([ "z".to_string(), "lib1".to_string(), "lib3".to_string(), @@ -575,9 +575,10 @@ mod tests { &LinterSettings { isort: super::settings::Settings { force_single_line: true, - single_line_exclusions: vec!["os".to_string(), "logging.handlers".to_string()] - .into_iter() - .collect::>(), + single_line_exclusions: FxHashSet::from_iter([ + "os".to_string(), + "logging.handlers".to_string(), + ]), ..super::settings::Settings::default() }, src: vec![test_resource_path("fixtures/isort")], @@ -636,7 +637,7 @@ mod tests { &LinterSettings { isort: super::settings::Settings { order_by_type: true, - classes: BTreeSet::from([ + classes: FxHashSet::from_iter([ "SVC".to_string(), "SELU".to_string(), "N_CLASS".to_string(), @@ -664,7 +665,7 @@ mod tests { &LinterSettings { isort: super::settings::Settings { order_by_type: true, - constants: BTreeSet::from([ + constants: FxHashSet::from_iter([ "Const".to_string(), "constant".to_string(), "First".to_string(), @@ -694,7 +695,7 @@ mod tests { &LinterSettings { isort: super::settings::Settings { order_by_type: true, - variables: BTreeSet::from([ + variables: FxHashSet::from_iter([ "VAR".to_string(), "Variable".to_string(), "MyVar".to_string(), @@ -721,7 +722,7 @@ mod tests { &LinterSettings { isort: super::settings::Settings { force_sort_within_sections: true, - force_to_top: BTreeSet::from(["z".to_string()]), + force_to_top: FxHashSet::from_iter(["z".to_string()]), ..super::settings::Settings::default() }, src: vec![test_resource_path("fixtures/isort")], @@ -771,7 +772,7 @@ mod tests { &LinterSettings { src: vec![test_resource_path("fixtures/isort")], isort: super::settings::Settings { - required_imports: BTreeSet::from([ + required_imports: BTreeSet::from_iter([ "from __future__ import annotations".to_string() ]), ..super::settings::Settings::default() @@ -801,7 +802,7 @@ mod tests { &LinterSettings { src: vec![test_resource_path("fixtures/isort")], isort: super::settings::Settings { - required_imports: BTreeSet::from([ + required_imports: BTreeSet::from_iter([ "from __future__ import annotations as _annotations".to_string(), ]), ..super::settings::Settings::default() @@ -824,7 +825,7 @@ mod tests { &LinterSettings { src: vec![test_resource_path("fixtures/isort")], isort: super::settings::Settings { - required_imports: BTreeSet::from([ + required_imports: BTreeSet::from_iter([ "from __future__ import annotations".to_string(), "from __future__ import generator_stop".to_string(), ]), @@ -848,7 +849,7 @@ mod tests { &LinterSettings { src: vec![test_resource_path("fixtures/isort")], isort: super::settings::Settings { - required_imports: BTreeSet::from(["from __future__ import annotations, \ + required_imports: BTreeSet::from_iter(["from __future__ import annotations, \ generator_stop" .to_string()]), ..super::settings::Settings::default() @@ -871,7 +872,7 @@ mod tests { &LinterSettings { src: vec![test_resource_path("fixtures/isort")], isort: super::settings::Settings { - required_imports: BTreeSet::from(["import os".to_string()]), + required_imports: BTreeSet::from_iter(["import os".to_string()]), ..super::settings::Settings::default() }, ..LinterSettings::for_rule(Rule::MissingRequiredImport) @@ -1002,7 +1003,7 @@ mod tests { Path::new("isort").join(path).as_path(), &LinterSettings { isort: super::settings::Settings { - no_lines_before: BTreeSet::from([ + no_lines_before: FxHashSet::from_iter([ ImportSection::Known(ImportType::Future), ImportSection::Known(ImportType::StandardLibrary), ImportSection::Known(ImportType::ThirdParty), @@ -1030,7 +1031,7 @@ mod tests { Path::new("isort").join(path).as_path(), &LinterSettings { isort: super::settings::Settings { - no_lines_before: BTreeSet::from([ + no_lines_before: FxHashSet::from_iter([ ImportSection::Known(ImportType::StandardLibrary), ImportSection::Known(ImportType::LocalFolder), ]), diff --git a/crates/ruff_linter/src/rules/isort/settings.rs b/crates/ruff_linter/src/rules/isort/settings.rs index f86d593a75968..8ae6464932123 100644 --- a/crates/ruff_linter/src/rules/isort/settings.rs +++ b/crates/ruff_linter/src/rules/isort/settings.rs @@ -5,12 +5,13 @@ use std::error::Error; use std::fmt; use std::fmt::{Display, Formatter}; +use rustc_hash::FxHashSet; use serde::{Deserialize, Serialize}; use strum::IntoEnumIterator; -use crate::display_settings; use ruff_macros::CacheKey; +use crate::display_settings; use crate::rules::isort::categorize::KnownModules; use crate::rules::isort::ImportType; @@ -52,17 +53,17 @@ pub struct Settings { pub force_sort_within_sections: bool, pub case_sensitive: bool, pub force_wrap_aliases: bool, - pub force_to_top: BTreeSet, + pub force_to_top: FxHashSet, pub known_modules: KnownModules, pub detect_same_package: bool, pub order_by_type: bool, pub relative_imports_order: RelativeImportsOrder, - pub single_line_exclusions: BTreeSet, + pub single_line_exclusions: FxHashSet, pub split_on_trailing_comma: bool, - pub classes: BTreeSet, - pub constants: BTreeSet, - pub variables: BTreeSet, - pub no_lines_before: BTreeSet, + pub classes: FxHashSet, + pub constants: FxHashSet, + pub variables: FxHashSet, + pub no_lines_before: FxHashSet, pub lines_after_imports: isize, pub lines_between_types: usize, pub forced_separate: Vec, @@ -77,23 +78,23 @@ pub struct Settings { impl Default for Settings { fn default() -> Self { Self { - required_imports: BTreeSet::new(), + required_imports: BTreeSet::default(), combine_as_imports: false, force_single_line: false, force_sort_within_sections: false, detect_same_package: true, case_sensitive: false, force_wrap_aliases: false, - force_to_top: BTreeSet::new(), + force_to_top: FxHashSet::default(), known_modules: KnownModules::default(), order_by_type: true, relative_imports_order: RelativeImportsOrder::default(), - single_line_exclusions: BTreeSet::new(), + single_line_exclusions: FxHashSet::default(), split_on_trailing_comma: true, - classes: BTreeSet::new(), - constants: BTreeSet::new(), - variables: BTreeSet::new(), - no_lines_before: BTreeSet::new(), + classes: FxHashSet::default(), + constants: FxHashSet::default(), + variables: FxHashSet::default(), + no_lines_before: FxHashSet::default(), lines_after_imports: -1, lines_between_types: 0, forced_separate: Vec::new(), @@ -113,23 +114,23 @@ impl Display for Settings { formatter = f, namespace = "linter.isort", fields = [ - self.required_imports | array, + self.required_imports | set, self.combine_as_imports, self.force_single_line, self.force_sort_within_sections, self.detect_same_package, self.case_sensitive, self.force_wrap_aliases, - self.force_to_top | array, + self.force_to_top | set, self.known_modules, self.order_by_type, self.relative_imports_order, - self.single_line_exclusions | array, + self.single_line_exclusions | set, self.split_on_trailing_comma, - self.classes | array, - self.constants | array, - self.variables | array, - self.no_lines_before | array, + self.classes | set, + self.constants | set, + self.variables | set, + self.no_lines_before | set, self.lines_after_imports, self.lines_between_types, self.forced_separate | array, @@ -155,7 +156,7 @@ pub enum SettingsError { InvalidUserDefinedSection(glob::PatternError), } -impl fmt::Display for SettingsError { +impl Display for SettingsError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { SettingsError::InvalidKnownThirdParty(err) => { diff --git a/crates/ruff_linter/src/rules/pydocstyle/settings.rs b/crates/ruff_linter/src/rules/pydocstyle/settings.rs index b65172e4102d3..1b3a177af64eb 100644 --- a/crates/ruff_linter/src/rules/pydocstyle/settings.rs +++ b/crates/ruff_linter/src/rules/pydocstyle/settings.rs @@ -97,8 +97,8 @@ impl fmt::Display for Settings { namespace = "linter.pydocstyle", fields = [ self.convention | optional, - self.ignore_decorators | debug, - self.property_decorators | debug + self.ignore_decorators | set, + self.property_decorators | set ] } Ok(()) diff --git a/crates/ruff_linter/src/rules/pylint/settings.rs b/crates/ruff_linter/src/rules/pylint/settings.rs index cdd55a9e3bbbf..c98698d5a283c 100644 --- a/crates/ruff_linter/src/rules/pylint/settings.rs +++ b/crates/ruff_linter/src/rules/pylint/settings.rs @@ -88,7 +88,7 @@ impl fmt::Display for Settings { namespace = "linter.pylint", fields = [ self.allow_magic_value_types | array, - self.allow_dunder_method_names | array, + self.allow_dunder_method_names | set, self.max_args, self.max_positional_args, self.max_returns, diff --git a/crates/ruff_linter/src/settings/mod.rs b/crates/ruff_linter/src/settings/mod.rs index 53f41534195ea..cdedabc6c0cd4 100644 --- a/crates/ruff_linter/src/settings/mod.rs +++ b/crates/ruff_linter/src/settings/mod.rs @@ -155,6 +155,38 @@ macro_rules! display_settings { } } }; + (@field $fmt:ident, $prefix:ident, $settings:ident.$field:ident | map) => { + { + use itertools::Itertools; + + write!($fmt, "{}{} = ", $prefix, stringify!($field))?; + if $settings.$field.is_empty() { + writeln!($fmt, "{{}}")?; + } else { + writeln!($fmt, "{{")?; + for (key, value) in $settings.$field.iter().sorted_by(|(left, _), (right, _)| left.cmp(right)) { + writeln!($fmt, "\t{key} = {value},")?; + } + writeln!($fmt, "}}")?; + } + } + }; + (@field $fmt:ident, $prefix:ident, $settings:ident.$field:ident | set) => { + { + use itertools::Itertools; + + write!($fmt, "{}{} = ", $prefix, stringify!($field))?; + if $settings.$field.is_empty() { + writeln!($fmt, "[]")?; + } else { + writeln!($fmt, "[")?; + for elem in $settings.$field.iter().sorted_by(|left, right| left.cmp(right)) { + writeln!($fmt, "\t{elem},")?; + } + writeln!($fmt, "]")?; + } + } + }; (@field $fmt:ident, $prefix:ident, $settings:ident.$field:ident | paths) => { { write!($fmt, "{}{} = ", $prefix, stringify!($field))?; diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index ad536a3f35631..d6679a4c920a2 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -8,6 +8,7 @@ use strum::IntoEnumIterator; use ruff_formatter::IndentStyle; use ruff_linter::line_width::{IndentWidth, LineLength}; +use ruff_linter::rules::flake8_import_conventions::settings::BannedAliases; use ruff_linter::rules::flake8_pytest_style::settings::SettingsError; use ruff_linter::rules::flake8_pytest_style::types; use ruff_linter::rules::flake8_quotes::settings::Quote; @@ -1309,7 +1310,7 @@ pub struct Flake8ImportConventionsOptions { "tensorflow.keras.backend" = ["K"] "# )] - pub banned_aliases: Option>>, + pub banned_aliases: Option>, /// A list of modules that should not be imported from using the /// `from ... import ...` syntax. @@ -2383,7 +2384,7 @@ impl IsortOptions { case_sensitive: self.case_sensitive.unwrap_or(false), force_wrap_aliases: self.force_wrap_aliases.unwrap_or(false), detect_same_package: self.detect_same_package.unwrap_or(true), - force_to_top: BTreeSet::from_iter(self.force_to_top.unwrap_or_default()), + force_to_top: FxHashSet::from_iter(self.force_to_top.unwrap_or_default()), known_modules: isort::categorize::KnownModules::new( known_first_party, known_third_party, @@ -2393,14 +2394,14 @@ impl IsortOptions { ), order_by_type: self.order_by_type.unwrap_or(true), relative_imports_order: self.relative_imports_order.unwrap_or_default(), - single_line_exclusions: BTreeSet::from_iter( + single_line_exclusions: FxHashSet::from_iter( self.single_line_exclusions.unwrap_or_default(), ), split_on_trailing_comma: self.split_on_trailing_comma.unwrap_or(true), - classes: BTreeSet::from_iter(self.classes.unwrap_or_default()), - constants: BTreeSet::from_iter(self.constants.unwrap_or_default()), - variables: BTreeSet::from_iter(self.variables.unwrap_or_default()), - no_lines_before: BTreeSet::from_iter(no_lines_before), + classes: FxHashSet::from_iter(self.classes.unwrap_or_default()), + constants: FxHashSet::from_iter(self.constants.unwrap_or_default()), + variables: FxHashSet::from_iter(self.variables.unwrap_or_default()), + no_lines_before: FxHashSet::from_iter(no_lines_before), lines_after_imports: self.lines_after_imports.unwrap_or(-1), lines_between_types, forced_separate: Vec::from_iter(self.forced_separate.unwrap_or_default()), diff --git a/ruff.schema.json b/ruff.schema.json index 5412af6e672fd..5d44debca2f2f 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -755,6 +755,12 @@ }, "additionalProperties": false }, + "BannedAliases": { + "type": "array", + "items": { + "type": "string" + } + }, "ConstantType": { "type": "string", "enum": [ @@ -1036,10 +1042,7 @@ "null" ], "additionalProperties": { - "type": "array", - "items": { - "type": "string" - } + "$ref": "#/definitions/BannedAliases" } }, "banned-from": {