Skip to content

[refurb] Extend support for Path.open (FURB101, FURB103) #21080

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ntBre merged 14 commits into from
Dec 17, 2025
Merged

[refurb] Extend support for Path.open (FURB101, FURB103) #21080

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
344eaea
feat: implement supoort for path.open in furb103
11happy Oct 26, 2025
7250d1c
refactor: common implementation and use previously implemented functi…
11happy Nov 2, 2025
ce6dd5e
test: fix snapshots
11happy Nov 2, 2025
c3a703c
refactor: make filename optional
11happy Nov 6, 2025
971414b
update snapshot
11happy Nov 10, 2025
0e5d291
test:update snapshot
11happy Nov 10, 2025
025b2b4
test: remove unreference snapshot
11happy Nov 10, 2025
8138e2c
nits
ntBre Nov 10, 2025
9b758d8
strict check for args for length 1
11happy Nov 12, 2025
9b38bc7
Merge branch 'main' into HEAD
ntBre Nov 19, 2025
fd9e0b1
introduce OpenArgument type
ntBre Nov 19, 2025
279b6cd
refactor: remove is_path_open
11happy Nov 26, 2025
201eb06
update furb101
11happy Dec 17, 2025
63ca121
remove prev snap
11happy Dec 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactor: common implementation and use previously implemented functi… 
…ons with qualified name resolution

Signed-off-by: 11happy <soni5happy@gmail.com>
  • Loading branch information
@11happy
11happy committed Nov 10, 2025
commit 7250d1c5024f06a339ec28d5bd23fa5f99eaca49
1 change: 1 addition & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB103.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from pathlib import Path
Comment thread
ntBre marked this conversation as resolved.
Resolved
def foo():
...

Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/rules/flake8_async/rules/blocking_open_call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ fn is_open_call(func: &Expr, semantic: &SemanticModel) -> bool {
}

/// Returns `true` if an expression resolves to a call to `pathlib.Path.open`.
fn is_open_call_from_pathlib(func: &Expr, semantic: &SemanticModel) -> bool {
pub(crate) fn is_open_call_from_pathlib(func: &Expr, semantic: &SemanticModel) -> bool {
let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func else {
return false;
};
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/rules/flake8_async/rules/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ mod async_zero_sleep;
mod blocking_http_call;
mod blocking_http_call_httpx;
mod blocking_input;
mod blocking_open_call;
pub(crate) mod blocking_open_call;
mod blocking_path_methods;
mod blocking_process_invocation;
mod blocking_sleep;
Expand Down
155 changes: 62 additions & 93 deletions crates/ruff_linter/src/rules/refurb/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
use std::borrow::Cow;

use crate::checkers::ast::Checker;
use crate::rules::flake8_async::rules::blocking_open_call::is_open_call_from_pathlib;
use crate::{Applicability, Edit, Fix};
Comment thread
ntBre marked this conversation as resolved.
Resolved
use ruff_python_ast::PythonVersion;
use ruff_python_ast::{self as ast, Expr, name::Name, parenthesize::parenthesized_range};
use ruff_python_codegen::Generator;
use ruff_python_semantic::{BindingId, ResolvedReference, SemanticModel};
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;
use crate::{Applicability, Edit, Fix};

/// Format a code snippet to call `name.method()`.
pub(super) fn generate_method_call(name: Name, method: &str, generator: Generator) -> String {
// Construct `name`.
Expand Down Expand Up @@ -155,44 +155,16 @@ pub(super) fn find_file_opens<'a>(
.collect()
}

/// Find `open` operation in the given `with` item.
fn find_file_open<'a>(
fn resolve_file_open<'a>(
item: &'a ast::WithItem,
with: &'a ast::StmtWith,
semantic: &'a SemanticModel<'a>,
read_mode: bool,
python_version: PythonVersion,
filename: &'a Expr,
mode: OpenMode,
keywords: Vec<&'a ast::Keyword>,
path_obj: Option<&'a Expr>,
) -> Option<FileOpen<'a>> {
// We want to match `open(...) as var`.
let ast::ExprCall {
func,
arguments: ast::Arguments { args, keywords, .. },
..
} = item.context_expr.as_call_expr()?;

let var = item.optional_vars.as_deref()?.as_name_expr()?;

// Ignore calls with `*args` and `**kwargs`. In the exact case of `open(*filename, mode="w")`,
// it could be a match; but in all other cases, the call _could_ contain unsupported keyword
// arguments, like `buffering`.
if args.iter().any(Expr::is_starred_expr)
|| keywords.iter().any(|keyword| keyword.arg.is_none())
{
return None;
}

if !semantic.match_builtin_expr(func, "open") {
return None;
}

// Match positional arguments, get filename and mode.
let (filename, pos_mode) = match_open_args(args)?;

// Match keyword arguments, get keyword arguments to forward and possibly mode.
let (keywords, kw_mode) = match_open_keywords(keywords, read_mode, python_version)?;

let mode = kw_mode.unwrap_or(pos_mode);

match mode {
OpenMode::ReadText | OpenMode::ReadBytes => {
if !read_mode {
Expand All @@ -206,33 +178,25 @@ fn find_file_open<'a>(
}
}

// Path.read_bytes and Path.write_bytes do not support any kwargs.
if matches!(mode, OpenMode::ReadBytes | OpenMode::WriteBytes) && !keywords.is_empty() {
return None;
}

// Now we need to find what is this variable bound to...
let var = item.optional_vars.as_deref()?.as_name_expr()?;
let scope = semantic.current_scope();
let bindings: Vec<BindingId> = scope.get_all(var.id.as_str()).collect();

let binding = bindings
.iter()
.map(|id| semantic.binding(*id))
// We might have many bindings with the same name, but we only care
// for the one we are looking at right now.
.find(|binding| binding.range() == var.range())?;
let binding = scope.get_all(var.id.as_str()).find_map(|id| {
let b = semantic.binding(id);
(b.range() == var.range()).then_some(b)
})?;

// Since many references can share the same binding, we can limit our attention span
// exclusively to the body of the current `with` statement.
let references: Vec<&ResolvedReference> = binding
.references
.iter()
.map(|id| semantic.reference(*id))
.filter(|reference| with.range().contains_range(reference.range()))
.collect();

// And even with all these restrictions, if the file handle gets used not exactly once,
// it doesn't fit the bill.
let [reference] = references.as_slice() else {
return None;
};
Expand All @@ -243,31 +207,67 @@ fn find_file_open<'a>(
mode,
keywords,
reference,
path_obj: None,
path_obj,
})
}

fn find_path_open<'a>(
/// Find `open` operation in the given `with` item.
fn find_file_open<'a>(
item: &'a ast::WithItem,
with: &'a ast::StmtWith,
semantic: &'a SemanticModel<'a>,
read_mode: bool,
python_version: PythonVersion,
) -> Option<FileOpen<'a>> {
// We want to match `open(...) as var`.
let ast::ExprCall {
func,
arguments: ast::Arguments { args, keywords, .. },
..
} = item.context_expr.as_call_expr()?;
let attr = func.as_attribute_expr()?;
if attr.attr.as_str() != "open" {

// Ignore calls with `*args` and `**kwargs`. In the exact case of `open(*filename, mode="w")`,
// it could be a match; but in all other cases, the call _could_ contain unsupported keyword
// arguments, like `buffering`.
if args.iter().any(Expr::is_starred_expr)
|| keywords.iter().any(|keyword| keyword.arg.is_none())
{
return None;
}
let path_call = attr.value.as_call_expr()?;
let path_func = path_call.func.as_name_expr()?;
if path_func.id.as_str() != "Path" {

if !semantic.match_builtin_expr(func, "open") {
return None;
}

// Match positional arguments, get filename and mode.
let (filename, pos_mode) = match_open_args(args)?;

// Match keyword arguments, get keyword arguments to forward and possibly mode.
let (keywords, kw_mode) = match_open_keywords(keywords, read_mode, python_version)?;

let mode = kw_mode.unwrap_or(pos_mode);
resolve_file_open(
item, with, semantic, read_mode, filename, mode, keywords, None,
)
}

fn find_path_open<'a>(
Comment thread
ntBre marked this conversation as resolved.
Resolved
item: &'a ast::WithItem,
with: &'a ast::StmtWith,
semantic: &'a SemanticModel<'a>,
read_mode: bool,
python_version: PythonVersion,
) -> Option<FileOpen<'a>> {
let ast::ExprCall {
func,
arguments: ast::Arguments { args, keywords, .. },
..
} = item.context_expr.as_call_expr()?;
if !is_open_call_from_pathlib(func, semantic) {
return None;
}
let attr = func.as_attribute_expr()?;
let path_call = attr.value.as_call_expr()?;
let filename = path_call.arguments.args.first()?;

@ntBre ntBre Nov 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we missing some of the argument validation code from find_file_open, especially these lines:

    if args.iter().any(Expr::is_starred_expr)
        || keywords.iter().any(|keyword| keyword.arg.is_none())
    {
        return None;
    }

I also think we're doing too much inspection on the attr.value. For example, I think is_open_call_from_pathlib should be able to identify a case like this:

p = Path("file.txt")
with p.open("r") as f:
    f.write("test")

but the current implementation expects a literal Path() call, not a name that resolves to a Path.

We might need to make the filename field optional on the FileOpen struct.

@11happy 11happy Nov 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done , also the message is formatted accordingly but i feel its getting nested & dosent feel good , not sure although

@11happy 11happy Nov 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also I have not added any comments yet , should i add similar comments like find_file_open

let mode = if args.is_empty() {
OpenMode::ReadText
Expand All @@ -276,47 +276,16 @@ fn find_path_open<'a>(
};
let (keywords, kw_mode) = match_open_keywords(keywords, read_mode, python_version)?;
let mode = kw_mode.unwrap_or(mode);
match mode {
OpenMode::ReadText | OpenMode::ReadBytes => {
if !read_mode {
return None;
}
}
OpenMode::WriteText | OpenMode::WriteBytes => {
if read_mode {
return None;
}
}
}
if matches!(mode, OpenMode::ReadBytes | OpenMode::WriteBytes) && !keywords.is_empty() {
return None;
}
let var = item.optional_vars.as_deref()?.as_name_expr()?;
let scope = semantic.current_scope();
let bindings: Vec<_> = scope.get_all(var.id.as_str()).collect();
let binding = bindings
.iter()
.map(|id| semantic.binding(*id))
.find(|binding| binding.range() == var.range())?;

let references: Vec<&ResolvedReference> = binding
.references
.iter()
.map(|id| semantic.reference(*id))
.filter(|reference| with.range().contains_range(reference.range()))
.collect();

let [reference] = references.as_slice() else {
return None;
};
Some(FileOpen {
resolve_file_open(
item,
with,
semantic,
read_mode,
filename,
mode,
keywords,
reference,
path_obj: Some(attr.value.as_ref()),
})
Some(attr.value.as_ref()),
)
}

/// Match positional arguments. Return expression for the file name and open mode.
Expand Down
Loading