Skip to content

Commit 7f8f1ab

Browse files
authored
[pyflakes] Add secondary annotation showing previous definition (F811) (#19900)
## Summary This is a second attempt at a first use of a new diagnostic feature after #19886. I'll blame rustc for this one because it also has a similar diagnostic: <img width="735" height="335" alt="image" src="https://github.com/user-attachments/assets/572fe1c3-1742-4ce4-b575-1d9196ff0932" /> We end up with a very similar diagnostic: <img width="764" height="401" alt="image" src="https://github.com/user-attachments/assets/01eaf0c7-2567-467b-a5d8-a27206b2c74c" /> ## Test Plan New snapshots and manual tests above
1 parent ef42246 commit 7f8f1ab

29 files changed

Lines changed: 200 additions & 63 deletions

File tree

crates/ruff/tests/lint.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5588,15 +5588,15 @@ fn cookiecutter_globbing() -> Result<()> {
55885588
.args(STDIN_BASE_OPTIONS)
55895589
.arg("--select=F811")
55905590
.current_dir(tempdir.path()), @r"
5591-
success: false
5592-
exit_code: 1
5593-
----- stdout -----
5594-
{{cookiecutter.repo_name}}/tests/maintest.py:3:8: F811 [*] Redefinition of unused `foo` from line 1
5595-
Found 1 error.
5596-
[*] 1 fixable with the `--fix` option.
5591+
success: false
5592+
exit_code: 1
5593+
----- stdout -----
5594+
{{cookiecutter.repo_name}}/tests/maintest.py:3:8: F811 [*] Redefinition of unused `foo` from line 1: `foo` redefined here
5595+
Found 1 error.
5596+
[*] 1 fixable with the `--fix` option.
55975597
5598-
----- stderr -----
5599-
");
5598+
----- stderr -----
5599+
");
56005600
});
56015601

56025602
Ok(())

crates/ruff_db/src/diagnostic/mod.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,11 @@ impl Diagnostic {
254254
.find(|ann| ann.is_primary)
255255
}
256256

257+
/// Returns a mutable borrow of all annotations of this diagnostic.
258+
pub fn annotations_mut(&mut self) -> impl Iterator<Item = &mut Annotation> {
259+
Arc::make_mut(&mut self.inner).annotations.iter_mut()
260+
}
261+
257262
/// Returns the "primary" span of this diagnostic if one exists.
258263
///
259264
/// When there are multiple primary spans, then the first one that was
@@ -310,6 +315,11 @@ impl Diagnostic {
310315
&self.inner.subs
311316
}
312317

318+
/// Returns a mutable borrow of the sub-diagnostics of this diagnostic.
319+
pub fn sub_diagnostics_mut(&mut self) -> impl Iterator<Item = &mut SubDiagnostic> {
320+
Arc::make_mut(&mut self.inner).subs.iter_mut()
321+
}
322+
313323
/// Returns the fix for this diagnostic if it exists.
314324
pub fn fix(&self) -> Option<&Fix> {
315325
self.inner.fix.as_ref()
@@ -621,6 +631,11 @@ impl SubDiagnostic {
621631
&self.inner.annotations
622632
}
623633

634+
/// Returns a mutable borrow of the annotations of this sub-diagnostic.
635+
pub fn annotations_mut(&mut self) -> impl Iterator<Item = &mut Annotation> {
636+
self.inner.annotations.iter_mut()
637+
}
638+
624639
/// Returns a shared borrow of the "primary" annotation of this diagnostic
625640
/// if one exists.
626641
///

crates/ruff_db/src/diagnostic/render.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,12 @@ impl<'a> ResolvedDiagnostic<'a> {
264264
.annotations
265265
.iter()
266266
.filter_map(|ann| {
267-
let path = ann.span.file.path(resolver);
267+
let path = ann
268+
.span
269+
.file
270+
.relative_path(resolver)
271+
.to_str()
272+
.unwrap_or_else(|| ann.span.file.path(resolver));
268273
let diagnostic_source = ann.span.file.diagnostic_source(resolver);
269274
ResolvedAnnotation::new(path, &diagnostic_source, ann, resolver)
270275
})

crates/ruff_linter/src/checkers/ast/mod.rs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ use itertools::Itertools;
2828
use log::debug;
2929
use rustc_hash::{FxHashMap, FxHashSet};
3030

31-
use ruff_db::diagnostic::Diagnostic;
31+
use ruff_db::diagnostic::{Annotation, Diagnostic, IntoDiagnosticMessage, Span};
3232
use ruff_diagnostics::{Applicability, Fix, IsolationLevel};
3333
use ruff_notebook::{CellOffsets, NotebookIndex};
3434
use ruff_python_ast::helpers::{collect_import_from_member, is_docstring_stmt, to_module_path};
@@ -3305,6 +3305,17 @@ impl DiagnosticGuard<'_, '_> {
33053305
Err(err) => log::debug!("Failed to create fix for {}: {}", self.name(), err),
33063306
}
33073307
}
3308+
3309+
/// Add a secondary annotation with the given message and range.
3310+
pub(crate) fn secondary_annotation<'a>(
3311+
&mut self,
3312+
message: impl IntoDiagnosticMessage + 'a,
3313+
range: impl Ranged,
3314+
) {
3315+
let span = Span::from(self.context.source_file.clone()).with_range(range.range());
3316+
let ann = Annotation::secondary(span).message(message);
3317+
self.diagnostic.as_mut().unwrap().annotate(ann);
3318+
}
33083319
}
33093320

33103321
impl std::ops::Deref for DiagnosticGuard<'_, '_> {

crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,14 +183,24 @@ pub(crate) fn redefined_while_unused(checker: &Checker, scope_id: ScopeId, scope
183183
// Create diagnostics for each statement.
184184
for (source, entries) in &redefinitions {
185185
for (shadowed, binding) in entries {
186+
let name = binding.name(checker.source());
186187
let mut diagnostic = checker.report_diagnostic(
187188
RedefinedWhileUnused {
188-
name: binding.name(checker.source()).to_string(),
189+
name: name.to_string(),
189190
row: checker.compute_source_row(shadowed.start()),
190191
},
191192
binding.range(),
192193
);
193194

195+
diagnostic.secondary_annotation(
196+
format_args!("previous definition of `{name}` here"),
197+
shadowed,
198+
);
199+
200+
if let Some(ann) = diagnostic.primary_annotation_mut() {
201+
ann.set_message(format_args!("`{name}` redefined here"));
202+
}
203+
194204
if let Some(range) = binding.parent_range(checker.semantic()) {
195205
diagnostic.set_parent(range.start());
196206
}

crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F811_F811_0.py.snap

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,14 @@ F811 Redefinition of unused `bar` from line 6
55
--> F811_0.py:10:5
66
|
77
10 | def bar():
8-
| ^^^
8+
| ^^^ `bar` redefined here
99
11 | pass
1010
|
11+
::: F811_0.py:6:5
12+
|
13+
5 | @foo
14+
6 | def bar():
15+
| --- previous definition of `bar` here
16+
7 | pass
17+
|
1118
help: Remove definition: `bar`

crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F811_F811_1.py.snap

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@
22
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
33
---
44
F811 Redefinition of unused `FU` from line 1
5-
--> F811_1.py:1:25
5+
--> F811_1.py:1:14
66
|
77
1 | import fu as FU, bar as FU
8-
| ^^
8+
| -- ^^ `FU` redefined here
9+
| |
10+
| previous definition of `FU` here
911
|
1012
help: Remove definition: `FU`

crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F811_F811_12.py.snap

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,16 @@
22
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
33
---
44
F811 Redefinition of unused `mixer` from line 2
5-
--> F811_12.py:6:20
5+
--> F811_12.py:2:20
66
|
7+
1 | try:
8+
2 | from aa import mixer
9+
| ----- previous definition of `mixer` here
10+
3 | except ImportError:
711
4 | pass
812
5 | else:
913
6 | from bb import mixer
10-
| ^^^^^
14+
| ^^^^^ `mixer` redefined here
1115
7 | mixer(123)
1216
|
1317
help: Remove definition: `mixer`

crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F811_F811_15.py.snap

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,12 @@ F811 Redefinition of unused `fu` from line 1
55
--> F811_15.py:4:5
66
|
77
4 | def fu():
8-
| ^^
8+
| ^^ `fu` redefined here
99
5 | pass
1010
|
11+
::: F811_15.py:1:8
12+
|
13+
1 | import fu
14+
| -- previous definition of `fu` here
15+
|
1116
help: Remove definition: `fu`

crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F811_F811_16.py.snap

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,14 @@ F811 Redefinition of unused `fu` from line 3
77
6 | def bar():
88
7 | def baz():
99
8 | def fu():
10-
| ^^
10+
| ^^ `fu` redefined here
1111
9 | pass
1212
|
13+
::: F811_16.py:3:8
14+
|
15+
1 | """Test that shadowing a global with a nested function generates a warning."""
16+
2 |
17+
3 | import fu
18+
| -- previous definition of `fu` here
19+
|
1320
help: Remove definition: `fu`

0 commit comments

Comments
 (0)