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

[pylint] Implement len-test (PLC1802) #14309

Merged
merged 10 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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
183 changes: 183 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pylint/len_as_condition.py
Lokejoke marked this conversation as resolved.
Show resolved Hide resolved
Lokejoke marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,183 @@
if len('TEST'): # [len-as-condition]
pass

if not len('TEST'): # [len-as-condition]
pass

z = []
if z and len(['T', 'E', 'S', 'T']): # [len-as-condition]
pass

if True or len('TEST'): # [len-as-condition]
pass

if len('TEST') == 0: # Should be fine
pass

if len('TEST') < 1: # Should be fine
pass

if len('TEST') <= 0: # Should be fine
pass

if 1 > len('TEST'): # Should be fine
pass

if 0 >= len('TEST'): # Should be fine
pass

if z and len('TEST') == 0: # Should be fine
pass

if 0 == len('TEST') < 10: # Should be fine
pass

# Should be fine
if 0 < 1 <= len('TEST') < 10: # [comparison-of-constants]
pass

if 10 > len('TEST') != 0: # Should be fine
pass

if 10 > len('TEST') > 1 > 0: # Should be fine
pass

if 0 <= len('TEST') < 100: # Should be fine
pass

if z or 10 > len('TEST') != 0: # Should be fine
pass

if z:
pass
elif len('TEST'): # [len-as-condition]
pass

if z:
pass
elif not len('TEST'): # [len-as-condition]
pass

while len('TEST'): # [len-as-condition]
pass

while not len('TEST'): # [len-as-condition]
pass

while z and len('TEST'): # [len-as-condition]
pass

while not len('TEST') and z: # [len-as-condition]
pass

assert len('TEST') > 0 # Should be fine

x = 1 if len('TEST') != 0 else 2 # Should be fine

f_o_o = len('TEST') or 42 # Should be fine

a = x and len(x) # Should be fine

def some_func():
return len('TEST') > 0 # Should be fine

def github_issue_1325():
l = [1, 2, 3]
length = len(l) if l else 0 # Should be fine
return length

def github_issue_1331(*args):
assert False, len(args) # Should be fine

def github_issue_1331_v2(*args):
assert len(args), args # [len-as-condition]

def github_issue_1331_v3(*args):
assert len(args) or z, args # [len-as-condition]

def github_issue_1331_v4(*args):
assert z and len(args), args # [len-as-condition]

b = bool(len(z)) # [len-as-condition]
c = bool(len('TEST') or 42) # [len-as-condition]

def github_issue_1879():

# class ClassWithBool(list):
# def __bool__(self):
# return True

# class ClassWithoutBool(list):
# pass

# class ChildClassWithBool(ClassWithBool):
# pass

# class ChildClassWithoutBool(ClassWithoutBool):
# pass

# assert len(ClassWithBool())
# assert len(ChildClassWithBool())
# assert len(ClassWithoutBool()) # [len-as-condition]
# assert len(ChildClassWithoutBool()) # [len-as-condition]
# assert len(range(0)) # [len-as-condition]
assert len([t + 1 for t in []]) # [len-as-condition]
assert len(u + 1 for u in []) # [len-as-condition]
assert len({"1":(v + 1) for v in {}}) # [len-as-condition]
assert len(set((w + 1) for w in set())) # [len-as-condition]


# import numpy
# numpy_array = numpy.array([0])
# if len(numpy_array) > 0:
# print('numpy_array')
# if len(numpy_array):
# print('numpy_array')
# if numpy_array:
# print('b')

# import pandas as pd
# pandas_df = pd.DataFrame()
# if len(pandas_df):
# print("this works, but pylint tells me not to use len() without comparison")
# if len(pandas_df) > 0:
# print("this works and pylint likes it, but it's not the solution intended by PEP-8")
# if pandas_df:
# print("this does not work (truth value of dataframe is ambiguous)")

# def function_returning_list(r):
# if r==1:
# return [1]
# return [2]

# def function_returning_int(r):
# if r==1:
# return 1
# return 2

# def function_returning_generator(r):
# for i in [r, 1, 2, 3]:
# yield i

# def function_returning_comprehension(r):
# return [x+1 for x in [r, 1, 2, 3]]

# def function_returning_function(r):
# return function_returning_generator(r)

# assert len(function_returning_list(z)) # [len-as-condition]
# assert len(function_returning_int(z))
# # This should raise a len-as-condition once astroid can infer it
# # See https://github.com/pylint-dev/pylint/pull/3821#issuecomment-743771514
# assert len(function_returning_generator(z))
# assert len(function_returning_comprehension(z))
# assert len(function_returning_function(z))


def github_issue_4215():
# Test undefined variables
# https://github.com/pylint-dev/pylint/issues/4215
if len(undefined_var): # [undefined-variable]
pass
if len(undefined_var2[0]): # [undefined-variable]
pass
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::SuperWithoutBrackets) {
pylint::rules::super_without_brackets(checker, func);
}
if checker.enabled(Rule::LenAsCondition) {
pylint::rules::len_as_condition(checker, call);
}
if checker.enabled(Rule::BitCount) {
refurb::rules::bit_count(checker, call);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "C0208") => (RuleGroup::Stable, rules::pylint::rules::IterationOverSet),
(Pylint, "C0414") => (RuleGroup::Stable, rules::pylint::rules::UselessImportAlias),
(Pylint, "C0415") => (RuleGroup::Preview, rules::pylint::rules::ImportOutsideTopLevel),
(Pylint, "C1802") => (RuleGroup::Preview, rules::pylint::rules::LenAsCondition),
(Pylint, "C1901") => (RuleGroup::Preview, rules::pylint::rules::CompareToEmptyString),
(Pylint, "C2401") => (RuleGroup::Stable, rules::pylint::rules::NonAsciiName),
(Pylint, "C2403") => (RuleGroup::Stable, rules::pylint::rules::NonAsciiImportName),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ mod tests {
Rule::BadStaticmethodArgument,
Path::new("bad_staticmethod_argument.py")
)]
#[test_case(Rule::LenAsCondition, Path::new("len_as_condition.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
158 changes: 158 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/len_as_condition.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
use crate::checkers::ast::Checker;
use crate::fix::snippet::SourceCodeSnippet;
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr, ExprCall, Parameter};
use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType};
use ruff_python_semantic::analyze::typing::find_binding_value;
use ruff_python_semantic::{BindingId, SemanticModel};
use ruff_text_size::Ranged;

/// ## What it does
/// Checks for usage of call of 'len' on sequences
/// in boolean test context.
///
/// ## Why is this bad?
/// Empty sequences are considered false in a boolean context.
/// You can either remove the call to 'len' (``if not x``)
/// or compare the length against a scalar (``if len(x) > 0``).
Lokejoke marked this conversation as resolved.
Show resolved Hide resolved
///
/// ## Example
Lokejoke marked this conversation as resolved.
Show resolved Hide resolved
/// ```python
/// fruits = ["orange", "apple"]
///
/// if len(fruits):
/// print(fruits)
/// ```
///
/// Use instead:
/// ```python
/// fruits = ["orange", "apple"]
///
/// if fruits:
/// print(fruits)
/// ```
Lokejoke marked this conversation as resolved.
Show resolved Hide resolved
#[violation]
pub struct LenAsCondition {
expression: SourceCodeSnippet,
}

impl AlwaysFixableViolation for LenAsCondition {
#[derive_message_formats]
fn message(&self) -> String {
if let Some(expression) = self.expression.full_display() {
format!("`len({expression})` without comparison used as condition")
} else {
"`len(SEQUENCE)` without comparison used as condition".to_string()
Lokejoke marked this conversation as resolved.
Show resolved Hide resolved
}
}

fn fix_title(&self) -> String {
"Remove `len`".to_string()
}
}

/// PLC1802
pub(crate) fn len_as_condition(checker: &mut Checker, call: &ExprCall) {
let ExprCall {
func, arguments, ..
} = call;
let semantic = checker.semantic();

if !semantic.in_boolean_test() {
return;
}

if !semantic.match_builtin_expr(func, "len") {
return;
}

// Single argument and no keyword arguments
let [argument] = &*arguments.args else { return };
if !arguments.keywords.is_empty() {
return;
}

// Simple inferred sequence type (e.g., list, set, dict, tuple, string, generator) or a vararg.
if !is_sequence(argument, semantic) && !is_indirect_sequence(argument, semantic) {
return;
}

let replacement = checker.locator().slice(argument.range()).to_string();

let mut diagnostic = Diagnostic::new(
LenAsCondition {
expression: SourceCodeSnippet::new(replacement.clone()),
},
call.range(),
);

diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
// Generator without parentheses would create syntax error
if argument.is_generator_expr() {
format!("({replacement})")
} else {
replacement
},
call.range(),
)));

checker.diagnostics.push(diagnostic);
}

fn is_indirect_sequence(expr: &Expr, semantic: &SemanticModel) -> bool {
let Expr::Name(ast::ExprName { id: name, .. }) = expr else {
return false;
};

let scope = semantic.current_scope();
let bindings: Vec<BindingId> = scope.get_all(name).collect();
let [binding_id] = bindings.as_slice() else {
return false;
};

let binding = semantic.binding(*binding_id);

// Attempt to find the binding's value
let Some(binding_value) = find_binding_value(binding, semantic) else {
// check for `vararg`
Lokejoke marked this conversation as resolved.
Show resolved Hide resolved
return binding.kind.is_argument()
&& binding
.statement(semantic)
.and_then(|statement| statement.as_function_def_stmt())
.and_then(|function| function.parameters.vararg.as_deref())
.map_or(false, |Parameter { name: var_arg, .. }| {
var_arg.id() == name
});
};

// If `binding_value` is found, check if it is a sequence
is_sequence(binding_value, semantic)
}

fn is_sequence(expr: &Expr, semantic: &SemanticModel) -> bool {
// Check if the expression type is a direct sequence match (dict, list, set, tuple, generator, string)
Lokejoke marked this conversation as resolved.
Show resolved Hide resolved
if matches!(
ResolvedPythonType::from(expr),
ResolvedPythonType::Atom(
PythonType::Dict
| PythonType::List
| PythonType::Set
| PythonType::Tuple
| PythonType::Generator
Lokejoke marked this conversation as resolved.
Show resolved Hide resolved
Lokejoke marked this conversation as resolved.
Show resolved Hide resolved
| PythonType::String
)
) {
return true;
}

// Check if the expression is a function call to a built-in sequence constructor
let Some(ExprCall { func, .. }) = expr.as_call_expr() else {
return false;
};

// Match against specific built-in constructors that return sequences
return semantic
.resolve_builtin_symbol(func)
.is_some_and(|func| matches!(func, "list" | "set" | "dict" | "tuple"));
Lokejoke marked this conversation as resolved.
Show resolved Hide resolved
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ pub(crate) use invalid_length_return::*;
pub(crate) use invalid_str_return::*;
pub(crate) use invalid_string_characters::*;
pub(crate) use iteration_over_set::*;
pub(crate) use len_as_condition::*;
pub(crate) use literal_membership::*;
pub(crate) use load_before_global_declaration::*;
pub(crate) use logging::*;
Expand Down Expand Up @@ -143,6 +144,7 @@ mod invalid_length_return;
mod invalid_str_return;
mod invalid_string_characters;
mod iteration_over_set;
mod len_as_condition;
mod literal_membership;
mod load_before_global_declaration;
mod logging;
Expand Down
Loading
Loading