From 448a66924b6b687e64ba0d4b36e88bf81a290f48 Mon Sep 17 00:00:00 2001 From: Dan Date: Wed, 5 Nov 2025 01:01:36 -0500 Subject: [PATCH 1/3] fix-21274 --- .../resources/test/fixtures/refurb/FURB101.py | 4 ++ .../src/rules/refurb/rules/read_whole_file.rs | 53 +++++++++++++++---- ...es__refurb__tests__FURB101_FURB101.py.snap | 10 ++++ ...rb__tests__preview_FURB101_FURB101.py.snap | 21 ++++++++ 4 files changed, 77 insertions(+), 11 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB101.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB101.py index 31b1ccd341e24..4ce84f1bc4bf5 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB101.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB101.py @@ -125,3 +125,7 @@ def bar(x): # `buffering`. with open(*filename, file="file.txt", mode="r") as f: x = f.read() + +# FURB101 +with open("file.txt", encoding="utf-8") as f: + contents: str = f.read() diff --git a/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs b/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs index b64f91829a640..f322a8e1d8edc 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs @@ -129,21 +129,43 @@ impl<'a> Visitor<'a> for ReadMatcher<'a, '_> { return; } - let target = match self.with_stmt.body.first() { + let (target, annotation) = match self.with_stmt.body.first() { Some(Stmt::Assign(assign)) if assign.value.range().contains_range(expr.range()) => { match assign.targets.first() { - Some(Expr::Name(name)) => Some(name.id.as_str()), - _ => None, + Some(Expr::Name(name)) => (Some(name.id.as_str()), None), + _ => (None, None), } } - _ => None, + Some(Stmt::AnnAssign(ann_assign)) + if ann_assign + .value + .as_ref() + .is_some_and(|v| v.range().contains_range(expr.range())) => + { + match ann_assign.target.as_ref() { + Expr::Name(name) => { + let annotation_code = self + .checker + .generator() + .expr(ann_assign.annotation.as_ref()); + (Some(name.id.as_str()), Some(annotation_code)) + } + _ => (None, None), + } + } + _ => (None, None), }; - if let Some(fix) = - generate_fix(self.checker, &open, target, self.with_stmt, &suggestion) - { + if let Some(fix) = generate_fix( + self.checker, + &open, + target, + annotation, + self.with_stmt, + &suggestion, + ) { diagnostic.set_fix(fix); } } @@ -195,10 +217,16 @@ fn generate_fix( checker: &Checker, open: &FileOpen, target: Option<&str>, + annotation: Option, with_stmt: &ast::StmtWith, suggestion: &str, ) -> Option { - if !(with_stmt.items.len() == 1 && matches!(with_stmt.body.as_slice(), [Stmt::Assign(_)])) { + if !(with_stmt.items.len() == 1 + && matches!( + with_stmt.body.as_slice(), + [Stmt::Assign(_) | Stmt::AnnAssign(_)] + )) + { return None; } @@ -214,9 +242,12 @@ fn generate_fix( ) .ok()?; - let replacement = match target { - Some(var) => format!("{var} = {binding}({filename_code}).{suggestion}"), - None => format!("{binding}({filename_code}).{suggestion}"), + let replacement = match (target, annotation) { + (Some(var), Some(ann)) => { + format!("{var}: {ann} = {binding}({filename_code}).{suggestion}") + } + (Some(var), None) => format!("{var} = {binding}({filename_code}).{suggestion}"), + (None, _) => format!("{binding}({filename_code}).{suggestion}"), }; let applicability = if checker.comment_ranges().intersects(with_stmt.range()) { diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB101_FURB101.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB101_FURB101.py.snap index 3f851c3f12833..ab42fe0bd9803 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB101_FURB101.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB101_FURB101.py.snap @@ -104,3 +104,13 @@ FURB101 `open` and `read` should be replaced by `Path("file.txt").read_text()` 51 | # the user reads the whole file and that bit they can replace. | help: Replace with `Path("file.txt").read_text()` + +FURB101 `open` and `read` should be replaced by `Path("file.txt").read_text(encoding="utf-8")` + --> FURB101.py:130:6 + | +129 | # FURB101 +130 | with open("file.txt", encoding="utf-8") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +131 | contents: str = f.read() + | +help: Replace with `Path("file.txt").read_text(encoding="utf-8")` diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__preview_FURB101_FURB101.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__preview_FURB101_FURB101.py.snap index 4131499c0cf35..308bc42c86a76 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__preview_FURB101_FURB101.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__preview_FURB101_FURB101.py.snap @@ -189,3 +189,24 @@ FURB101 `open` and `read` should be replaced by `Path("file.txt").read_text()` 51 | # the user reads the whole file and that bit they can replace. | help: Replace with `Path("file.txt").read_text()` + +FURB101 [*] `open` and `read` should be replaced by `Path("file.txt").read_text(encoding="utf-8")` + --> FURB101.py:130:6 + | +129 | # FURB101 +130 | with open("file.txt", encoding="utf-8") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +131 | contents: str = f.read() + | +help: Replace with `Path("file.txt").read_text(encoding="utf-8")` +1 + import pathlib +2 | def foo(): +3 | ... +4 | +-------------------------------------------------------------------------------- +128 | x = f.read() +129 | +130 | # FURB101 + - with open("file.txt", encoding="utf-8") as f: + - contents: str = f.read() +131 + contents: str = pathlib.Path("file.txt").read_text(encoding="utf-8") From 63575799ab3e92f978fa3fee077b21c68d62c236 Mon Sep 17 00:00:00 2001 From: Dan Date: Thu, 6 Nov 2025 16:48:12 -0500 Subject: [PATCH 2/3] simplify - Move extraction logic into `generate_fix` to eliminate duplication - Replace `generator().expr()` with `locator().slice()` for annotation code - Simplify function signature by passing `expr` instead of pre-extracted values --- .../src/rules/refurb/rules/read_whole_file.rs | 68 ++++++++----------- 1 file changed, 29 insertions(+), 39 deletions(-) diff --git a/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs b/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs index f322a8e1d8edc..54e3756f27c08 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs @@ -129,43 +129,9 @@ impl<'a> Visitor<'a> for ReadMatcher<'a, '_> { return; } - let (target, annotation) = match self.with_stmt.body.first() { - Some(Stmt::Assign(assign)) - if assign.value.range().contains_range(expr.range()) => - { - match assign.targets.first() { - Some(Expr::Name(name)) => (Some(name.id.as_str()), None), - _ => (None, None), - } - } - Some(Stmt::AnnAssign(ann_assign)) - if ann_assign - .value - .as_ref() - .is_some_and(|v| v.range().contains_range(expr.range())) => - { - match ann_assign.target.as_ref() { - Expr::Name(name) => { - let annotation_code = self - .checker - .generator() - .expr(ann_assign.annotation.as_ref()); - (Some(name.id.as_str()), Some(annotation_code)) - } - _ => (None, None), - } - } - _ => (None, None), - }; - - if let Some(fix) = generate_fix( - self.checker, - &open, - target, - annotation, - self.with_stmt, - &suggestion, - ) { + if let Some(fix) = + generate_fix(self.checker, &open, expr, self.with_stmt, &suggestion) + { diagnostic.set_fix(fix); } } @@ -216,8 +182,7 @@ fn make_suggestion(open: &FileOpen<'_>, generator: Generator) -> String { fn generate_fix( checker: &Checker, open: &FileOpen, - target: Option<&str>, - annotation: Option, + expr: &Expr, with_stmt: &ast::StmtWith, suggestion: &str, ) -> Option { @@ -231,6 +196,31 @@ fn generate_fix( } let locator = checker.locator(); + + let (target, annotation) = match with_stmt.body.first() { + Some(Stmt::Assign(assign)) if assign.value.range().contains_range(expr.range()) => { + match assign.targets.first() { + Some(Expr::Name(name)) => (Some(name.id.as_str()), None), + _ => (None, None), + } + } + Some(Stmt::AnnAssign(ann_assign)) + if ann_assign + .value + .as_ref() + .is_some_and(|v| v.range().contains_range(expr.range())) => + { + match ann_assign.target.as_ref() { + Expr::Name(name) => { + let annotation_code = locator.slice(ann_assign.annotation.range()); + (Some(name.id.as_str()), Some(annotation_code.to_string())) + } + _ => (None, None), + } + } + _ => (None, None), + }; + let filename_code = locator.slice(open.filename.range()); let (import_edit, binding) = checker From 890142a2950682eed4088db4fcfb2ef905ce4e41 Mon Sep 17 00:00:00 2001 From: Brent Westbrook Date: Fri, 7 Nov 2025 18:19:54 -0500 Subject: [PATCH 3/3] simplify a bit further, fix some preexisting issues the first() and contains_range() checks weren't exactly correct, as shown in the new tests. using first() instead of an exact length check could lead to errors where additional assignment targets were dropped. using contains_range() instead of an exact range comparison could similarly drop other parts of the expression beyond the read() call. --- .../resources/test/fixtures/refurb/FURB101.py | 11 +++ .../src/rules/refurb/rules/read_whole_file.rs | 68 +++++++++---------- ...es__refurb__tests__FURB101_FURB101.py.snap | 34 ++++++++++ 3 files changed, 78 insertions(+), 35 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB101.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB101.py index 4ce84f1bc4bf5..77306cfe1840e 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB101.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB101.py @@ -129,3 +129,14 @@ def bar(x): # FURB101 with open("file.txt", encoding="utf-8") as f: contents: str = f.read() + +# FURB101 but no fix because it would remove the assignment to `x` +with open("file.txt", encoding="utf-8") as f: + contents, x = f.read(), 2 + +# FURB101 but no fix because it would remove the `process_contents` call +with open("file.txt", encoding="utf-8") as f: + contents = process_contents(f.read()) + +with open("file.txt", encoding="utf-8") as f: + contents: str = process_contents(f.read()) diff --git a/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs b/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs index 82dbf23c62088..2b43af89a821b 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/read_whole_file.rs @@ -182,41 +182,12 @@ fn generate_fix( with_stmt: &ast::StmtWith, suggestion: &str, ) -> Option { - if !(with_stmt.items.len() == 1 - && matches!( - with_stmt.body.as_slice(), - [Stmt::Assign(_) | Stmt::AnnAssign(_)] - )) - { + if with_stmt.items.len() != 1 { return None; } let locator = checker.locator(); - let (target, annotation) = match with_stmt.body.first() { - Some(Stmt::Assign(assign)) if assign.value.range().contains_range(expr.range()) => { - match assign.targets.first() { - Some(Expr::Name(name)) => (Some(name.id.as_str()), None), - _ => (None, None), - } - } - Some(Stmt::AnnAssign(ann_assign)) - if ann_assign - .value - .as_ref() - .is_some_and(|v| v.range().contains_range(expr.range())) => - { - match ann_assign.target.as_ref() { - Expr::Name(name) => { - let annotation_code = locator.slice(ann_assign.annotation.range()); - (Some(name.id.as_str()), Some(annotation_code.to_string())) - } - _ => (None, None), - } - } - _ => (None, None), - }; - let filename_code = locator.slice(open.filename.range()); let (import_edit, binding) = checker @@ -228,12 +199,39 @@ fn generate_fix( ) .ok()?; - let replacement = match (target, annotation) { - (Some(var), Some(ann)) => { - format!("{var}: {ann} = {binding}({filename_code}).{suggestion}") + // Only replace context managers with a single assignment or annotated assignment in the body. + // The assignment's RHS must also be the same as the `read` call in `expr`, otherwise this fix + // would remove the rest of the expression. + let replacement = match with_stmt.body.as_slice() { + [Stmt::Assign(ast::StmtAssign { targets, value, .. })] if value.range() == expr.range() => { + match targets.as_slice() { + [Expr::Name(name)] => { + format!( + "{name} = {binding}({filename_code}).{suggestion}", + name = name.id + ) + } + _ => return None, + } } - (Some(var), None) => format!("{var} = {binding}({filename_code}).{suggestion}"), - (None, _) => format!("{binding}({filename_code}).{suggestion}"), + [ + Stmt::AnnAssign(ast::StmtAnnAssign { + target, + annotation, + value: Some(value), + .. + }), + ] if value.range() == expr.range() => match target.as_ref() { + Expr::Name(name) => { + format!( + "{var}: {ann} = {binding}({filename_code}).{suggestion}", + var = name.id, + ann = locator.slice(annotation.range()) + ) + } + _ => return None, + }, + _ => return None, }; let applicability = if checker.comment_ranges().intersects(with_stmt.range()) { diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB101_FURB101.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB101_FURB101.py.snap index 308bc42c86a76..3fea418d76eca 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB101_FURB101.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB101_FURB101.py.snap @@ -210,3 +210,37 @@ help: Replace with `Path("file.txt").read_text(encoding="utf-8")` - with open("file.txt", encoding="utf-8") as f: - contents: str = f.read() 131 + contents: str = pathlib.Path("file.txt").read_text(encoding="utf-8") +132 | +133 | # FURB101 but no fix because it would remove the assignment to `x` +134 | with open("file.txt", encoding="utf-8") as f: + +FURB101 `open` and `read` should be replaced by `Path("file.txt").read_text(encoding="utf-8")` + --> FURB101.py:134:6 + | +133 | # FURB101 but no fix because it would remove the assignment to `x` +134 | with open("file.txt", encoding="utf-8") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +135 | contents, x = f.read(), 2 + | +help: Replace with `Path("file.txt").read_text(encoding="utf-8")` + +FURB101 `open` and `read` should be replaced by `Path("file.txt").read_text(encoding="utf-8")` + --> FURB101.py:138:6 + | +137 | # FURB101 but no fix because it would remove the `process_contents` call +138 | with open("file.txt", encoding="utf-8") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +139 | contents = process_contents(f.read()) + | +help: Replace with `Path("file.txt").read_text(encoding="utf-8")` + +FURB101 `open` and `read` should be replaced by `Path("file.txt").read_text(encoding="utf-8")` + --> FURB101.py:141:6 + | +139 | contents = process_contents(f.read()) +140 | +141 | with open("file.txt", encoding="utf-8") as f: + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +142 | contents: str = process_contents(f.read()) + | +help: Replace with `Path("file.txt").read_text(encoding="utf-8")`