[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
This commit is contained in:
Brent Westbrook
2025-08-14 13:23:43 -04:00
committed by GitHub
parent ef422460de
commit 7f8f1ab2c1
29 changed files with 200 additions and 63 deletions

View File

@@ -5588,15 +5588,15 @@ fn cookiecutter_globbing() -> Result<()> {
.args(STDIN_BASE_OPTIONS)
.arg("--select=F811")
.current_dir(tempdir.path()), @r"
success: false
exit_code: 1
----- stdout -----
{{cookiecutter.repo_name}}/tests/maintest.py:3:8: F811 [*] Redefinition of unused `foo` from line 1
Found 1 error.
[*] 1 fixable with the `--fix` option.
success: false
exit_code: 1
----- stdout -----
{{cookiecutter.repo_name}}/tests/maintest.py:3:8: F811 [*] Redefinition of unused `foo` from line 1: `foo` redefined here
Found 1 error.
[*] 1 fixable with the `--fix` option.
----- stderr -----
");
----- stderr -----
");
});
Ok(())

View File

@@ -254,6 +254,11 @@ impl Diagnostic {
.find(|ann| ann.is_primary)
}
/// Returns a mutable borrow of all annotations of this diagnostic.
pub fn annotations_mut(&mut self) -> impl Iterator<Item = &mut Annotation> {
Arc::make_mut(&mut self.inner).annotations.iter_mut()
}
/// Returns the "primary" span of this diagnostic if one exists.
///
/// When there are multiple primary spans, then the first one that was
@@ -310,6 +315,11 @@ impl Diagnostic {
&self.inner.subs
}
/// Returns a mutable borrow of the sub-diagnostics of this diagnostic.
pub fn sub_diagnostics_mut(&mut self) -> impl Iterator<Item = &mut SubDiagnostic> {
Arc::make_mut(&mut self.inner).subs.iter_mut()
}
/// Returns the fix for this diagnostic if it exists.
pub fn fix(&self) -> Option<&Fix> {
self.inner.fix.as_ref()
@@ -621,6 +631,11 @@ impl SubDiagnostic {
&self.inner.annotations
}
/// Returns a mutable borrow of the annotations of this sub-diagnostic.
pub fn annotations_mut(&mut self) -> impl Iterator<Item = &mut Annotation> {
self.inner.annotations.iter_mut()
}
/// Returns a shared borrow of the "primary" annotation of this diagnostic
/// if one exists.
///

View File

@@ -264,7 +264,12 @@ impl<'a> ResolvedDiagnostic<'a> {
.annotations
.iter()
.filter_map(|ann| {
let path = ann.span.file.path(resolver);
let path = ann
.span
.file
.relative_path(resolver)
.to_str()
.unwrap_or_else(|| ann.span.file.path(resolver));
let diagnostic_source = ann.span.file.diagnostic_source(resolver);
ResolvedAnnotation::new(path, &diagnostic_source, ann, resolver)
})

View File

@@ -28,7 +28,7 @@ use itertools::Itertools;
use log::debug;
use rustc_hash::{FxHashMap, FxHashSet};
use ruff_db::diagnostic::Diagnostic;
use ruff_db::diagnostic::{Annotation, Diagnostic, IntoDiagnosticMessage, Span};
use ruff_diagnostics::{Applicability, Fix, IsolationLevel};
use ruff_notebook::{CellOffsets, NotebookIndex};
use ruff_python_ast::helpers::{collect_import_from_member, is_docstring_stmt, to_module_path};
@@ -3305,6 +3305,17 @@ impl DiagnosticGuard<'_, '_> {
Err(err) => log::debug!("Failed to create fix for {}: {}", self.name(), err),
}
}
/// Add a secondary annotation with the given message and range.
pub(crate) fn secondary_annotation<'a>(
&mut self,
message: impl IntoDiagnosticMessage + 'a,
range: impl Ranged,
) {
let span = Span::from(self.context.source_file.clone()).with_range(range.range());
let ann = Annotation::secondary(span).message(message);
self.diagnostic.as_mut().unwrap().annotate(ann);
}
}
impl std::ops::Deref for DiagnosticGuard<'_, '_> {

View File

@@ -183,14 +183,24 @@ pub(crate) fn redefined_while_unused(checker: &Checker, scope_id: ScopeId, scope
// Create diagnostics for each statement.
for (source, entries) in &redefinitions {
for (shadowed, binding) in entries {
let name = binding.name(checker.source());
let mut diagnostic = checker.report_diagnostic(
RedefinedWhileUnused {
name: binding.name(checker.source()).to_string(),
name: name.to_string(),
row: checker.compute_source_row(shadowed.start()),
},
binding.range(),
);
diagnostic.secondary_annotation(
format_args!("previous definition of `{name}` here"),
shadowed,
);
if let Some(ann) = diagnostic.primary_annotation_mut() {
ann.set_message(format_args!("`{name}` redefined here"));
}
if let Some(range) = binding.parent_range(checker.semantic()) {
diagnostic.set_parent(range.start());
}

View File

@@ -5,7 +5,14 @@ F811 Redefinition of unused `bar` from line 6
--> F811_0.py:10:5
|
10 | def bar():
| ^^^
| ^^^ `bar` redefined here
11 | pass
|
::: F811_0.py:6:5
|
5 | @foo
6 | def bar():
| --- previous definition of `bar` here
7 | pass
|
help: Remove definition: `bar`

View File

@@ -2,9 +2,11 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 Redefinition of unused `FU` from line 1
--> F811_1.py:1:25
--> F811_1.py:1:14
|
1 | import fu as FU, bar as FU
| ^^
| -- ^^ `FU` redefined here
| |
| previous definition of `FU` here
|
help: Remove definition: `FU`

View File

@@ -2,12 +2,16 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 Redefinition of unused `mixer` from line 2
--> F811_12.py:6:20
--> F811_12.py:2:20
|
1 | try:
2 | from aa import mixer
| ----- previous definition of `mixer` here
3 | except ImportError:
4 | pass
5 | else:
6 | from bb import mixer
| ^^^^^
| ^^^^^ `mixer` redefined here
7 | mixer(123)
|
help: Remove definition: `mixer`

View File

@@ -5,7 +5,12 @@ F811 Redefinition of unused `fu` from line 1
--> F811_15.py:4:5
|
4 | def fu():
| ^^
| ^^ `fu` redefined here
5 | pass
|
::: F811_15.py:1:8
|
1 | import fu
| -- previous definition of `fu` here
|
help: Remove definition: `fu`

View File

@@ -7,7 +7,14 @@ F811 Redefinition of unused `fu` from line 3
6 | def bar():
7 | def baz():
8 | def fu():
| ^^
| ^^ `fu` redefined here
9 | pass
|
::: F811_16.py:3:8
|
1 | """Test that shadowing a global with a nested function generates a warning."""
2 |
3 | import fu
| -- previous definition of `fu` here
|
help: Remove definition: `fu`

View File

@@ -6,10 +6,16 @@ F811 [*] Redefinition of unused `fu` from line 2
|
5 | def bar():
6 | import fu
| ^^
| ^^ `fu` redefined here
7 |
8 | def baz():
|
::: F811_17.py:2:8
|
1 | """Test that shadowing a global name with a nested function generates a warning."""
2 | import fu
| -- previous definition of `fu` here
|
help: Remove definition: `fu`
Safe fix
@@ -22,11 +28,15 @@ help: Remove definition: `fu`
9 8 | def fu():
F811 Redefinition of unused `fu` from line 6
--> F811_17.py:9:13
--> F811_17.py:6:12
|
5 | def bar():
6 | import fu
| -- previous definition of `fu` here
7 |
8 | def baz():
9 | def fu():
| ^^
| ^^ `fu` redefined here
10 | pass
|
help: Remove definition: `fu`

View File

@@ -2,9 +2,11 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 Redefinition of unused `FU` from line 1
--> F811_2.py:1:34
--> F811_2.py:1:23
|
1 | from moo import fu as FU, bar as FU
| ^^
| -- ^^ `FU` redefined here
| |
| previous definition of `FU` here
|
help: Remove definition: `FU`

View File

@@ -7,9 +7,17 @@ F811 [*] Redefinition of unused `Sequence` from line 26
30 | from typing import (
31 | List, # noqa: F811
32 | Sequence,
| ^^^^^^^^
| ^^^^^^^^ `Sequence` redefined here
33 | )
|
::: F811_21.py:26:5
|
24 | from typing import (
25 | List, # noqa
26 | Sequence, # noqa
| -------- previous definition of `Sequence` here
27 | )
|
help: Remove definition: `Sequence`
Safe fix

View File

@@ -2,10 +2,13 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 Redefinition of unused `foo` from line 3
--> F811_23.py:4:15
--> F811_23.py:3:15
|
1 | """Test that shadowing an explicit re-export produces a warning."""
2 |
3 | import foo as foo
| --- previous definition of `foo` here
4 | import bar as foo
| ^^^
| ^^^ `foo` redefined here
|
help: Remove definition: `foo`

View File

@@ -2,12 +2,15 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 Redefinition of unused `func` from line 2
--> F811_26.py:5:9
--> F811_26.py:2:9
|
1 | class Class:
2 | def func(self):
| ---- previous definition of `func` here
3 | pass
4 |
5 | def func(self):
| ^^^^
| ^^^^ `func` redefined here
6 | pass
|
help: Remove definition: `func`

View File

@@ -2,11 +2,14 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 Redefinition of unused `datetime` from line 3
--> F811_28.py:4:22
--> F811_28.py:3:8
|
1 | """Regression test for: https://github.com/astral-sh/ruff/issues/10384"""
2 |
3 | import datetime
| -------- previous definition of `datetime` here
4 | from datetime import datetime
| ^^^^^^^^
| ^^^^^^^^ `datetime` redefined here
5 |
6 | datetime(1, 2, 3)
|

View File

@@ -2,11 +2,17 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 Redefinition of unused `Bar` from line 3
--> F811_29.pyi:8:1
--> F811_29.pyi:3:24
|
1 | """Regression test for: https://github.com/astral-sh/ruff/issues/10509"""
2 |
3 | from foo import Bar as Bar
| --- previous definition of `Bar` here
4 |
5 | class Eggs:
6 | Bar: int # OK
7 |
8 | Bar = 1 # F811
| ^^^
| ^^^ `Bar` redefined here
|
help: Remove definition: `Bar`

View File

@@ -2,9 +2,11 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 Redefinition of unused `fu` from line 1
--> F811_3.py:1:12
--> F811_3.py:1:8
|
1 | import fu; fu = 3
| ^^
| -- ^^ `fu` redefined here
| |
| previous definition of `fu` here
|
help: Remove definition: `fu`

View File

@@ -2,32 +2,43 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 Redefinition of unused `bar` from line 10
--> F811_30.py:12:9
--> F811_30.py:10:5
|
8 | """Foo."""
9 |
10 | bar = foo
| --- previous definition of `bar` here
11 |
12 | def bar(self) -> None:
| ^^^
| ^^^ `bar` redefined here
13 | """Bar."""
|
help: Remove definition: `bar`
F811 Redefinition of unused `baz` from line 18
--> F811_30.py:21:5
--> F811_30.py:18:9
|
16 | class B:
17 | """B."""
18 | def baz(self) -> None:
| --- previous definition of `baz` here
19 | """Baz."""
20 |
21 | baz = 1
| ^^^
| ^^^ `baz` redefined here
|
help: Remove definition: `baz`
F811 Redefinition of unused `foo` from line 26
--> F811_30.py:29:12
--> F811_30.py:26:9
|
24 | class C:
25 | """C."""
26 | def foo(self) -> None:
| --- previous definition of `foo` here
27 | """Foo."""
28 |
29 | bar = (foo := 1)
| ^^^
| ^^^ `foo` redefined here
|
help: Remove definition: `foo`

View File

@@ -2,12 +2,14 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 Redefinition of unused `baz` from line 17
--> F811_31.py:19:29
--> F811_31.py:17:5
|
16 | try:
17 | baz = None
| --- previous definition of `baz` here
18 |
19 | from some_module import baz
| ^^^
| ^^^ `baz` redefined here
20 | except ImportError:
21 | pass
|

View File

@@ -2,12 +2,13 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 [*] Redefinition of unused `List` from line 4
--> F811_32.py:5:5
--> F811_32.py:4:5
|
3 | from typing import (
4 | List,
| ---- previous definition of `List` here
5 | List,
| ^^^^
| ^^^^ `List` redefined here
6 | )
|
help: Remove definition: `List`

View File

@@ -2,9 +2,11 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 Redefinition of unused `fu` from line 1
--> F811_4.py:1:12
--> F811_4.py:1:8
|
1 | import fu; fu, bar = 3
| ^^
| -- ^^ `fu` redefined here
| |
| previous definition of `fu` here
|
help: Remove definition: `fu`

View File

@@ -2,9 +2,11 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 Redefinition of unused `fu` from line 1
--> F811_5.py:1:13
--> F811_5.py:1:8
|
1 | import fu; [fu, bar] = 3
| ^^
| -- ^^ `fu` redefined here
| |
| previous definition of `fu` here
|
help: Remove definition: `fu`

View File

@@ -2,12 +2,14 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 [*] Redefinition of unused `os` from line 5
--> F811_6.py:6:12
--> F811_6.py:5:12
|
3 | i = 2
4 | if i == 1:
5 | import os
| -- previous definition of `os` here
6 | import os
| ^^
| ^^ `os` redefined here
7 | os.path
|
help: Remove definition: `os`

View File

@@ -2,12 +2,13 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 [*] Redefinition of unused `os` from line 4
--> F811_8.py:5:12
--> F811_8.py:4:12
|
3 | try:
4 | import os
| -- previous definition of `os` here
5 | import os
| ^^
| ^^ `os` redefined here
6 | except:
7 | pass
|

View File

@@ -19,11 +19,14 @@ help: Remove unused import: `os`
5 4 | import os
F811 [*] Redefinition of unused `os` from line 2
--> <filename>:5:12
--> <filename>:2:8
|
2 | import os
| -- previous definition of `os` here
3 |
4 | def f():
5 | import os
| ^^
| ^^ `os` redefined here
6 |
7 | # Despite this `del`, `import os` in `f` should still be flagged as shadowing an unused
|

View File

@@ -19,11 +19,14 @@ help: Remove unused import: `os`
5 4 | os = 1
F811 Redefinition of unused `os` from line 2
--> <filename>:5:5
--> <filename>:2:8
|
2 | import os
| -- previous definition of `os` here
3 |
4 | def f():
5 | os = 1
| ^^
| ^^ `os` redefined here
6 | print(os)
|
help: Remove definition: `os`

View File

@@ -2,12 +2,13 @@
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811 [*] Redefinition of unused `os` from line 3
--> <filename>:4:12
--> <filename>:3:12
|
2 | def f():
3 | import os
| -- previous definition of `os` here
4 | import os
| ^^
| ^^ `os` redefined here
5 |
6 | # Despite this `del`, `import os` should still be flagged as shadowing an unused
|

View File

@@ -9,7 +9,7 @@ use anyhow::Result;
use itertools::Itertools;
use rustc_hash::FxHashMap;
use ruff_db::diagnostic::Diagnostic;
use ruff_db::diagnostic::{Diagnostic, Span};
use ruff_notebook::Notebook;
#[cfg(not(fuzzing))]
use ruff_notebook::NotebookError;
@@ -281,10 +281,16 @@ Either ensure you always emit a fix or change `Violation::FIX_AVAILABILITY` to e
// noqa offset and the source file
let range = diagnostic.expect_range();
diagnostic.set_noqa_offset(directives.noqa_line_for.resolve(range.start()));
if let Some(annotation) = diagnostic.primary_annotation_mut() {
annotation.set_span(
ruff_db::diagnostic::Span::from(source_code.clone()).with_range(range),
);
// This part actually is necessary to avoid long relative paths in snapshots.
for annotation in diagnostic.annotations_mut() {
let range = annotation.get_span().range().unwrap();
annotation.set_span(Span::from(source_code.clone()).with_range(range));
}
for sub in diagnostic.sub_diagnostics_mut() {
for annotation in sub.annotations_mut() {
let range = annotation.get_span().range().unwrap();
annotation.set_span(Span::from(source_code.clone()).with_range(range));
}
}
diagnostic