mirror of
https://github.com/astral-sh/ruff.git
synced 2025-07-07 13:15:06 +00:00
[isort] Check full module path against project root(s) when categorizing first-party (#16565)
When attempting to determine whether `import foo.bar.baz` is a known first-party import relative to [user-provided source paths](https://docs.astral.sh/ruff/settings/#src), when `preview` is enabled we now check that `SRC/foo/bar/baz` is a directory or `SRC/foo/bar/baz.py` or `SRC/foo/bar/baz.pyi` exist. Previously, we just checked the analogous thing for `SRC/foo`, but this can be misleading in situations with disjoint namespace packages that share a common base name (e.g. we may be working inside the namespace package `foo.buzz` and importing `foo.bar` from elsewhere). Supersedes #12987 Closes #12984
This commit is contained in:
Dylan
GitHub
parent
5e2c818417
commit
965a4dd731
12 changed files with 652 additions and 21 deletions
1
Cargo.lock
generated
1
Cargo.lock
generated
|
|
@ -2834,6 +2834,7 @@ dependencies = [
|
|||
"smallvec",
|
||||
"strum",
|
||||
"strum_macros",
|
||||
"tempfile",
|
||||
"test-case",
|
||||
"thiserror 2.0.12",
|
||||
"toml",
|
||||
|
|
|
|||
|
|
@ -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`
|
||||
|
|
|
|||
|
|
@ -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 = []
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<PackageRoot<'_>>, 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<P: AsRef<Path>>(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<P: AsRef<Path>>(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<PathBuf> = vec![];
|
||||
|
||||
// Empty paths should return None
|
||||
assert_eq!(
|
||||
match_sources(&paths, "mypackage", MatchSourceStrategy::Root),
|
||||
None
|
||||
);
|
||||
assert_eq!(
|
||||
match_sources(&paths, "mypackage", MatchSourceStrategy::FullPath),
|
||||
None
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<PackageRoot<'_>>,
|
||||
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();
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
);
|
||||
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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)]
|
||||
|
|
|
|||
26
docs/faq.md
26
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.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue