Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict builtin-attribute-shadowing to actual shadowed references #9462

Merged
merged 1 commit into from
Jan 11, 2024
Merged

Restrict builtin-attribute-shadowing to actual shadowed references #9462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 5 additions & 37 deletions crates/ruff_linter/resources/test/fixtures/flake8_builtins/A003.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,46 +8,14 @@ def __init__(self):
self.id = 10
self.dir = "."

def str(self):
def int(self):
pass


from typing import TypedDict


class MyClass(TypedDict):
id: int


from threading import Event


class CustomEvent(Event):
def set(self) -> None:
...

def str(self) -> None:
...


from logging import Filter, LogRecord


class CustomFilter(Filter):
def filter(self, record: LogRecord) -> bool:
...

def str(self) -> None:
...


from typing_extensions import override


class MyClass:
@override
def str(self):
pass

def int(self):
def method_usage(self) -> str:
pass

def attribute_usage(self) -> id:
pass
16 changes: 15 additions & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ use crate::checkers::ast::Checker;
use crate::codes::Rule;
use crate::fix;
use crate::rules::{
flake8_pyi, flake8_type_checking, flake8_unused_arguments, pyflakes, pylint, ruff,
flake8_builtins, flake8_pyi, flake8_type_checking, flake8_unused_arguments, pyflakes, pylint,
ruff,
};

/// Run lint rules over all deferred scopes in the [`SemanticModel`].
Expand All @@ -27,6 +28,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
Rule::UndefinedLocal,
Rule::UnusedAnnotation,
Rule::UnusedClassMethodArgument,
Rule::BuiltinAttributeShadowing,
Rule::UnusedFunctionArgument,
Rule::UnusedImport,
Rule::UnusedLambdaArgument,
Expand Down Expand Up @@ -297,6 +299,18 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
ruff::rules::asyncio_dangling_binding(scope, &checker.semantic, &mut diagnostics);
}

if let Some(class_def) = scope.kind.as_class() {
if checker.enabled(Rule::BuiltinAttributeShadowing) {
flake8_builtins::rules::builtin_attribute_shadowing(
checker,
scope_id,
scope,
class_def,
&mut diagnostics,
);
}
}

if matches!(scope.kind, ScopeKind::Function(_) | ScopeKind::Lambda(_)) {
if checker.enabled(Rule::UnusedVariable) {
pyflakes::rules::unused_variable(checker, scope, &mut diagnostics);
Expand Down
8 changes: 1 addition & 7 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,13 +242,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
checker.diagnostics.push(diagnostic);
}
}
if let ScopeKind::Class(class_def) = checker.semantic.current_scope().kind {
if checker.enabled(Rule::BuiltinAttributeShadowing) {
flake8_builtins::rules::builtin_attribute_shadowing(
checker, class_def, id, *range,
);
}
} else {
if !checker.semantic.current_scope().kind.is_class() {
if checker.enabled(Rule::BuiltinVariableShadowing) {
flake8_builtins::rules::builtin_variable_shadowing(checker, id, *range);
}
Expand Down
12 changes: 1 addition & 11 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,17 +347,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::FStringDocstring) {
flake8_bugbear::rules::f_string_docstring(checker, body);
}
if let ScopeKind::Class(class_def) = checker.semantic.current_scope().kind {
if checker.enabled(Rule::BuiltinAttributeShadowing) {
flake8_builtins::rules::builtin_method_shadowing(
checker,
class_def,
name,
decorator_list,
name.range(),
);
}
} else {
if !checker.semantic.current_scope().kind.is_class() {
if checker.enabled(Rule::BuiltinVariableShadowing) {
flake8_builtins::rules::builtin_variable_shadowing(checker, name, name.range());
}
Expand Down
211 changes: 102 additions & 109 deletions crates/ruff_linter/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
use ruff_python_ast as ast;
use ruff_python_ast::{Arguments, Decorator};
use ruff_text_size::TextRange;

use ruff_diagnostics::Diagnostic;
use ruff_diagnostics::Violation;
use ruff_macros::{derive_message_formats, violation};
use ruff_python_semantic::SemanticModel;
use ruff_python_ast as ast;
use ruff_python_semantic::{BindingKind, Scope, ScopeId};
use ruff_source_file::SourceRow;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::rules::flake8_builtins::helpers::shadows_builtin;
Expand All @@ -20,6 +19,23 @@ use crate::rules::flake8_builtins::helpers::shadows_builtin;
/// non-obvious errors, as readers may mistake the attribute for the
/// builtin and vice versa.
///
/// Since methods and class attributes typically cannot be referenced directly
/// from outside the class scope, this rule only applies to those methods
/// and attributes that both shadow a builtin _and_ are referenced from within
/// the class scope, as in the following example, where the `list[int]` return
/// type annotation resolves to the `list` method, rather than the builtin:
///
/// ```python
/// class Class:
/// @staticmethod
/// def list() -> None:
/// pass
///
/// @staticmethod
/// def repeat(value: int, times: int) -> list[int]:
/// return [value] * times
/// ```
///
/// Builtins can be marked as exceptions to this rule via the
/// [`flake8-builtins.builtins-ignorelist`] configuration option, or
/// converted to the appropriate dunder method. Methods decorated with
Expand All @@ -28,135 +44,112 @@ use crate::rules::flake8_builtins::helpers::shadows_builtin;
///
/// ## Example
/// ```python
/// class Shadow:
/// def int():
/// return 0
/// ```
///
/// Use instead:
/// ```python
/// class Shadow:
/// def to_int():
/// return 0
/// ```
/// class Class:
/// @staticmethod
/// def list() -> None:
/// pass
///
/// Or:
/// ```python
/// class Shadow:
/// # Callable as `int(shadow)`
/// def __int__():
/// return 0
/// @staticmethod
/// def repeat(value: int, times: int) -> list[int]:
/// return [value] * times
/// ```
///
/// ## Options
/// - `flake8-builtins.builtins-ignorelist`
///
/// ## References
/// - [_Is it bad practice to use a built-in function name as an attribute or method identifier?_](https://stackoverflow.com/questions/9109333/is-it-bad-practice-to-use-a-built-in-function-name-as-an-attribute-or-method-ide)
/// - [_Why is it a bad idea to name a variable `id` in Python?_](https://stackoverflow.com/questions/77552/id-is-a-bad-variable-name-in-python)
#[violation]
pub struct BuiltinAttributeShadowing {
kind: Kind,
name: String,
row: SourceRow,
}

impl Violation for BuiltinAttributeShadowing {
#[derive_message_formats]
fn message(&self) -> String {
let BuiltinAttributeShadowing { name } = self;
format!("Class attribute `{name}` is shadowing a Python builtin")
let BuiltinAttributeShadowing { kind, name, row } = self;
match kind {
Kind::Attribute => {
format!("Python builtin is shadowed by class attribute `{name}` from {row}")
}
Kind::Method => {
format!("Python builtin is shadowed by method `{name}` from {row}")
}
}
}
}

/// A003
pub(crate) fn builtin_attribute_shadowing(
checker: &mut Checker,
checker: &Checker,
scope_id: ScopeId,
scope: &Scope,
class_def: &ast::StmtClassDef,
name: &str,
range: TextRange,
diagnostics: &mut Vec<Diagnostic>,
) {
if shadows_builtin(
name,
&checker.settings.flake8_builtins.builtins_ignorelist,
checker.source_type,
) {
// Ignore shadowing within `TypedDict` definitions, since these are only accessible through
// subscripting and not through attribute access.
if class_def
.bases()
.iter()
.any(|base| checker.semantic().match_typing_expr(base, "TypedDict"))
{
return;
}
for (name, binding_id) in scope.all_bindings() {
let binding = checker.semantic().binding(binding_id);

checker.diagnostics.push(Diagnostic::new(
BuiltinAttributeShadowing {
name: name.to_string(),
},
range,
));
}
}
// We only care about methods and attributes.
let kind = match binding.kind {
BindingKind::Assignment | BindingKind::Annotation => Kind::Attribute,
BindingKind::FunctionDefinition(_) => Kind::Method,
_ => continue,
};

/// A003
pub(crate) fn builtin_method_shadowing(
checker: &mut Checker,
class_def: &ast::StmtClassDef,
name: &str,
decorator_list: &[Decorator],
range: TextRange,
) {
if shadows_builtin(
name,
&checker.settings.flake8_builtins.builtins_ignorelist,
checker.source_type,
) {
// Ignore some standard-library methods. Ideally, we'd ignore all overridden methods, since
// those should be flagged on the superclass, but that's more difficult.
if is_standard_library_override(name, class_def, checker.semantic()) {
return;
}
if shadows_builtin(
name,
&checker.settings.flake8_builtins.builtins_ignorelist,
checker.source_type,
) {
// Ignore explicit overrides.
if class_def.decorator_list.iter().any(|decorator| {
checker
.semantic()
.match_typing_expr(&decorator.expression, "override")
}) {
return;
}

// Ignore explicit overrides.
if decorator_list.iter().any(|decorator| {
checker
.semantic()
.match_typing_expr(&decorator.expression, "override")
}) {
return;
// Class scopes are special, in that you can only reference a binding defined in a
// class scope from within the class scope itself. As such, we can safely ignore
// methods that weren't referenced from within the class scope. In other words, we're
// only trying to identify shadowing as in:
// ```python
// class Class:
// @staticmethod
// def list() -> None:
// pass
//
// @staticmethod
// def repeat(value: int, times: int) -> list[int]:
// return [value] * times
// ```
for reference in binding
.references
.iter()
.map(|reference_id| checker.semantic().reference(*reference_id))
.filter(|reference| {
checker
.semantic()
.first_non_type_parent_scope_id(reference.scope_id())
== Some(scope_id)
})
{
diagnostics.push(Diagnostic::new(
BuiltinAttributeShadowing {
kind,
name: name.to_string(),
row: checker.compute_source_row(binding.start()),
},
reference.range(),
));
}
}

checker.diagnostics.push(Diagnostic::new(
BuiltinAttributeShadowing {
name: name.to_string(),
},
range,
));
}
}

/// Return `true` if an attribute appears to be an override of a standard-library method.
fn is_standard_library_override(
name: &str,
class_def: &ast::StmtClassDef,
semantic: &SemanticModel,
) -> bool {
let Some(Arguments { args: bases, .. }) = class_def.arguments.as_deref() else {
return false;
};
match name {
// Ex) `Event.set`
"set" => bases.iter().any(|base| {
semantic
.resolve_call_path(base)
.is_some_and(|call_path| matches!(call_path.as_slice(), ["threading", "Event"]))
}),
// Ex) `Filter.filter`
"filter" => bases.iter().any(|base| {
semantic
.resolve_call_path(base)
.is_some_and(|call_path| matches!(call_path.as_slice(), ["logging", "Filter"]))
}),
_ => false,
}
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum Kind {
Attribute,
Method,
}
Loading
Loading