Skip to content

Commit

Permalink
Improve handling of builtin symbols in linter rules (#10919)
Browse files Browse the repository at this point in the history
Add a new method to the semantic model to simplify and improve the correctness of a common pattern
  • Loading branch information
AlexWaygood committed Apr 16, 2024
1 parent effd518 commit f779bab
Show file tree
Hide file tree
Showing 93 changed files with 886 additions and 588 deletions.
19 changes: 19 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B004.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,25 @@ def this_is_a_bug():
print("Ooh, callable! Or is it?")


def still_a_bug():
import builtins
o = object()
if builtins.hasattr(o, "__call__"):
print("B U G")
if builtins.getattr(o, "__call__", False):
print("B U G")


def trickier_fix_for_this_one():
o = object()

def callable(x):
return True

if hasattr(o, "__call__"):
print("STILL a bug!")


def this_is_fine():
o = object()
if callable(o):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,6 @@
# Regression test for: https://github.com/astral-sh/ruff/issues/7455#issuecomment-1739800901
getattr(self.
registration.registry, '__name__')

import builtins
builtins.getattr(foo, "bar")
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,7 @@
# Errors (limited iterators).
zip([1, 2, 3], repeat(1, 1))
zip([1, 2, 3], repeat(1, times=4))

import builtins
# Still an error even though it uses the qualified name
builtins.zip([1, 2, 3])
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
# PIE808
range(0, 10)

import builtins
builtins.range(0, 10)

# OK
range(x, 10)
range(-15, 10)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,10 @@ class BadFive:
class BadSix:
def __exit__(self, typ, exc, tb, weird_extra_arg, extra_arg2 = None) -> None: ... # PYI036: Extra arg must have default
async def __aexit__(self, typ, exc, tb, *, weird_extra_arg) -> None: ... # PYI036: kwargs must have default


def isolated_scope():
from builtins import type as Type

class ShouldNotError:
def __exit__(self, typ: Type[BaseException] | None, exc: BaseException | None, tb: TracebackType | None) -> None: ...
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,9 @@ class Bad(Tuple[str, int, float]): # SLOT001

class Good(Tuple[str, int, float]): # OK
__slots__ = ("foo",)


import builtins

class AlsoBad(builtins.tuple[int, int]): # SLOT001
pass
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,8 @@
for i in list(foo_list): # OK
if True:
del foo_list[i + 1]

import builtins

for i in builtins.list(nested_tuple): # PERF101
pass
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,8 @@ def type():

#: E721
dtype == float

import builtins

if builtins.type(res) == memoryview: # E721
pass
5 changes: 5 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pyflakes/F901.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,8 @@ def f() -> None:

def g() -> None:
raise NotImplemented


def h() -> None:
NotImplementedError = "foo"
raise NotImplemented
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,6 @@
pathlib.Path(NAME).open("rwx") # [bad-open-mode]
pathlib.Path(NAME).open(mode="rwx") # [bad-open-mode]
pathlib.Path(NAME).open("rwx", encoding="utf-8") # [bad-open-mode]

import builtins
builtins.open(NAME, "Ua", encoding="utf-8")
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@
if y == npy_nan:
pass

import builtins

# PLW0117
if x == builtins.float("nan"):
pass

# OK
if math.isnan(x):
pass
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,6 @@

# Starred argument should be copied as it is.
max(1, max(*a))

import builtins
builtins.min(1, min(2, 3))
6 changes: 6 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pyupgrade/UP004.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,3 +152,9 @@ class A(object):

class B(object):
...


import builtins

class Unusual(builtins.object):
...
15 changes: 15 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB103.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,21 @@ def bar(x):
f.write(foobar)


import builtins


# FURB103
with builtins.open("file.txt", "w", newline="\r\n") as f:
f.write(foobar)


from builtins import open as o


# FURB103
with o("file.txt", "w", newline="\r\n") as f:
f.write(foobar)

# Non-errors.

with open("file.txt", errors="ignore", mode="wb") as f:
Expand Down
16 changes: 16 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB129.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,22 @@ def func():
pass


import builtins


with builtins.open("FURB129.py") as f:
for line in f.readlines():
pass


from builtins import open as o


with o("FURB129.py") as f:
for line in f.readlines():
pass


# False positives
def func(f):
for _line in f.readlines():
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF024.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
dict.fromkeys(pierogi_fillings, set())
dict.fromkeys(pierogi_fillings, {"pre": "populated!"})
dict.fromkeys(pierogi_fillings, dict())
import builtins
builtins.dict.fromkeys(pierogi_fillings, dict())

# Okay.
dict.fromkeys(pierogi_fillings)
Expand Down
17 changes: 7 additions & 10 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -784,17 +784,13 @@ impl<'a> Visitor<'a> for Checker<'a> {
}) => {
let mut handled_exceptions = Exceptions::empty();
for type_ in extract_handled_exceptions(handlers) {
if let Some(qualified_name) = self.semantic.resolve_qualified_name(type_) {
match qualified_name.segments() {
["", "NameError"] => {
handled_exceptions |= Exceptions::NAME_ERROR;
}
["", "ModuleNotFoundError"] => {
if let Some(builtins_name) = self.semantic.resolve_builtin_symbol(type_) {
match builtins_name {
"NameError" => handled_exceptions |= Exceptions::NAME_ERROR,
"ModuleNotFoundError" => {
handled_exceptions |= Exceptions::MODULE_NOT_FOUND_ERROR;
}
["", "ImportError"] => {
handled_exceptions |= Exceptions::IMPORT_ERROR;
}
"ImportError" => handled_exceptions |= Exceptions::IMPORT_ERROR,
_ => {}
}
}
Expand Down Expand Up @@ -1125,7 +1121,8 @@ impl<'a> Visitor<'a> for Checker<'a> {
]
) {
Some(typing::Callable::MypyExtension)
} else if matches!(qualified_name.segments(), ["", "bool"]) {
} else if matches!(qualified_name.segments(), ["" | "builtins", "bool"])
{
Some(typing::Callable::Bool)
} else {
None
Expand Down
25 changes: 25 additions & 0 deletions crates/ruff_linter/src/importer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,31 @@ impl<'a> Importer<'a> {
.map_or_else(|| self.import_symbol(symbol, at, None, semantic), Ok)
}

/// For a given builtin symbol, determine whether an [`Edit`] is necessary to make the symbol
/// available in the current scope. For example, if `zip` has been overridden in the relevant
/// scope, the `builtins` module will need to be imported in order for a `Fix` to reference
/// `zip`; but otherwise, that won't be necessary.
///
/// Returns a two-item tuple. The first item is either `Some(Edit)` (indicating) that an
/// edit is necessary to make the symbol available, or `None`, indicating that the symbol has
/// not been overridden in the current scope. The second item in the tuple is the bound name
/// of the symbol.
///
/// Attempts to reuse existing imports when possible.
pub(crate) fn get_or_import_builtin_symbol(
&self,
symbol: &str,
at: TextSize,
semantic: &SemanticModel,
) -> Result<(Option<Edit>, String), ResolutionError> {
if semantic.is_builtin(symbol) {
return Ok((None, symbol.to_string()));
}
let (import_edit, binding) =
self.get_or_import_symbol(&ImportRequest::import("builtins", symbol), at, semantic)?;
Ok((Some(import_edit), binding))
}

/// Return the [`ImportedName`] to for existing symbol, if it's present in the given [`SemanticModel`].
fn find_symbol(
symbol: &ImportRequest,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ fn is_open_sleep_or_subprocess_call(func: &Expr, semantic: &SemanticModel) -> bo
.is_some_and(|qualified_name| {
matches!(
qualified_name.segments(),
["", "open"]
["" | "builtins", "open"]
| ["time", "sleep"]
| [
"subprocess",
Expand Down
18 changes: 4 additions & 14 deletions crates/ruff_linter/src/rules/flake8_bandit/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,23 +24,13 @@ pub(super) fn is_untyped_exception(type_: Option<&Expr>, semantic: &SemanticMode
if let Expr::Tuple(ast::ExprTuple { elts, .. }) = &type_ {
elts.iter().any(|type_| {
semantic
.resolve_qualified_name(type_)
.is_some_and(|qualified_name| {
matches!(
qualified_name.segments(),
["", "Exception" | "BaseException"]
)
})
.resolve_builtin_symbol(type_)
.is_some_and(|builtin| matches!(builtin, "Exception" | "BaseException"))
})
} else {
semantic
.resolve_qualified_name(type_)
.is_some_and(|qualified_name| {
matches!(
qualified_name.segments(),
["", "Exception" | "BaseException"]
)
})
.resolve_builtin_symbol(type_)
.is_some_and(|builtin| matches!(builtin, "Exception" | "BaseException"))
}
})
}
Original file line number Diff line number Diff line change
Expand Up @@ -95,24 +95,22 @@ pub(crate) fn assert_raises_exception(checker: &mut Checker, items: &[WithItem])
return;
};

let Some(exception) =
checker
.semantic()
.resolve_qualified_name(arg)
.and_then(|qualified_name| match qualified_name.segments() {
["", "Exception"] => Some(ExceptionKind::Exception),
["", "BaseException"] => Some(ExceptionKind::BaseException),
_ => None,
})
else {
let semantic = checker.semantic();

let Some(builtin_symbol) = semantic.resolve_builtin_symbol(arg) else {
return;
};

let exception = match builtin_symbol {
"Exception" => ExceptionKind::Exception,
"BaseException" => ExceptionKind::BaseException,
_ => return,
};

let assertion = if matches!(func.as_ref(), Expr::Attribute(ast::ExprAttribute { attr, .. }) if attr == "assertRaises")
{
AssertionKind::AssertRaises
} else if checker
.semantic()
} else if semantic
.resolve_qualified_name(func)
.is_some_and(|qualified_name| matches!(qualified_name.segments(), ["pytest", "raises"]))
&& arguments.find_keyword("match").is_none()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,6 @@ pub(crate) fn getattr_with_constant(
func: &Expr,
args: &[Expr],
) {
let Expr::Name(ast::ExprName { id, .. }) = func else {
return;
};
if id != "getattr" {
return;
}
let [obj, arg] = args else {
return;
};
Expand All @@ -75,7 +69,7 @@ pub(crate) fn getattr_with_constant(
if is_mangled_private(value.to_str()) {
return;
}
if !checker.semantic().is_builtin("getattr") {
if !checker.semantic().match_builtin_expr(func, "getattr") {
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,6 @@ pub(crate) fn setattr_with_constant(
func: &Expr,
args: &[Expr],
) {
let Expr::Name(ast::ExprName { id, .. }) = func else {
return;
};
if id != "setattr" {
return;
}
let [obj, name, value] = args else {
return;
};
Expand All @@ -89,7 +83,7 @@ pub(crate) fn setattr_with_constant(
if is_mangled_private(name.to_str()) {
return;
}
if !checker.semantic().is_builtin("setattr") {
if !checker.semantic().match_builtin_expr(func, "setattr") {
return;
}

Expand Down

0 comments on commit f779bab

Please sign in to comment.