From d726f09cf045d74d4de60ac498e175b75605a854 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Fri, 4 Oct 2024 08:48:47 -0500 Subject: [PATCH] Fix `PTH123` false positive when `open` is passed a file descriptor (#13616) Closes https://github.com/astral-sh/ruff/issues/12871 Includes some minor semantic type inference extensions changes to help with reliably detecting integers --- .../fixtures/flake8_use_pathlib/full_name.py | 9 ++++++ .../rules/replaceable_by_pathlib.rs | 31 ++++++++++++++++++- ...ake8_use_pathlib__tests__full_name.py.snap | 2 -- .../src/analyze/typing.rs | 27 +++++++++++----- 4 files changed, 59 insertions(+), 10 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 index 5d2fca01873df..20197b9703863 100644 --- 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 @@ -46,3 +46,12 @@ def opener(path, flags): 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) + +# Cannot be upgraded `pathlib.Open` does not support fds +# See https://github.com/astral-sh/ruff/issues/12871 +open(1) +open(1, "w") +x = 2 +open(x) +def foo(y: int): + open(y) 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 index fabcdd40e3ad7..aa1db51279f61 100644 --- 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,7 @@ use ruff_diagnostics::{Diagnostic, DiagnosticKind}; -use ruff_python_ast::{Expr, ExprBooleanLiteral, ExprCall}; +use ruff_python_ast::{self as ast, Expr, ExprBooleanLiteral, ExprCall}; +use ruff_python_semantic::analyze::typing; +use ruff_python_semantic::SemanticModel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -124,6 +126,10 @@ pub(crate) fn replaceable_by_pathlib(checker: &mut Checker, call: &ExprCall) { .arguments .find_argument("opener", 7) .is_some_and(|expr| !expr.is_none_literal_expr()) + || call + .arguments + .find_positional(0) + .is_some_and(|expr| is_file_descriptor(expr, checker.semantic())) { return None; } @@ -159,3 +165,26 @@ pub(crate) fn replaceable_by_pathlib(checker: &mut Checker, call: &ExprCall) { } } } + +/// Returns `true` if the given expression looks like a file descriptor, i.e., if it is an integer. +fn is_file_descriptor(expr: &Expr, semantic: &SemanticModel) -> bool { + if matches!( + expr, + Expr::NumberLiteral(ast::ExprNumberLiteral { + value: ast::Number::Int(_), + .. + }) + ) { + return true; + }; + + let Some(name) = expr.as_name_expr() else { + return false; + }; + + let Some(binding) = semantic.only_binding(name).map(|id| semantic.binding(id)) else { + return false; + }; + + typing::is_int(binding, semantic) +} diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__full_name.py.snap b/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__full_name.py.snap index 92dd4a47e4b2e..ddcff48231649 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__full_name.py.snap +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__full_name.py.snap @@ -317,5 +317,3 @@ full_name.py:47:1: PTH123 `open()` should be replaced by `Path.open()` | ^^^^ PTH123 48 | open(p, 'r', - 1, None, None, None, False, opener) | - - diff --git a/crates/ruff_python_semantic/src/analyze/typing.rs b/crates/ruff_python_semantic/src/analyze/typing.rs index 2b1531dce88a6..073b6334a6515 100644 --- a/crates/ruff_python_semantic/src/analyze/typing.rs +++ b/crates/ruff_python_semantic/src/analyze/typing.rs @@ -11,7 +11,7 @@ use ruff_python_stdlib::typing::{ }; use ruff_text_size::Ranged; -use crate::analyze::type_inference::{PythonType, ResolvedPythonType}; +use crate::analyze::type_inference::{NumberLike, PythonType, ResolvedPythonType}; use crate::model::SemanticModel; use crate::{Binding, BindingKind, Modules}; @@ -576,7 +576,7 @@ trait BuiltinTypeChecker { /// Builtin type name. const BUILTIN_TYPE_NAME: &'static str; /// Type name as found in the `Typing` module. - const TYPING_NAME: &'static str; + const TYPING_NAME: Option<&'static str>; /// [`PythonType`] associated with the intended type. const EXPR_TYPE: PythonType; @@ -584,7 +584,7 @@ trait BuiltinTypeChecker { fn match_annotation(annotation: &Expr, semantic: &SemanticModel) -> bool { let value = map_subscript(annotation); semantic.match_builtin_expr(value, Self::BUILTIN_TYPE_NAME) - || semantic.match_typing_expr(value, Self::TYPING_NAME) + || Self::TYPING_NAME.is_some_and(|name| semantic.match_typing_expr(value, name)) } /// Check initializer expression to match the intended type. @@ -624,7 +624,7 @@ struct ListChecker; impl BuiltinTypeChecker for ListChecker { const BUILTIN_TYPE_NAME: &'static str = "list"; - const TYPING_NAME: &'static str = "List"; + const TYPING_NAME: Option<&'static str> = Some("List"); const EXPR_TYPE: PythonType = PythonType::List; } @@ -632,7 +632,7 @@ struct DictChecker; impl BuiltinTypeChecker for DictChecker { const BUILTIN_TYPE_NAME: &'static str = "dict"; - const TYPING_NAME: &'static str = "Dict"; + const TYPING_NAME: Option<&'static str> = Some("Dict"); const EXPR_TYPE: PythonType = PythonType::Dict; } @@ -640,7 +640,7 @@ struct SetChecker; impl BuiltinTypeChecker for SetChecker { const BUILTIN_TYPE_NAME: &'static str = "set"; - const TYPING_NAME: &'static str = "Set"; + const TYPING_NAME: Option<&'static str> = Some("Set"); const EXPR_TYPE: PythonType = PythonType::Set; } @@ -648,10 +648,18 @@ struct TupleChecker; impl BuiltinTypeChecker for TupleChecker { const BUILTIN_TYPE_NAME: &'static str = "tuple"; - const TYPING_NAME: &'static str = "Tuple"; + const TYPING_NAME: Option<&'static str> = Some("Tuple"); const EXPR_TYPE: PythonType = PythonType::Tuple; } +struct IntChecker; + +impl BuiltinTypeChecker for IntChecker { + const BUILTIN_TYPE_NAME: &'static str = "int"; + const TYPING_NAME: Option<&'static str> = None; + const EXPR_TYPE: PythonType = PythonType::Number(NumberLike::Integer); +} + pub struct IoBaseChecker; impl TypeChecker for IoBaseChecker { @@ -761,6 +769,11 @@ pub fn is_dict(binding: &Binding, semantic: &SemanticModel) -> bool { check_type::(binding, semantic) } +/// Test whether the given binding can be considered an integer. +pub fn is_int(binding: &Binding, semantic: &SemanticModel) -> bool { + check_type::(binding, semantic) +} + /// Test whether the given binding can be considered a set. /// /// For this, we check what value might be associated with it through it's initialization and