Return DiagnosticGuard from Checker::report_diagnostic

[ntBre]

Summary

This PR adds a DiagnosticGuard type to ruff that is adapted from the DiagnosticGuard and LintDiagnosticGuard types from ty. This guard is returned by Checker::report_diagnostic and derefs to a ruff_diagnostics::Diagnostic (OldDiagnostic), allowing methods like OldDiagnostic::set_fix to be called on the result. On Drop the DiagnosticGuard pushes its contained OldDiagnostic to the Checker.

The main motivation for this is to make a following PR adding a SourceFile to each diagnostic easier. For every rule where a Checker is available, this will now only require modifying Checker::report_diagnostic rather than all the rules.

In the few cases where we need to create a diagnostic before we know if we actually want to emit it, there is a DiagnosticGuard::defuse method, which consumes the guard without emitting the diagnostic. I was able to restructure about half of the rules that naively called this to avoid calling it, but a handful of rules still need it.

One of the fairly common patterns where defuse was needed initially was something like

let diagnostic = Diagnostic::new(DiagnosticKind, range);

if !checker.enabled(diagnostic.rule()) {
    return;
}

So I also added a Checker::checked_report_diagnostic method that handles this check internally. That helped to avoid some additional defuse calls. The name is a bit repetitive, so I'm definitely open to suggestions there. I included a warning against using it in the docs since, as we've seen, the conversion from a diagnostic to a rule is actually pretty expensive.

Test Plan

Existing tests

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[MichaReiser]

This is great. I didn't review all rule changes (there are too many).

I think it would be good to add rule to Violation to avoid the need to go through diagnostic.rule only to check if the rule is enabled (which could lead to a somewhat significant perf regression).

[MichaReiser]

It's unfortunate that this has to go through diagnostic.rule which requires a lookup. I'm leaning towards adding a rule method to ViolationMetadata. That should be easy enough to derive and does make sense to me. This will also allow us to resolve the noqa code in a future version.

That makes me wonder if report_diagnostic should always to the self::enabled call but I suspect that handling with the Option return type is too annoying in many cases that it isn't worth it.

[ntBre]

Oh nice, I'll try to clean #18234 up for review at some point then. That makes sense to resolve the NoqaCode and store that on the Diagnostic instead but use the Rule directly here.

It does sound pretty appealing to always do the enabled check if it's cheap enough too. I think in most cases it should just be a let-else that returns if None, but it will be another big refactor to apply that everywhere for sure. I'm getting better at using ast-grep for these, though 😄

[MichaReiser]

It does sound pretty appealing to always do the enabled check if it's cheap enough too. I think in most cases it should just be a let-else that returns if None, but it will be another big refactor to apply that everywhere for sure. I'm getting better at using ast-grep for these, though 😄

Let's keep it at what we have for now. We can consider this if it proves necessary

[MichaReiser]

It would be nice if we could avoid creating the diagnsotic by moving this check before the report_diagnostic call. Diagnostics are rather heavy weight and creating them for what's valid code is non ideal.

[ntBre]

I reworked this a bit to avoid the defuse call. I also stored a QualifiedName directly on the rule struct to avoid a to_string call, but I needed to pass along a lifetime parameter in the derive macro using split_for_impl too. I could revert that part if we wanted, but it seemed like it could be helpful in general.

[ntBre]

On second thought, I could definitely revert this. 2/4 airflow rules I updated actually needed Strings, so I couldn't change those to QualifiedName and the mix is a bit inconsistent.