Add keyword argument handling for redundant open modes. (#906)

resources/test/fixtures/U015.py

@@ -36,3 +36,35 @@ with open("foo", "U") as fa, open("bar", "U") as fb:
     pass
 with open("foo", "Ub") as fa, open("bar", "Ub") as fb:
     pass
+
+open("foo", mode="U")
+open(name="foo", mode="U")
+open(mode="U", name="foo")
+
+with open("foo", mode="U") as f:
+    pass
+with open(name="foo", mode="U") as f:
+    pass
+with open(mode="U", name="foo") as f:
+    pass
+
+open("foo", mode="Ub")
+open(name="foo", mode="Ub")
+open(mode="Ub", name="foo")
+
+with open("foo", mode="Ub") as f:
+    pass
+with open(name="foo", mode="Ub") as f:
+    pass
+with open(mode="Ub", name="foo") as f:
+    pass
+
+open(file="foo", mode='U', buffering=- 1, encoding=None, errors=None, newline=None, closefd=True, opener=None)
+open(file="foo", buffering=- 1, encoding=None, errors=None, newline=None, closefd=True, opener=None, mode='U')
+open(file="foo", buffering=- 1, encoding=None, errors=None, mode='U', newline=None, closefd=True, opener=None)
+open(mode='U', file="foo", buffering=- 1, encoding=None, errors=None, newline=None, closefd=True, opener=None)
+
+open(file="foo", mode='Ub', buffering=- 1, encoding=None, errors=None, newline=None, closefd=True, opener=None)
+open(file="foo", buffering=- 1, encoding=None, errors=None, newline=None, closefd=True, opener=None, mode='Ub')
+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)

src/pyupgrade/plugins/redundant_open_modes.rs

@@ -2,7 +2,7 @@ use std::str::FromStr;
 
 use anyhow::{anyhow, Result};
 use log::error;
-use rustpython_ast::{Constant, Expr, ExprKind, Located, Location};
+use rustpython_ast::{Constant, Expr, ExprKind, Keyword, KeywordData, Location};
 use rustpython_parser::lexer;
 use rustpython_parser::token::Tok;
 
@@ -14,6 +14,7 @@ use crate::checks::{Check, CheckCode, CheckKind};
 use crate::source_code_locator::SourceCodeLocator;
 
 const OPEN_FUNC_NAME: &str = "open";
+const MODE_KEYWORD_ARGUMENT: &str = "mode";
 
 enum OpenMode {
     U,
@@ -56,15 +57,20 @@ impl OpenMode {
     }
 }
 
-fn match_open(expr: &Expr) -> Option<&Expr> {
-    if let ExprKind::Call { func, args, .. } = &expr.node {
+fn match_open(expr: &Expr) -> (Option<&Expr>, Vec<Keyword>) {
+    if let ExprKind::Call {
+        func,
+        args,
+        keywords,
+    } = &expr.node
+    {
         // TODO(andberger): Verify that "open" is still bound to the built-in function.
         if match_name_or_attr(func, OPEN_FUNC_NAME) {
-            // Return the "open mode" parameter.
-            return args.get(1);
+            // Return the "open mode" parameter and keywords.
+            return (args.get(1), keywords.clone());
         }
     }
-    None
+    (None, vec![])
 }
 
 fn create_check(
@@ -101,19 +107,36 @@ fn create_remove_param_fix(
         location: expr.location,
         end_location: expr.end_location.unwrap(),
     });
-    // Find the last comma before mode_param
-    // and delete that comma as well as mode_param.
+    // Find the last comma before mode_param and create a deletion fix
+    // starting from the comma and ending after mode_param.
     let mut fix_start: Option<Location> = None;
     let mut fix_end: Option<Location> = None;
+    let mut is_first_arg: bool = false;
+    let mut delete_first_arg: bool = false;
     for (start, tok, end) in lexer::make_tokenizer(&content).flatten() {
         let start = helpers::to_absolute(start, expr.location);
         let end = helpers::to_absolute(end, expr.location);
         if start == mode_param.location {
+            if is_first_arg {
+                delete_first_arg = true;
+                continue;
+            }
             fix_end = Some(end);
             break;
         }
+        if delete_first_arg && matches!(tok, Tok::Name { .. }) {
+            fix_end = Some(start);
+            break;
+        }
+        if matches!(tok, Tok::Lpar) {
+            is_first_arg = true;
+            fix_start = Some(end);
+        }
         if matches!(tok, Tok::Comma) {
-            fix_start = Some(start);
+            is_first_arg = false;
+            if !delete_first_arg {
+                fix_start = Some(start);
+            }
         }
     }
     match (fix_start, fix_end) {
@@ -126,20 +149,41 @@ fn create_remove_param_fix(
 
 /// U015
 pub fn redundant_open_modes(checker: &mut Checker, expr: &Expr) {
-    // TODO(andberger): Add "mode" keyword argument handling to handle invocations
-    // on the following formats:
-    // - `open("foo", mode="U")`
-    // - `open(name="foo", mode="U")`
-    // - `open(mode="U", name="foo")`
-    if let Some(mode_param) = match_open(expr) {
-        if let Located {
-            node:
-                ExprKind::Constant {
-                    value: Constant::Str(mode_param_value),
-                    ..
-                },
+    let (mode_param, keywords): (Option<&Expr>, Vec<Keyword>) = match_open(expr);
+    if mode_param.is_none() && !keywords.is_empty() {
+        if let Some(value) = keywords.iter().find_map(|keyword| {
+            let KeywordData { arg, value } = &keyword.node;
+            if arg
+                .as_ref()
+                .map(|arg| arg == MODE_KEYWORD_ARGUMENT)
+                .unwrap_or_default()
+            {
+                Some(value)
+            } else {
+                None
+            }
+        }) {
+            if let ExprKind::Constant {
+                value: Constant::Str(mode_param_value),
+                ..
+            } = &value.node
+            {
+                if let Ok(mode) = OpenMode::from_str(mode_param_value.as_str()) {
+                    checker.add_check(create_check(
+                        expr,
+                        value,
+                        mode.replacement_value(),
+                        checker.locator,
+                        checker.patch(&CheckCode::U015),
+                    ));
+                }
+            }
+        }
+    } else if let Some(mode_param) = mode_param {
+        if let ExprKind::Constant {
+            value: Constant::Str(mode_param_value),
             ..
-        } = mode_param
+        } = &mode_param.node
         {
             if let Ok(mode) = OpenMode::from_str(mode_param_value.as_str()) {
                 checker.add_check(create_check(

src/snapshots/ruff__linter__tests__U015_U015.py.snap

@@ -386,4 +386,324 @@ expression: checks
       end_location:
         row: 37
         column: 46
+- kind: RedundantOpenModes
+  location:
+    row: 40
+    column: 0
+  end_location:
+    row: 40
+    column: 21
+  fix:
+    patch:
+      content: ""
+      location:
+        row: 40
+        column: 10
+      end_location:
+        row: 40
+        column: 20
+- kind: RedundantOpenModes
+  location:
+    row: 41
+    column: 0
+  end_location:
+    row: 41
+    column: 26
+  fix:
+    patch:
+      content: ""
+      location:
+        row: 41
+        column: 15
+      end_location:
+        row: 41
+        column: 25
+- kind: RedundantOpenModes
+  location:
+    row: 42
+    column: 0
+  end_location:
+    row: 42
+    column: 26
+  fix:
+    patch:
+      content: ""
+      location:
+        row: 42
+        column: 5
+      end_location:
+        row: 42
+        column: 15
+- kind: RedundantOpenModes
+  location:
+    row: 44
+    column: 5
+  end_location:
+    row: 44
+    column: 26
+  fix:
+    patch:
+      content: ""
+      location:
+        row: 44
+        column: 15
+      end_location:
+        row: 44
+        column: 25
+- kind: RedundantOpenModes
+  location:
+    row: 46
+    column: 5
+  end_location:
+    row: 46
+    column: 31
+  fix:
+    patch:
+      content: ""
+      location:
+        row: 46
+        column: 20
+      end_location:
+        row: 46
+        column: 30
+- kind: RedundantOpenModes
+  location:
+    row: 48
+    column: 5
+  end_location:
+    row: 48
+    column: 31
+  fix:
+    patch:
+      content: ""
+      location:
+        row: 48
+        column: 10
+      end_location:
+        row: 48
+        column: 20
+- kind: RedundantOpenModes
+  location:
+    row: 51
+    column: 0
+  end_location:
+    row: 51
+    column: 22
+  fix:
+    patch:
+      content: "\"rb\""
+      location:
+        row: 51
+        column: 17
+      end_location:
+        row: 51
+        column: 21
+- kind: RedundantOpenModes
+  location:
+    row: 52
+    column: 0
+  end_location:
+    row: 52
+    column: 27
+  fix:
+    patch:
+      content: "\"rb\""
+      location:
+        row: 52
+        column: 22
+      end_location:
+        row: 52
+        column: 26
+- kind: RedundantOpenModes
+  location:
+    row: 53
+    column: 0
+  end_location:
+    row: 53
+    column: 27
+  fix:
+    patch:
+      content: "\"rb\""
+      location:
+        row: 53
+        column: 10
+      end_location:
+        row: 53
+        column: 14
+- kind: RedundantOpenModes
+  location:
+    row: 55
+    column: 5
+  end_location:
+    row: 55
+    column: 27
+  fix:
+    patch:
+      content: "\"rb\""
+      location:
+        row: 55
+        column: 22
+      end_location:
+        row: 55
+        column: 26
+- kind: RedundantOpenModes
+  location:
+    row: 57
+    column: 5
+  end_location:
+    row: 57
+    column: 32
+  fix:
+    patch:
+      content: "\"rb\""
+      location:
+        row: 57
+        column: 27
+      end_location:
+        row: 57
+        column: 31
+- kind: RedundantOpenModes
+  location:
+    row: 59
+    column: 5
+  end_location:
+    row: 59
+    column: 32
+  fix:
+    patch:
+      content: "\"rb\""
+      location:
+        row: 59
+        column: 15
+      end_location:
+        row: 59
+        column: 19
+- kind: RedundantOpenModes
+  location:
+    row: 62
+    column: 0
+  end_location:
+    row: 62
+    column: 110
+  fix:
+    patch:
+      content: ""
+      location:
+        row: 62
+        column: 15
+      end_location:
+        row: 62
+        column: 25
+- kind: RedundantOpenModes
+  location:
+    row: 63
+    column: 0
+  end_location:
+    row: 63
+    column: 110
+  fix:
+    patch:
+      content: ""
+      location:
+        row: 63
+        column: 99
+      end_location:
+        row: 63
+        column: 109
+- kind: RedundantOpenModes
+  location:
+    row: 64
+    column: 0
+  end_location:
+    row: 64
+    column: 110
+  fix:
+    patch:
+      content: ""
+      location:
+        row: 64
+        column: 58
+      end_location:
+        row: 64
+        column: 68
+- kind: RedundantOpenModes
+  location:
+    row: 65
+    column: 0
+  end_location:
+    row: 65
+    column: 110
+  fix:
+    patch:
+      content: ""
+      location:
+        row: 65
+        column: 5
+      end_location:
+        row: 65
+        column: 15
+- kind: RedundantOpenModes
+  location:
+    row: 67
+    column: 0
+  end_location:
+    row: 67
+    column: 111
+  fix:
+    patch:
+      content: "\"rb\""
+      location:
+        row: 67
+        column: 22
+      end_location:
+        row: 67
+        column: 26
+- kind: RedundantOpenModes
+  location:
+    row: 68
+    column: 0
+  end_location:
+    row: 68
+    column: 111
+  fix:
+    patch:
+      content: "\"rb\""
+      location:
+        row: 68
+        column: 106
+      end_location:
+        row: 68
+        column: 110
+- kind: RedundantOpenModes
+  location:
+    row: 69
+    column: 0
+  end_location:
+    row: 69
+    column: 111
+  fix:
+    patch:
+      content: "\"rb\""
+      location:
+        row: 69
+        column: 65
+      end_location:
+        row: 69
+        column: 69
+- kind: RedundantOpenModes
+  location:
+    row: 70
+    column: 0
+  end_location:
+    row: 70
+    column: 111
+  fix:
+    patch:
+      content: "\"rb\""
+      location:
+        row: 70
+        column: 10
+      end_location:
+        row: 70
+        column: 14