Detect permutations in redundant open modes (#14255)

## Summary

Closes https://github.com/astral-sh/ruff/issues/14235.

Cargo.lock

@@ -2911,6 +2911,7 @@ dependencies = [
 name = "ruff_python_stdlib"
 version = "0.0.0"
 dependencies = [
+ "bitflags 2.6.0",
  "unicode-ident",
 ]
 

crates/ruff_linter/resources/test/fixtures/pyupgrade/UP015.py

@@ -6,6 +6,7 @@ open("foo", "r")
 open("foo", "rt")
 open("f", "r", encoding="UTF-8")
 open("f", "wt")
+open("f", "tw")
 
 with open("foo", "U") as f:
     pass
@@ -69,19 +70,14 @@ open(file="foo", buffering=-1, encoding=None, errors=None, newline=None, closefd
 open(file="foo", buffering=-1, encoding=None, errors=None, mode='Ub', newline=None, closefd=True, opener=None)
 open(mode='Ub', file="foo", buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None)
 
-open = 1
-open("foo", "U")
-open("foo", "Ur")
-open("foo", "Ub")
-open("foo", "rUb")
-open("foo", "r")
-open("foo", "rt")
-open("f", "r", encoding="UTF-8")
-open("f", "wt")
-
-
 import aiofiles
 
 aiofiles.open("foo", "U")
 aiofiles.open("foo", "r")
 aiofiles.open("foo", mode="r")
+
+open("foo", "r+")
+open("foo", "rb")
+open("foo", "r+b")
+open("foo", "UU")
+open("foo", "wtt")

crates/ruff_linter/src/rules/pylint/rules/bad_open_mode.rs

@@ -1,9 +1,8 @@
-use bitflags::bitflags;
-
 use ruff_diagnostics::{Diagnostic, Violation};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::{self as ast, Expr};
 use ruff_python_semantic::SemanticModel;
+use ruff_python_stdlib::open_mode::OpenMode;
 use ruff_text_size::Ranged;
 
 use crate::checkers::ast::Checker;
@@ -59,11 +58,11 @@ pub(crate) fn bad_open_mode(checker: &mut Checker, call: &ast::ExprCall) {
         return;
     };
 
-    let ast::Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = mode else {
+    let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = mode else {
         return;
     };
 
-    if is_valid_mode(value) {
+    if OpenMode::from_chars(value.chars()).is_ok() {
         return;
     }
 
@@ -112,83 +111,3 @@ fn extract_mode(call: &ast::ExprCall, kind: Kind) -> Option<&Expr> {
         Kind::Pathlib => call.arguments.find_argument("mode", 0),
     }
 }
-
-bitflags! {
-    #[derive(Copy, Clone, Debug, PartialEq, Eq)]
-    pub(super) struct OpenMode: u8 {
-        /// `r`
-        const READ = 0b0001;
-        /// `w`
-        const WRITE = 0b0010;
-        /// `a`
-        const APPEND = 0b0100;
-        /// `x`
-        const CREATE = 0b1000;
-        /// `b`
-        const BINARY = 0b10000;
-        /// `t`
-        const TEXT = 0b10_0000;
-        /// `+`
-        const PLUS = 0b100_0000;
-        /// `U`
-        const UNIVERSAL_NEWLINES = 0b1000_0000;
-
-    }
-}
-
-impl TryFrom<char> for OpenMode {
-    type Error = ();
-
-    fn try_from(value: char) -> Result<Self, Self::Error> {
-        match value {
-            'r' => Ok(Self::READ),
-            'w' => Ok(Self::WRITE),
-            'a' => Ok(Self::APPEND),
-            'x' => Ok(Self::CREATE),
-            'b' => Ok(Self::BINARY),
-            't' => Ok(Self::TEXT),
-            '+' => Ok(Self::PLUS),
-            'U' => Ok(Self::UNIVERSAL_NEWLINES),
-            _ => Err(()),
-        }
-    }
-}
-
-/// Returns `true` if the open mode is valid.
-fn is_valid_mode(mode: &ast::StringLiteralValue) -> bool {
-    // Flag duplicates and invalid characters.
-    let mut flags = OpenMode::empty();
-    for char in mode.chars() {
-        let Ok(flag) = OpenMode::try_from(char) else {
-            return false;
-        };
-        if flags.intersects(flag) {
-            return false;
-        }
-        flags.insert(flag);
-    }
-
-    // Both text and binary mode cannot be set at the same time.
-    if flags.contains(OpenMode::TEXT | OpenMode::BINARY) {
-        return false;
-    }
-
-    // The `U` mode is only valid with `r`.
-    if flags.contains(OpenMode::UNIVERSAL_NEWLINES)
-        && flags.intersects(OpenMode::WRITE | OpenMode::APPEND | OpenMode::CREATE)
-    {
-        return false;
-    }
-
-    // Otherwise, reading, writing, creating, and appending are mutually exclusive.
-    [
-        OpenMode::READ | OpenMode::UNIVERSAL_NEWLINES,
-        OpenMode::WRITE,
-        OpenMode::CREATE,
-        OpenMode::APPEND,
-    ]
-    .into_iter()
-    .filter(|flag| flags.intersects(*flag))
-    .count()
-        == 1
-}

crates/ruff_linter/src/rules/pyupgrade/rules/redundant_open_modes.rs

@@ -1,11 +1,10 @@
-use std::str::FromStr;
+use anyhow::Result;
-
-use anyhow::{anyhow, Result};
-
 use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::{self as ast, Expr};
+use ruff_python_codegen::Stylist;
 use ruff_python_parser::{TokenKind, Tokens};
+use ruff_python_stdlib::open_mode::OpenMode;
 use ruff_text_size::{Ranged, TextSize};
 
 use crate::checkers::ast::Checker;
@@ -33,26 +32,26 @@ use crate::checkers::ast::Checker;
 /// - [Python documentation: `open`](https://docs.python.org/3/library/functions.html#open)
 #[violation]
 pub struct RedundantOpenModes {
-    replacement: Option<String>,
+    replacement: String,
 }
 
 impl AlwaysFixableViolation for RedundantOpenModes {
     #[derive_message_formats]
     fn message(&self) -> String {
-        match &self.replacement {
+        let RedundantOpenModes { replacement } = self;
-            None => "Unnecessary open mode parameters".to_string(),
+        if replacement.is_empty() {
-            Some(replacement) => {
+            "Unnecessary open mode parameters".to_string()
-                format!("Unnecessary open mode parameters, use \"{replacement}\"")
+        } else {
-            }
+            format!("Unnecessary open mode parameters, use \"{replacement}\"")
         }
     }
 
     fn fix_title(&self) -> String {
-        match &self.replacement {
+        let RedundantOpenModes { replacement } = self;
-            None => "Remove open mode parameters".to_string(),
+        if replacement.is_empty() {
-            Some(replacement) => {
+            "Remove open mode parameters".to_string()
-                format!("Replace with \"{replacement}\"")
+        } else {
-            }
+            format!("Replace with \"{replacement}\"")
         }
     }
 }
@@ -81,13 +80,17 @@ pub(crate) fn redundant_open_modes(checker: &mut Checker, call: &ast::ExprCall)
                         ..
                     }) = &keyword.value
                     {
-                        if let Ok(mode) = OpenMode::from_str(mode_param_value.to_str()) {
+                        if let Ok(mode) = OpenMode::from_chars(mode_param_value.chars()) {
-                            checker.diagnostics.push(create_diagnostic(
+                            let reduced = mode.reduce();
-                                call,
+                            if reduced != mode {
-                                &keyword.value,
+                                checker.diagnostics.push(create_diagnostic(
-                                mode.replacement_value(),
+                                    call,
-                                checker.tokens(),
+                                    &keyword.value,
-                            ));
+                                    reduced,
+                                    checker.tokens(),
+                                    checker.stylist(),
+                                ));
+                            }
                         }
                     }
                 }
@@ -95,82 +98,45 @@ pub(crate) fn redundant_open_modes(checker: &mut Checker, call: &ast::ExprCall)
         }
         Some(mode_param) => {
             if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = &mode_param {
-                if let Ok(mode) = OpenMode::from_str(value.to_str()) {
+                if let Ok(mode) = OpenMode::from_chars(value.chars()) {
-                    checker.diagnostics.push(create_diagnostic(
+                    let reduced = mode.reduce();
-                        call,
+                    if reduced != mode {
-                        mode_param,
+                        checker.diagnostics.push(create_diagnostic(
-                        mode.replacement_value(),
+                            call,
-                        checker.tokens(),
+                            mode_param,
-                    ));
+                            reduced,
+                            checker.tokens(),
+                            checker.stylist(),
+                        ));
+                    }
                 }
             }
         }
     }
 }
 
-#[derive(Debug, Copy, Clone)]
-enum OpenMode {
-    U,
-    Ur,
-    Ub,
-    RUb,
-    R,
-    Rt,
-    Wt,
-}
-
-impl FromStr for OpenMode {
-    type Err = anyhow::Error;
-
-    fn from_str(string: &str) -> Result<Self, Self::Err> {
-        match string {
-            "U" => Ok(Self::U),
-            "Ur" => Ok(Self::Ur),
-            "Ub" => Ok(Self::Ub),
-            "rUb" => Ok(Self::RUb),
-            "r" => Ok(Self::R),
-            "rt" => Ok(Self::Rt),
-            "wt" => Ok(Self::Wt),
-            _ => Err(anyhow!("Unknown open mode: {}", string)),
-        }
-    }
-}
-
-impl OpenMode {
-    fn replacement_value(self) -> Option<&'static str> {
-        match self {
-            Self::U => None,
-            Self::Ur => None,
-            Self::Ub => Some("\"rb\""),
-            Self::RUb => Some("\"rb\""),
-            Self::R => None,
-            Self::Rt => None,
-            Self::Wt => Some("\"w\""),
-        }
-    }
-}
-
 fn create_diagnostic(
     call: &ast::ExprCall,
     mode_param: &Expr,
-    replacement_value: Option<&str>,
+    mode: OpenMode,
     tokens: &Tokens,
+    stylist: &Stylist,
 ) -> Diagnostic {
     let mut diagnostic = Diagnostic::new(
         RedundantOpenModes {
-            replacement: replacement_value.map(ToString::to_string),
+            replacement: mode.to_string(),
         },
         call.range(),
     );
 
-    if let Some(content) = replacement_value {
+    if mode.is_empty() {
-        diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
-            content.to_string(),
-            mode_param.range(),
-        )));
-    } else {
         diagnostic
             .try_set_fix(|| create_remove_param_fix(call, mode_param, tokens).map(Fix::safe_edit));
+    } else {
+        diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
+            format!("{}{mode}{}", stylist.quote(), stylist.quote()),
+            mode_param.range(),
+        )));
     }
 
     diagnostic

crates/ruff_python_stdlib/Cargo.toml

@@ -13,6 +13,7 @@ license = { workspace = true }
 [lib]
 
 [dependencies]
+bitflags = { workspace = true }
 unicode-ident = { workspace = true }
 
 [lints]

crates/ruff_python_stdlib/src/lib.rs

@@ -3,6 +3,7 @@ pub mod future;
 pub mod identifiers;
 pub mod keyword;
 pub mod logging;
+pub mod open_mode;
 pub mod path;
 pub mod str;
 pub mod sys;

crates/ruff_python_stdlib/src/open_mode.rs

@@ -0,0 +1,146 @@
+bitflags::bitflags! {
+    #[derive(Copy, Clone, Debug, PartialEq, Eq)]
+    pub struct OpenMode: u8 {
+        /// `r`
+        const READ = 0b0001;
+        /// `w`
+        const WRITE = 0b0010;
+        /// `a`
+        const APPEND = 0b0100;
+        /// `x`
+        const CREATE = 0b1000;
+        /// `b`
+        const BINARY = 0b10000;
+        /// `t`
+        const TEXT = 0b10_0000;
+        /// `+`
+        const PLUS = 0b100_0000;
+        /// `U`
+        const UNIVERSAL_NEWLINES = 0b1000_0000;
+    }
+}
+
+impl OpenMode {
+    /// Parse an [`OpenMode`] from a sequence of characters.
+    pub fn from_chars(chars: impl Iterator<Item = char>) -> Result<Self, String> {
+        let mut open_mode = OpenMode::empty();
+        for c in chars {
+            let flag = OpenMode::try_from(c)?;
+            if flag.intersects(open_mode) {
+                return Err(format!("Open mode contains duplicate flag: `{c}`"));
+            }
+            open_mode.insert(flag);
+        }
+
+        // Both text and binary mode cannot be set at the same time.
+        if open_mode.contains(OpenMode::TEXT | OpenMode::BINARY) {
+            return Err(
+                "Open mode cannot contain both text (`t`) and binary (`b`) flags".to_string(),
+            );
+        }
+
+        // The `U` mode is only valid with `r`.
+        if open_mode.contains(OpenMode::UNIVERSAL_NEWLINES)
+            && open_mode.intersects(OpenMode::WRITE | OpenMode::APPEND | OpenMode::CREATE)
+        {
+            return Err("Open mode cannot contain the universal newlines (`U`) flag with write (`w`), append (`a`), or create (`x`) flags".to_string());
+        }
+
+        // Otherwise, reading, writing, creating, and appending are mutually exclusive.
+        if [
+            OpenMode::READ | OpenMode::UNIVERSAL_NEWLINES,
+            OpenMode::WRITE,
+            OpenMode::CREATE,
+            OpenMode::APPEND,
+        ]
+        .into_iter()
+        .filter(|flag| open_mode.intersects(*flag))
+        .count()
+            != 1
+        {
+            return Err("Open mode must contain exactly one of the following flags: read (`r`), write (`w`), create (`x`), or append (`a`)".to_string());
+        }
+
+        Ok(open_mode)
+    }
+
+    /// Remove any redundant flags from the open mode.
+    #[must_use]
+    pub fn reduce(self) -> Self {
+        let mut open_mode = self;
+
+        // `t` is always redundant.
+        open_mode.remove(Self::TEXT);
+
+        // `U` is always redundant.
+        open_mode.remove(Self::UNIVERSAL_NEWLINES);
+
+        // `r` is redundant, unless `b` or `+` is also set, in which case, we need one of `w`, `a`, `r`, or `x`.
+        if open_mode.intersects(Self::BINARY | Self::PLUS) {
+            if !open_mode.intersects(Self::WRITE | Self::CREATE | Self::APPEND) {
+                open_mode.insert(Self::READ);
+            }
+        } else {
+            open_mode.remove(Self::READ);
+        }
+
+        open_mode
+    }
+}
+
+/// Write the [`OpenMode`] as a string.
+impl std::fmt::Display for OpenMode {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        if self.contains(OpenMode::READ) {
+            write!(f, "r")?;
+        }
+        if self.contains(OpenMode::WRITE) {
+            write!(f, "w")?;
+        }
+        if self.contains(OpenMode::APPEND) {
+            write!(f, "a")?;
+        }
+        if self.contains(OpenMode::CREATE) {
+            write!(f, "x")?;
+        }
+        if self.contains(OpenMode::UNIVERSAL_NEWLINES) {
+            write!(f, "U")?;
+        }
+        if self.contains(OpenMode::BINARY) {
+            write!(f, "b")?;
+        }
+        if self.contains(OpenMode::TEXT) {
+            write!(f, "t")?;
+        }
+        if self.contains(OpenMode::PLUS) {
+            write!(f, "+")?;
+        }
+        Ok(())
+    }
+}
+
+impl TryFrom<char> for OpenMode {
+    type Error = String;
+
+    fn try_from(value: char) -> Result<Self, Self::Error> {
+        match value {
+            'r' => Ok(Self::READ),
+            'w' => Ok(Self::WRITE),
+            'a' => Ok(Self::APPEND),
+            'x' => Ok(Self::CREATE),
+            'b' => Ok(Self::BINARY),
+            't' => Ok(Self::TEXT),
+            '+' => Ok(Self::PLUS),
+            'U' => Ok(Self::UNIVERSAL_NEWLINES),
+            _ => Err(format!("Invalid open mode flag: `{value}`")),
+        }
+    }
+}
+
+impl TryFrom<&str> for OpenMode {
+    type Error = String;
+
+    fn try_from(value: &str) -> Result<Self, Self::Error> {
+        OpenMode::from_chars(value.chars())
+    }
+}