Skip to content

Commit

Permalink
[flake8-simplify] Stabilize detection of Yoda conditions for "const…
Browse files Browse the repository at this point in the history
…ant" collections (`SIM300`) (#12050)

Co-authored-by: Alex Waygood <[email protected]>
  • Loading branch information
charliermarsh and AlexWaygood authored Jun 27, 2024
1 parent 79f815f commit 8dabbaa
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 456 deletions.
19 changes: 0 additions & 19 deletions crates/ruff_linter/src/rules/flake8_simplify/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ mod tests {
use test_case::test_case;

use crate::registry::Rule;
use crate::settings::types::PreviewMode;
use crate::test::test_path;
use crate::{assert_messages, settings};

Expand Down Expand Up @@ -55,22 +54,4 @@ mod tests {
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test_case(Rule::YodaConditions, Path::new("SIM300.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"preview__{}_{}",
rule_code.noqa_code(),
path.to_string_lossy()
);
let diagnostics = test_path(
Path::new("flake8_simplify").join(path).as_path(),
&settings::LinterSettings {
preview: PreviewMode::Enabled,
..settings::LinterSettings::for_rule(rule_code)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ use crate::cst::helpers::or_space;
use crate::cst::matchers::{match_comparison, transform_expression};
use crate::fix::edits::pad;
use crate::fix::snippet::SourceCodeSnippet;
use crate::settings::types::PreviewMode;

/// ## What it does
/// Checks for conditions that position a constant on the left-hand side of the
Expand Down Expand Up @@ -58,26 +57,15 @@ impl Violation for YodaConditions {

#[derive_message_formats]
fn message(&self) -> String {
let YodaConditions { suggestion } = self;
if let Some(suggestion) = suggestion
.as_ref()
.and_then(SourceCodeSnippet::full_display)
{
format!("Yoda conditions are discouraged, use `{suggestion}` instead")
} else {
format!("Yoda conditions are discouraged")
}
format!("Yoda condition detected")
}

fn fix_title(&self) -> Option<String> {
let YodaConditions { suggestion } = self;
suggestion.as_ref().map(|suggestion| {
if let Some(suggestion) = suggestion.full_display() {
format!("Replace Yoda condition with `{suggestion}`")
} else {
format!("Replace Yoda condition")
}
})
suggestion
.as_ref()
.and_then(|suggestion| suggestion.full_display())
.map(|suggestion| format!("Rewrite as `{suggestion}`"))
}
}

Expand All @@ -94,9 +82,9 @@ enum ConstantLikelihood {
Definitely = 2,
}

impl ConstantLikelihood {
impl From<&Expr> for ConstantLikelihood {
/// Determine the [`ConstantLikelihood`] of an expression.
fn from_expression(expr: &Expr, preview: PreviewMode) -> Self {
fn from(expr: &Expr) -> Self {
match expr {
_ if expr.is_literal_expr() => ConstantLikelihood::Definitely,
Expr::Attribute(ast::ExprAttribute { attr, .. }) => {
Expand All @@ -105,34 +93,36 @@ impl ConstantLikelihood {
Expr::Name(ast::ExprName { id, .. }) => ConstantLikelihood::from_identifier(id),
Expr::Tuple(ast::ExprTuple { elts, .. }) => elts
.iter()
.map(|expr| ConstantLikelihood::from_expression(expr, preview))
.map(ConstantLikelihood::from)
.min()
.unwrap_or(ConstantLikelihood::Definitely),
Expr::List(ast::ExprList { elts, .. }) if preview.is_enabled() => elts
Expr::List(ast::ExprList { elts, .. }) => elts
.iter()
.map(|expr| ConstantLikelihood::from_expression(expr, preview))
.map(ConstantLikelihood::from)
.min()
.unwrap_or(ConstantLikelihood::Definitely),
Expr::Dict(ast::ExprDict { items, .. }) if preview.is_enabled() => {
Expr::Dict(ast::ExprDict { items, .. }) => {
if items.is_empty() {
ConstantLikelihood::Definitely
} else {
ConstantLikelihood::Probably
}
}
Expr::BinOp(ast::ExprBinOp { left, right, .. }) => cmp::min(
ConstantLikelihood::from_expression(left, preview),
ConstantLikelihood::from_expression(right, preview),
ConstantLikelihood::from(&**left),
ConstantLikelihood::from(&**right),
),
Expr::UnaryOp(ast::ExprUnaryOp {
op: UnaryOp::UAdd | UnaryOp::USub | UnaryOp::Invert,
operand,
range: _,
}) => ConstantLikelihood::from_expression(operand, preview),
}) => ConstantLikelihood::from(&**operand),
_ => ConstantLikelihood::Unlikely,
}
}
}

impl ConstantLikelihood {
/// Determine the [`ConstantLikelihood`] of an identifier.
fn from_identifier(identifier: &str) -> Self {
if str::is_cased_uppercase(identifier) {
Expand Down Expand Up @@ -230,9 +220,7 @@ pub(crate) fn yoda_conditions(
return;
}

if ConstantLikelihood::from_expression(left, checker.settings.preview)
<= ConstantLikelihood::from_expression(right, checker.settings.preview)
{
if ConstantLikelihood::from(left) <= ConstantLikelihood::from(right) {
return;
}

Expand Down
Loading

0 comments on commit 8dabbaa

Please sign in to comment.