From 2d597bc1fbfe1a7ed072fb1151991c769945453d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 10 Jun 2023 00:23:43 -0400 Subject: [PATCH] Parenthesize expressions prior to lexing in F632 (#5001) --- .../resources/test/fixtures/pyflakes/F632.py | 3 + crates/ruff/src/checkers/ast/mod.rs | 8 +-- .../rules/invalid_literal_comparisons.rs | 43 ++++++----- ..._rules__pyflakes__tests__F632_F632.py.snap | 24 +++++++ crates/ruff_python_ast/src/helpers.rs | 71 ++++++++++++------- 5 files changed, 96 insertions(+), 53 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/pyflakes/F632.py b/crates/ruff/resources/test/fixtures/pyflakes/F632.py index 1940233218..9f40368ad8 100644 --- a/crates/ruff/resources/test/fixtures/pyflakes/F632.py +++ b/crates/ruff/resources/test/fixtures/pyflakes/F632.py @@ -22,3 +22,6 @@ if "123" != (x is 3): {2 is not ''} + +{2 is + not ''} diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 43b788d0b4..481bddf0ce 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -3329,13 +3329,7 @@ where } if self.enabled(Rule::IsLiteral) { - pyflakes::rules::invalid_literal_comparison( - self, - left, - ops, - comparators, - expr.range(), - ); + pyflakes::rules::invalid_literal_comparison(self, left, ops, comparators, expr); } if self.enabled(Rule::TypeComparison) { diff --git a/crates/ruff/src/rules/pyflakes/rules/invalid_literal_comparisons.rs b/crates/ruff/src/rules/pyflakes/rules/invalid_literal_comparisons.rs index 80cdb42882..10beb70983 100644 --- a/crates/ruff/src/rules/pyflakes/rules/invalid_literal_comparisons.rs +++ b/crates/ruff/src/rules/pyflakes/rules/invalid_literal_comparisons.rs @@ -1,8 +1,7 @@ use itertools::izip; use log::error; use once_cell::unsync::Lazy; -use ruff_text_size::TextRange; -use rustpython_parser::ast::{Cmpop, Expr}; +use rustpython_parser::ast::{Cmpop, Expr, Ranged}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; @@ -11,22 +10,6 @@ use ruff_python_ast::helpers; use crate::checkers::ast::Checker; use crate::registry::AsRule; -#[derive(Debug, PartialEq, Eq, Copy, Clone)] -pub(crate) enum IsCmpop { - Is, - IsNot, -} - -impl From<&Cmpop> for IsCmpop { - fn from(cmpop: &Cmpop) -> Self { - match cmpop { - Cmpop::Is => IsCmpop::Is, - Cmpop::IsNot => IsCmpop::IsNot, - _ => panic!("Expected Cmpop::Is | Cmpop::IsNot"), - } - } -} - /// ## What it does /// Checks for `is` and `is not` comparisons against constant literals, like /// integers and strings. @@ -92,16 +75,16 @@ pub(crate) fn invalid_literal_comparison( left: &Expr, ops: &[Cmpop], comparators: &[Expr], - location: TextRange, + expr: &Expr, ) { - let located = Lazy::new(|| helpers::locate_cmpops(checker.locator.slice(location))); + let located = Lazy::new(|| helpers::locate_cmpops(expr, checker.locator)); let mut left = left; for (index, (op, right)) in izip!(ops, comparators).enumerate() { if matches!(op, Cmpop::Is | Cmpop::IsNot) && (helpers::is_constant_non_singleton(left) || helpers::is_constant_non_singleton(right)) { - let mut diagnostic = Diagnostic::new(IsLiteral { cmpop: op.into() }, location); + let mut diagnostic = Diagnostic::new(IsLiteral { cmpop: op.into() }, expr.range()); if checker.patch(diagnostic.kind.rule()) { if let Some(located_op) = &located.get(index) { assert_eq!(located_op.op, *op); @@ -116,7 +99,7 @@ pub(crate) fn invalid_literal_comparison( #[allow(deprecated)] diagnostic.set_fix(Fix::unspecified(Edit::range_replacement( content, - located_op.range + location.start(), + located_op.range + expr.start(), ))); } } else { @@ -128,3 +111,19 @@ pub(crate) fn invalid_literal_comparison( left = right; } } + +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +enum IsCmpop { + Is, + IsNot, +} + +impl From<&Cmpop> for IsCmpop { + fn from(cmpop: &Cmpop) -> Self { + match cmpop { + Cmpop::Is => IsCmpop::Is, + Cmpop::IsNot => IsCmpop::IsNot, + _ => panic!("Expected Cmpop::Is | Cmpop::IsNot"), + } + } +} diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F632_F632.py.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F632_F632.py.snap index db70a3c783..e84c43f460 100644 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F632_F632.py.snap +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F632_F632.py.snap @@ -147,6 +147,8 @@ F632.py:23:2: F632 [*] Use `!=` to compare constant literals | __^ 24 | | not ''} | |______^ F632 +25 | +26 | {2 is | = help: Replace `is not` with `!=` @@ -157,5 +159,27 @@ F632.py:23:2: F632 [*] Use `!=` to compare constant literals 23 |-{2 is 24 |-not ''} 23 |+{2 != ''} +25 24 | +26 25 | {2 is +27 26 | not ''} + +F632.py:26:2: F632 [*] Use `!=` to compare constant literals + | +24 | not ''} +25 | +26 | {2 is + | __^ +27 | | not ''} + | |_______^ F632 + | + = help: Replace `is not` with `!=` + +ℹ Suggested fix +23 23 | {2 is +24 24 | not ''} +25 25 | +26 |-{2 is +27 |- not ''} + 26 |+{2 != ''} diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 205a8cfc73..035e10bfec 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -1,4 +1,5 @@ use std::borrow::Cow; +use std::ops::Sub; use std::path::Path; use itertools::Itertools; @@ -1444,15 +1445,18 @@ impl LocatedCmpop { /// `RustPython` doesn't include line and column information on [`Cmpop`] nodes. /// `CPython` doesn't either. This method iterates over the token stream and /// re-identifies [`Cmpop`] nodes, annotating them with valid ranges. -pub fn locate_cmpops(contents: &str) -> Vec { - let mut tok_iter = lexer::lex(contents, Mode::Expression) +pub fn locate_cmpops(expr: &Expr, locator: &Locator) -> Vec { + // If `Expr` is a multi-line expression, we need to parenthesize it to + // ensure that it's lexed correctly. + let contents = locator.slice(expr.range()); + let parenthesized_contents = format!("({contents})"); + let mut tok_iter = lexer::lex(&parenthesized_contents, Mode::Expression) .flatten() - .filter(|(tok, _)| - // Skip whitespace, including Tok::Newline. It should be sufficient to skip - // Tok::NonLogicalNewline, but the lexer doesn't know that we're within an - // expression, and so it may confusion logical and non-logical newlines. - !matches!(tok, Tok::Newline | Tok::NonLogicalNewline | Tok::Comment(_))) + .skip(1) + .map(|(tok, range)| (tok, range.sub(TextSize::from(1)))) + .filter(|(tok, _)| !matches!(tok, Tok::NonLogicalNewline | Tok::Comment(_))) .peekable(); + let mut ops: Vec = vec![]; let mut count = 0u32; loop { @@ -1617,7 +1621,7 @@ mod tests { use anyhow::Result; use ruff_text_size::{TextLen, TextRange, TextSize}; - use rustpython_ast::{Stmt, Suite}; + use rustpython_ast::{Expr, Stmt, Suite}; use rustpython_parser::ast::Cmpop; use rustpython_parser::Parse; @@ -1808,13 +1812,11 @@ else: #[test] fn extract_elif_else_range() -> Result<()> { - let contents = " -if a: + let contents = "if a: ... elif b: ... -" - .trim_start(); +"; let program = Suite::parse(contents, "")?; let stmt = program.first().unwrap(); let stmt = Stmt::as_if_stmt(stmt).unwrap(); @@ -1823,13 +1825,11 @@ elif b: assert_eq!(range.start(), TextSize::from(14)); assert_eq!(range.end(), TextSize::from(18)); - let contents = " -if a: + let contents = "if a: ... else: ... -" - .trim_start(); +"; let program = Suite::parse(contents, "")?; let stmt = program.first().unwrap(); let stmt = Stmt::as_if_stmt(stmt).unwrap(); @@ -1842,61 +1842,84 @@ else: } #[test] - fn extract_cmpop_location() { + fn extract_cmpop_location() -> Result<()> { + let contents = "x == 1"; + let expr = Expr::parse(contents, "")?; + let locator = Locator::new(contents); assert_eq!( - locate_cmpops("x == 1"), + locate_cmpops(&expr, &locator), vec![LocatedCmpop::new( TextSize::from(2)..TextSize::from(4), Cmpop::Eq )] ); + let contents = "x != 1"; + let expr = Expr::parse(contents, "")?; + let locator = Locator::new(contents); assert_eq!( - locate_cmpops("x != 1"), + locate_cmpops(&expr, &locator), vec![LocatedCmpop::new( TextSize::from(2)..TextSize::from(4), Cmpop::NotEq )] ); + let contents = "x is 1"; + let expr = Expr::parse(contents, "")?; + let locator = Locator::new(contents); assert_eq!( - locate_cmpops("x is 1"), + locate_cmpops(&expr, &locator), vec![LocatedCmpop::new( TextSize::from(2)..TextSize::from(4), Cmpop::Is )] ); + let contents = "x is not 1"; + let expr = Expr::parse(contents, "")?; + let locator = Locator::new(contents); assert_eq!( - locate_cmpops("x is not 1"), + locate_cmpops(&expr, &locator), vec![LocatedCmpop::new( TextSize::from(2)..TextSize::from(8), Cmpop::IsNot )] ); + let contents = "x in 1"; + let expr = Expr::parse(contents, "")?; + let locator = Locator::new(contents); assert_eq!( - locate_cmpops("x in 1"), + locate_cmpops(&expr, &locator), vec![LocatedCmpop::new( TextSize::from(2)..TextSize::from(4), Cmpop::In )] ); + let contents = "x not in 1"; + let expr = Expr::parse(contents, "")?; + let locator = Locator::new(contents); assert_eq!( - locate_cmpops("x not in 1"), + locate_cmpops(&expr, &locator), vec![LocatedCmpop::new( TextSize::from(2)..TextSize::from(8), Cmpop::NotIn )] ); + let contents = "x != (1 is not 2)"; + let expr = Expr::parse(contents, "")?; + let locator = Locator::new(contents); assert_eq!( - locate_cmpops("x != (1 is not 2)"), + locate_cmpops(&expr, &locator), vec![LocatedCmpop::new( TextSize::from(2)..TextSize::from(4), Cmpop::NotEq )] ); + + Ok(()) } }