Parenthesize expressions prior to lexing in F632 (#5001)

crates/ruff/resources/test/fixtures/pyflakes/F632.py

@@ -22,3 +22,6 @@ if "123" != (x is 3):
 
 {2 is
 not ''}
+
+{2 is
+ not ''}

crates/ruff/src/checkers/ast/mod.rs

@@ -3329,13 +3329,7 @@ where
                 }
 
                 if self.enabled(Rule::IsLiteral) {
-                    pyflakes::rules::invalid_literal_comparison(
+                    pyflakes::rules::invalid_literal_comparison(self, left, ops, comparators, expr);
-                        self,
-                        left,
-                        ops,
-                        comparators,
-                        expr.range(),
-                    );
                 }
 
                 if self.enabled(Rule::TypeComparison) {

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, Ranged};
-use rustpython_parser::ast::{Cmpop, Expr};
 
 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"),
+        }
+    }
+}

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 != ''}
 
 

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<LocatedCmpop> {
+pub fn locate_cmpops(expr: &Expr, locator: &Locator) -> Vec<LocatedCmpop> {
-    let mut tok_iter = lexer::lex(contents, Mode::Expression)
+    // 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(1)
-            // Skip whitespace, including Tok::Newline. It should be sufficient to skip
+        .map(|(tok, range)| (tok, range.sub(TextSize::from(1))))
-            // Tok::NonLogicalNewline, but the lexer doesn't know that we're within an
+        .filter(|(tok, _)| !matches!(tok, Tok::NonLogicalNewline | Tok::Comment(_)))
-            // expression, and so it may confusion logical and non-logical newlines.
-            !matches!(tok, Tok::Newline | Tok::NonLogicalNewline | Tok::Comment(_)))
         .peekable();
+
     let mut ops: Vec<LocatedCmpop> = 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 = "
+        let contents = "if a:
-if a:
     ...
 elif b:
     ...
-"
+";
-        .trim_start();
         let program = Suite::parse(contents, "<filename>")?;
         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 = "
+        let contents = "if a:
-if a:
     ...
 else:
     ...
-"
+";
-        .trim_start();
         let program = Suite::parse(contents, "<filename>")?;
         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, "<filename>")?;
+        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, "<filename>")?;
+        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, "<filename>")?;
+        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, "<filename>")?;
+        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, "<filename>")?;
+        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, "<filename>")?;
+        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, "<filename>")?;
+        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(())
     }
 }