diff --git a/resources/test/fixtures/U015.py b/resources/test/fixtures/U015.py index 9e276f28c9..e0cdb6321f 100644 --- a/resources/test/fixtures/U015.py +++ b/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) diff --git a/src/pyupgrade/plugins/redundant_open_modes.rs b/src/pyupgrade/plugins/redundant_open_modes.rs index f5d95391cd..6ec1a29510 100644 --- a/src/pyupgrade/plugins/redundant_open_modes.rs +++ b/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) { + 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 = None; let mut fix_end: Option = 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) = 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( diff --git a/src/snapshots/ruff__linter__tests__U015_U015.py.snap b/src/snapshots/ruff__linter__tests__U015_U015.py.snap index fa114c355b..dca5bdb087 100644 --- a/src/snapshots/ruff__linter__tests__U015_U015.py.snap +++ b/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