Don't suggest replacing builtin.open() with Path.open() if the la…

…tter doesn't support all options (#7637) **Summary** Check that `closefd` and `opener` aren't being used with `builtin.open()` before suggesting `Path.open()` because pathlib doesn't support these arguments. Closes #7620 **Test Plan** New cases in the fixture.
astral-sh · Sep 26, 2023 · 26f9b4a · 26f9b4a
1 parent 93b5d8a
commit 26f9b4a
Show file tree

Hide file tree

Showing 6 changed files with 175 additions and 99 deletions.
diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/full_name.py b/crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/full_name.py
@@ -33,3 +33,14 @@
     fp.read()
 open(p).close()
 os.getcwdb(p)
+
+# https://github.com/astral-sh/ruff/issues/7620
+def opener(path, flags):
+    return os.open(path, flags, dir_fd=os.open('somedir', os.O_RDONLY))
+
+
+open(p, closefd=False)
+open(p, opener=opener)
+open(p, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None)
+open(p, 'r', - 1, None, None, None, True, None)
+open(p, 'r', - 1, None, None, None, False, opener)
diff --git a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP015.py b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP015.py
@@ -59,15 +59,15 @@
 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='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)
+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)
 
 open = 1
 open("foo", "U")

diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs
@@ -863,7 +863,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
                 Rule::OsPathGetctime,
                 Rule::Glob,
             ]) {
-                flake8_use_pathlib::rules::replaceable_by_pathlib(checker, func);
+                flake8_use_pathlib::rules::replaceable_by_pathlib(checker, call);
             }
             if checker.enabled(Rule::PathConstructorCurrentDirectory) {
                 flake8_use_pathlib::rules::path_constructor_current_directory(checker, expr, func);

diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/replaceable_by_pathlib.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/replaceable_by_pathlib.rs
@@ -1,5 +1,5 @@
 use ruff_diagnostics::{Diagnostic, DiagnosticKind};
-use ruff_python_ast::Expr;
+use ruff_python_ast::{Constant, Expr, ExprCall, ExprConstant};
 use ruff_text_size::Ranged;
 
 use crate::checkers::ast::Checker;
@@ -15,11 +15,11 @@ use crate::rules::flake8_use_pathlib::violations::{
 };
 use crate::settings::types::PythonVersion;
 
-pub(crate) fn replaceable_by_pathlib(checker: &mut Checker, expr: &Expr) {
+pub(crate) fn replaceable_by_pathlib(checker: &mut Checker, call: &ExprCall) {
     if let Some(diagnostic_kind) =
         checker
             .semantic()
-            .resolve_call_path(expr)
+            .resolve_call_path(&call.func)
             .and_then(|call_path| match call_path.as_slice() {
                 // PTH100
                 ["os", "path", "abspath"] => Some(OsPathAbspath.into()),
@@ -86,7 +86,51 @@ pub(crate) fn replaceable_by_pathlib(checker: &mut Checker, expr: &Expr) {
                 // PTH205
                 ["os", "path", "getctime"] => Some(OsPathGetctime.into()),
                 // PTH123
-                ["" | "builtin", "open"] => Some(BuiltinOpen.into()),
+                ["" | "builtin", "open"] => {
+                    // `closefd` and `openener` are not supported by pathlib, so check if they are
+                    // are set to non-default values.
+                    // https://github.com/astral-sh/ruff/issues/7620
+                    // Signature as of Python 3.11 (https://docs.python.org/3/library/functions.html#open):
+                    // ```text
+                    //      0     1         2             3              4            5
+                    // open(file, mode='r', buffering=-1, encoding=None, errors=None, newline=None,
+                    //      6             7
+                    //      closefd=True, opener=None)
+                    //              ^^^^         ^^^^
+                    // ```
+                    // For `pathlib` (https://docs.python.org/3/library/pathlib.html#pathlib.Path.open):
+                    // ```text
+                    // Path.open(mode='r', buffering=-1, encoding=None, errors=None, newline=None)
+                    // ```
+                    if call
+                        .arguments
+                        .find_argument("closefd", 6)
+                        .is_some_and(|expr| {
+                            !matches!(
+                                expr,
+                                Expr::Constant(ExprConstant {
+                                    value: Constant::Bool(true),
+                                    ..
+                                })
+                            )
+                        })
+                        || call
+                            .arguments
+                            .find_argument("opener", 7)
+                            .is_some_and(|expr| {
+                                !matches!(
+                                    expr,
+                                    Expr::Constant(ExprConstant {
+                                        value: Constant::None,
+                                        ..
+                                    })
+                                )
+                            })
+                    {
+                        return None;
+                    }
+                    Some(BuiltinOpen.into())
+                }
                 // PTH124
                 ["py", "path", "local"] => Some(PyPath.into()),
                 // PTH207
@@ -110,7 +154,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &mut Checker, expr: &Expr) {
                 _ => None,
             })
     {
-        let diagnostic = Diagnostic::new::<DiagnosticKind>(diagnostic_kind, expr.range());
+        let diagnostic = Diagnostic::new::<DiagnosticKind>(diagnostic_kind, call.func.range());
 
         if checker.enabled(diagnostic.kind.rule()) {
             checker.diagnostics.push(diagnostic);

diff --git a/...e8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__full_name.py.snap b/...e8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__full_name.py.snap
@@ -275,6 +275,27 @@ full_name.py:35:1: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
 34 | open(p).close()
 35 | os.getcwdb(p)
    | ^^^^^^^^^^ PTH109
+36 | 
+37 | # https://github.com/astral-sh/ruff/issues/7620
+   |
+
+full_name.py:44:1: PTH123 `open()` should be replaced by `Path.open()`
+   |
+42 | open(p, closefd=False)
+43 | open(p, opener=opener)
+44 | open(p, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None)
+   | ^^^^ PTH123
+45 | open(p, 'r', - 1, None, None, None, True, None)
+46 | open(p, 'r', - 1, None, None, None, False, opener)
+   |
+
+full_name.py:45:1: PTH123 `open()` should be replaced by `Path.open()`
+   |
+43 | open(p, opener=opener)
+44 | open(p, mode='r', buffering=-1, encoding=None, errors=None, newline=None, closefd=True, opener=None)
+45 | open(p, 'r', - 1, None, None, None, True, None)
+   | ^^^^ PTH123
+46 | open(p, 'r', - 1, None, None, None, False, opener)
    |