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.

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

Improve handling of builtin symbols in linter rules #10919

Merged
merged 9 commits into from
Apr 16, 2024
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") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The counterpoint is that we're now doing other work that isn't strictly necessary (for example, if this is a name, and builtins isn't imported, and the name isn't getattr, then the call to is_mangled_private wasn't necessary -- we could've exited much earlier; similarly, we risk calling is_identifier on many more function calls that won't ever match).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is clearly still the right tradeoff, but that's the tradeoff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way around this would be to, like, have some could_be_builtin("getattr") method you call at the top that returns false if builtins isn't imported, it's not a name, or the name isn't getattr, but IDK, that seems not great / worthwhile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These things (like is_identifier) do show up in traces sometimes because we tend to be calling these rules for every function call, which is why being able to gate on something as cheap as "the name of the function" is useful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to change anything here but wanted to raise visibility on it.

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
Loading
Loading