[`pyflakes`] Add secondary annotation showing previous definition (`F811`) #19900

BurntSushi · 2025-08-14T15:21:00Z

Out of curiosity, what is the motivation for this change?

Just to match the path in from_diagnostic above. This isn't used in this PR now that we're using a secondary annotation, so I can revert it!

I'll double-check if it's actually needed for full diagnostics too.

Using path instead of relative_path changes some of our CLI snapshots for the main diagnostic, so I think we'll need this for sub-diagnostics too, once we use any sub-diagnostics with paths in Ruff.

──────────────────────────────────────────────────────────────────────────────── -old snapshot +new results ────────────┬─────────────────────────────────────────────────────────────────── 0 0 │ success: false 1 1 │ exit_code: 1 2 2 │ ----- stdout ----- 3 3 │ F401 [*] `bar` imported but unused 4 │- --> bar.py:2:8 4 │+ --> /tmp/.tmpJqw1Qi/bar.py:2:8 5 5 │ | 6 6 │ 2 | import bar # unused import 7 7 │ | ^^^ 8 8 │ | 9 9 │ help: Remove unused import: `bar` 10 10 │ 11 11 │ F401 [*] `foo` imported but unused 12 │- --> foo.py:2:8 12 │+ --> /tmp/.tmpJqw1Qi/foo.py:2:8 13 13 │ | 14 14 │ 2 | import foo # unused import 15 15 │ | ^^^ 16 16 │ | ────────────┴───────────────────────────────────────────────────────────────────

(I think I'll go ahead and leave this instead of reverting, since it seems like it will be useful in the future)

MichaReiser · 2025-08-14T06:44:49Z

I'm not sure about this. I think it's important that a lint rule author is in full control about how a diagnostic is rendered. If they decide that a sub diagnostic with lower severity should come first, than so be it. I'm sure there's a reason for it.

I know what you mean, I was just trying to work around the automatic help sub-diagnostic for every rule that has a fix as easily as possible. I'm assuming we don't want lint authors to have to control that, but maybe we can just expose the sub-diagnostic Vec or an API that lets them insert additional sub-diagnostics wherever they want. Or another level of DiagnosticGuard that pushes the help/fix_title last instead of immediately when the Diagnostic is created from a Violation.

A more strimlined version of report_diagnostic, e.g. report_custom or similar, seems the most reasonable to me. Ideally, we'd just remove fix and help, but I believe those fields are used on the website or other output formats? If not, that would be the simplest: just remove help and fix_title and set them manually

-Original file line number
+Diff line change
@@ Expand Up / @@ -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
@@ Expand Down Expand Up / @@ -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()
@@ Expand Down Expand Up / @@ -487,6 +497,16 @@ impl Diagnostic { @@
                 other.expect_range().start(),
             ))
         }
+        /// Sort this diagnostic's sub-diagnostics by descending severity.
+        ///
+        /// This is particularly helpful to move `help`-level sub-diagnostics to the end, after any
+        /// longer, `info`-level diagnostics.
+        pub fn sort_sub_diagnostics(&mut self) {
+            Arc::make_mut(&mut self.inner)
+                .subs
+                .sort_by_key(|sub| std::cmp::Reverse(sub.inner.severity));
+        }
     }
     #[derive(Debug, Clone, Eq, PartialEq, Hash, get_size2::GetSize)]
@@ Expand Down Expand Up / @@ -621,6 +641,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.
         ///
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -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)
                 })
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -28,7 +28,9 @@ use itertools::Itertools; @@
     use log::debug;
     use rustc_hash::{FxHashMap, FxHashSet};
-    use ruff_db::diagnostic::Diagnostic;
+    use ruff_db::diagnostic::{
+        Annotation, Diagnostic, IntoDiagnosticMessage, Span, SubDiagnostic, SubDiagnosticSeverity,
+    };
     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};
@@ Expand Down Expand Up / @@ -3305,6 +3307,22 @@ impl DiagnosticGuard<'_, '_> { @@
                 Err(err) => log::debug!("Failed to create fix for {}: {}", self.name(), err),
             }
         }
+        /// Add an "info" sub-diagnostic with the given message and range.
+        ///
+        /// Note that this shadows `Diagnostic::info` from the `Deref` implementation because we'll
+        /// usually want to attach a range here.
+        pub(crate) fn info<'a>(
+            &mut self,
+            message: impl IntoDiagnosticMessage + 'a,
+            range: impl Ranged,
+        ) {
+            let mut sub = SubDiagnostic::new(SubDiagnosticSeverity::Info, message);
+            let span = Span::from(self.context.source_file.clone()).with_range(range.range());
+            let ann = Annotation::primary(span);
+            sub.annotate(ann);
+            self.diagnostic.as_mut().unwrap().sub(sub);
+        }
     }
     impl std::ops::Deref for DiagnosticGuard<'_, '_> {
@@ Expand All / @@ -3331,7 +3349,8 @@ impl Drop for DiagnosticGuard<'_, '_> { @@
                 return;
             }
-            if let Some(diagnostic) = self.diagnostic.take() {
+            if let Some(mut diagnostic) = self.diagnostic.take() {
+                diagnostic.sort_sub_diagnostics();
                 self.context.diagnostics.borrow_mut().push(diagnostic);
             }
         }
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
         // 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.info(
+                    format_args!("previous definition of `{name}` here"),
+                    shadowed,
+                );
                 if let Some(range) = binding.parent_range(checker.semantic()) {
                     diagnostic.set_parent(range.start());
                 }
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -8,4 +8,12 @@ F811 Redefinition of unused `bar` from line 6 @@
        |     ^^^
 |     pass
        |
+    info: previous definition of `bar` here
+     --> F811_0.py:6:5
+      |
+| @foo
+| def bar():
+      |     ^^^
+|     pass
+      |
     help: Remove definition: `bar`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[`pyflakes`] Add secondary annotation showing previous definition (`F811`) #19900

Uh oh!

Diff view

Diff view

There are no files selected for viewing

BurntSushi Aug 14, 2025

Uh oh!

ntBre Aug 14, 2025

Uh oh!

ntBre Aug 14, 2025

Uh oh!

ntBre Aug 14, 2025

Uh oh!

MichaReiser Aug 14, 2025

Uh oh!

ntBre Aug 14, 2025

Uh oh!

MichaReiser Aug 14, 2025

Uh oh!

Uh oh!

Uh oh!

-Original file line number
+Diff line change
@@ Expand Up / @@ -7,4 +7,10 @@ F811 Redefinition of unused `FU` from line 1 @@
 | import fu as FU, bar as FU
       |                         ^^
       |
+    info: previous definition of `FU` here
+     --> F811_1.py:1:14
+      |
+| import fu as FU, bar as FU
+      |              ^^
+      |
     help: Remove definition: `FU`

-Original file line number
+Diff line change
@@ Expand Up / @@ -10,4 +10,13 @@ F811 Redefinition of unused `mixer` from line 2 @@
       |                    ^^^^^
 | mixer(123)
       |
+    info: previous definition of `mixer` here
+     --> F811_12.py:2:20
+      |
+| try:
+|     from aa import mixer
+      |                    ^^^^^
+| except ImportError:
+|     pass
+      |
     help: Remove definition: `mixer`

-Original file line number
+Diff line change
@@ Expand Up / @@ -8,4 +8,10 @@ F811 Redefinition of unused `fu` from line 1 @@
       |     ^^
 |     pass
       |
+    info: previous definition of `fu` here
+     --> F811_15.py:1:8
+      |
+| import fu
+      |        ^^
+      |
     help: Remove definition: `fu`

-Original file line number
+Diff line change
@@ Expand Up / @@ -10,6 +10,15 @@ F811 [*] Redefinition of unused `Sequence` from line 26 @@
        |     ^^^^^^^^
 | )
        |
+    info: previous definition of `Sequence` here
+      --> F811_21.py:26:5
+       |
+| from typing import (
+|     List,  # noqa
+|     Sequence,  # noqa
+       |     ^^^^^^^^
+| )
+       |
     help: Remove definition: `Sequence`
     ℹ Safe fix
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -8,4 +8,13 @@ F811 Redefinition of unused `foo` from line 3 @@
 | import bar as foo
       |               ^^^
       |
+    info: previous definition of `foo` here
+     --> F811_23.py:3:15
+      |
+| """Test that shadowing an explicit re-export produces a warning."""
+|
+| import foo as foo
+      |               ^^^
+| import bar as foo
+      |
     help: Remove definition: `foo`

-Original file line number
+Diff line change
@@ Expand Up / @@ -10,4 +10,12 @@ F811 Redefinition of unused `func` from line 2 @@
       |         ^^^^
 |         pass
       |
+    info: previous definition of `func` here
+     --> F811_26.py:2:9
+      |
+| class Class:
+|     def func(self):
+      |         ^^^^
+|         pass
+      |
     help: Remove definition: `func`

-Original file line number
+Diff line change
@@ Expand Up / @@ -10,4 +10,13 @@ F811 Redefinition of unused `datetime` from line 3 @@
 |
 | datetime(1, 2, 3)
       |
+    info: previous definition of `datetime` here
+     --> F811_28.py:3:8
+      |
+| """Regression test for: https://github.com/astral-sh/ruff/issues/10384"""
+|
+| import datetime
+      |        ^^^^^^^^
+| from datetime import datetime
+      |
     help: Remove definition: `datetime`

-Original file line number
+Diff line change
@@ Expand Up / @@ -9,4 +9,14 @@ F811 Redefinition of unused `Bar` from line 3 @@
 | Bar = 1  # F811
       | ^^^
       |
+    info: previous definition of `Bar` here
+     --> F811_29.pyi:3:24
+      |
+| """Regression test for: https://github.com/astral-sh/ruff/issues/10509"""
+|
+| from foo import Bar as Bar
+      |                        ^^^
+|
+| class Eggs:
+      |
     help: Remove definition: `Bar`

-Original file line number
+Diff line change
@@ Expand Up / @@ -10,6 +10,16 @@ F811 Redefinition of unused `bar` from line 10 @@
        |         ^^^
 |         """Bar."""
        |
+    info: previous definition of `bar` here
+      --> F811_30.py:10:5
+       |
+|         """Foo."""
+|
+|     bar = foo
+       |     ^^^
+|
+|     def bar(self) -> None:
+       |
     help: Remove definition: `bar`
     F811 Redefinition of unused `baz` from line 18
@@ Expand All / @@ -20,6 +30,15 @@ F811 Redefinition of unused `baz` from line 18 @@
 |     baz = 1
        |     ^^^
        |
+    info: previous definition of `baz` here
+      --> F811_30.py:18:9
+       |
+| class B:
+|     """B."""
+|     def baz(self) -> None:
+       |         ^^^
+|         """Baz."""
+       |
     help: Remove definition: `baz`
     F811 Redefinition of unused `foo` from line 26
@@ Expand All / @@ -30,4 +49,13 @@ F811 Redefinition of unused `foo` from line 26 @@
 |     bar = (foo := 1)
        |            ^^^
        |
+    info: previous definition of `foo` here
+      --> F811_30.py:26:9
+       |
+| class C:
+|     """C."""
+|     def foo(self) -> None:
+       |         ^^^
+|         """Foo."""
+       |
     help: Remove definition: `foo`

[pyflakes] Add secondary annotation showing previous definition (F811) #19900

Uh oh!

[pyflakes] Add secondary annotation showing previous definition (F811) #19900

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

BurntSushi Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[`pyflakes`] Add secondary annotation showing previous definition (`F811`) #19900

[`pyflakes`] Add secondary annotation showing previous definition (`F811`) #19900