Add a `SourceFile` to `OldDiagnostic` by ntBre · Pull Request #18356 · astral-sh/ruff

ntBre · 2025-05-28T18:38:48Z

Summary

This is the last main difference between the OldDiagnostic and Message
types, so attaching a SourceFile to OldDiagnostic should make combining the
two types almost trivial.

Initially I updated the remaining rules without access to a Checker to take a
&SourceFile directly, but after Micha's suggestion in #18356 (comment), I updated all of these calls to take a
LintContext instead. This new type is a thin wrapper around a RefCell<Vec<OldDiagnostic>>
and a SourceFile and now has the report_diagnostic method returning a DiagnosticGuard instead of Checker.
This allows the same Drop-based implementation to be used in cases without a Checker and also avoids a lot of intermediate allocations of Vec<OldDiagnostic>s.

Checker now also contains a LintContext, which it defers to for its report_diagnostic methods, which I preserved for convenience.

Test Plan

Existing tests

Summary -- It's a bit late in the refactoring process, but I think there are still a couple of PRs left before getting rid of this type entirely, so I thought it would still be worth doing. This PR is just a quick rename with no other changes. Test Plan -- Existing tests

Summary -- This is the last main difference between the `OldDiagnostic` and `Message` types, so attaching a `SourceFile` to `OldDiagnostic` should make combining the two types almost trivial. Most of the `OldDiagnostic::new` calls in lint rules were already replaced in the `DiagnosticGuard`/`Checker::report_diagnostic` refactor, but there were still quite a few rules without access to a `Checker` that required passing a `&SourceFile` directly. Test Plan -- Existing tests

github-actions · 2025-05-28T18:44:57Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

MichaReiser

This looks good. The only annoyance is that this introduces a new argument to so many functions. It may be worth checking if we can replace some Locator usages by using SourceFile instead (str implements Located which gives access to most methods on Locator))

crates/ruff_linter/src/linter.rs

crates/ruff_linter/src/rules/pycodestyle/rules/logical_lines/whitespace_before_parameters.rs

crates/ruff_linter/src/rules/flake8_pyi/rules/type_comment_in_stub.rs

ntBre · 2025-05-29T18:32:54Z

I ran with your suggestion about the DiagnosticsCollector and replaced pretty much all of the places I had previously passed a &SourceFile. This seems quite a bit nicer, and we get to use the new guard API too. I also opened #18376 to track the Locator idea. The new commits are PR-sized on their own, so this could probably use another review if you want.

MichaReiser

I haven't done a full review but here some thoughts

MichaReiser · 2025-05-29T21:00:51Z

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

    semantic_checker: SemanticSyntaxChecker,
    /// Errors collected by the `semantic_checker`.
    semantic_errors: RefCell<Vec<SemanticSyntaxError>>,
+    collector: &'a DiagnosticsCollector<'a>,


Nit. Maybe rename to diagnostics. It's the more important part of the name

Ah okay, that makes sense. I replaced all of them!

Do you think diagnostics.report_diagnostic is too repetitive? And diagnostics.into_diagnostics, though that one is much less common.

It's not ideal for sure :) I also thought about LintContext That would allow us to put more stuff on it that's needed by all lint rules and isn't directly tied to Checker.

I like that. I can do one more rename to LintContext and update the variable names to context.

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

MichaReiser · 2025-05-29T21:08:16Z

crates/ruff_linter/src/checkers/noqa.rs

-        if matches!(rule, Rule::BlanketNOQA) {
-            continue;
-        }
+    collector.with_diagnostics(|diagnostics| {


I think this api opens the door for multiple mutable borrows. First when calling with_diagnostics and then when calling report_diagnostics on the collector. I think I would instead add an as_mut_slice method that takes a mut self. The method here should then also require no changes

Thanks, these all make sense. I saw RefCell and just made everything &self without thinking about this.

Actually, I may be slightly misunderstanding in this case. What do you mean by the as_mut_slice method? I tried writing

pub(crate) fn as_mut_slice(&mut self) -> &mut [OldDiagnostic] { self.diagnostics.borrow_mut().as_mut_slice() }

but the temporary value from borrow_mut is dropped in the function and can't be returned. This is actually the original problem I ran into. All I really wanted was to call self.diagnostics.borrow().iter(), but the borrow gets dropped. That's what led me to add with_diagnostics.

For now I've just updated with_diagnostics to take a &mut self like retain and swap_remove.

You can use self.diagnostics.get_mut when you have a &self mut because the borrow checker than guarantees that no one else can be mutating diagnostic.

Oh 🤦 Thank you!

I think I'll just replace all of these helper methods with an as_mut method. Some of them actually need the Vec, so I'm thinking either as_mut_vec or maybe as_diagnostics for the name.

I also added an iter method, also using get_mut, just to avoid having to call as_mut_vec().iter().

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

MichaReiser · 2025-05-30T13:25:06Z

crates/ruff_linter/src/linter.rs

+        if let Some(line_index) = locator.line_index() {
+            builder.set_line_index(line_index.clone());
+        }


We can probably just remove this. At this point, it's almost guaranteed that the line index is none (unless I"m mistaken)

* main: [ty] support callability of bound/constrained typevars (#18389) [ty] Minor tweaks to "list all members" docs and tests (#18388) [ty] Fix broken property tests for disjointness (#18384) [ty] List available members for a given type (#18251) [`airflow`] Add unsafe fix for module moved cases (`AIR312`) (#18363) Add a `SourceFile` to `OldDiagnostic` (#18356) Update salsa past generational id change (#18362) [`airflow`] Add unsafe fix for module moved cases (`AIR311`) (#18366) [`airflow`] Add unsafe fix for module moved cases (`AIR301`) (#18367) [ty] Improve tests for `site-packages` discovery (#18374) [ty] _typeshed.Self is not a special form (#18377) [ty] Callable types are disjoint from non-callable `@final` nominal instance types (#18368) [ty] Add diagnosis for function with no return statement but with return type annotation (#18359) [`airflow`] Add unsafe fix module moved cases (`AIR302`) (#18093) Rename `ruff_linter::Diagnostic` to `OldDiagnostic` (#18355) [`refurb`] Add coverage of `set` and `frozenset` calls (`FURB171`) (#18035)

ntBre added 4 commits May 28, 2025 12:58

fix docs

24e6bab

move source_file to LogicalLinesContext

712f57f

ntBre added internal An internal refactor or improvement diagnostics Related to reporting of diagnostics. labels May 28, 2025

ntBre marked this pull request as ready for review May 28, 2025 19:08

ntBre requested a review from AlexWaygood as a code owner May 28, 2025 19:08

ntBre requested review from MichaReiser and removed request for AlexWaygood May 28, 2025 19:09

MichaReiser approved these changes May 29, 2025

View reviewed changes

ntBre added 3 commits May 29, 2025 09:47

Add DiagnosticsCollector and use internally in Checker

7355a4d

construct the SourceFile eagerly

5f08793

use DiagnosticCollector instead of SourceFile in most cases

9214a3c

ntBre mentioned this pull request May 29, 2025

Consider combining Locator and SourceFile #18376

Open

Base automatically changed from brent/rename-old-diagnostic to main May 29, 2025 19:04

Merge branch 'main' into brent/diagnostic-source-file

0c19b40

MichaReiser reviewed May 29, 2025

View reviewed changes

ntBre added 6 commits May 29, 2025 17:34

collector -> diagnostics

b90527b

make retain and swap_remove take &mut self

068c6de

make with_diagnostics take &mut self

ef7fa4f

replace diagnostics helpers with as_mut_vec and iter

347fe60

DiagnosticsCollector -> LintContext

8d16bb6

diagnostics -> context, or lint_context when context was taken

48d3476

MichaReiser approved these changes May 30, 2025

View reviewed changes

skip locator.line_index() check

4a35895

ntBre enabled auto-merge (squash) May 30, 2025 13:33

ntBre merged commit c713e76 into main May 30, 2025
33 checks passed

ntBre deleted the brent/diagnostic-source-file branch May 30, 2025 13:34

Comments

Conversation

ntBre commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ntBre commented May 29, 2025

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ntBre commented May 28, 2025 •

edited

Loading

github-actions bot commented May 28, 2025 •

edited

Loading

`ruff-ecosystem` results

MichaReiser May 30, 2025 •

edited

Loading