diff --git a/Cargo.lock b/Cargo.lock index 8f3269e73a..893ea46197 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2834,6 +2834,7 @@ dependencies = [ "smallvec", "strum", "strum_macros", + "tempfile", "test-case", "thiserror 2.0.12", "toml", diff --git a/crates/ruff/tests/snapshots/integration_test__rule_f401.snap b/crates/ruff/tests/snapshots/integration_test__rule_f401.snap index 3e9270c33e..a861fe395c 100644 --- a/crates/ruff/tests/snapshots/integration_test__rule_f401.snap +++ b/crates/ruff/tests/snapshots/integration_test__rule_f401.snap @@ -5,7 +5,6 @@ info: args: - rule - F401 -snapshot_kind: text --- success: true exit_code: 0 @@ -84,6 +83,11 @@ else: print("numpy is not installed") ``` +## Preview +When [preview](https://docs.astral.sh/ruff/preview/) is enabled, +the criterion for determining whether an import is first-party +is stricter, which could affect the suggested fix. See [this FAQ section](https://docs.astral.sh/ruff/faq/#how-does-ruff-determine-which-of-my-imports-are-first-party-third-party-etc) for more details. + ## Options - `lint.ignore-init-module-imports` - `lint.pyflakes.allowed-unused-imports` diff --git a/crates/ruff_linter/Cargo.toml b/crates/ruff_linter/Cargo.toml index 37be59af5e..2e48fbde19 100644 --- a/crates/ruff_linter/Cargo.toml +++ b/crates/ruff_linter/Cargo.toml @@ -76,6 +76,7 @@ insta = { workspace = true, features = ["filters", "json", "redactions"] } test-case = { workspace = true } # Disable colored output in tests colored = { workspace = true, features = ["no-color"] } +tempfile = { workspace = true } [features] default = [] diff --git a/crates/ruff_linter/src/message/snapshots/ruff_linter__message__sarif__tests__results.snap b/crates/ruff_linter/src/message/snapshots/ruff_linter__message__sarif__tests__results.snap index 287d91e6f1..224cdf75ae 100644 --- a/crates/ruff_linter/src/message/snapshots/ruff_linter__message__sarif__tests__results.snap +++ b/crates/ruff_linter/src/message/snapshots/ruff_linter__message__sarif__tests__results.snap @@ -81,7 +81,7 @@ expression: value "rules": [ { "fullDescription": { - "text": "## What it does\nChecks for unused imports.\n\n## Why is this bad?\nUnused imports add a performance overhead at runtime, and risk creating\nimport cycles. They also increase the cognitive load of reading the code.\n\nIf an import statement is used to check for the availability or existence\nof a module, consider using `importlib.util.find_spec` instead.\n\nIf an import statement is used to re-export a symbol as part of a module's\npublic interface, consider using a \"redundant\" import alias, which\ninstructs Ruff (and other tools) to respect the re-export, and avoid\nmarking it as unused, as in:\n\n```python\nfrom module import member as member\n```\n\nAlternatively, you can use `__all__` to declare a symbol as part of the module's\ninterface, as in:\n\n```python\n# __init__.py\nimport some_module\n\n__all__ = [\"some_module\"]\n```\n\n## Fix safety\n\nFixes to remove unused imports are safe, except in `__init__.py` files.\n\nApplying fixes to `__init__.py` files is currently in preview. The fix offered depends on the\ntype of the unused import. Ruff will suggest a safe fix to export first-party imports with\neither a redundant alias or, if already present in the file, an `__all__` entry. If multiple\n`__all__` declarations are present, Ruff will not offer a fix. Ruff will suggest an unsafe fix\nto remove third-party and standard library imports -- the fix is unsafe because the module's\ninterface changes.\n\n## Example\n\n```python\nimport numpy as np # unused import\n\n\ndef area(radius):\n return 3.14 * radius**2\n```\n\nUse instead:\n\n```python\ndef area(radius):\n return 3.14 * radius**2\n```\n\nTo check the availability of a module, use `importlib.util.find_spec`:\n\n```python\nfrom importlib.util import find_spec\n\nif find_spec(\"numpy\") is not None:\n print(\"numpy is installed\")\nelse:\n print(\"numpy is not installed\")\n```\n\n## Options\n- `lint.ignore-init-module-imports`\n- `lint.pyflakes.allowed-unused-imports`\n\n## References\n- [Python documentation: `import`](https://docs.python.org/3/reference/simple_stmts.html#the-import-statement)\n- [Python documentation: `importlib.util.find_spec`](https://docs.python.org/3/library/importlib.html#importlib.util.find_spec)\n- [Typing documentation: interface conventions](https://typing.python.org/en/latest/source/libraries.html#library-interface-public-and-private-symbols)\n" + "text": "## What it does\nChecks for unused imports.\n\n## Why is this bad?\nUnused imports add a performance overhead at runtime, and risk creating\nimport cycles. They also increase the cognitive load of reading the code.\n\nIf an import statement is used to check for the availability or existence\nof a module, consider using `importlib.util.find_spec` instead.\n\nIf an import statement is used to re-export a symbol as part of a module's\npublic interface, consider using a \"redundant\" import alias, which\ninstructs Ruff (and other tools) to respect the re-export, and avoid\nmarking it as unused, as in:\n\n```python\nfrom module import member as member\n```\n\nAlternatively, you can use `__all__` to declare a symbol as part of the module's\ninterface, as in:\n\n```python\n# __init__.py\nimport some_module\n\n__all__ = [\"some_module\"]\n```\n\n## Fix safety\n\nFixes to remove unused imports are safe, except in `__init__.py` files.\n\nApplying fixes to `__init__.py` files is currently in preview. The fix offered depends on the\ntype of the unused import. Ruff will suggest a safe fix to export first-party imports with\neither a redundant alias or, if already present in the file, an `__all__` entry. If multiple\n`__all__` declarations are present, Ruff will not offer a fix. Ruff will suggest an unsafe fix\nto remove third-party and standard library imports -- the fix is unsafe because the module's\ninterface changes.\n\n## Example\n\n```python\nimport numpy as np # unused import\n\n\ndef area(radius):\n return 3.14 * radius**2\n```\n\nUse instead:\n\n```python\ndef area(radius):\n return 3.14 * radius**2\n```\n\nTo check the availability of a module, use `importlib.util.find_spec`:\n\n```python\nfrom importlib.util import find_spec\n\nif find_spec(\"numpy\") is not None:\n print(\"numpy is installed\")\nelse:\n print(\"numpy is not installed\")\n```\n\n## Preview\nWhen [preview](https://docs.astral.sh/ruff/preview/) is enabled,\nthe criterion for determining whether an import is first-party\nis stricter, which could affect the suggested fix. See [this FAQ section](https://docs.astral.sh/ruff/faq/#how-does-ruff-determine-which-of-my-imports-are-first-party-third-party-etc) for more details.\n\n## Options\n- `lint.ignore-init-module-imports`\n- `lint.pyflakes.allowed-unused-imports`\n\n## References\n- [Python documentation: `import`](https://docs.python.org/3/reference/simple_stmts.html#the-import-statement)\n- [Python documentation: `importlib.util.find_spec`](https://docs.python.org/3/library/importlib.html#importlib.util.find_spec)\n- [Typing documentation: interface conventions](https://typing.python.org/en/latest/source/libraries.html#library-interface-public-and-private-symbols)\n" }, "help": { "text": "`{name}` imported but unused; consider using `importlib.util.find_spec` to test for availability" diff --git a/crates/ruff_linter/src/preview.rs b/crates/ruff_linter/src/preview.rs index 0edf643f12..425088baea 100644 --- a/crates/ruff_linter/src/preview.rs +++ b/crates/ruff_linter/src/preview.rs @@ -22,6 +22,11 @@ pub(crate) const fn is_py314_support_enabled(settings: &LinterSettings) -> bool settings.preview.is_enabled() } +// https://github.com/astral-sh/ruff/pull/16565 +pub(crate) const fn is_full_path_match_source_strategy_enabled(settings: &LinterSettings) -> bool { + settings.preview.is_enabled() +} + // Rule-specific behavior // https://github.com/astral-sh/ruff/pull/17136 diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs index 777a7b1237..2ddafcc71e 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs @@ -12,10 +12,12 @@ use crate::checkers::ast::Checker; use crate::codes::Rule; use crate::fix; use crate::importer::ImportedMembers; +use crate::preview::is_full_path_match_source_strategy_enabled; use crate::rules::flake8_type_checking::helpers::{ filter_contained, is_typing_reference, quote_annotation, }; use crate::rules::flake8_type_checking::imports::ImportBinding; +use crate::rules::isort::categorize::MatchSourceStrategy; use crate::rules::isort::{categorize, ImportSection, ImportType}; /// ## What it does @@ -63,6 +65,12 @@ use crate::rules::isort::{categorize, ImportSection, ImportType}; /// return len(sized) /// ``` /// +/// +/// ## Preview +/// When [preview](https://docs.astral.sh/ruff/preview/) is enabled, +/// the criterion for determining whether an import is first-party +/// is stricter, which could affect whether this lint is triggered vs [`TC001`](https://docs.astral.sh/ruff/rules/typing-only-third-party-import/). See [this FAQ section](https://docs.astral.sh/ruff/faq/#how-does-ruff-determine-which-of-my-imports-are-first-party-third-party-etc) for more details. +/// /// ## Options /// - `lint.flake8-type-checking.quote-annotations` /// - `lint.flake8-type-checking.runtime-evaluated-base-classes` @@ -138,6 +146,11 @@ impl Violation for TypingOnlyFirstPartyImport { /// return len(df) /// ``` /// +/// ## Preview +/// When [preview](https://docs.astral.sh/ruff/preview/) is enabled, +/// the criterion for determining whether an import is first-party +/// is stricter, which could affect whether this lint is triggered vs [`TC001`](https://docs.astral.sh/ruff/rules/typing-only-first-party-import/). See [this FAQ section](https://docs.astral.sh/ruff/faq/#how-does-ruff-determine-which-of-my-imports-are-first-party-third-party-etc) for more details. +/// /// ## Options /// - `lint.flake8-type-checking.quote-annotations` /// - `lint.flake8-type-checking.runtime-evaluated-base-classes` @@ -299,9 +312,18 @@ pub(crate) fn typing_only_runtime_import( continue; } + let source_name = import.source_name().join("."); + // Categorize the import, using coarse-grained categorization. + let match_source_strategy = + if is_full_path_match_source_strategy_enabled(checker.settings) { + MatchSourceStrategy::FullPath + } else { + MatchSourceStrategy::Root + }; + let import_type = match categorize( - &qualified_name.to_string(), + &source_name, qualified_name.is_unresolved_import(), &checker.settings.src, checker.package(), @@ -311,6 +333,7 @@ pub(crate) fn typing_only_runtime_import( checker.settings.isort.no_sections, &checker.settings.isort.section_order, &checker.settings.isort.default_section, + match_source_strategy, ) { ImportSection::Known(ImportType::LocalFolder | ImportType::FirstParty) => { ImportType::FirstParty diff --git a/crates/ruff_linter/src/rules/isort/categorize.rs b/crates/ruff_linter/src/rules/isort/categorize.rs index 0ed97f9a67..2e1e5800a2 100644 --- a/crates/ruff_linter/src/rules/isort/categorize.rs +++ b/crates/ruff_linter/src/rules/isort/categorize.rs @@ -1,7 +1,8 @@ use std::collections::BTreeMap; use std::fmt; +use std::fs; +use std::iter; use std::path::{Path, PathBuf}; -use std::{fs, iter}; use log::debug; use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet}; @@ -100,6 +101,7 @@ pub(crate) fn categorize<'a>( no_sections: bool, section_order: &'a [ImportSection], default_section: &'a ImportSection, + match_source_strategy: MatchSourceStrategy, ) -> &'a ImportSection { let module_base = module_name.split('.').next().unwrap(); let (mut import_type, mut reason) = { @@ -127,7 +129,7 @@ pub(crate) fn categorize<'a>( &ImportSection::Known(ImportType::FirstParty), Reason::SamePackage, ) - } else if let Some(src) = match_sources(src, module_base) { + } else if let Some(src) = match_sources(src, module_name, match_source_strategy) { ( &ImportSection::Known(ImportType::FirstParty), Reason::SourceMatch(src), @@ -156,20 +158,64 @@ fn same_package(package: Option>, module_base: &str) -> bool { .is_some_and(|package| package.ends_with(module_base)) } -fn match_sources<'a>(paths: &'a [PathBuf], base: &str) -> Option<&'a Path> { - for path in paths { - if let Ok(metadata) = fs::metadata(path.join(base)) { - if metadata.is_dir() { - return Some(path); +/// Returns the source path with respect to which the module `name` +/// should be considered first party, or `None` if no path is found. +/// +/// The [`MatchSourceStrategy`] is the criterion used to decide whether +/// the module path matches a given source directory. +/// +/// # Examples +/// +/// - The module named `foo` will match `[SRC]` if `[SRC]/foo` is a directory, +/// no matter the strategy. +/// +/// - With `match_source_strategy == MatchSourceStrategy::Root`, the module +/// named `foo.baz` will match `[SRC]` if `[SRC]/foo` is a +/// directory or `[SRC]/foo.py` exists. +/// +/// - With `match_source_stratgy == MatchSourceStrategy::FullPath`, the module +/// named `foo.baz` will match `[SRC]` only if `[SRC]/foo/baz` is a directory, +/// or `[SRC]/foo/baz.py` exists or `[SRC]/foo/baz.pyi` exists. +fn match_sources<'a>( + paths: &'a [PathBuf], + name: &str, + match_source_strategy: MatchSourceStrategy, +) -> Option<&'a Path> { + match match_source_strategy { + MatchSourceStrategy::Root => { + let base = name.split('.').next()?; + for path in paths { + if let Ok(metadata) = fs::metadata(path.join(base)) { + if metadata.is_dir() { + return Some(path); + } + } + if let Ok(metadata) = fs::metadata(path.join(format!("{base}.py"))) { + if metadata.is_file() { + return Some(path); + } + } } + None } - if let Ok(metadata) = fs::metadata(path.join(format!("{base}.py"))) { - if metadata.is_file() { - return Some(path); + MatchSourceStrategy::FullPath => { + let relative_path: PathBuf = name.split('.').collect(); + relative_path.components().next()?; + for root in paths { + let candidate = root.join(&relative_path); + if candidate.is_dir() { + return Some(root); + } + if ["py", "pyi"] + .into_iter() + .any(|extension| candidate.with_extension(extension).is_file()) + { + return Some(root); + } } + None } } - None } #[expect(clippy::too_many_arguments)] @@ -183,6 +229,7 @@ pub(crate) fn categorize_imports<'a>( no_sections: bool, section_order: &'a [ImportSection], default_section: &'a ImportSection, + match_source_strategy: MatchSourceStrategy, ) -> BTreeMap<&'a ImportSection, ImportBlock<'a>> { let mut block_by_type: BTreeMap<&ImportSection, ImportBlock> = BTreeMap::default(); // Categorize `Stmt::Import`. @@ -198,6 +245,7 @@ pub(crate) fn categorize_imports<'a>( no_sections, section_order, default_section, + match_source_strategy, ); block_by_type .entry(import_type) @@ -218,6 +266,7 @@ pub(crate) fn categorize_imports<'a>( no_sections, section_order, default_section, + match_source_strategy, ); block_by_type .entry(classification) @@ -238,6 +287,7 @@ pub(crate) fn categorize_imports<'a>( no_sections, section_order, default_section, + match_source_strategy, ); block_by_type .entry(classification) @@ -258,6 +308,7 @@ pub(crate) fn categorize_imports<'a>( no_sections, section_order, default_section, + match_source_strategy, ); block_by_type .entry(classification) @@ -409,3 +460,463 @@ impl fmt::Display for KnownModules { Ok(()) } } + +/// Rule to determine whether a module path matches +/// a relative path from a source directory. +#[derive(Debug, Clone, Copy)] +pub(crate) enum MatchSourceStrategy { + /// Matches if first term in module path is found in file system + /// + /// # Example + /// Module is `foo.bar.baz` and `[SRC]/foo` exists + Root, + /// Matches only if full module path is reflected in file system + /// + /// # Example + /// Module is `foo.bar.baz` and `[SRC]/foo/bar/baz` exists + FullPath, +} + +#[cfg(test)] +mod tests { + use crate::rules::isort::categorize::{match_sources, MatchSourceStrategy}; + + use std::fs; + use std::path::{Path, PathBuf}; + use tempfile::tempdir; + + /// Helper function to create a file with parent directories + fn create_file>(path: P) { + let path = path.as_ref(); + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).unwrap(); + } + fs::write(path, "").unwrap(); + } + + /// Helper function to create a directory and all parent directories + fn create_dir>(path: P) { + fs::create_dir_all(path).unwrap(); + } + + /// Tests a traditional Python package layout: + /// ``` + /// project/ + /// └── mypackage/ + /// ├── __init__.py + /// ├── module1.py + /// └── module2.py + /// ``` + #[test] + fn test_traditional_layout() { + let temp_dir = tempdir().unwrap(); + let project_dir = temp_dir.path().join("project"); + + // Create traditional layout + create_dir(project_dir.join("mypackage")); + create_file(project_dir.join("mypackage/__init__.py")); + create_file(project_dir.join("mypackage/module1.py")); + create_file(project_dir.join("mypackage/module2.py")); + + let paths = vec![project_dir.clone()]; + + // Test with Root strategy + + assert_eq!( + match_sources(&paths, "mypackage", MatchSourceStrategy::Root), + Some(project_dir.as_path()) + ); + + assert_eq!( + match_sources(&paths, "mypackage.module1", MatchSourceStrategy::Root), + Some(project_dir.as_path()) + ); + + assert_eq!( + match_sources(&paths, "mypackage.nonexistent", MatchSourceStrategy::Root), + Some(project_dir.as_path()) + ); + + assert_eq!( + match_sources(&paths, "nonexistent", MatchSourceStrategy::Root), + None + ); + + // Test with FullPath strategy + + assert_eq!( + match_sources(&paths, "mypackage", MatchSourceStrategy::FullPath), + Some(project_dir.as_path()) + ); + + assert_eq!( + match_sources(&paths, "mypackage.module1", MatchSourceStrategy::FullPath), + Some(project_dir.as_path()) + ); + + // Differs in behavior from [`MatchSourceStrategy::Root`] + assert_eq!( + match_sources( + &paths, + "mypackage.nonexistent", + MatchSourceStrategy::FullPath + ), + None + ); + } + + /// Tests a src-based Python package layout: + /// ``` + /// project/ + /// └── src/ + /// └── mypackage/ + /// ├── __init__.py + /// └── module1.py + /// ``` + #[test] + fn test_src_layout() { + let temp_dir = tempdir().unwrap(); + let project_dir = temp_dir.path().join("project"); + let src_dir = project_dir.join("src"); + + // Create src layout + create_dir(src_dir.join("mypackage")); + create_file(src_dir.join("mypackage/__init__.py")); + create_file(src_dir.join("mypackage/module1.py")); + + let paths = vec![src_dir.clone()]; + + // Test with Root strategy + + assert_eq!( + match_sources(&paths, "mypackage", MatchSourceStrategy::Root), + Some(src_dir.as_path()) + ); + + assert_eq!( + match_sources(&paths, "mypackage.module1", MatchSourceStrategy::Root), + Some(src_dir.as_path()) + ); + + assert_eq!( + match_sources(&paths, "mypackage.nonexistent", MatchSourceStrategy::Root), + Some(src_dir.as_path()) + ); + + // Test with FullPath strategy + + assert_eq!( + match_sources(&paths, "mypackage.module1", MatchSourceStrategy::FullPath), + Some(src_dir.as_path()) + ); + + // Differs in behavior from [`MatchSourceStrategy::Root`] + assert_eq!( + match_sources( + &paths, + "mypackage.nonexistent", + MatchSourceStrategy::FullPath + ), + None + ); + } + + /// Tests a nested package layout: + /// ``` + /// project/ + /// └── mypackage/ + /// ├── __init__.py + /// ├── module1.py + /// └── subpackage/ + /// ├── __init__.py + /// └── module2.py + /// ``` + #[test] + fn test_nested_packages() { + let temp_dir = tempdir().unwrap(); + let project_dir = temp_dir.path().join("project"); + + // Create nested package layout + create_dir(project_dir.join("mypackage/subpackage")); + create_file(project_dir.join("mypackage/__init__.py")); + create_file(project_dir.join("mypackage/module1.py")); + create_file(project_dir.join("mypackage/subpackage/__init__.py")); + create_file(project_dir.join("mypackage/subpackage/module2.py")); + + let paths = vec![project_dir.clone()]; + + // Test with Root strategy + assert_eq!( + match_sources(&paths, "mypackage", MatchSourceStrategy::Root), + Some(project_dir.as_path()) + ); + + assert_eq!( + match_sources(&paths, "mypackage.subpackage", MatchSourceStrategy::Root), + Some(project_dir.as_path()) + ); + + // Test with FullPath strategy + + assert_eq!( + match_sources( + &paths, + "mypackage.subpackage.module2", + MatchSourceStrategy::FullPath + ), + Some(project_dir.as_path()) + ); + + // Differs in behavior from [`MatchSourceStrategy::Root`] + assert_eq!( + match_sources( + &paths, + "mypackage.subpackage.nonexistent", + MatchSourceStrategy::FullPath + ), + None + ); + } + + /// Tests a namespace package layout (PEP 420): + /// ``` + /// project/ + /// └── namespace/ # No __init__.py (namespace package) + /// └── package1/ + /// ├── __init__.py + /// └── module1.py + /// ``` + #[test] + fn test_namespace_packages() { + let temp_dir = tempdir().unwrap(); + let project_dir = temp_dir.path().join("project"); + + // Create namespace package layout + create_dir(project_dir.join("namespace/package1")); + create_file(project_dir.join("namespace/package1/__init__.py")); + create_file(project_dir.join("namespace/package1/module1.py")); + + let paths = vec![project_dir.clone()]; + // Test with Root strategy + + assert_eq!( + match_sources(&paths, "namespace", MatchSourceStrategy::Root), + Some(project_dir.as_path()) + ); + + assert_eq!( + match_sources(&paths, "namespace.package1", MatchSourceStrategy::Root), + Some(project_dir.as_path()) + ); + + assert_eq!( + match_sources( + &paths, + "namespace.package2.module1", + MatchSourceStrategy::Root + ), + Some(project_dir.as_path()) + ); + + // Test with FullPath strategy + + assert_eq!( + match_sources(&paths, "namespace.package1", MatchSourceStrategy::FullPath), + Some(project_dir.as_path()) + ); + + assert_eq!( + match_sources( + &paths, + "namespace.package1.module1", + MatchSourceStrategy::FullPath + ), + Some(project_dir.as_path()) + ); + + // Differs in behavior from [`MatchSourceStrategy::Root`] + assert_eq!( + match_sources( + &paths, + "namespace.package2.module1", + MatchSourceStrategy::FullPath + ), + None + ); + } + + /// Tests a package with type stubs (.pyi files): + /// ``` + /// project/ + /// └── mypackage/ + /// ├── __init__.py + /// └── module1.pyi # Only .pyi file, no .py + /// ``` + #[test] + fn test_type_stubs() { + let temp_dir = tempdir().unwrap(); + let project_dir = temp_dir.path().join("project"); + + // Create package with type stub + create_dir(project_dir.join("mypackage")); + create_file(project_dir.join("mypackage/__init__.py")); + create_file(project_dir.join("mypackage/module1.pyi")); // Only create .pyi file, not .py + + // Test with FullPath strategy + let paths = vec![project_dir.clone()]; + + // Module "mypackage.module1" should match project_dir using .pyi file + assert_eq!( + match_sources(&paths, "mypackage.module1", MatchSourceStrategy::FullPath), + Some(project_dir.as_path()) + ); + } + + /// Tests a package with both a module and a directory having the same name: + /// ``` + /// project/ + /// └── mypackage/ + /// ├── __init__.py + /// ├── feature.py # Module with same name as directory + /// └── feature/ # Directory with same name as module + /// ├── __init__.py + /// └── submodule.py + /// ``` + #[test] + fn test_same_name_module_and_directory() { + let temp_dir = tempdir().unwrap(); + let project_dir = temp_dir.path().join("project"); + + // Create package with module and directory of the same name + create_dir(project_dir.join("mypackage/feature")); + create_file(project_dir.join("mypackage/__init__.py")); + create_file(project_dir.join("mypackage/feature.py")); // Module with same name as directory + create_file(project_dir.join("mypackage/feature/__init__.py")); + create_file(project_dir.join("mypackage/feature/submodule.py")); + + // Test with Root strategy + let paths = vec![project_dir.clone()]; + + // Module "mypackage.feature" should match project_dir (matches the file first) + assert_eq!( + match_sources(&paths, "mypackage.feature", MatchSourceStrategy::Root), + Some(project_dir.as_path()) + ); + + // Test with FullPath strategy + + // Module "mypackage.feature" should match project_dir + assert_eq!( + match_sources(&paths, "mypackage.feature", MatchSourceStrategy::FullPath), + Some(project_dir.as_path()) + ); + + // Module "mypackage.feature.submodule" should match project_dir + assert_eq!( + match_sources( + &paths, + "mypackage.feature.submodule", + MatchSourceStrategy::FullPath + ), + Some(project_dir.as_path()) + ); + } + + /// Tests multiple source directories with different packages: + /// ``` + /// project1/ + /// └── package1/ + /// ├── __init__.py + /// └── module1.py + /// + /// project2/ + /// └── package2/ + /// ├── __init__.py + /// └── module2.py + /// ``` + #[test] + fn test_multiple_source_paths() { + let temp_dir = tempdir().unwrap(); + let project1_dir = temp_dir.path().join("project1"); + let project2_dir = temp_dir.path().join("project2"); + + // Create files in project1 + create_dir(project1_dir.join("package1")); + create_file(project1_dir.join("package1/__init__.py")); + create_file(project1_dir.join("package1/module1.py")); + + // Create files in project2 + create_dir(project2_dir.join("package2")); + create_file(project2_dir.join("package2/__init__.py")); + create_file(project2_dir.join("package2/module2.py")); + + // Test with multiple paths in search order + let paths = vec![project1_dir.clone(), project2_dir.clone()]; + + // Module "package1" should match project1_dir + assert_eq!( + match_sources(&paths, "package1", MatchSourceStrategy::Root), + Some(project1_dir.as_path()) + ); + + // Module "package2" should match project2_dir + assert_eq!( + match_sources(&paths, "package2", MatchSourceStrategy::Root), + Some(project2_dir.as_path()) + ); + + // Try with reversed order to check search order + let paths_reversed = vec![project2_dir, project1_dir.clone()]; + + // Module "package1" should still match project1_dir + assert_eq!( + match_sources(&paths_reversed, "package1", MatchSourceStrategy::Root), + Some(project1_dir.as_path()) + ); + } + + /// Tests behavior with an empty module name + /// ``` + /// project/ + /// └── mypackage/ + /// ``` + /// + /// In theory this should never happen since we expect + /// module names to have been normalized by the time we + /// call `match_sources`. But it is worth noting that the + /// behavior is different depending on the [`MatchSourceStrategy`] + #[test] + fn test_empty_module_name() { + let temp_dir = tempdir().unwrap(); + let project_dir = temp_dir.path().join("project"); + + create_dir(project_dir.join("mypackage")); + + let paths = vec![project_dir.clone()]; + + assert_eq!( + match_sources(&paths, "", MatchSourceStrategy::Root), + Some(project_dir.as_path()) + ); + assert_eq!( + match_sources(&paths, "", MatchSourceStrategy::FullPath), + None + ); + } + + /// Tests behavior with an empty list of source paths + #[test] + fn test_empty_paths() { + let paths: Vec = vec![]; + + // Empty paths should return None + assert_eq!( + match_sources(&paths, "mypackage", MatchSourceStrategy::Root), + None + ); + assert_eq!( + match_sources(&paths, "mypackage", MatchSourceStrategy::FullPath), + None + ); + } +} diff --git a/crates/ruff_linter/src/rules/isort/mod.rs b/crates/ruff_linter/src/rules/isort/mod.rs index 05d516f663..782b2ec1ea 100644 --- a/crates/ruff_linter/src/rules/isort/mod.rs +++ b/crates/ruff_linter/src/rules/isort/mod.rs @@ -5,7 +5,7 @@ use std::path::PathBuf; use annotate::annotate_imports; use block::{Block, Trailer}; pub(crate) use categorize::categorize; -use categorize::categorize_imports; +use categorize::{categorize_imports, MatchSourceStrategy}; pub use categorize::{ImportSection, ImportType}; use comments::Comment; use normalize::normalize_imports; @@ -76,6 +76,7 @@ pub(crate) fn format_imports( source_type: PySourceType, target_version: PythonVersion, settings: &Settings, + match_source_strategy: MatchSourceStrategy, tokens: &Tokens, ) -> String { let trailer = &block.trailer; @@ -103,6 +104,7 @@ pub(crate) fn format_imports( package, target_version, settings, + match_source_strategy, ); if !block_output.is_empty() && !output.is_empty() { @@ -159,6 +161,7 @@ fn format_import_block( package: Option>, target_version: PythonVersion, settings: &Settings, + match_source_strategy: MatchSourceStrategy, ) -> String { #[derive(Debug, Copy, Clone, PartialEq, Eq)] enum LineInsertion { @@ -169,7 +172,6 @@ fn format_import_block( Inserted, } - // Categorize by type (e.g., first-party vs. third-party). let mut block_by_type = categorize_imports( block, src, @@ -180,6 +182,7 @@ fn format_import_block( settings.no_sections, &settings.section_order, &settings.default_section, + match_source_strategy, ); let mut output = String::new(); diff --git a/crates/ruff_linter/src/rules/isort/rules/organize_imports.rs b/crates/ruff_linter/src/rules/isort/rules/organize_imports.rs index b982eae70b..30ec8be7d1 100644 --- a/crates/ruff_linter/src/rules/isort/rules/organize_imports.rs +++ b/crates/ruff_linter/src/rules/isort/rules/organize_imports.rs @@ -15,6 +15,8 @@ use super::super::block::Block; use super::super::{comments, format_imports}; use crate::line_width::LineWidthBuilder; use crate::package::PackageRoot; +use crate::preview::is_full_path_match_source_strategy_enabled; +use crate::rules::isort::categorize::MatchSourceStrategy; use crate::settings::LinterSettings; use crate::Locator; @@ -36,6 +38,13 @@ use crate::Locator; /// import numpy as np /// import pandas /// ``` +/// +/// ## Preview +/// When [`preview`](https://docs.astral.sh/ruff/preview/) mode is enabled, Ruff applies a stricter criterion +/// for determining whether an import should be classified as first-party. +/// Specifically, for an import of the form `import foo.bar.baz`, Ruff will +/// check that `foo/bar`, relative to a [user-specified `src`](https://docs.astral.sh/ruff/settings/#src) directory, contains either +/// the directory `baz` or else a file with the name `baz.py` or `baz.pyi`. #[derive(ViolationMetadata)] pub(crate) struct UnsortedImports; @@ -117,6 +126,12 @@ pub(crate) fn organize_imports( trailing_lines_end(block.imports.last().unwrap(), locator.contents()) }; + let match_source_strategy = if is_full_path_match_source_strategy_enabled(settings) { + MatchSourceStrategy::FullPath + } else { + MatchSourceStrategy::Root + }; + // Generate the sorted import block. let expected = format_imports( block, @@ -130,6 +145,7 @@ pub(crate) fn organize_imports( source_type, target_version, &settings.isort, + match_source_strategy, tokens, ); diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs b/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs index 91fcb878a2..2cd22c8510 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs @@ -16,8 +16,11 @@ use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; use crate::fix; -use crate::preview::is_dunder_init_fix_unused_import_enabled; +use crate::preview::{ + is_dunder_init_fix_unused_import_enabled, is_full_path_match_source_strategy_enabled, +}; use crate::registry::Rule; +use crate::rules::isort::categorize::MatchSourceStrategy; use crate::rules::{isort, isort::ImportSection, isort::ImportType}; /// ## What it does @@ -88,6 +91,11 @@ use crate::rules::{isort, isort::ImportSection, isort::ImportType}; /// print("numpy is not installed") /// ``` /// +/// ## Preview +/// When [preview](https://docs.astral.sh/ruff/preview/) is enabled, +/// the criterion for determining whether an import is first-party +/// is stricter, which could affect the suggested fix. See [this FAQ section](https://docs.astral.sh/ruff/faq/#how-does-ruff-determine-which-of-my-imports-are-first-party-third-party-etc) for more details. +/// /// ## Options /// - `lint.ignore-init-module-imports` /// - `lint.pyflakes.allowed-unused-imports` @@ -222,10 +230,15 @@ enum UnusedImportContext { } fn is_first_party(import: &AnyImport, checker: &Checker) -> bool { - let qualified_name = import.qualified_name(); + let source_name = import.source_name().join("."); + let match_source_strategy = if is_full_path_match_source_strategy_enabled(checker.settings) { + MatchSourceStrategy::FullPath + } else { + MatchSourceStrategy::Root + }; let category = isort::categorize( - &qualified_name.to_string(), - qualified_name.is_unresolved_import(), + &source_name, + import.qualified_name().is_unresolved_import(), &checker.settings.src, checker.package(), checker.settings.isort.detect_same_package, @@ -234,6 +247,7 @@ fn is_first_party(import: &AnyImport, checker: &Checker) -> bool { checker.settings.isort.no_sections, &checker.settings.isort.section_order, &checker.settings.isort.default_section, + match_source_strategy, ); matches! { category, diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index a2f9c8ee34..73614a7a16 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -714,6 +714,15 @@ pub trait Imported<'a> { /// Returns the member name of the imported symbol. For a straight import, this is equivalent /// to the qualified name; for a `from` import, this is the name of the imported symbol. fn member_name(&self) -> Cow<'a, str>; + + /// Returns the source module of the imported symbol. + /// + /// For example: + /// + /// - `import foo` returns `["foo"]` + /// - `import foo.bar` returns `["foo","bar"]` + /// - `from foo import bar` returns `["foo"]` + fn source_name(&self) -> &[&'a str]; } impl<'a> Imported<'a> for Import<'a> { @@ -731,6 +740,10 @@ impl<'a> Imported<'a> for Import<'a> { fn member_name(&self) -> Cow<'a, str> { Cow::Owned(self.qualified_name().to_string()) } + + fn source_name(&self) -> &[&'a str] { + self.qualified_name.segments() + } } impl<'a> Imported<'a> for SubmoduleImport<'a> { @@ -748,6 +761,10 @@ impl<'a> Imported<'a> for SubmoduleImport<'a> { fn member_name(&self) -> Cow<'a, str> { Cow::Owned(self.qualified_name().to_string()) } + + fn source_name(&self) -> &[&'a str] { + self.qualified_name.segments() + } } impl<'a> Imported<'a> for FromImport<'a> { @@ -765,6 +782,10 @@ impl<'a> Imported<'a> for FromImport<'a> { fn member_name(&self) -> Cow<'a, str> { Cow::Borrowed(self.qualified_name.segments()[self.qualified_name.segments().len() - 1]) } + + fn source_name(&self) -> &[&'a str] { + self.module_name() + } } /// A wrapper around an import [`BindingKind`] that can be any of the three types of imports. @@ -799,6 +820,14 @@ impl<'ast> Imported<'ast> for AnyImport<'_, 'ast> { Self::FromImport(import) => import.member_name(), } } + + fn source_name(&self) -> &[&'ast str] { + match self { + Self::Import(import) => import.source_name(), + Self::SubmoduleImport(import) => import.source_name(), + Self::FromImport(import) => import.source_name(), + } + } } #[cfg(test)] diff --git a/docs/faq.md b/docs/faq.md index d2dde46530..6ed942e1f3 100644 --- a/docs/faq.md +++ b/docs/faq.md @@ -309,7 +309,31 @@ my_project When Ruff sees an import like `import foo`, it will then iterate over the `src` directories, looking for a corresponding Python module (in reality, a directory named `foo` or a file named -`foo.py`). +`foo.py`). For module paths with multiple components like `import foo.bar`, +the default behavior is to search only for a directory named `foo` or a file +named `foo.py`. However, if `preview` is enabled, Ruff will require that the full relative path `foo/bar` exists as a directory, or that `foo/bar.py` or `foo/bar.pyi` exist as files. Finally, imports of the form `from foo import bar`, Ruff will only use `foo` when determining whether a module is first-party or third-party. + +If there is a directory +whose name matches a third-party package, but does not contain Python code, +it could happen that the above algorithm incorrectly infers an import to be first-party. +To prevent this, you can modify the [`known-third-party`](settings.md#lint_isort_known-third-party) setting. For example, if you import +the package `wandb` but also have a subdirectory of your `src` with +the same name, you can add the following: + +=== "pyproject.toml" + + ```toml + [tool.ruff.lint.isort] + known-third-party = ["wandb"] + ``` + +=== "ruff.toml" + + ```toml + [lint.isort] + known-third-party = ["wandb"] + ``` + If the `src` field is omitted, Ruff will default to using the "project root", along with a `"src"` subdirectory, as the first-party sources, to support both flat and nested project layouts.