Skip to content

Commit

Permalink
feat(pylint): simplify PLR6103
Browse files Browse the repository at this point in the history
  • Loading branch information
@vincevannoort
vincevannoort committed Sep 6, 2024
1 parent 7bba332 commit 84c2180
Show file tree
Hide file tree
Showing 3 changed files with 141 additions and 262 deletions.
45 changes: 21 additions & 24 deletions crates/ruff_linter/resources/test/fixtures/pylint/unnecessary_assignment.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,7 @@ def foo():
if bad4: # [consider-using-assignment-expr]
pass

bad5 = 'example'
if bad5: # [consider-using-assignment-expr]
print(bad5)


bad6_1 = 0
bad6_2 = 0
if True:
pass
elif bad6_1: # [consider-using-assignment-expr]
pass
elif bad6_2: # [consider-using-assignment-expr]
pass

bad7 = (
bad5 = (
'example',
'example',
'example',
Expand All @@ -42,21 +28,32 @@ def foo():
'example',
'example',
)
if bad7: # [consider-using-assignment-expr]
if bad5: # [consider-using-assignment-expr]
pass

bad8 = 'example'
if bad8 is not None: # [consider-using-assignment-expr]
bad6 = 'example'
if bad6 is not None: # [consider-using-assignment-expr]
pass

good1_1 = 'example'
good1_2 = good1_1
if good1_1: # correct, walrus cannot be used because expression is already used before if
good1_2 = 'example'
if good1_1: # correct, assignment is not the previous statement
pass

good2_1 = 'example'
good2_2 = good2_1
if good2_1: # correct, assignment is not the previous statement
pass

if good2 := 'example': # correct
if good3 := 'example': # correct, used like it is intented
pass

def test(good3: str | None = None):
if good3 is None:
good3 = 'test'
def test(good4: str | None = None):
if good4 is None:
good4 = 'test'

def bar():
good4_5 = 'example'
good4_2 = good4_5
if good4_5: # assignment is not the previous statement
pass
210 changes: 81 additions & 129 deletions crates/ruff_linter/src/rules/pylint/rules/unnecessary_assignment.rs
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{name::Name, Expr, ExprName, Stmt, StmtAssign, StmtIf};
use ruff_python_ast::{name::Name, AstNode, Expr, ExprName, Stmt, StmtAssign, StmtIf};
use ruff_python_codegen::Generator;
use ruff_python_index::Indexer;
use ruff_python_semantic::{Binding, BindingKind, NodeRef, ResolvedReferenceId, SemanticModel};
use ruff_python_semantic::{
Binding, BindingKind, NodeId, NodeRef, ResolvedReferenceId, Scope, SemanticModel,
};
use ruff_source_file::Locator;
use ruff_text_size::Ranged;

Expand Down Expand Up @@ -77,176 +79,126 @@ impl Violation for UnnecessaryAssignment {
}
}

type UnreferencedBinding<'a> = (Expr, &'a Binding<'a>, ExprName, StmtAssign);

fn find_unreferenced_binding_from_check<'a>(
origin: &Expr,
semantic: &'a SemanticModel,
check: &Expr,
) -> Option<UnreferencedBinding<'a>> {
// only care about variable expressions, like `if test1:`
let Expr::Name(check_variable) = check.clone() else {
return None;
};

let scope: &ruff_python_semantic::Scope<'_> = semantic.current_scope();
let Some(binding) = scope
.bindings()
.find(|(_, binding_id)| {
let binding = semantic.binding(*binding_id);

// only bindings that come before the check
if binding.range().start() > check_variable.range().start() {
return false;
}

// we are only interested in assignment bindings
let BindingKind::Assignment = binding.kind else {
return false;
};

let assignment = semantic.node(binding.source.unwrap());

// then we only care for expressions
let NodeRef::Expr(assignment_expr) = assignment else {
return false;
};

// and only if those are named expressions
let Expr::Name(assignment_variable) = assignment_expr else {
return false;
};

// ignore if the ids do not match
if check_variable.id != assignment_variable.id {
return false;
}

// we can only walrus if the binding has no references
if !binding
.references
.iter()
// only keep references that come before the `if_value`
.filter(|&&reference_id| {
let reference = semantic.reference(reference_id);
reference.range().start() < check_variable.range().start()
})
.collect::<Vec<&ResolvedReferenceId>>()
.is_empty()
{
return false;
}

true
})
.map(|(_, binding_id)| semantic.binding(binding_id))
else {
// we did not find a binding matching our criteria
return None;
};

let assignment: &ruff_python_ast::StmtAssign = semantic
.statement(binding.source.unwrap())
.as_assign_stmt()
.unwrap();

Some((origin.clone(), binding, check_variable, assignment.clone()))
}

fn create_diagnostic(
locator: &Locator,
indexer: &Indexer,
generator: Generator,
unreferenced_binding: UnreferencedBinding,
) -> Diagnostic {
let (origin, _, expr_name, assignment) = unreferenced_binding;
let value_expr = generator.expr(&assignment.value.clone());
let use_parentheses = origin.is_bool_op_expr() || !assignment.value.is_name_expr();

let mut diagnostic = Diagnostic::new(
UnnecessaryAssignment {
check: true,
name: expr_name.clone().id,
assignment: value_expr.clone(),
parentheses: use_parentheses,
},
expr_name.clone().range(),
// assignment.clone().range(),
);

let format = if use_parentheses {
format!("({} := {})", expr_name.clone().id, value_expr.clone())
} else {
format!("{} := {}", expr_name.clone().id, value_expr.clone())
};

let delete_assignment_edit = delete_stmt(&Stmt::from(assignment), None, locator, indexer);
let use_walrus_edit = Edit::range_replacement(format, diagnostic.range());

diagnostic.set_fix(Fix::unsafe_edits(delete_assignment_edit, [use_walrus_edit]));

diagnostic
}
type AssignmentBeforeIf<'a> = (Expr, ExprName, StmtAssign);

/// PLR6103
pub(crate) fn unnecessary_assignment(checker: &mut Checker, stmt: &StmtIf) {
let if_check = *stmt.test.clone();
let semantic = checker.semantic();
let mut unreferenced_bindings: Vec<UnreferencedBinding> = Vec::new();
let mut errors: Vec<AssignmentBeforeIf> = Vec::new();

// case - if check (`if test1:`)
if let Some(unreferenced_binding) =
find_unreferenced_binding_from_check(&if_check, checker.semantic(), &if_check)
find_assignment_before_if(&if_check, checker.semantic(), &if_check)
{
unreferenced_bindings.push(unreferenced_binding);
errors.push(unreferenced_binding);
};

// case - bool operations (`if test1 and test2:`)
if let Expr::BoolOp(expr) = if_check.clone() {
unreferenced_bindings.extend(
errors.extend(
expr.values
.iter()
.filter_map(|value| {
find_unreferenced_binding_from_check(&if_check, semantic, value)
})
.collect::<Vec<UnreferencedBinding>>(),
.filter_map(|value| find_assignment_before_if(&if_check, semantic, value))
.collect::<Vec<AssignmentBeforeIf>>(),
);
}

// case - compare (`if test1 is not None:`)
if let Expr::Compare(compare) = if_check.clone() {
if let Some(unreferenced_binding) =
find_unreferenced_binding_from_check(&if_check, checker.semantic(), &compare.left)
if let Some(error) = find_assignment_before_if(&if_check, checker.semantic(), &compare.left)
{
unreferenced_bindings.push(unreferenced_binding);
errors.push(error);
};
}

// case - elif else clauses (`elif test1:`)
let elif_else_clauses = stmt.elif_else_clauses.clone();
unreferenced_bindings.extend(
errors.extend(
elif_else_clauses
.iter()
.filter(|elif_else_clause| elif_else_clause.test.is_some())
.filter_map(|elif_else_clause| {
let elif_check = elif_else_clause.test.clone().unwrap();
find_unreferenced_binding_from_check(&elif_check, semantic, &elif_check)
find_assignment_before_if(&elif_check, semantic, &elif_check)
})
.collect::<Vec<UnreferencedBinding>>(),
.collect::<Vec<AssignmentBeforeIf>>(),
);

// add found diagnostics
checker.diagnostics.extend(
unreferenced_bindings
errors
.into_iter()
.map(|unreferenced_binding| {
.map(|error| {
create_diagnostic(
checker.locator(),
checker.indexer(),
checker.generator(),
unreferenced_binding,
error,
)
})
.collect::<Vec<Diagnostic>>(),
);
}

fn find_assignment_before_if<'a>(
origin: &Expr,
semantic: &'a SemanticModel,
check: &Expr,
) -> Option<AssignmentBeforeIf<'a>> {
// only care about variable expressions, like `if test1:`
let Expr::Name(check_variable) = check.clone() else {
return None;
};

let current_statement = semantic.current_statement();
let previous_statement = semantic
.previous_statement(current_statement)?
.as_assign_stmt()?;

// only care about single assignment target like `x = 'example'`
let [target] = &previous_statement.targets[..] else {
return None;
};

// check whether the check variable is the assignment variable
if check_variable.id != target.as_name_expr()?.id {
return None;
}

Some((origin.clone(), check_variable, previous_statement.clone()))
}

fn create_diagnostic(
locator: &Locator,
indexer: &Indexer,
generator: Generator,
error: AssignmentBeforeIf,
) -> Diagnostic {
let (origin, expr_name, assignment) = error;
let value_expr = generator.expr(&assignment.value.clone());
let use_parentheses = origin.is_bool_op_expr() || !assignment.value.is_name_expr();

let mut diagnostic = Diagnostic::new(
UnnecessaryAssignment {
check: true,
name: expr_name.clone().id,
assignment: value_expr.clone(),
parentheses: use_parentheses,
},
expr_name.clone().range(),
);

let format = if use_parentheses {
format!("({} := {})", expr_name.clone().id, value_expr.clone())
} else {
format!("{} := {}", expr_name.clone().id, value_expr.clone())
};

let delete_assignment_edit = delete_stmt(&Stmt::from(assignment), None, locator, indexer);
let use_walrus_edit = Edit::range_replacement(format, diagnostic.range());

diagnostic.set_fix(Fix::unsafe_edits(delete_assignment_edit, [use_walrus_edit]));

diagnostic
}
Loading

0 comments on commit 84c2180

Please sign in to comment.