From b23414e3ccbd169ae6d430cf3f56d427943bd30b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 17 Apr 2024 21:42:50 -0400 Subject: [PATCH] Resolve classes and functions relative to script name (#10965) ## Summary If the user is analyzing a script (i.e., we have no module path), it seems reasonable to use the script name when trying to identify paths to objects defined _within_ the script. Closes https://github.com/astral-sh/ruff/issues/10960. ## Test Plan Ran: ```shell check --isolated --select=B008 \ --config 'lint.flake8-bugbear.extend-immutable-calls=["test.A"]' \ test.py ``` On: ```python class A: pass def f(a=A()): pass ``` --- .../fixtures/flake8_bugbear/B008_extended.py | 12 +++++ .../src/checkers/ast/analyze/statement.rs | 30 +++++++---- crates/ruff_linter/src/checkers/ast/mod.rs | 21 +++++--- .../src/rules/flake8_bugbear/mod.rs | 1 + ...s__extend_immutable_calls_arg_default.snap | 7 ++- .../src/analyze/visibility.rs | 54 ++++++++----------- crates/ruff_python_semantic/src/definition.rs | 32 +++++++---- crates/ruff_python_semantic/src/model.rs | 44 +++++++++++---- 8 files changed, 127 insertions(+), 74 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B008_extended.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B008_extended.py index bf97f793d1..a9a6f14963 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B008_extended.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B008_extended.py @@ -23,3 +23,15 @@ def okay(data: custom.ImmutableTypeA = foo()): def error_due_to_missing_import(data: List[str] = Depends(None)): ... + + +class Class: + pass + + +def okay(obj=Class()): + ... + + +def error(obj=OtherClass()): + ... diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index f6f6321fd4..712296030a 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -612,7 +612,8 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { pylint::rules::manual_from_import(checker, stmt, alias, names); } if checker.enabled(Rule::ImportSelf) { - if let Some(diagnostic) = pylint::rules::import_self(alias, checker.module_path) + if let Some(diagnostic) = + pylint::rules::import_self(alias, checker.module.qualified_name()) { checker.diagnostics.push(diagnostic); } @@ -776,9 +777,11 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { flake8_bandit::rules::suspicious_imports(checker, stmt); } if checker.enabled(Rule::BannedApi) { - if let Some(module) = - helpers::resolve_imported_module_path(level, module, checker.module_path) - { + if let Some(module) = helpers::resolve_imported_module_path( + level, + module, + checker.module.qualified_name(), + ) { flake8_tidy_imports::rules::banned_api( checker, &flake8_tidy_imports::matchers::NameMatchPolicy::MatchNameOrParent( @@ -805,9 +808,11 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { } } if checker.enabled(Rule::BannedModuleLevelImports) { - if let Some(module) = - helpers::resolve_imported_module_path(level, module, checker.module_path) - { + if let Some(module) = helpers::resolve_imported_module_path( + level, + module, + checker.module.qualified_name(), + ) { flake8_tidy_imports::rules::banned_module_level_imports( checker, &flake8_tidy_imports::matchers::NameMatchPolicy::MatchNameOrParent( @@ -894,7 +899,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { stmt, level, module, - checker.module_path, + checker.module.qualified_name(), checker.settings.flake8_tidy_imports.ban_relative_imports, ) { checker.diagnostics.push(diagnostic); @@ -993,9 +998,12 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { } } if checker.enabled(Rule::ImportSelf) { - if let Some(diagnostic) = - pylint::rules::import_from_self(level, module, names, checker.module_path) - { + if let Some(diagnostic) = pylint::rules::import_from_self( + level, + module, + names, + checker.module.qualified_name(), + ) { checker.diagnostics.push(diagnostic); } } diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index 8cf3cd09a1..9beb89895f 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -50,11 +50,11 @@ use ruff_python_ast::{helpers, str, visitor, PySourceType}; use ruff_python_codegen::{Generator, Stylist}; use ruff_python_index::Indexer; use ruff_python_parser::typing::{parse_type_annotation, AnnotationKind}; -use ruff_python_semantic::analyze::{imports, typing, visibility}; +use ruff_python_semantic::analyze::{imports, typing}; use ruff_python_semantic::{ BindingFlags, BindingId, BindingKind, Exceptions, Export, FromImport, Globals, Import, Module, - ModuleKind, NodeId, ScopeId, ScopeKind, SemanticModel, SemanticModelFlags, StarImport, - SubmoduleImport, + ModuleKind, ModuleSource, NodeId, ScopeId, ScopeKind, SemanticModel, SemanticModelFlags, + StarImport, SubmoduleImport, }; use ruff_python_stdlib::builtins::{IPYTHON_BUILTINS, MAGIC_GLOBALS, PYTHON_BUILTINS}; use ruff_source_file::{Locator, OneIndexed, SourceRow}; @@ -110,7 +110,7 @@ pub(crate) struct Checker<'a> { /// The [`Path`] to the package containing the current file. package: Option<&'a Path>, /// The module representation of the current file (e.g., `foo.bar`). - module_path: Option<&'a [String]>, + module: Module<'a>, /// The [`PySourceType`] of the current file. pub(crate) source_type: PySourceType, /// The [`CellOffsets`] for the current file, if it's a Jupyter notebook. @@ -174,7 +174,7 @@ impl<'a> Checker<'a> { noqa, path, package, - module_path: module.path(), + module, source_type, locator, stylist, @@ -2282,10 +2282,15 @@ pub(crate) fn check_ast( } else { ModuleKind::Module }, - source: if let Some(module_path) = module_path.as_ref() { - visibility::ModuleSource::Path(module_path) + name: if let Some(module_path) = &module_path { + module_path.last().map(String::as_str) } else { - visibility::ModuleSource::File(path) + path.file_stem().and_then(std::ffi::OsStr::to_str) + }, + source: if let Some(module_path) = module_path.as_ref() { + ModuleSource::Path(module_path) + } else { + ModuleSource::File(path) }, python_ast, }; diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs b/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs index bbe8cd4d3a..640007b307 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/mod.rs @@ -114,6 +114,7 @@ mod tests { "fastapi.Depends".to_string(), "fastapi.Query".to_string(), "custom.ImmutableTypeA".to_string(), + "B008_extended.Class".to_string(), ], }, ..LinterSettings::for_rule(Rule::FunctionCallInDefaultArgument) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__extend_immutable_calls_arg_default.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__extend_immutable_calls_arg_default.snap index 064b5f1e0c..185294223e 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__extend_immutable_calls_arg_default.snap +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__extend_immutable_calls_arg_default.snap @@ -8,4 +8,9 @@ B008_extended.py:24:51: B008 Do not perform function call `Depends` in argument 25 | ... | - +B008_extended.py:36:15: B008 Do not perform function call `OtherClass` in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable + | +36 | def error(obj=OtherClass()): + | ^^^^^^^^^^^^ B008 +37 | ... + | diff --git a/crates/ruff_python_semantic/src/analyze/visibility.rs b/crates/ruff_python_semantic/src/analyze/visibility.rs index 816e7678fa..3e107e3af3 100644 --- a/crates/ruff_python_semantic/src/analyze/visibility.rs +++ b/crates/ruff_python_semantic/src/analyze/visibility.rs @@ -1,11 +1,10 @@ -use std::path::Path; - use ruff_python_ast::{self as ast, Decorator}; use ruff_python_ast::helpers::map_callable; use ruff_python_ast::name::{QualifiedName, UnqualifiedName}; use crate::model::SemanticModel; +use crate::{Module, ModuleSource}; #[derive(Debug, Clone, Copy, is_macro::Is)] pub enum Visibility { @@ -134,44 +133,31 @@ fn stem(path: &str) -> &str { } } -/// A Python module can either be defined as a module path (i.e., the dot-separated path to the -/// module) or, if the module can't be resolved, as a file path (i.e., the path to the file defining -/// the module). -#[derive(Debug)] -pub enum ModuleSource<'a> { - /// A module path is a dot-separated path to the module. - Path(&'a [String]), - /// A file path is the path to the file defining the module, often a script outside of a - /// package. - File(&'a Path), -} - -impl ModuleSource<'_> { - /// Return the `Visibility` of the module. - pub(crate) fn to_visibility(&self) -> Visibility { - match self { - Self::Path(path) => { - if path.iter().any(|m| is_private_module(m)) { +/// Infer the [`Visibility`] of a module from its path. +pub(crate) fn module_visibility(module: &Module) -> Visibility { + match &module.source { + ModuleSource::Path(path) => { + if path.iter().any(|m| is_private_module(m)) { + return Visibility::Private; + } + } + ModuleSource::File(path) => { + // Check to see if the filename itself indicates private visibility. + // Ex) `_foo.py` (but not `__init__.py`) + let mut components = path.iter().rev(); + if let Some(filename) = components.next() { + let module_name = filename.to_string_lossy(); + let module_name = stem(&module_name); + if is_private_module(module_name) { return Visibility::Private; } } - Self::File(path) => { - // Check to see if the filename itself indicates private visibility. - // Ex) `_foo.py` (but not `__init__.py`) - let mut components = path.iter().rev(); - if let Some(filename) = components.next() { - let module_name = filename.to_string_lossy(); - let module_name = stem(&module_name); - if is_private_module(module_name) { - return Visibility::Private; - } - } - } } - Visibility::Public } + Visibility::Public } +/// Infer the [`Visibility`] of a function from its name. pub(crate) fn function_visibility(function: &ast::StmtFunctionDef) -> Visibility { if function.name.starts_with('_') { Visibility::Private @@ -180,6 +166,7 @@ pub(crate) fn function_visibility(function: &ast::StmtFunctionDef) -> Visibility } } +/// Infer the [`Visibility`] of a method from its name and decorators. pub fn method_visibility(function: &ast::StmtFunctionDef) -> Visibility { // Is this a setter or deleter? if function.decorator_list.iter().any(|decorator| { @@ -204,6 +191,7 @@ pub fn method_visibility(function: &ast::StmtFunctionDef) -> Visibility { Visibility::Private } +/// Infer the [`Visibility`] of a class from its name. pub(crate) fn class_visibility(class: &ast::StmtClassDef) -> Visibility { if class.name.starts_with('_') { Visibility::Private diff --git a/crates/ruff_python_semantic/src/definition.rs b/crates/ruff_python_semantic/src/definition.rs index 6b8f1d5d86..90e24fbfe0 100644 --- a/crates/ruff_python_semantic/src/definition.rs +++ b/crates/ruff_python_semantic/src/definition.rs @@ -3,13 +3,14 @@ use std::fmt::Debug; use std::ops::Deref; +use std::path::Path; use ruff_index::{newtype_index, IndexSlice, IndexVec}; use ruff_python_ast::{self as ast, all::DunderAllName, Stmt}; use ruff_text_size::{Ranged, TextRange}; use crate::analyze::visibility::{ - class_visibility, function_visibility, method_visibility, ModuleSource, Visibility, + class_visibility, function_visibility, method_visibility, module_visibility, Visibility, }; /// Id uniquely identifying a definition in a program. @@ -24,7 +25,19 @@ impl DefinitionId { } } -#[derive(Debug, is_macro::Is)] +/// A Python module can either be defined as a module path (i.e., the dot-separated path to the +/// module) or, if the module can't be resolved, as a file path (i.e., the path to the file defining +/// the module). +#[derive(Debug, Copy, Clone)] +pub enum ModuleSource<'a> { + /// A module path is a dot-separated path to the module. + Path(&'a [String]), + /// A file path is the path to the file defining the module, often a script outside of a + /// package. + File(&'a Path), +} + +#[derive(Debug, Copy, Clone, is_macro::Is)] pub enum ModuleKind { /// A Python file that represents a module within a package. Module, @@ -33,15 +46,17 @@ pub enum ModuleKind { } /// A Python module. -#[derive(Debug)] +#[derive(Debug, Copy, Clone)] pub struct Module<'a> { pub kind: ModuleKind, pub source: ModuleSource<'a>, pub python_ast: &'a [Stmt], + pub name: Option<&'a str>, } impl<'a> Module<'a> { - pub fn path(&self) -> Option<&'a [String]> { + /// Return the fully-qualified path of the module. + pub const fn qualified_name(&self) -> Option<&'a [String]> { if let ModuleSource::Path(path) = self.source { Some(path) } else { @@ -50,11 +65,8 @@ impl<'a> Module<'a> { } /// Return the name of the module. - pub fn name(&self) -> Option<&'a str> { - match self.source { - ModuleSource::Path(path) => path.last().map(Deref::deref), - ModuleSource::File(file) => file.file_stem().and_then(std::ffi::OsStr::to_str), - } + pub const fn name(&self) -> Option<&'a str> { + self.name } } @@ -196,7 +208,7 @@ impl<'a> Definitions<'a> { // visibility. let visibility = { match &definition { - Definition::Module(module) => module.source.to_visibility(), + Definition::Module(module) => module_visibility(module), Definition::Member(member) => match member.kind { MemberKind::Class(class) => { let parent = &definitions[member.parent]; diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 04f697e15b..aa83ed626f 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -28,7 +28,7 @@ use crate::Imported; /// A semantic model for a Python module, to enable querying the module's semantic information. pub struct SemanticModel<'a> { typing_modules: &'a [String], - module_path: Option<&'a [String]>, + module: Module<'a>, /// Stack of all AST nodes in the program. nodes: Nodes<'a>, @@ -134,7 +134,7 @@ impl<'a> SemanticModel<'a> { pub fn new(typing_modules: &'a [String], path: &Path, module: Module<'a>) -> Self { Self { typing_modules, - module_path: module.path(), + module, nodes: Nodes::default(), node_id: None, branches: Branches::default(), @@ -791,7 +791,11 @@ impl<'a> SemanticModel<'a> { .first() .map_or(false, |segment| *segment == ".") { - from_relative_import(self.module_path?, qualified_name.segments(), tail)? + from_relative_import( + self.module.qualified_name()?, + qualified_name.segments(), + tail, + )? } else { qualified_name .segments() @@ -817,14 +821,32 @@ impl<'a> SemanticModel<'a> { } } BindingKind::ClassDefinition(_) | BindingKind::FunctionDefinition(_) => { - let value_name = UnqualifiedName::from_expr(value)?; - let resolved: QualifiedName = self - .module_path? - .iter() - .map(String::as_str) - .chain(value_name.segments().iter().copied()) - .collect(); - Some(resolved) + // If we have a fully-qualified path for the module, use it. + if let Some(path) = self.module.qualified_name() { + Some( + path.iter() + .map(String::as_str) + .chain( + UnqualifiedName::from_expr(value)? + .segments() + .iter() + .copied(), + ) + .collect(), + ) + } else { + // Otherwise, if we're in (e.g.) a script, use the module name. + Some( + std::iter::once(self.module.name()?) + .chain( + UnqualifiedName::from_expr(value)? + .segments() + .iter() + .copied(), + ) + .collect(), + ) + } } _ => None, }