Skip to content

Commit

Permalink
Fix PTH123 false positive when open is passed a file descriptor (#…
Browse files Browse the repository at this point in the history
…13616)

Closes #12871

Includes some minor semantic type inference extensions changes to help
with reliably detecting integers
  • Loading branch information
zanieb authored Oct 4, 2024
1 parent 975be9c commit d726f09
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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)
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
|


27 changes: 20 additions & 7 deletions crates/ruff_python_semantic/src/analyze/typing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -576,15 +576,15 @@ 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;

/// Check annotation expression to match the intended type.
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.
Expand Down Expand Up @@ -624,34 +624,42 @@ 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;
}

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;
}

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;
}

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 {
Expand Down Expand Up @@ -761,6 +769,11 @@ pub fn is_dict(binding: &Binding, semantic: &SemanticModel) -> bool {
check_type::<DictChecker>(binding, semantic)
}

/// Test whether the given binding can be considered an integer.
pub fn is_int(binding: &Binding, semantic: &SemanticModel) -> bool {
check_type::<IntChecker>(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
Expand Down

0 comments on commit d726f09

Please sign in to comment.