Handle parentheses

crates/ruff_linter/resources/test/fixtures/pylint/unspecified_encoding.py

@@ -45,3 +45,19 @@ def func(*args, **kwargs):
 
 open("test.txt",)
 open()
+open(
+    "test.txt",  # comment
+)
+open(
+    "test.txt",
+    # comment
+)
+open(("test.txt"),)
+open()
+open(
+    ("test.txt"),  # comment
+)
+open(
+    ("test.txt"),
+    # comment
+)

crates/ruff_linter/src/fix/edits.rs

@@ -1,11 +1,12 @@
 //! Interface for generating fix edits from higher-level actions (e.g., "remove an argument").
 
+use std::ops::Sub;
+
 use anyhow::{Context, Result};
-use itertools::{Either, Itertools};
 
 use ruff_diagnostics::Edit;
+use ruff_python_ast::AnyNodeRef;
 use ruff_python_ast::{self as ast, Arguments, ExceptHandler, Stmt};
-use ruff_python_ast::{AnyNodeRef, ArgOrKeyword};
 use ruff_python_codegen::Stylist;
 use ruff_python_index::Indexer;
 use ruff_python_trivia::{
@@ -139,66 +140,30 @@ pub(crate) fn remove_argument<T: Ranged>(
     }
 }
 
-#[derive(Debug, Copy, Clone)]
-pub(crate) enum ArgumentType {
-    Keyword,
-}
+/// Generic function to add arguments or keyword arguments to function calls.
+pub(crate) fn add_argument(argument: &str, arguments: &Arguments, source: &str) -> Edit {
+    if let Some(last) = arguments.arguments_source_order().last() {
+        // Case 1: existing arguments, so append after the last argument.
+        let tokenizer = SimpleTokenizer::new(
+            source,
+            TextRange::new(last.end(), arguments.end().sub(TextSize::from(1))),
+        );
 
-/// Generic function to add arguments or keyword arguments to function
-/// calls and class definitions. (For classes `args` should be considered
-/// `bases`)
-pub(crate) fn add_argument(
-    argument: &str,
-    arguments: &Arguments,
-    arg_type: ArgumentType,
-    source: &str,
-) -> Result<Edit> {
-    // Partition into arguments before and after the argument to remove.
-    let (positional, keyword): (Vec<_>, Vec<_>) =
-        arguments
-            .arguments_source_order()
-            .partition_map(|arg| match arg {
-                ArgOrKeyword::Arg(_) => Either::Left(arg),
-                ArgOrKeyword::Keyword(_) => Either::Right(arg),
-            });
-
-    if positional.is_empty() && keyword.is_empty() {
-        // Case 1: no arguments. Add argument to call without comma
-        // TODO: Check no parentheses case
-        Ok(Edit::insertion(
-            argument.to_string(),
-            arguments.start() + TextSize::from(1),
-        ))
-    } else {
-        match arg_type {
-            ArgumentType::Keyword => {
-                // Case 3: Keyword arg passed. Can be added at the end of call
-                // Look ahead back from last argument for a trailing comma
-                let Some(last_arg) = arguments.args.last() else {
-                    panic!("No last argument found")
-                };
-                let mut tokenizer = SimpleTokenizer::starts_at(last_arg.range().end(), source);
-
-                // Find the next non-whitespace token.
-                let next = tokenizer
-                    .find(|token| {
-                        token.kind != SimpleTokenKind::Whitespace
-                            && token.kind != SimpleTokenKind::Newline
-                    })
-                    .context("Unable to find next token")?;
-
-                match next.kind {
-                    SimpleTokenKind::Comma => Ok(Edit::insertion(
-                        argument.to_string(),
-                        arguments.end() - TextSize::from(1),
-                    )),
-                    _ => Ok(Edit::insertion(
-                        format!(", {argument}"),
-                        arguments.end() - TextSize::from(1),
-                    )),
-                }
-            }
+        // Skip any parentheses.
+        if let Some(token) = tokenizer
+            .skip_while(|token| token.kind.is_trivia())
+            .next()
+            .filter(|token| token.kind == SimpleTokenKind::RParen)
+        {
+            // Ex) Insert after `func(x=(1))`.
+            Edit::insertion(format!(", {argument}"), token.end())
+        } else {
+            // Ex) Insert after `func(x=1)`.
+            Edit::insertion(format!(", {argument}"), last.end())
         }
+    } else {
+        // Case 2: no arguments. Add argument, without any trailing comma.
+        Edit::insertion(argument.to_string(), arguments.start() + TextSize::from(1))
     }
 }
 

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

@@ -1,14 +1,15 @@
 use anyhow::Result;
+
 use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast as ast;
 use ruff_python_ast::call_path::{format_call_path, CallPath};
-use ruff_python_ast::imports::{AnyImport, Import};
-use ruff_text_size::{Ranged, TextSize};
+use ruff_python_ast::Expr;
+use ruff_text_size::{Ranged, TextRange};
 
 use crate::checkers::ast::Checker;
 use crate::fix::edits::add_argument;
-use crate::fix::edits::ArgumentType::Keyword;
+use crate::importer::ImportRequest;
 use crate::settings::types::PythonVersion;
 
 /// ## What it does
@@ -20,7 +21,9 @@ use crate::settings::types::PythonVersion;
 /// non-portable code, with differing behavior across platforms.
 ///
 /// Instead, consider using the `encoding` parameter to enforce a specific
-/// encoding.
+/// encoding. [PEP 597] recommends using `locale.getpreferredencoding(False)`
+/// as the default encoding on versions earlier than Python 3.10, and
+/// `encoding="locale"` on Python 3.10 and later.
 ///
 /// ## Example
 /// ```python
@@ -34,6 +37,8 @@ use crate::settings::types::PythonVersion;
 ///
 /// ## References
 /// - [Python documentation: `open`](https://docs.python.org/3/library/functions.html#open)
+///
+/// [PEP 597]: https://peps.python.org/pep-0597/
 #[violation]
 pub struct UnspecifiedEncoding {
     function_name: String,
@@ -88,40 +93,52 @@ pub(crate) fn unspecified_encoding(checker: &mut Checker, call: &ast::ExprCall)
     );
 
     if checker.settings.target_version >= PythonVersion::Py310 {
-        diagnostic.try_set_fix(|| {
-            add_argument(
-                "encoding=\"locale\"",
-                &call.arguments,
-                Keyword,
-                checker.locator().contents(),
-            )
-            .map(Fix::unsafe_edit)
-        });
+        diagnostic.set_fix(generate_keyword_fix(checker, call));
     } else {
-        diagnostic.try_set_fix(|| generate_fix(checker, call));
+        diagnostic.try_set_fix(|| generate_import_fix(checker, call));
     }
 
     checker.diagnostics.push(diagnostic);
 }
 
-/// Generate a [`Edit`] for Python39 and older.
-fn generate_fix(checker: &Checker, call: &ast::ExprCall) -> Result<Fix> {
-    Ok(Fix::unsafe_edits(
-        checker.importer().add_import(
-            &AnyImport::Import(Import::module("locale")),
-            TextSize::default(),
+/// Generate an [`Edit`] for Python 3.10 and later.
+fn generate_keyword_fix(checker: &Checker, call: &ast::ExprCall) -> Fix {
+    Fix::unsafe_edit(add_argument(
+        &format!(
+            "encoding={}",
+            checker
+                .generator()
+                .expr(&Expr::StringLiteral(ast::ExprStringLiteral {
+                    value: ast::StringLiteralValue::single(ast::StringLiteral {
+                        value: "locale".to_string(),
+                        unicode: false,
+                        range: TextRange::default(),
+                    }),
+                    range: TextRange::default(),
+                }))
         ),
-        [add_argument(
-            "encoding=locale.getpreferredencoding(False)",
-            &call.arguments,
-            Keyword,
-            checker.locator().contents(),
-        )?],
+        &call.arguments,
+        checker.locator().contents(),
     ))
 }
 
+/// Generate an [`Edit`] for Python 3.9 and earlier.
+fn generate_import_fix(checker: &Checker, call: &ast::ExprCall) -> Result<Fix> {
+    let (import_edit, binding) = checker.importer().get_or_import_symbol(
+        &ImportRequest::import("locale", "getpreferredencoding"),
+        call.start(),
+        checker.semantic(),
+    )?;
+    let argument_edit = add_argument(
+        &format!("encoding={binding}(False)"),
+        &call.arguments,
+        checker.locator().contents(),
+    );
+    Ok(Fix::unsafe_edits(import_edit, [argument_edit]))
+}
+
 /// Returns `true` if the given expression is a string literal containing a `b` character.
-fn is_binary_mode(expr: &ast::Expr) -> Option<bool> {
+fn is_binary_mode(expr: &Expr) -> Option<bool> {
     Some(
         expr.as_string_literal_expr()?
             .value
@@ -133,12 +150,7 @@ fn is_binary_mode(expr: &ast::Expr) -> Option<bool> {
 /// Returns `true` if the given call lacks an explicit `encoding`.
 fn is_violation(call: &ast::ExprCall, call_path: &CallPath) -> bool {
     // If we have something like `*args`, which might contain the encoding argument, abort.
-    if call
-        .arguments
-        .args
-        .iter()
-        .any(ruff_python_ast::Expr::is_starred_expr)
-    {
+    if call.arguments.args.iter().any(Expr::is_starred_expr) {
         return false;
     }
     // If we have something like `**kwargs`, which might contain the encoding argument, abort.