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
```
This commit is contained in:
Charlie Marsh 2024-04-17 21:42:50 -04:00 committed by GitHub
parent 1480d72643
commit b23414e3cc
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 127 additions and 74 deletions

View File

@ -23,3 +23,15 @@ def okay(data: custom.ImmutableTypeA = foo()):
def error_due_to_missing_import(data: List[str] = Depends(None)): def error_due_to_missing_import(data: List[str] = Depends(None)):
... ...
class Class:
pass
def okay(obj=Class()):
...
def error(obj=OtherClass()):
...

View File

@ -612,7 +612,8 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
pylint::rules::manual_from_import(checker, stmt, alias, names); pylint::rules::manual_from_import(checker, stmt, alias, names);
} }
if checker.enabled(Rule::ImportSelf) { 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); checker.diagnostics.push(diagnostic);
} }
@ -776,9 +777,11 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
flake8_bandit::rules::suspicious_imports(checker, stmt); flake8_bandit::rules::suspicious_imports(checker, stmt);
} }
if checker.enabled(Rule::BannedApi) { if checker.enabled(Rule::BannedApi) {
if let Some(module) = if let Some(module) = helpers::resolve_imported_module_path(
helpers::resolve_imported_module_path(level, module, checker.module_path) level,
{ module,
checker.module.qualified_name(),
) {
flake8_tidy_imports::rules::banned_api( flake8_tidy_imports::rules::banned_api(
checker, checker,
&flake8_tidy_imports::matchers::NameMatchPolicy::MatchNameOrParent( &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 checker.enabled(Rule::BannedModuleLevelImports) {
if let Some(module) = if let Some(module) = helpers::resolve_imported_module_path(
helpers::resolve_imported_module_path(level, module, checker.module_path) level,
{ module,
checker.module.qualified_name(),
) {
flake8_tidy_imports::rules::banned_module_level_imports( flake8_tidy_imports::rules::banned_module_level_imports(
checker, checker,
&flake8_tidy_imports::matchers::NameMatchPolicy::MatchNameOrParent( &flake8_tidy_imports::matchers::NameMatchPolicy::MatchNameOrParent(
@ -894,7 +899,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
stmt, stmt,
level, level,
module, module,
checker.module_path, checker.module.qualified_name(),
checker.settings.flake8_tidy_imports.ban_relative_imports, checker.settings.flake8_tidy_imports.ban_relative_imports,
) { ) {
checker.diagnostics.push(diagnostic); checker.diagnostics.push(diagnostic);
@ -993,9 +998,12 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
} }
} }
if checker.enabled(Rule::ImportSelf) { if checker.enabled(Rule::ImportSelf) {
if let Some(diagnostic) = if let Some(diagnostic) = pylint::rules::import_from_self(
pylint::rules::import_from_self(level, module, names, checker.module_path) level,
{ module,
names,
checker.module.qualified_name(),
) {
checker.diagnostics.push(diagnostic); checker.diagnostics.push(diagnostic);
} }
} }

View File

@ -50,11 +50,11 @@ use ruff_python_ast::{helpers, str, visitor, PySourceType};
use ruff_python_codegen::{Generator, Stylist}; use ruff_python_codegen::{Generator, Stylist};
use ruff_python_index::Indexer; use ruff_python_index::Indexer;
use ruff_python_parser::typing::{parse_type_annotation, AnnotationKind}; 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::{ use ruff_python_semantic::{
BindingFlags, BindingId, BindingKind, Exceptions, Export, FromImport, Globals, Import, Module, BindingFlags, BindingId, BindingKind, Exceptions, Export, FromImport, Globals, Import, Module,
ModuleKind, NodeId, ScopeId, ScopeKind, SemanticModel, SemanticModelFlags, StarImport, ModuleKind, ModuleSource, NodeId, ScopeId, ScopeKind, SemanticModel, SemanticModelFlags,
SubmoduleImport, StarImport, SubmoduleImport,
}; };
use ruff_python_stdlib::builtins::{IPYTHON_BUILTINS, MAGIC_GLOBALS, PYTHON_BUILTINS}; use ruff_python_stdlib::builtins::{IPYTHON_BUILTINS, MAGIC_GLOBALS, PYTHON_BUILTINS};
use ruff_source_file::{Locator, OneIndexed, SourceRow}; 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. /// The [`Path`] to the package containing the current file.
package: Option<&'a Path>, package: Option<&'a Path>,
/// The module representation of the current file (e.g., `foo.bar`). /// 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. /// The [`PySourceType`] of the current file.
pub(crate) source_type: PySourceType, pub(crate) source_type: PySourceType,
/// The [`CellOffsets`] for the current file, if it's a Jupyter notebook. /// The [`CellOffsets`] for the current file, if it's a Jupyter notebook.
@ -174,7 +174,7 @@ impl<'a> Checker<'a> {
noqa, noqa,
path, path,
package, package,
module_path: module.path(), module,
source_type, source_type,
locator, locator,
stylist, stylist,
@ -2282,10 +2282,15 @@ pub(crate) fn check_ast(
} else { } else {
ModuleKind::Module ModuleKind::Module
}, },
source: if let Some(module_path) = module_path.as_ref() { name: if let Some(module_path) = &module_path {
visibility::ModuleSource::Path(module_path) module_path.last().map(String::as_str)
} else { } 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, python_ast,
}; };

View File

@ -114,6 +114,7 @@ mod tests {
"fastapi.Depends".to_string(), "fastapi.Depends".to_string(),
"fastapi.Query".to_string(), "fastapi.Query".to_string(),
"custom.ImmutableTypeA".to_string(), "custom.ImmutableTypeA".to_string(),
"B008_extended.Class".to_string(),
], ],
}, },
..LinterSettings::for_rule(Rule::FunctionCallInDefaultArgument) ..LinterSettings::for_rule(Rule::FunctionCallInDefaultArgument)

View File

@ -8,4 +8,9 @@ B008_extended.py:24:51: B008 Do not perform function call `Depends` in argument
25 | ... 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 | ...
|

View File

@ -1,11 +1,10 @@
use std::path::Path;
use ruff_python_ast::{self as ast, Decorator}; use ruff_python_ast::{self as ast, Decorator};
use ruff_python_ast::helpers::map_callable; use ruff_python_ast::helpers::map_callable;
use ruff_python_ast::name::{QualifiedName, UnqualifiedName}; use ruff_python_ast::name::{QualifiedName, UnqualifiedName};
use crate::model::SemanticModel; use crate::model::SemanticModel;
use crate::{Module, ModuleSource};
#[derive(Debug, Clone, Copy, is_macro::Is)] #[derive(Debug, Clone, Copy, is_macro::Is)]
pub enum Visibility { 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 /// Infer the [`Visibility`] of a module from its path.
/// module) or, if the module can't be resolved, as a file path (i.e., the path to the file defining pub(crate) fn module_visibility(module: &Module) -> Visibility {
/// the module). match &module.source {
#[derive(Debug)] ModuleSource::Path(path) => {
pub enum ModuleSource<'a> { if path.iter().any(|m| is_private_module(m)) {
/// A module path is a dot-separated path to the module. return Visibility::Private;
Path(&'a [String]), }
/// A file path is the path to the file defining the module, often a script outside of a }
/// package. ModuleSource::File(path) => {
File(&'a Path), // Check to see if the filename itself indicates private visibility.
} // Ex) `_foo.py` (but not `__init__.py`)
let mut components = path.iter().rev();
impl ModuleSource<'_> { if let Some(filename) = components.next() {
/// Return the `Visibility` of the module. let module_name = filename.to_string_lossy();
pub(crate) fn to_visibility(&self) -> Visibility { let module_name = stem(&module_name);
match self { if is_private_module(module_name) {
Self::Path(path) => {
if path.iter().any(|m| is_private_module(m)) {
return Visibility::Private; 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 { pub(crate) fn function_visibility(function: &ast::StmtFunctionDef) -> Visibility {
if function.name.starts_with('_') { if function.name.starts_with('_') {
Visibility::Private 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 { pub fn method_visibility(function: &ast::StmtFunctionDef) -> Visibility {
// Is this a setter or deleter? // Is this a setter or deleter?
if function.decorator_list.iter().any(|decorator| { if function.decorator_list.iter().any(|decorator| {
@ -204,6 +191,7 @@ pub fn method_visibility(function: &ast::StmtFunctionDef) -> Visibility {
Visibility::Private Visibility::Private
} }
/// Infer the [`Visibility`] of a class from its name.
pub(crate) fn class_visibility(class: &ast::StmtClassDef) -> Visibility { pub(crate) fn class_visibility(class: &ast::StmtClassDef) -> Visibility {
if class.name.starts_with('_') { if class.name.starts_with('_') {
Visibility::Private Visibility::Private

View File

@ -3,13 +3,14 @@
use std::fmt::Debug; use std::fmt::Debug;
use std::ops::Deref; use std::ops::Deref;
use std::path::Path;
use ruff_index::{newtype_index, IndexSlice, IndexVec}; use ruff_index::{newtype_index, IndexSlice, IndexVec};
use ruff_python_ast::{self as ast, all::DunderAllName, Stmt}; use ruff_python_ast::{self as ast, all::DunderAllName, Stmt};
use ruff_text_size::{Ranged, TextRange}; use ruff_text_size::{Ranged, TextRange};
use crate::analyze::visibility::{ 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. /// 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 { pub enum ModuleKind {
/// A Python file that represents a module within a package. /// A Python file that represents a module within a package.
Module, Module,
@ -33,15 +46,17 @@ pub enum ModuleKind {
} }
/// A Python module. /// A Python module.
#[derive(Debug)] #[derive(Debug, Copy, Clone)]
pub struct Module<'a> { pub struct Module<'a> {
pub kind: ModuleKind, pub kind: ModuleKind,
pub source: ModuleSource<'a>, pub source: ModuleSource<'a>,
pub python_ast: &'a [Stmt], pub python_ast: &'a [Stmt],
pub name: Option<&'a str>,
} }
impl<'a> Module<'a> { 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 { if let ModuleSource::Path(path) = self.source {
Some(path) Some(path)
} else { } else {
@ -50,11 +65,8 @@ impl<'a> Module<'a> {
} }
/// Return the name of the module. /// Return the name of the module.
pub fn name(&self) -> Option<&'a str> { pub const fn name(&self) -> Option<&'a str> {
match self.source { self.name
ModuleSource::Path(path) => path.last().map(Deref::deref),
ModuleSource::File(file) => file.file_stem().and_then(std::ffi::OsStr::to_str),
}
} }
} }
@ -196,7 +208,7 @@ impl<'a> Definitions<'a> {
// visibility. // visibility.
let visibility = { let visibility = {
match &definition { match &definition {
Definition::Module(module) => module.source.to_visibility(), Definition::Module(module) => module_visibility(module),
Definition::Member(member) => match member.kind { Definition::Member(member) => match member.kind {
MemberKind::Class(class) => { MemberKind::Class(class) => {
let parent = &definitions[member.parent]; let parent = &definitions[member.parent];

View File

@ -28,7 +28,7 @@ use crate::Imported;
/// A semantic model for a Python module, to enable querying the module's semantic information. /// A semantic model for a Python module, to enable querying the module's semantic information.
pub struct SemanticModel<'a> { pub struct SemanticModel<'a> {
typing_modules: &'a [String], typing_modules: &'a [String],
module_path: Option<&'a [String]>, module: Module<'a>,
/// Stack of all AST nodes in the program. /// Stack of all AST nodes in the program.
nodes: Nodes<'a>, nodes: Nodes<'a>,
@ -134,7 +134,7 @@ impl<'a> SemanticModel<'a> {
pub fn new(typing_modules: &'a [String], path: &Path, module: Module<'a>) -> Self { pub fn new(typing_modules: &'a [String], path: &Path, module: Module<'a>) -> Self {
Self { Self {
typing_modules, typing_modules,
module_path: module.path(), module,
nodes: Nodes::default(), nodes: Nodes::default(),
node_id: None, node_id: None,
branches: Branches::default(), branches: Branches::default(),
@ -791,7 +791,11 @@ impl<'a> SemanticModel<'a> {
.first() .first()
.map_or(false, |segment| *segment == ".") .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 { } else {
qualified_name qualified_name
.segments() .segments()
@ -817,14 +821,32 @@ impl<'a> SemanticModel<'a> {
} }
} }
BindingKind::ClassDefinition(_) | BindingKind::FunctionDefinition(_) => { BindingKind::ClassDefinition(_) | BindingKind::FunctionDefinition(_) => {
let value_name = UnqualifiedName::from_expr(value)?; // If we have a fully-qualified path for the module, use it.
let resolved: QualifiedName = self if let Some(path) = self.module.qualified_name() {
.module_path? Some(
.iter() path.iter()
.map(String::as_str) .map(String::as_str)
.chain(value_name.segments().iter().copied()) .chain(
.collect(); UnqualifiedName::from_expr(value)?
Some(resolved) .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, _ => None,
} }