From c7d48e10e6ee6ff9857472df6714c27fe860b1cf Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 9 Nov 2024 22:03:34 -0500 Subject: [PATCH] Detect empty implicit namespace packages (#14236) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary The implicit namespace package rule currently fails to detect cases like the following: ```text foo/ ├── __init__.py └── bar/ └── baz/ └── __init__.py ``` The problem is that we detect a root at `foo`, and then an independent root at `baz`. We _would_ detect that `bar` is an implicit namespace package, but it doesn't contain any files! So we never check it, and have no place to raise the diagnostic. This PR adds detection for these kinds of nested packages, and augments the `INP` rule to flag the `__init__.py` file above with a specialized message. As a side effect, I've introduced a dedicated `PackageRoot` struct which we can pass around in lieu of Yet Another `Path`. For now, I'm only enabling this in preview (and the approach doesn't affect any other rules). It's a bug fix, but it may end up expanding the rule. Closes https://github.com/astral-sh/ruff/issues/13519. --- crates/ruff_linter/__init__.py => bar/baz.py | 0 crates/ruff/src/cache.rs | 14 +++-- crates/ruff/src/commands/analyze_graph.rs | 8 ++- crates/ruff/src/commands/check.rs | 7 ++- crates/ruff/src/commands/check_stdin.rs | 3 +- crates/ruff/src/commands/format.rs | 5 +- crates/ruff/src/diagnostics.rs | 5 +- crates/ruff/tests/lint.rs | 61 +++++++++++++++++-- crates/ruff_linter/src/checkers/ast/mod.rs | 15 +++-- crates/ruff_linter/src/checkers/filesystem.rs | 4 +- crates/ruff_linter/src/checkers/imports.rs | 4 +- crates/ruff_linter/src/lib.rs | 1 + crates/ruff_linter/src/linter.rs | 9 +-- crates/ruff_linter/src/package.rs | 40 ++++++++++++ .../rules/builtin_module_shadowing.rs | 8 +-- .../src/rules/flake8_no_pep420/mod.rs | 5 +- .../rules/implicit_namespace_package.rs | 51 +++++++++++++--- .../ruff_linter/src/rules/isort/categorize.rs | 16 ++--- crates/ruff_linter/src/rules/isort/mod.rs | 7 ++- .../src/rules/isort/rules/organize_imports.rs | 12 ++-- .../pep8_naming/rules/invalid_module_name.rs | 8 +-- .../rules/pylint/rules/import_private_name.rs | 2 + crates/ruff_linter/src/test.rs | 4 +- crates/ruff_server/src/fix.rs | 17 +++--- crates/ruff_server/src/lint.rs | 15 ++--- crates/ruff_workspace/src/resolver.rs | 44 +++++++++++-- 26 files changed, 282 insertions(+), 83 deletions(-) rename crates/ruff_linter/__init__.py => bar/baz.py (100%) create mode 100644 crates/ruff_linter/src/package.rs diff --git a/crates/ruff_linter/__init__.py b/bar/baz.py similarity index 100% rename from crates/ruff_linter/__init__.py rename to bar/baz.py diff --git a/crates/ruff/src/cache.rs b/crates/ruff/src/cache.rs index 814f6e0348..40b495189a 100644 --- a/crates/ruff/src/cache.rs +++ b/crates/ruff/src/cache.rs @@ -20,6 +20,7 @@ use tempfile::NamedTempFile; use ruff_cache::{CacheKey, CacheKeyHasher}; use ruff_diagnostics::{DiagnosticKind, Fix}; use ruff_linter::message::{DiagnosticMessage, Message}; +use ruff_linter::package::PackageRoot; use ruff_linter::{warn_user, VERSION}; use ruff_macros::CacheKey; use ruff_notebook::NotebookIndex; @@ -497,7 +498,7 @@ pub(crate) struct PackageCacheMap<'a>(FxHashMap<&'a Path, Cache>); impl<'a> PackageCacheMap<'a> { pub(crate) fn init( - package_roots: &FxHashMap<&'a Path, Option<&'a Path>>, + package_roots: &FxHashMap<&'a Path, Option>>, resolver: &Resolver, ) -> Self { fn init_cache(path: &Path) { @@ -513,7 +514,9 @@ impl<'a> PackageCacheMap<'a> { Self( package_roots .iter() - .map(|(package, package_root)| package_root.unwrap_or(package)) + .map(|(package, package_root)| { + package_root.map(PackageRoot::path).unwrap_or(package) + }) .unique() .par_bridge() .map(|cache_root| { @@ -587,6 +590,7 @@ mod tests { use ruff_cache::CACHE_DIR_NAME; use ruff_linter::message::Message; + use ruff_linter::package::PackageRoot; use ruff_linter::settings::flags; use ruff_linter::settings::types::UnsafeFixes; use ruff_python_ast::PySourceType; @@ -641,7 +645,7 @@ mod tests { let diagnostics = lint_path( &path, - Some(&package_root), + Some(PackageRoot::root(&package_root)), &settings.linter, Some(&cache), flags::Noqa::Enabled, @@ -683,7 +687,7 @@ mod tests { for path in paths { got_diagnostics += lint_path( &path, - Some(&package_root), + Some(PackageRoot::root(&package_root)), &settings.linter, Some(&cache), flags::Noqa::Enabled, @@ -1056,7 +1060,7 @@ mod tests { ) -> Result { lint_path( &self.package_root.join(path), - Some(&self.package_root), + Some(PackageRoot::root(&self.package_root)), &self.settings.linter, Some(cache), flags::Noqa::Enabled, diff --git a/crates/ruff/src/commands/analyze_graph.rs b/crates/ruff/src/commands/analyze_graph.rs index f95592623e..f8a0684d42 100644 --- a/crates/ruff/src/commands/analyze_graph.rs +++ b/crates/ruff/src/commands/analyze_graph.rs @@ -6,6 +6,7 @@ use log::{debug, warn}; use path_absolutize::CWD; use ruff_db::system::{SystemPath, SystemPathBuf}; use ruff_graph::{Direction, ImportMap, ModuleDb, ModuleImports}; +use ruff_linter::package::PackageRoot; use ruff_linter::{warn_user, warn_user_once}; use ruff_python_ast::{PySourceType, SourceType}; use ruff_workspace::resolver::{match_exclusion, python_files_in_path, ResolvedFile}; @@ -49,7 +50,12 @@ pub(crate) fn analyze_graph( .collect::>(), ) .into_iter() - .map(|(path, package)| (path.to_path_buf(), package.map(Path::to_path_buf))) + .map(|(path, package)| { + ( + path.to_path_buf(), + package.map(PackageRoot::path).map(Path::to_path_buf), + ) + }) .collect::>(); // Create a database from the source roots. diff --git a/crates/ruff/src/commands/check.rs b/crates/ruff/src/commands/check.rs index 24bc3d94d9..0a547b3551 100644 --- a/crates/ruff/src/commands/check.rs +++ b/crates/ruff/src/commands/check.rs @@ -13,6 +13,7 @@ use rustc_hash::FxHashMap; use ruff_diagnostics::Diagnostic; use ruff_linter::message::Message; +use ruff_linter::package::PackageRoot; use ruff_linter::registry::Rule; use ruff_linter::settings::types::UnsafeFixes; use ruff_linter::settings::{flags, LinterSettings}; @@ -87,7 +88,9 @@ pub(crate) fn check( return None; } - let cache_root = package.unwrap_or_else(|| path.parent().unwrap_or(path)); + let cache_root = package + .map(PackageRoot::path) + .unwrap_or_else(|| path.parent().unwrap_or(path)); let cache = caches.get(cache_root); lint_path( @@ -181,7 +184,7 @@ pub(crate) fn check( #[allow(clippy::too_many_arguments)] fn lint_path( path: &Path, - package: Option<&Path>, + package: Option>, settings: &LinterSettings, cache: Option<&Cache>, noqa: flags::Noqa, diff --git a/crates/ruff/src/commands/check_stdin.rs b/crates/ruff/src/commands/check_stdin.rs index d300dd4c2a..4e7be0b540 100644 --- a/crates/ruff/src/commands/check_stdin.rs +++ b/crates/ruff/src/commands/check_stdin.rs @@ -1,7 +1,7 @@ use std::path::Path; use anyhow::Result; - +use ruff_linter::package::PackageRoot; use ruff_linter::packaging; use ruff_linter::settings::flags; use ruff_workspace::resolver::{match_exclusion, python_file_at_path, PyprojectConfig, Resolver}; @@ -42,6 +42,7 @@ pub(crate) fn check_stdin( let stdin = read_from_stdin()?; let package_root = filename.and_then(Path::parent).and_then(|path| { packaging::detect_package_root(path, &resolver.base_settings().linter.namespace_packages) + .map(PackageRoot::root) }); let mut diagnostics = lint_stdin( filename, diff --git a/crates/ruff/src/commands/format.rs b/crates/ruff/src/commands/format.rs index 4ad789d139..d346a323f9 100644 --- a/crates/ruff/src/commands/format.rs +++ b/crates/ruff/src/commands/format.rs @@ -18,6 +18,7 @@ use tracing::debug; use ruff_diagnostics::SourceMap; use ruff_linter::fs; use ruff_linter::logging::{DisplayParseError, LogLevel}; +use ruff_linter::package::PackageRoot; use ruff_linter::registry::Rule; use ruff_linter::rules::flake8_quotes::settings::Quote; use ruff_linter::source_kind::{SourceError, SourceKind}; @@ -136,7 +137,9 @@ pub(crate) fn format( .parent() .and_then(|parent| package_roots.get(parent).copied()) .flatten(); - let cache_root = package.unwrap_or_else(|| path.parent().unwrap_or(path)); + let cache_root = package + .map(PackageRoot::path) + .unwrap_or_else(|| path.parent().unwrap_or(path)); let cache = caches.get(cache_root); Some( diff --git a/crates/ruff/src/diagnostics.rs b/crates/ruff/src/diagnostics.rs index 99fab94051..620be66454 100644 --- a/crates/ruff/src/diagnostics.rs +++ b/crates/ruff/src/diagnostics.rs @@ -16,6 +16,7 @@ use ruff_diagnostics::Diagnostic; use ruff_linter::codes::Rule; use ruff_linter::linter::{lint_fix, lint_only, FixTable, FixerResult, LinterResult, ParseSource}; use ruff_linter::message::{Message, SyntaxErrorMessage}; +use ruff_linter::package::PackageRoot; use ruff_linter::pyproject_toml::lint_pyproject_toml; use ruff_linter::settings::types::UnsafeFixes; use ruff_linter::settings::{flags, LinterSettings}; @@ -180,7 +181,7 @@ impl AddAssign for FixMap { /// Lint the source code at the given `Path`. pub(crate) fn lint_path( path: &Path, - package: Option<&Path>, + package: Option>, settings: &LinterSettings, cache: Option<&Cache>, noqa: flags::Noqa, @@ -373,7 +374,7 @@ pub(crate) fn lint_path( /// stdin. pub(crate) fn lint_stdin( path: Option<&Path>, - package: Option<&Path>, + package: Option>, contents: String, settings: &Settings, noqa: flags::Noqa, diff --git a/crates/ruff/tests/lint.rs b/crates/ruff/tests/lint.rs index 450e8e7158..eb00486a9a 100644 --- a/crates/ruff/tests/lint.rs +++ b/crates/ruff/tests/lint.rs @@ -8,6 +8,7 @@ use std::process::Command; use std::str; use anyhow::Result; +use assert_fs::fixture::{ChildPath, FileTouch, PathChild}; use insta_cmd::{assert_cmd_snapshot, get_cargo_bin}; use tempfile::TempDir; @@ -1224,10 +1225,7 @@ fn negated_per_file_ignores_absolute() -> Result<()> { let ignored = tempdir.path().join("ignored.py"); fs::write(ignored, "")?; - insta::with_settings!({filters => vec![ - // Replace windows paths - (r"\\", "/"), - ]}, { + insta::with_settings!({filters => vec![(r"\\", "/")]}, { assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) .args(STDIN_BASE_OPTIONS) .arg("--config") @@ -1918,3 +1916,58 @@ fn checks_notebooks_in_stable() -> anyhow::Result<()> { "###); Ok(()) } + +/// Verify that implicit namespace packages are detected even when they are nested. +/// +/// See: +#[test] +fn nested_implicit_namespace_package() -> Result<()> { + let tempdir = TempDir::new()?; + let root = ChildPath::new(tempdir.path()); + + root.child("foo").child("__init__.py").touch()?; + root.child("foo") + .child("bar") + .child("baz") + .child("__init__.py") + .touch()?; + root.child("foo") + .child("bar") + .child("baz") + .child("bop.py") + .touch()?; + + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(STDIN_BASE_OPTIONS) + .arg("--select") + .arg("INP") + .current_dir(&tempdir) + , @r###" + success: true + exit_code: 0 + ----- stdout ----- + All checks passed! + + ----- stderr ----- + "###); + + insta::with_settings!({filters => vec![(r"\\", "/")]}, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(STDIN_BASE_OPTIONS) + .arg("--select") + .arg("INP") + .arg("--preview") + .current_dir(&tempdir) + , @r###" + success: false + exit_code: 1 + ----- stdout ----- + foo/bar/baz/__init__.py:1:1: INP001 File `foo/bar/baz/__init__.py` declares a package, but is nested under an implicit namespace package. Add an `__init__.py` to `foo/bar`. + Found 1 error. + + ----- stderr ----- + "###); + }); + + Ok(()) +} diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index 86852bf950..ce3dcaf9b0 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -66,6 +66,7 @@ use crate::checkers::ast::annotation::AnnotationContext; use crate::docstrings::extraction::ExtractionTarget; use crate::importer::Importer; use crate::noqa::NoqaMapping; +use crate::package::PackageRoot; use crate::registry::Rule; use crate::rules::{flake8_pyi, flake8_type_checking, pyflakes, pyupgrade}; use crate::settings::{flags, LinterSettings}; @@ -186,7 +187,7 @@ pub(crate) struct Checker<'a> { /// The [`Path`] to the file under analysis. path: &'a Path, /// The [`Path`] to the package containing the current file. - package: Option<&'a Path>, + package: Option>, /// The module representation of the current file (e.g., `foo.bar`). module: Module<'a>, /// The [`PySourceType`] of the current file. @@ -238,7 +239,7 @@ impl<'a> Checker<'a> { noqa_line_for: &'a NoqaMapping, noqa: flags::Noqa, path: &'a Path, - package: Option<&'a Path>, + package: Option>, module: Module<'a>, locator: &'a Locator, stylist: &'a Stylist, @@ -247,7 +248,7 @@ impl<'a> Checker<'a> { cell_offsets: Option<&'a CellOffsets>, notebook_index: Option<&'a NotebookIndex>, ) -> Checker<'a> { - Checker { + Self { parsed, parsed_type_annotation: None, parsed_annotations_cache: ParsedAnnotationsCache::new(parsed_annotations_arena), @@ -383,7 +384,7 @@ impl<'a> Checker<'a> { } /// The [`Path`] to the package containing the current file. - pub(crate) const fn package(&self) -> Option<&'a Path> { + pub(crate) const fn package(&self) -> Option> { self.package } @@ -2483,12 +2484,14 @@ pub(crate) fn check_ast( settings: &LinterSettings, noqa: flags::Noqa, path: &Path, - package: Option<&Path>, + package: Option>, source_type: PySourceType, cell_offsets: Option<&CellOffsets>, notebook_index: Option<&NotebookIndex>, ) -> Vec { - let module_path = package.and_then(|package| to_module_path(package, path)); + let module_path = package + .map(PackageRoot::path) + .and_then(|package| to_module_path(package, path)); let module = Module { kind: if path.ends_with("__init__.py") { ModuleKind::Package diff --git a/crates/ruff_linter/src/checkers/filesystem.rs b/crates/ruff_linter/src/checkers/filesystem.rs index 669ee4877f..15ba2ed5f6 100644 --- a/crates/ruff_linter/src/checkers/filesystem.rs +++ b/crates/ruff_linter/src/checkers/filesystem.rs @@ -3,6 +3,7 @@ use std::path::Path; use ruff_diagnostics::Diagnostic; use ruff_python_trivia::CommentRanges; +use crate::package::PackageRoot; use crate::registry::Rule; use crate::rules::flake8_builtins::rules::builtin_module_shadowing; use crate::rules::flake8_no_pep420::rules::implicit_namespace_package; @@ -12,7 +13,7 @@ use crate::Locator; pub(crate) fn check_file_path( path: &Path, - package: Option<&Path>, + package: Option>, locator: &Locator, comment_ranges: &CommentRanges, settings: &LinterSettings, @@ -28,6 +29,7 @@ pub(crate) fn check_file_path( comment_ranges, &settings.project_root, &settings.src, + settings.preview, ) { diagnostics.push(diagnostic); } diff --git a/crates/ruff_linter/src/checkers/imports.rs b/crates/ruff_linter/src/checkers/imports.rs index d956167553..7b3f1462c2 100644 --- a/crates/ruff_linter/src/checkers/imports.rs +++ b/crates/ruff_linter/src/checkers/imports.rs @@ -1,5 +1,4 @@ //! Lint rules based on import analysis. -use std::path::Path; use ruff_diagnostics::Diagnostic; use ruff_notebook::CellOffsets; @@ -10,6 +9,7 @@ use ruff_python_index::Indexer; use ruff_python_parser::Parsed; use crate::directives::IsortDirectives; +use crate::package::PackageRoot; use crate::registry::Rule; use crate::rules::isort; use crate::rules::isort::block::{Block, BlockBuilder}; @@ -24,7 +24,7 @@ pub(crate) fn check_imports( directives: &IsortDirectives, settings: &LinterSettings, stylist: &Stylist, - package: Option<&Path>, + package: Option>, source_type: PySourceType, cell_offsets: Option<&CellOffsets>, ) -> Vec { diff --git a/crates/ruff_linter/src/lib.rs b/crates/ruff_linter/src/lib.rs index e9641737d8..5c024cb5f8 100644 --- a/crates/ruff_linter/src/lib.rs +++ b/crates/ruff_linter/src/lib.rs @@ -32,6 +32,7 @@ mod locator; pub mod logging; pub mod message; mod noqa; +pub mod package; pub mod packaging; pub mod pyproject_toml; pub mod registry; diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index 375c81715f..9d08f30135 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -28,6 +28,7 @@ use crate::doc_lines::{doc_lines_from_ast, doc_lines_from_tokens}; use crate::fix::{fix_file, FixResult}; use crate::message::Message; use crate::noqa::add_noqa; +use crate::package::PackageRoot; use crate::registry::{AsRule, Rule, RuleSet}; #[cfg(any(feature = "test-rules", test))] use crate::rules::ruff::rules::test_rules::{self, TestRule, TEST_RULES}; @@ -60,7 +61,7 @@ pub struct FixerResult<'a> { #[allow(clippy::too_many_arguments)] pub fn check_path( path: &Path, - package: Option<&Path>, + package: Option>, locator: &Locator, stylist: &Stylist, indexer: &Indexer, @@ -323,7 +324,7 @@ const MAX_ITERATIONS: usize = 100; /// Add any missing `# noqa` pragmas to the source code at the given `Path`. pub fn add_noqa_to_path( path: &Path, - package: Option<&Path>, + package: Option>, source_kind: &SourceKind, source_type: PySourceType, settings: &LinterSettings, @@ -380,7 +381,7 @@ pub fn add_noqa_to_path( /// code. pub fn lint_only( path: &Path, - package: Option<&Path>, + package: Option>, settings: &LinterSettings, noqa: flags::Noqa, source_kind: &SourceKind, @@ -467,7 +468,7 @@ fn diagnostics_to_messages( #[allow(clippy::too_many_arguments)] pub fn lint_fix<'a>( path: &Path, - package: Option<&Path>, + package: Option>, noqa: flags::Noqa, unsafe_fixes: UnsafeFixes, settings: &LinterSettings, diff --git a/crates/ruff_linter/src/package.rs b/crates/ruff_linter/src/package.rs new file mode 100644 index 0000000000..dd11f17b12 --- /dev/null +++ b/crates/ruff_linter/src/package.rs @@ -0,0 +1,40 @@ +use std::path::Path; + +/// The root directory of a Python package. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PackageRoot<'a> { + /// A normal package root. + Root { path: &'a Path }, + /// A nested package root. That is, a package root that's a subdirectory (direct or indirect) of + /// another Python package root. + /// + /// For example, `foo/bar/baz` in: + /// ```text + /// foo/ + /// ├── __init__.py + /// └── bar/ + /// └── baz/ + /// └── __init__.py + /// ``` + Nested { path: &'a Path }, +} + +impl<'a> PackageRoot<'a> { + /// Create a [`PackageRoot::Root`] variant. + pub fn root(path: &'a Path) -> Self { + Self::Root { path } + } + + /// Create a [`PackageRoot::Nested`] variant. + pub fn nested(path: &'a Path) -> Self { + Self::Nested { path } + } + + /// Return the [`Path`] of the package root. + pub fn path(self) -> &'a Path { + match self { + Self::Root { path } => path, + Self::Nested { path } => path, + } + } +} diff --git a/crates/ruff_linter/src/rules/flake8_builtins/rules/builtin_module_shadowing.rs b/crates/ruff_linter/src/rules/flake8_builtins/rules/builtin_module_shadowing.rs index 14d42fb7e9..254b390561 100644 --- a/crates/ruff_linter/src/rules/flake8_builtins/rules/builtin_module_shadowing.rs +++ b/crates/ruff_linter/src/rules/flake8_builtins/rules/builtin_module_shadowing.rs @@ -1,5 +1,7 @@ use std::path::Path; +use crate::package::PackageRoot; +use crate::settings::types::PythonVersion; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::PySourceType; @@ -7,8 +9,6 @@ use ruff_python_stdlib::path::is_module_file; use ruff_python_stdlib::sys::is_known_standard_library; use ruff_text_size::TextRange; -use crate::settings::types::PythonVersion; - /// ## What it does /// Checks for modules that use the same names as Python builtin modules. /// @@ -39,7 +39,7 @@ impl Violation for BuiltinModuleShadowing { /// A005 pub(crate) fn builtin_module_shadowing( path: &Path, - package: Option<&Path>, + package: Option>, allowed_modules: &[String], target_version: PythonVersion, ) -> Option { @@ -49,7 +49,7 @@ pub(crate) fn builtin_module_shadowing( if let Some(package) = package { let module_name = if is_module_file(path) { - package.file_name().unwrap().to_string_lossy() + package.path().file_name().unwrap().to_string_lossy() } else { path.file_stem().unwrap().to_string_lossy() }; diff --git a/crates/ruff_linter/src/rules/flake8_no_pep420/mod.rs b/crates/ruff_linter/src/rules/flake8_no_pep420/mod.rs index cd2df547be..f02022737e 100644 --- a/crates/ruff_linter/src/rules/flake8_no_pep420/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_no_pep420/mod.rs @@ -8,8 +8,9 @@ mod tests { use anyhow::Result; use test_case::test_case; - use crate::assert_messages; use crate::registry::Rule; + + use crate::assert_messages; use crate::settings::LinterSettings; use crate::test::{test_path, test_resource_path}; @@ -22,7 +23,7 @@ mod tests { #[test_case(Path::new("test_pass_pyi"), Path::new("example.pyi"))] #[test_case(Path::new("test_pass_script"), Path::new("script"))] #[test_case(Path::new("test_pass_shebang"), Path::new("example.py"))] - fn test_flake8_no_pep420(path: &Path, filename: &Path) -> Result<()> { + fn default(path: &Path, filename: &Path) -> Result<()> { let snapshot = format!("{}", path.to_string_lossy()); let p = PathBuf::from(format!( "flake8_no_pep420/{}/{}", diff --git a/crates/ruff_linter/src/rules/flake8_no_pep420/rules/implicit_namespace_package.rs b/crates/ruff_linter/src/rules/flake8_no_pep420/rules/implicit_namespace_package.rs index dd8da4a599..7590092eee 100644 --- a/crates/ruff_linter/src/rules/flake8_no_pep420/rules/implicit_namespace_package.rs +++ b/crates/ruff_linter/src/rules/flake8_no_pep420/rules/implicit_namespace_package.rs @@ -9,6 +9,8 @@ use ruff_text_size::{TextRange, TextSize}; use crate::comments::shebang::ShebangDirective; use crate::fs; +use crate::package::PackageRoot; +use crate::settings::types::PreviewMode; use crate::Locator; /// ## What it does @@ -32,24 +34,33 @@ use crate::Locator; #[violation] pub struct ImplicitNamespacePackage { filename: String, + parent: Option, } impl Violation for ImplicitNamespacePackage { #[derive_message_formats] fn message(&self) -> String { - let ImplicitNamespacePackage { filename } = self; - format!("File `{filename}` is part of an implicit namespace package. Add an `__init__.py`.") + let ImplicitNamespacePackage { filename, parent } = self; + match parent { + None => { + format!("File `{filename}` is part of an implicit namespace package. Add an `__init__.py`.") + } + Some(parent) => { + format!("File `{filename}` declares a package, but is nested under an implicit namespace package. Add an `__init__.py` to `{parent}`.") + } + } } } /// INP001 pub(crate) fn implicit_namespace_package( path: &Path, - package: Option<&Path>, + package: Option>, locator: &Locator, comment_ranges: &CommentRanges, project_root: &Path, src: &[PathBuf], + preview: PreviewMode, ) -> Option { if package.is_none() // Ignore non-`.py` files, which don't require an `__init__.py`. @@ -73,13 +84,39 @@ pub(crate) fn implicit_namespace_package( let path = path .to_string_lossy() .replace(std::path::MAIN_SEPARATOR, "/"); // The snapshot test expects / as the path separator. - Some(Diagnostic::new( + return Some(Diagnostic::new( ImplicitNamespacePackage { filename: fs::relativize_path(path), + parent: None, }, TextRange::default(), - )) - } else { - None + )); } + + if preview.is_enabled() { + if let Some(PackageRoot::Nested { path: root }) = package.as_ref() { + if path.ends_with("__init__.py") { + // Identify the intermediary package that's missing the `__init__.py` file. + if let Some(parent) = root + .ancestors() + .find(|parent| !parent.join("__init__.py").exists()) + { + #[cfg(all(test, windows))] + let path = path + .to_string_lossy() + .replace(std::path::MAIN_SEPARATOR, "/"); // The snapshot test expects / as the path separator. + + return Some(Diagnostic::new( + ImplicitNamespacePackage { + filename: fs::relativize_path(path), + parent: Some(fs::relativize_path(parent)), + }, + TextRange::default(), + )); + } + } + } + } + + None } diff --git a/crates/ruff_linter/src/rules/isort/categorize.rs b/crates/ruff_linter/src/rules/isort/categorize.rs index 4c82122611..0b85b54146 100644 --- a/crates/ruff_linter/src/rules/isort/categorize.rs +++ b/crates/ruff_linter/src/rules/isort/categorize.rs @@ -8,11 +8,11 @@ use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet}; use serde::{Deserialize, Serialize}; use strum_macros::EnumIter; -use ruff_macros::CacheKey; -use ruff_python_stdlib::sys::is_known_standard_library; - +use crate::package::PackageRoot; use crate::settings::types::PythonVersion; use crate::warn_user_once; +use ruff_macros::CacheKey; +use ruff_python_stdlib::sys::is_known_standard_library; use super::types::{ImportBlock, Importable}; @@ -93,7 +93,7 @@ pub(crate) fn categorize<'a>( module_name: &str, is_relative: bool, src: &[PathBuf], - package: Option<&Path>, + package: Option>, detect_same_package: bool, known_modules: &'a KnownModules, target_version: PythonVersion, @@ -153,8 +153,10 @@ pub(crate) fn categorize<'a>( import_type } -fn same_package(package: Option<&Path>, module_base: &str) -> bool { - package.is_some_and(|package| package.ends_with(module_base)) +fn same_package(package: Option>, module_base: &str) -> bool { + package + .map(PackageRoot::path) + .is_some_and(|package| package.ends_with(module_base)) } fn match_sources<'a>(paths: &'a [PathBuf], base: &str) -> Option<&'a Path> { @@ -177,7 +179,7 @@ fn match_sources<'a>(paths: &'a [PathBuf], base: &str) -> Option<&'a Path> { pub(crate) fn categorize_imports<'a>( block: ImportBlock<'a>, src: &[PathBuf], - package: Option<&Path>, + package: Option>, detect_same_package: bool, known_modules: &'a KnownModules, target_version: PythonVersion, diff --git a/crates/ruff_linter/src/rules/isort/mod.rs b/crates/ruff_linter/src/rules/isort/mod.rs index 613284b34b..afd3bc6fab 100644 --- a/crates/ruff_linter/src/rules/isort/mod.rs +++ b/crates/ruff_linter/src/rules/isort/mod.rs @@ -1,6 +1,6 @@ //! Rules from [isort](https://pypi.org/project/isort/). -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use annotate::annotate_imports; use block::{Block, Trailer}; @@ -18,6 +18,7 @@ use types::EitherImport::{Import, ImportFrom}; use types::{AliasData, ImportBlock, TrailingComma}; use crate::line_width::{LineLength, LineWidthBuilder}; +use crate::package::PackageRoot; use crate::settings::types::PythonVersion; use crate::Locator; @@ -71,7 +72,7 @@ pub(crate) fn format_imports( indentation_width: LineWidthBuilder, stylist: &Stylist, src: &[PathBuf], - package: Option<&Path>, + package: Option>, source_type: PySourceType, target_version: PythonVersion, settings: &Settings, @@ -155,7 +156,7 @@ fn format_import_block( indentation_width: LineWidthBuilder, stylist: &Stylist, src: &[PathBuf], - package: Option<&Path>, + package: Option>, target_version: PythonVersion, settings: &Settings, ) -> String { 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 a9f159e090..4b67296ea1 100644 --- a/crates/ruff_linter/src/rules/isort/rules/organize_imports.rs +++ b/crates/ruff_linter/src/rules/isort/rules/organize_imports.rs @@ -1,5 +1,3 @@ -use std::path::Path; - use itertools::{EitherOrBoth, Itertools}; use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; @@ -13,12 +11,12 @@ use ruff_python_trivia::{leading_indentation, textwrap::indent, PythonWhitespace use ruff_source_file::{LineRanges, UniversalNewlines}; use ruff_text_size::{Ranged, TextRange}; -use crate::line_width::LineWidthBuilder; -use crate::settings::LinterSettings; -use crate::Locator; - use super::super::block::Block; use super::super::{comments, format_imports}; +use crate::line_width::LineWidthBuilder; +use crate::package::PackageRoot; +use crate::settings::LinterSettings; +use crate::Locator; /// ## What it does /// De-duplicates, groups, and sorts imports based on the provided `isort` settings. @@ -87,7 +85,7 @@ pub(crate) fn organize_imports( stylist: &Stylist, indexer: &Indexer, settings: &LinterSettings, - package: Option<&Path>, + package: Option>, source_type: PySourceType, tokens: &Tokens, ) -> Option { diff --git a/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_module_name.rs b/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_module_name.rs index d525aa1d62..1637bd5742 100644 --- a/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_module_name.rs +++ b/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_module_name.rs @@ -1,6 +1,8 @@ use std::ffi::OsStr; use std::path::Path; +use crate::package::PackageRoot; +use crate::rules::pep8_naming::settings::IgnoreNames; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::PySourceType; @@ -8,8 +10,6 @@ use ruff_python_stdlib::identifiers::{is_migration_name, is_module_name}; use ruff_python_stdlib::path::is_module_file; use ruff_text_size::TextRange; -use crate::rules::pep8_naming::settings::IgnoreNames; - /// ## What it does /// Checks for module names that do not follow the `snake_case` naming /// convention or are otherwise invalid. @@ -51,7 +51,7 @@ impl Violation for InvalidModuleName { /// N999 pub(crate) fn invalid_module_name( path: &Path, - package: Option<&Path>, + package: Option>, ignore_names: &IgnoreNames, ) -> Option { if !PySourceType::try_from_path(path).is_some_and(PySourceType::is_py_file_or_stub) { @@ -60,7 +60,7 @@ pub(crate) fn invalid_module_name( if let Some(package) = package { let module_name = if is_module_file(path) { - package.file_name().unwrap().to_string_lossy() + package.path().file_name().unwrap().to_string_lossy() } else { path.file_stem().unwrap().to_string_lossy() }; diff --git a/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs b/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs index 275f5f2183..7681d69228 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/import_private_name.rs @@ -9,6 +9,7 @@ use ruff_python_semantic::{FromImport, Import, Imported, ResolvedReference, Scop use ruff_text_size::Ranged; use crate::checkers::ast::Checker; +use crate::package::PackageRoot; /// ## What it does /// Checks for import statements that import a private name (a name starting @@ -106,6 +107,7 @@ pub(crate) fn import_private_name( // Ex) `from foo import _bar` within `foo/baz.py` if checker .package() + .map(PackageRoot::path) .is_some_and(|path| path.ends_with(root_module)) { continue; diff --git a/crates/ruff_linter/src/test.rs b/crates/ruff_linter/src/test.rs index a0c1fe4fba..6ded54565e 100644 --- a/crates/ruff_linter/src/test.rs +++ b/crates/ruff_linter/src/test.rs @@ -24,6 +24,7 @@ use ruff_text_size::Ranged; use crate::fix::{fix_file, FixResult}; use crate::linter::check_path; use crate::message::{Emitter, EmitterContext, Message, TextEmitter}; +use crate::package::PackageRoot; use crate::packaging::detect_package_root; use crate::registry::AsRule; use crate::settings::types::UnsafeFixes; @@ -122,7 +123,8 @@ pub(crate) fn test_contents<'a>( let diagnostics = check_path( path, path.parent() - .and_then(|parent| detect_package_root(parent, &settings.namespace_packages)), + .and_then(|parent| detect_package_root(parent, &settings.namespace_packages)) + .map(|path| PackageRoot::Root { path }), &locator, &stylist, &indexer, diff --git a/crates/ruff_server/src/fix.rs b/crates/ruff_server/src/fix.rs index 2b80f80a8f..616cf4caff 100644 --- a/crates/ruff_server/src/fix.rs +++ b/crates/ruff_server/src/fix.rs @@ -2,20 +2,20 @@ use std::borrow::Cow; use rustc_hash::FxHashMap; -use ruff_linter::{ - linter::{FixerResult, LinterResult}, - packaging::detect_package_root, - settings::{flags, types::UnsafeFixes, LinterSettings}, -}; -use ruff_notebook::SourceValue; -use ruff_source_file::LineIndex; - use crate::{ edit::{Replacement, ToRangeExt}, resolve::is_document_excluded, session::DocumentQuery, PositionEncoding, }; +use ruff_linter::package::PackageRoot; +use ruff_linter::{ + linter::{FixerResult, LinterResult}, + packaging::detect_package_root, + settings::{flags, types::UnsafeFixes, LinterSettings}, +}; +use ruff_notebook::SourceValue; +use ruff_source_file::LineIndex; /// A simultaneous fix made across a single text document or among an arbitrary /// number of notebook cells. @@ -49,6 +49,7 @@ pub(crate) fn fix_all( .expect("a path to a document should have a parent path"), &linter_settings.namespace_packages, ) + .map(PackageRoot::root) } else { None }; diff --git a/crates/ruff_server/src/lint.rs b/crates/ruff_server/src/lint.rs index cb3fe3499b..5b95488b32 100644 --- a/crates/ruff_server/src/lint.rs +++ b/crates/ruff_server/src/lint.rs @@ -3,7 +3,14 @@ use rustc_hash::FxHashMap; use serde::{Deserialize, Serialize}; +use crate::{ + edit::{NotebookRange, ToRangeExt}, + resolve::is_document_excluded, + session::DocumentQuery, + PositionEncoding, DIAGNOSTIC_NAME, +}; use ruff_diagnostics::{Applicability, Diagnostic, DiagnosticKind, Edit, Fix}; +use ruff_linter::package::PackageRoot; use ruff_linter::{ directives::{extract_directives, Flags}, generate_noqa_edits, @@ -21,13 +28,6 @@ use ruff_python_parser::ParseError; use ruff_source_file::LineIndex; use ruff_text_size::{Ranged, TextRange}; -use crate::{ - edit::{NotebookRange, ToRangeExt}, - resolve::is_document_excluded, - session::DocumentQuery, - PositionEncoding, DIAGNOSTIC_NAME, -}; - /// This is serialized on the diagnostic `data` field. #[derive(Serialize, Deserialize, Debug, Clone)] pub(crate) struct AssociatedDiagnosticData { @@ -89,6 +89,7 @@ pub(crate) fn check( .expect("a path to a document should have a parent path"), &linter_settings.namespace_packages, ) + .map(PackageRoot::root) } else { None }; diff --git a/crates/ruff_workspace/src/resolver.rs b/crates/ruff_workspace/src/resolver.rs index ae9f46f1a9..fd77839c1d 100644 --- a/crates/ruff_workspace/src/resolver.rs +++ b/crates/ruff_workspace/src/resolver.rs @@ -2,6 +2,7 @@ //! filesystem. use std::cmp::Ordering; +use std::collections::BTreeSet; use std::ffi::OsStr; use std::path::{Path, PathBuf}; use std::sync::RwLock; @@ -18,6 +19,7 @@ use path_slash::PathExt; use rustc_hash::{FxHashMap, FxHashSet}; use ruff_linter::fs; +use ruff_linter::package::PackageRoot; use ruff_linter::packaging::is_package; use crate::configuration::Configuration; @@ -147,8 +149,8 @@ impl<'a> Resolver<'a> { fn add(&mut self, path: &Path, settings: Settings) { self.settings.push(settings); - // normalize the path to use `/` separators and escape the '{' and '}' characters, - // which matchit uses for routing parameters + // Normalize the path to use `/` separators and escape the '{' and '}' characters, + // which matchit uses for routing parameters. let path = path.to_slash_lossy().replace('{', "{{").replace('}', "}}"); match self @@ -181,7 +183,10 @@ impl<'a> Resolver<'a> { } /// Return a mapping from Python package to its package root. - pub fn package_roots(&'a self, files: &[&'a Path]) -> FxHashMap<&'a Path, Option<&'a Path>> { + pub fn package_roots( + &'a self, + files: &[&'a Path], + ) -> FxHashMap<&'a Path, Option>> { // Pre-populate the module cache, since the list of files could (but isn't // required to) contain some `__init__.py` files. let mut package_cache: FxHashMap<&Path, bool> = FxHashMap::default(); @@ -200,7 +205,7 @@ impl<'a> Resolver<'a> { .any(|settings| !settings.linter.namespace_packages.is_empty()); // Search for the package root for each file. - let mut package_roots: FxHashMap<&Path, Option<&Path>> = FxHashMap::default(); + let mut package_roots: FxHashMap<&Path, Option>> = FxHashMap::default(); for file in files { if let Some(package) = file.parent() { package_roots.entry(package).or_insert_with(|| { @@ -210,10 +215,41 @@ impl<'a> Resolver<'a> { &[] }; detect_package_root_with_cache(package, namespace_packages, &mut package_cache) + .map(|path| PackageRoot::Root { path }) }); } } + // Discard any nested roots. + // + // For example, if `./foo/__init__.py` is a root, and then `./foo/bar` is empty, and + // `./foo/bar/baz/__init__.py` was detected as a root, we should only consider + // `./foo/__init__.py`. + let mut non_roots = FxHashSet::default(); + let mut router: Router<&Path> = Router::new(); + for root in package_roots + .values() + .flatten() + .copied() + .map(PackageRoot::path) + .collect::>() + { + // Normalize the path to use `/` separators and escape the '{' and '}' characters, + // which matchit uses for routing parameters. + let path = root.to_slash_lossy().replace('{', "{{").replace('}', "}}"); + if let Ok(matched) = router.at_mut(&path) { + debug!( + "Ignoring nested package root: {} (under {})", + root.display(), + matched.value.display() + ); + package_roots.insert(root, Some(PackageRoot::nested(root))); + non_roots.insert(root); + } else { + let _ = router.insert(format!("{path}/{{*filepath}}"), root); + } + } + package_roots }