diff --git a/crates/ruff/src/rules/isort/categorize.rs b/crates/ruff/src/rules/isort/categorize.rs index c9ec34447a71a..3ed3d8d1a3ff4 100644 --- a/crates/ruff/src/rules/isort/categorize.rs +++ b/crates/ruff/src/rules/isort/categorize.rs @@ -1,9 +1,10 @@ use std::collections::BTreeMap; +use std::hash::BuildHasherDefault; use std::path::{Path, PathBuf}; use std::{fs, iter}; use log::debug; -use rustc_hash::FxHashMap; +use rustc_hash::{FxHashMap, FxHashSet}; use serde::{Deserialize, Serialize}; use strum_macros::EnumIter; @@ -210,20 +211,20 @@ pub(crate) fn categorize_imports<'a>( #[derive(Debug, Default, CacheKey)] pub struct KnownModules { /// A map of known modules to their section. - known: FxHashMap, + known: Vec<(glob::Pattern, ImportSection)>, /// Whether any of the known modules are submodules (e.g., `foo.bar`, as opposed to `foo`). has_submodules: bool, } impl KnownModules { pub(crate) fn new( - first_party: Vec, - third_party: Vec, - local_folder: Vec, - standard_library: Vec, - user_defined: FxHashMap>, + first_party: Vec, + third_party: Vec, + local_folder: Vec, + standard_library: Vec, + user_defined: FxHashMap>, ) -> Self { - let modules = user_defined + let known: Vec<(glob::Pattern, ImportSection)> = user_defined .into_iter() .flat_map(|(section, modules)| { modules @@ -249,19 +250,23 @@ impl KnownModules { standard_library .into_iter() .map(|module| (module, ImportSection::Known(ImportType::StandardLibrary))), - ); + ) + .collect(); - let mut known = FxHashMap::with_capacity_and_hasher( - modules.size_hint().0, - std::hash::BuildHasherDefault::default(), - ); - modules.for_each(|(module, section)| { - if known.insert(module, section).is_some() { - warn_user_once!("One or more modules are part of multiple import sections."); + // Warn in the case of duplicate modules. + let mut seen = + FxHashSet::with_capacity_and_hasher(known.len(), BuildHasherDefault::default()); + for (module, _) in &known { + if !seen.insert(module) { + warn_user_once!("One or more modules are part of multiple import sections, including: `{module}`"); + break; } - }); + } - let has_submodules = known.keys().any(|module| module.contains('.')); + // Check if any of the known modules are submodules. + let has_submodules = known + .iter() + .any(|(module, _)| module.as_str().contains('.')); Self { known, @@ -296,31 +301,37 @@ impl KnownModules { } fn categorize_submodule(&self, submodule: &str) -> Option<(&ImportSection, Reason)> { - if let Some(section) = self.known.get(submodule) { - let reason = match section { - ImportSection::UserDefined(_) => Reason::UserDefinedSection, - ImportSection::Known(ImportType::FirstParty) => Reason::KnownFirstParty, - ImportSection::Known(ImportType::ThirdParty) => Reason::KnownThirdParty, - ImportSection::Known(ImportType::LocalFolder) => Reason::KnownLocalFolder, - ImportSection::Known(ImportType::StandardLibrary) => Reason::ExtraStandardLibrary, - ImportSection::Known(ImportType::Future) => { - unreachable!("__future__ imports are not known") - } - }; - Some((section, reason)) - } else { - None - } + let section = self.known.iter().find_map(|(pattern, section)| { + if pattern.matches(submodule) { + Some(section) + } else { + None + } + })?; + let reason = match section { + ImportSection::UserDefined(_) => Reason::UserDefinedSection, + ImportSection::Known(ImportType::FirstParty) => Reason::KnownFirstParty, + ImportSection::Known(ImportType::ThirdParty) => Reason::KnownThirdParty, + ImportSection::Known(ImportType::LocalFolder) => Reason::KnownLocalFolder, + ImportSection::Known(ImportType::StandardLibrary) => Reason::ExtraStandardLibrary, + ImportSection::Known(ImportType::Future) => { + unreachable!("__future__ imports are not known") + } + }; + Some((section, reason)) } /// Return the list of modules that are known to be of a given type. - pub(crate) fn modules_for_known_type(&self, import_type: ImportType) -> Vec { + pub(crate) fn modules_for_known_type( + &self, + import_type: ImportType, + ) -> impl Iterator { self.known .iter() - .filter_map(|(module, known_section)| { + .filter_map(move |(module, known_section)| { if let ImportSection::Known(section) = known_section { if *section == import_type { - Some(module.clone()) + Some(module) } else { None } @@ -328,18 +339,17 @@ impl KnownModules { None } }) - .collect() } /// Return the list of user-defined modules, indexed by section. - pub(crate) fn user_defined(&self) -> FxHashMap> { - let mut user_defined: FxHashMap> = FxHashMap::default(); + pub(crate) fn user_defined(&self) -> FxHashMap<&str, Vec<&glob::Pattern>> { + let mut user_defined: FxHashMap<&str, Vec<&glob::Pattern>> = FxHashMap::default(); for (module, section) in &self.known { if let ImportSection::UserDefined(section_name) = section { user_defined - .entry(section_name.clone()) + .entry(section_name.as_str()) .or_default() - .push(module.clone()); + .push(module); } } user_defined diff --git a/crates/ruff/src/rules/isort/mod.rs b/crates/ruff/src/rules/isort/mod.rs index f156451770db6..bcc2f1d9a9fe0 100644 --- a/crates/ruff/src/rules/isort/mod.rs +++ b/crates/ruff/src/rules/isort/mod.rs @@ -369,6 +369,10 @@ mod tests { Ok(()) } + fn pattern(pattern: &str) -> glob::Pattern { + glob::Pattern::new(pattern).unwrap() + } + #[test_case(Path::new("separate_subpackage_first_and_third_party_imports.py"))] fn separate_modules(path: &Path) -> Result<()> { let snapshot = format!("1_{}", path.to_string_lossy()); @@ -377,8 +381,32 @@ mod tests { &Settings { isort: super::settings::Settings { known_modules: KnownModules::new( - vec!["foo.bar".to_string(), "baz".to_string()], - vec!["foo".to_string(), "__future__".to_string()], + vec![pattern("foo.bar"), pattern("baz")], + vec![pattern("foo"), pattern("__future__")], + vec![], + vec![], + FxHashMap::default(), + ), + ..super::settings::Settings::default() + }, + src: vec![test_resource_path("fixtures/isort")], + ..Settings::for_rule(Rule::UnsortedImports) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } + + #[test_case(Path::new("separate_subpackage_first_and_third_party_imports.py"))] + fn separate_modules_glob(path: &Path) -> Result<()> { + let snapshot = format!("glob_1_{}", path.to_string_lossy()); + let diagnostics = test_path( + Path::new("isort").join(path).as_path(), + &Settings { + isort: super::settings::Settings { + known_modules: KnownModules::new( + vec![pattern("foo.*"), pattern("baz")], + vec![pattern("foo"), pattern("__future__")], vec![], vec![], FxHashMap::default(), @@ -401,8 +429,8 @@ mod tests { &Settings { isort: super::settings::Settings { known_modules: KnownModules::new( - vec!["foo".to_string()], - vec!["foo.bar".to_string()], + vec![pattern("foo")], + vec![pattern("foo.bar")], vec![], vec![], FxHashMap::default(), @@ -445,7 +473,7 @@ mod tests { known_modules: KnownModules::new( vec![], vec![], - vec!["ruff".to_string()], + vec![pattern("ruff")], vec![], FxHashMap::default(), ), @@ -961,7 +989,7 @@ mod tests { vec![], vec![], vec![], - FxHashMap::from_iter([("django".to_string(), vec!["django".to_string()])]), + FxHashMap::from_iter([("django".to_string(), vec![pattern("django")])]), ), section_order: vec![ ImportSection::Known(ImportType::Future), @@ -990,11 +1018,11 @@ mod tests { src: vec![test_resource_path("fixtures/isort")], isort: super::settings::Settings { known_modules: KnownModules::new( - vec!["library".to_string()], + vec![pattern("library")], vec![], vec![], vec![], - FxHashMap::from_iter([("django".to_string(), vec!["django".to_string()])]), + FxHashMap::from_iter([("django".to_string(), vec![pattern("django")])]), ), section_order: vec![ ImportSection::Known(ImportType::Future), diff --git a/crates/ruff/src/rules/isort/settings.rs b/crates/ruff/src/rules/isort/settings.rs index f01aa4afd168a..8e9648df1bba2 100644 --- a/crates/ruff/src/rules/isort/settings.rs +++ b/crates/ruff/src/rules/isort/settings.rs @@ -1,6 +1,8 @@ //! Settings for the `isort` plugin. use std::collections::BTreeSet; +use std::error::Error; +use std::fmt; use std::hash::BuildHasherDefault; use rustc_hash::{FxHashMap, FxHashSet}; @@ -11,6 +13,7 @@ use ruff_macros::{CacheKey, CombineOptions, ConfigurationOptions}; use crate::rules::isort::categorize::KnownModules; use crate::rules::isort::ImportType; +use crate::settings::types::IdentifierPattern; use crate::warn_user_once; use super::categorize::ImportSection; @@ -154,6 +157,9 @@ pub struct Options { )] /// A list of modules to consider first-party, regardless of whether they /// can be identified as such via introspection of the local filesystem. + /// + /// Supports glob patterns. For more information on the glob syntax, refer + /// to the [`globset` documentation](https://docs.rs/globset/latest/globset/#syntax). pub known_first_party: Option>, #[option( default = r#"[]"#, @@ -164,6 +170,9 @@ pub struct Options { )] /// A list of modules to consider third-party, regardless of whether they /// can be identified as such via introspection of the local filesystem. + /// + /// Supports glob patterns. For more information on the glob syntax, refer + /// to the [`globset` documentation](https://docs.rs/globset/latest/globset/#syntax). pub known_third_party: Option>, #[option( default = r#"[]"#, @@ -174,6 +183,9 @@ pub struct Options { )] /// A list of modules to consider being a local folder. /// Generally, this is reserved for relative imports (`from . import module`). + /// + /// Supports glob patterns. For more information on the glob syntax, refer + /// to the [`globset` documentation](https://docs.rs/globset/latest/globset/#syntax). pub known_local_folder: Option>, #[option( default = r#"[]"#, @@ -184,6 +196,9 @@ pub struct Options { )] /// A list of modules to consider standard-library, in addition to those /// known to Ruff in advance. + /// + /// Supports glob patterns. For more information on the glob syntax, refer + /// to the [`globset` documentation](https://docs.rs/globset/latest/globset/#syntax). pub extra_standard_library: Option>, #[option( default = r#"furthest-to-closest"#, @@ -357,21 +372,63 @@ impl Default for Settings { } } -impl From for Settings { - fn from(options: Options) -> Self { +impl TryFrom for Settings { + type Error = SettingsError; + + fn try_from(options: Options) -> Result { // Extract any configuration options that deal with user-defined sections. let mut section_order: Vec<_> = options .section_order .unwrap_or_else(|| ImportType::iter().map(ImportSection::Known).collect()); - let known_first_party = options.known_first_party.unwrap_or_default(); - let known_third_party = options.known_third_party.unwrap_or_default(); - let known_local_folder = options.known_local_folder.unwrap_or_default(); - let extra_standard_library = options.extra_standard_library.unwrap_or_default(); + let known_first_party = options + .known_first_party + .map(|names| { + names + .into_iter() + .map(|name| IdentifierPattern::new(&name)) + .collect() + }) + .transpose() + .map_err(SettingsError::InvalidKnownFirstParty)? + .unwrap_or_default(); + let known_third_party = options + .known_third_party + .map(|names| { + names + .into_iter() + .map(|name| IdentifierPattern::new(&name)) + .collect() + }) + .transpose() + .map_err(SettingsError::InvalidKnownThirdParty)? + .unwrap_or_default(); + let known_local_folder = options + .known_local_folder + .map(|names| { + names + .into_iter() + .map(|name| IdentifierPattern::new(&name)) + .collect() + }) + .transpose() + .map_err(SettingsError::InvalidKnownLocalFolder)? + .unwrap_or_default(); + let extra_standard_library = options + .extra_standard_library + .map(|names| { + names + .into_iter() + .map(|name| IdentifierPattern::new(&name)) + .collect() + }) + .transpose() + .map_err(SettingsError::InvalidExtraStandardLibrary)? + .unwrap_or_default(); let no_lines_before = options.no_lines_before.unwrap_or_default(); let sections = options.sections.unwrap_or_default(); // Verify that `sections` doesn't contain any built-in sections. - let sections: FxHashMap> = sections + let sections: FxHashMap> = sections .into_iter() .filter_map(|(section, modules)| match section { ImportSection::Known(section) => { @@ -380,7 +437,17 @@ impl From for Settings { } ImportSection::UserDefined(section) => Some((section, modules)), }) - .collect(); + .map(|(section, modules)| { + let modules = modules + .into_iter() + .map(|module| { + IdentifierPattern::new(&module) + .map_err(SettingsError::InvalidUserDefinedSection) + }) + .collect::, Self::Error>>()?; + Ok((section, modules)) + }) + .collect::>()?; // Verify that `section_order` doesn't contain any duplicates. let mut seen = @@ -435,7 +502,7 @@ impl From for Settings { } } - Self { + Ok(Self { required_imports: BTreeSet::from_iter(options.required_imports.unwrap_or_default()), combine_as_imports: options.combine_as_imports.unwrap_or(false), force_single_line: options.force_single_line.unwrap_or(false), @@ -464,6 +531,50 @@ impl From for Settings { lines_between_types: options.lines_between_types.unwrap_or_default(), forced_separate: Vec::from_iter(options.forced_separate.unwrap_or_default()), section_order, + }) + } +} + +/// Error returned by the [`TryFrom`] implementation of [`Settings`]. +#[derive(Debug)] +pub enum SettingsError { + InvalidKnownFirstParty(glob::PatternError), + InvalidKnownThirdParty(glob::PatternError), + InvalidKnownLocalFolder(glob::PatternError), + InvalidExtraStandardLibrary(glob::PatternError), + InvalidUserDefinedSection(glob::PatternError), +} + +impl fmt::Display for SettingsError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + SettingsError::InvalidKnownThirdParty(err) => { + write!(f, "invalid known third-party pattern: {err}") + } + SettingsError::InvalidKnownFirstParty(err) => { + write!(f, "invalid known first-party pattern: {err}") + } + SettingsError::InvalidKnownLocalFolder(err) => { + write!(f, "invalid known local folder pattern: {err}") + } + SettingsError::InvalidExtraStandardLibrary(err) => { + write!(f, "invalid extra standard library pattern: {err}") + } + SettingsError::InvalidUserDefinedSection(err) => { + write!(f, "invalid user-defined section pattern: {err}") + } + } + } +} + +impl Error for SettingsError { + fn source(&self) -> Option<&(dyn Error + 'static)> { + match self { + SettingsError::InvalidKnownThirdParty(err) => Some(err), + SettingsError::InvalidKnownFirstParty(err) => Some(err), + SettingsError::InvalidKnownLocalFolder(err) => Some(err), + SettingsError::InvalidExtraStandardLibrary(err) => Some(err), + SettingsError::InvalidUserDefinedSection(err) => Some(err), } } } @@ -476,7 +587,9 @@ impl From for Options { extra_standard_library: Some( settings .known_modules - .modules_for_known_type(ImportType::StandardLibrary), + .modules_for_known_type(ImportType::StandardLibrary) + .map(ToString::to_string) + .collect(), ), force_single_line: Some(settings.force_single_line), force_sort_within_sections: Some(settings.force_sort_within_sections), @@ -486,17 +599,23 @@ impl From for Options { known_first_party: Some( settings .known_modules - .modules_for_known_type(ImportType::FirstParty), + .modules_for_known_type(ImportType::FirstParty) + .map(ToString::to_string) + .collect(), ), known_third_party: Some( settings .known_modules - .modules_for_known_type(ImportType::ThirdParty), + .modules_for_known_type(ImportType::ThirdParty) + .map(ToString::to_string) + .collect(), ), known_local_folder: Some( settings .known_modules - .modules_for_known_type(ImportType::LocalFolder), + .modules_for_known_type(ImportType::LocalFolder) + .map(ToString::to_string) + .collect(), ), order_by_type: Some(settings.order_by_type), relative_imports_order: Some(settings.relative_imports_order), @@ -515,7 +634,12 @@ impl From for Options { .known_modules .user_defined() .into_iter() - .map(|(section, modules)| (ImportSection::UserDefined(section), modules)) + .map(|(section, modules)| { + ( + ImportSection::UserDefined(section.to_string()), + modules.into_iter().map(ToString::to_string).collect(), + ) + }) .collect(), ), } diff --git a/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__glob_1_separate_subpackage_first_and_third_party_imports.py.snap b/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__glob_1_separate_subpackage_first_and_third_party_imports.py.snap new file mode 100644 index 0000000000000..41e6eb47f6bd7 --- /dev/null +++ b/crates/ruff/src/rules/isort/snapshots/ruff__rules__isort__tests__glob_1_separate_subpackage_first_and_third_party_imports.py.snap @@ -0,0 +1,33 @@ +--- +source: crates/ruff/src/rules/isort/mod.rs +--- +separate_subpackage_first_and_third_party_imports.py:1:1: I001 [*] Import block is un-sorted or un-formatted + | +1 | / import sys +2 | | import baz +3 | | from foo import bar, baz +4 | | from foo.bar import blah, blub +5 | | from foo.bar.baz import something +6 | | import foo +7 | | import foo.bar +8 | | import foo.bar.baz + | + = help: Organize imports + +ℹ Fix +1 1 | import sys + 2 |+ + 3 |+import foo + 4 |+from foo import bar, baz + 5 |+ +2 6 | import baz +3 |-from foo import bar, baz + 7 |+import foo.bar + 8 |+import foo.bar.baz +4 9 | from foo.bar import blah, blub +5 10 | from foo.bar.baz import something +6 |-import foo +7 |-import foo.bar +8 |-import foo.bar.baz + + diff --git a/crates/ruff/src/settings/mod.rs b/crates/ruff/src/settings/mod.rs index f00a4833b2f75..b398292771a39 100644 --- a/crates/ruff/src/settings/mod.rs +++ b/crates/ruff/src/settings/mod.rs @@ -258,7 +258,8 @@ impl Settings { .unwrap_or_default(), isort: config .isort - .map(isort::settings::Settings::from) + .map(isort::settings::Settings::try_from) + .transpose()? .unwrap_or_default(), mccabe: config .mccabe