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

[refurb] Avoid None | None as well as better detection and fix (FURB168) #15779

Merged
merged 4 commits into from
Jan 31, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
44 changes: 40 additions & 4 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB168.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,45 @@
if isinstance(foo, type(None)):
pass

if isinstance(foo, (type(None))):
if isinstance(foo and bar, type(None)):
pass

if isinstance(foo, (type(None), type(None), type(None))):
pass

if isinstance(foo, None | None):
if isinstance(foo, type(None)) is True:
pass

if isinstance(foo, (None | None)):
if -isinstance(foo, type(None)):
pass

if isinstance(foo, None | type(None)):
pass

if isinstance(foo, (None | type(None))):
if isinstance(foo, type(None) | type(None)):
pass

# A bit contrived, but is both technically valid and equivalent to the above.
if isinstance(foo, (type(None) | ((((type(None))))) | ((None | type(None))))):
pass

if isinstance(
foo, # Comment
None
):
...

from typing import Union

if isinstance(foo, Union[None]):
...

if isinstance(foo, Union[None, None]):
...

if isinstance(foo, Union[None, type(None)]):
...


# Okay.

Expand All @@ -42,10 +59,29 @@
if isinstance(foo, (int, type(None), str)):
pass

if isinstance(foo, str | None):
pass

if isinstance(foo, Union[None, str]):
...

# This is a TypeError, which the rule ignores.
if isinstance(foo, None):
pass

# This is also a TypeError, which the rule ignores.
if isinstance(foo, (None,)):
pass

if isinstance(foo, None | None):
pass

if isinstance(foo, (type(None) | ((((type(None))))) | ((None | None | type(None))))):
pass

# https://github.com/astral-sh/ruff/issues/15776
def _():
def type(*args): ...

if isinstance(foo, type(None)):
...
124 changes: 88 additions & 36 deletions crates/ruff_linter/src/rules/refurb/rules/isinstance_type_none.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_python_ast::{self as ast, Expr, Operator};
use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_python_ast::{self as ast, CmpOp, Expr, Operator};

use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_text_size::Ranged;
use ruff_python_semantic::SemanticModel;
use ruff_text_size::TextRange;

use crate::checkers::ast::Checker;
use crate::fix::edits::pad;
use crate::rules::refurb::helpers::generate_none_identity_comparison;

/// ## What it does
/// Checks for uses of `isinstance` that check if an object is of type `None`.
Expand All @@ -25,6 +24,9 @@ use crate::rules::refurb::helpers::generate_none_identity_comparison;
/// obj is None
/// ```
///
/// ## Fix safety
/// The fix will be marked as unsafe if there are any comments within the call.
///
/// ## References
/// - [Python documentation: `isinstance`](https://docs.python.org/3/library/functions.html#isinstance)
/// - [Python documentation: `None`](https://docs.python.org/3/library/constants.html#None)
Expand All @@ -48,37 +50,35 @@ impl Violation for IsinstanceTypeNone {

/// FURB168
pub(crate) fn isinstance_type_none(checker: &mut Checker, call: &ast::ExprCall) {
let Some(types) = call.arguments.find_positional(1) else {
let semantic = checker.semantic();
let (func, arguments) = (&call.func, &call.arguments);

if !arguments.keywords.is_empty() {
return;
}

let [expr, types] = arguments.args.as_ref() else {
return;
};
if !checker
.semantic()
.match_builtin_expr(&call.func, "isinstance")
{

if !semantic.match_builtin_expr(func, "isinstance") {
return;
}
if is_none(types) {
let Some(Expr::Name(ast::ExprName {
id: object_name, ..
})) = call.arguments.find_positional(0)
else {
return;
};
let mut diagnostic = Diagnostic::new(IsinstanceTypeNone, call.range());
let replacement =
generate_none_identity_comparison(object_name.clone(), false, checker.generator());
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
pad(replacement, call.range(), checker.locator()),
call.range(),
)));
checker.diagnostics.push(diagnostic);

if !is_none(types, semantic) {
return;
}

let fix = replace_with_identity_check(expr, call.range, checker);
let diagnostic = Diagnostic::new(IsinstanceTypeNone, call.range);

checker.diagnostics.push(diagnostic.with_fix(fix));
}

/// Returns `true` if the given expression is equivalent to checking if the
/// object type is `None` when used with the `isinstance` builtin.
fn is_none(expr: &Expr) -> bool {
fn inner(expr: &Expr, in_union_context: bool) -> bool {
fn is_none(expr: &Expr, semantic: &SemanticModel) -> bool {
fn inner(expr: &Expr, in_union_context: bool, semantic: &SemanticModel) -> bool {
match expr {
// Ex) `None`
// Note: `isinstance` only accepts `None` as a type when used with
Expand All @@ -88,29 +88,81 @@ fn is_none(expr: &Expr) -> bool {
// Ex) `type(None)`
Expr::Call(ast::ExprCall {
func, arguments, ..
}) if arguments.len() == 1 => {
if let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() {
if id.as_str() == "type" {
return matches!(arguments.args.first(), Some(Expr::NoneLiteral(_)));
}
}) => {
if !semantic.match_builtin_expr(func, "type") {
return false;
}

if !arguments.keywords.is_empty() {
return false;
}
false

matches!(arguments.args.as_ref(), [Expr::NoneLiteral(_)])
}

// Ex) `(type(None),)`
Expr::Tuple(tuple) => tuple.iter().all(|element| inner(element, false)),
Expr::Tuple(tuple) => tuple.iter().all(|element| inner(element, false, semantic)),

// Ex) `type(None) | type(None)`
Expr::BinOp(ast::ExprBinOp {
left,
op: Operator::BitOr,
right,
..
}) => inner(left, true) && inner(right, true),
}) => {
// `None | None` is a `TypeError` at runtime
if left.is_none_literal_expr() && right.is_none_literal_expr() {
return false;
}

inner(left, true, semantic) && inner(right, true, semantic)
}

// Ex) `Union[None, ...]`
Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => {
if !semantic.match_typing_expr(value, "Union") {
return false;
}

match slice.as_ref() {
Expr::Tuple(ast::ExprTuple { elts, .. }) => {
elts.iter().all(|element| inner(element, true, semantic))
}
slice => inner(slice, true, semantic),
}
}

// Otherwise, return false.
_ => false,
}
}
inner(expr, false)
inner(expr, false, semantic)
}

fn replace_with_identity_check(expr: &Expr, range: TextRange, checker: &Checker) -> Fix {
let (semantic, generator) = (checker.semantic(), checker.generator());

let new_expr = Expr::Compare(ast::ExprCompare {
left: expr.clone().into(),
ops: [CmpOp::Is].into(),
comparators: [ast::ExprNoneLiteral::default().into()].into(),
range: TextRange::default(),
});

let new_content = generator.expr(&new_expr);
let new_content = if semantic.current_expression_parent().is_some() {
format!("({new_content})")
} else {
new_content
};

let applicability = if checker.comment_ranges().intersects(range) {
Applicability::Unsafe
} else {
Applicability::Safe
};

let edit = Edit::range_replacement(new_content, range);

Fix::applicable_edit(edit, applicability)
}
Loading
Loading