From 8c68d30c3a50ac4661beff40e466ba85987f62b6 Mon Sep 17 00:00:00 2001 From: Victor Hugo Gomes Date: Tue, 29 Apr 2025 17:51:38 -0300 Subject: [PATCH] [`flake8-use-pathlib`] Fix `PTH123` false positive when `open` is passed a file descriptor from a function call (#17705) ## Summary Includes minor changes to the semantic type inference to help detect the return type of function call. Fixes #17691 ## Test Plan Snapshot tests --- .../fixtures/flake8_use_pathlib/full_name.py | 13 +++++++ .../rules/replaceable_by_pathlib.rs | 38 ++++++++++++++++--- .../src/analyze/typing.rs | 12 ++++++ 3 files changed, 58 insertions(+), 5 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 20197b9703..8680212b20 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 @@ -55,3 +55,16 @@ x = 2 open(x) def foo(y: int): open(y) + +# https://github.com/astral-sh/ruff/issues/17691 +def f() -> int: + return 1 +open(f()) + +open(b"foo") +byte_str = b"bar" +open(byte_str) + +def bytes_str_func() -> bytes: + return b"foo" +open(bytes_str_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 index 07e3bf10c1..58e33f040f 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 @@ -126,10 +126,9 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) { .arguments .find_argument_value("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())) + || call.arguments.find_positional(0).is_some_and(|expr| { + is_file_descriptor_or_bytes_str(expr, checker.semantic()) + }) { return None; } @@ -168,6 +167,10 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) { } } +fn is_file_descriptor_or_bytes_str(expr: &Expr, semantic: &SemanticModel) -> bool { + is_file_descriptor(expr, semantic) || is_bytes_string(expr, semantic) +} + /// 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!( @@ -180,7 +183,7 @@ fn is_file_descriptor(expr: &Expr, semantic: &SemanticModel) -> bool { return true; } - let Some(name) = expr.as_name_expr() else { + let Some(name) = get_name_expr(expr) else { return false; }; @@ -190,3 +193,28 @@ fn is_file_descriptor(expr: &Expr, semantic: &SemanticModel) -> bool { typing::is_int(binding, semantic) } + +/// Returns `true` if the given expression is a bytes string. +fn is_bytes_string(expr: &Expr, semantic: &SemanticModel) -> bool { + if matches!(expr, Expr::BytesLiteral(_)) { + return true; + } + + let Some(name) = get_name_expr(expr) else { + return false; + }; + + let Some(binding) = semantic.only_binding(name).map(|id| semantic.binding(id)) else { + return false; + }; + + typing::is_bytes(binding, semantic) +} + +fn get_name_expr(expr: &Expr) -> Option<&ast::ExprName> { + match expr { + Expr::Name(name) => Some(name), + Expr::Call(ast::ExprCall { func, .. }) => get_name_expr(func), + _ => None, + } +} diff --git a/crates/ruff_python_semantic/src/analyze/typing.rs b/crates/ruff_python_semantic/src/analyze/typing.rs index 9b1f1c43f9..c588da272a 100644 --- a/crates/ruff_python_semantic/src/analyze/typing.rs +++ b/crates/ruff_python_semantic/src/analyze/typing.rs @@ -639,6 +639,18 @@ pub fn check_type(binding: &Binding, semantic: &SemanticModel) - _ => false, }, + BindingKind::FunctionDefinition(_) => match binding.statement(semantic) { + // ```python + // def foo() -> int: + // ... + // ``` + Some(Stmt::FunctionDef(ast::StmtFunctionDef { returns, .. })) => returns + .as_ref() + .is_some_and(|return_ann| T::match_annotation(return_ann, semantic)), + + _ => false, + }, + _ => false, } }