Skip to content

Commit

Permalink
Ignore some common builtin overrides on standard library subclasses (#…
Browse files Browse the repository at this point in the history
…6074)

## Summary

If a user subclasses `threading.Event`, e.g. with:

```python
from threading import Event


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

They no control over the method name (`set`). This PR allows
`threading.Event#set` and `logging.Filter#filter` overrides, and avoids
flagging A003 in such cases. Ideally, we'd avoid flagging all overridden
methods, but... that's a lot more difficult, and this is at least
_better_ than what we do now.

Closes #6057.

Closes #5956.
  • Loading branch information
charliermarsh authored Jul 25, 2023
1 parent c996b61 commit 5f63b8b
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 0 deletions.
22 changes: 22 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_builtins/A003.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,25 @@ def str(self):

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:
...
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use rustpython_parser::ast;
use ruff_diagnostics::Diagnostic;
use ruff_diagnostics::Violation;
use ruff_macros::{derive_message_formats, violation};
use ruff_python_semantic::SemanticModel;

use crate::checkers::ast::Checker;
use crate::rules::flake8_builtins::helpers::shadows_builtin;
Expand Down Expand Up @@ -80,6 +81,12 @@ pub(crate) fn builtin_attribute_shadowing(
return;
}

// 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;
}

checker.diagnostics.push(Diagnostic::new(
BuiltinAttributeShadowing {
name: name.to_string(),
Expand All @@ -88,3 +95,26 @@ pub(crate) fn builtin_attribute_shadowing(
));
}
}

/// 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,
model: &SemanticModel,
) -> bool {
match name {
// Ex) `Event#set`
"set" => class_def.bases.iter().any(|base| {
model.resolve_call_path(base).map_or(false, |call_path| {
matches!(call_path.as_slice(), ["threading", "Event"])
})
}),
// Ex) `Filter#filter`
"filter" => class_def.bases.iter().any(|base| {
model.resolve_call_path(base).map_or(false, |call_path| {
matches!(call_path.as_slice(), ["logging", "Filter"])
})
}),
_ => false,
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,22 @@ A003.py:11:9: A003 Class attribute `str` is shadowing a Python builtin
12 | pass
|

A003.py:29:9: A003 Class attribute `str` is shadowing a Python builtin
|
27 | ...
28 |
29 | def str(self) -> None:
| ^^^ A003
30 | ...
|

A003.py:40:9: A003 Class attribute `str` is shadowing a Python builtin
|
38 | ...
39 |
40 | def str(self) -> None:
| ^^^ A003
41 | ...
|


Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,22 @@ A003.py:11:9: A003 Class attribute `str` is shadowing a Python builtin
12 | pass
|

A003.py:29:9: A003 Class attribute `str` is shadowing a Python builtin
|
27 | ...
28 |
29 | def str(self) -> None:
| ^^^ A003
30 | ...
|

A003.py:40:9: A003 Class attribute `str` is shadowing a Python builtin
|
38 | ...
39 |
40 | def str(self) -> None:
| ^^^ A003
41 | ...
|


0 comments on commit 5f63b8b

Please sign in to comment.