Return DiagnosticGuard from Checker::report_diagnostic (#18232)

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

```rust
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

Author: ntBre

crates/ruff_linter/src/checkers/ast/analyze/bindings.rs

@@ -1,4 +1,4 @@
-use ruff_diagnostics::{Diagnostic, Fix};
+use ruff_diagnostics::Fix;
 use ruff_text_size::Ranged;
 
 use crate::checkers::ast::Checker;
@@ -38,92 +38,64 @@ pub(crate) fn bindings(checker: &Checker) {
                     .dummy_variable_rgx
                     .is_match(binding.name(checker.source()))
             {
-                let mut diagnostic = Diagnostic::new(
-                    pyflakes::rules::UnusedVariable {
-                        name: binding.name(checker.source()).to_string(),
-                    },
-                    binding.range(),
-                );
-                diagnostic.try_set_fix(|| {
-                    pyflakes::fixes::remove_exception_handler_assignment(binding, checker.locator)
+                checker
+                    .report_diagnostic(
+                        pyflakes::rules::UnusedVariable {
+                            name: binding.name(checker.source()).to_string(),
+                        },
+                        binding.range(),
+                    )
+                    .try_set_fix(|| {
+                        pyflakes::fixes::remove_exception_handler_assignment(
+                            binding,
+                            checker.locator,
+                        )
                         .map(Fix::safe_edit)
-                });
-                checker.report_diagnostic(diagnostic);
+                    });
             }
         }
         if checker.enabled(Rule::InvalidAllFormat) {
-            if let Some(diagnostic) = pylint::rules::invalid_all_format(binding) {
-                checker.report_diagnostic(diagnostic);
-            }
+            pylint::rules::invalid_all_format(checker, binding);
         }
         if checker.enabled(Rule::InvalidAllObject) {
-            if let Some(diagnostic) = pylint::rules::invalid_all_object(binding) {
-                checker.report_diagnostic(diagnostic);
-            }
+            pylint::rules::invalid_all_object(checker, binding);
         }
         if checker.enabled(Rule::NonAsciiName) {
-            if let Some(diagnostic) = pylint::rules::non_ascii_name(binding, checker.locator) {
-                checker.report_diagnostic(diagnostic);
-            }
+            pylint::rules::non_ascii_name(checker, binding);
         }
         if checker.enabled(Rule::UnconventionalImportAlias) {
-            if let Some(diagnostic) = flake8_import_conventions::rules::unconventional_import_alias(
+            flake8_import_conventions::rules::unconventional_import_alias(
                 checker,
                 binding,
                 &checker.settings.flake8_import_conventions.aliases,
-            ) {
-                checker.report_diagnostic(diagnostic);
-            }
+            );
         }
         if checker.enabled(Rule::UnaliasedCollectionsAbcSetImport) {
-            if let Some(diagnostic) =
-                flake8_pyi::rules::unaliased_collections_abc_set_import(checker, binding)
-            {
-                checker.report_diagnostic(diagnostic);
-            }
+            flake8_pyi::rules::unaliased_collections_abc_set_import(checker, binding);
         }
         if !checker.source_type.is_stub() && checker.enabled(Rule::UnquotedTypeAlias) {
             flake8_type_checking::rules::unquoted_type_alias(checker, binding);
         }
         if checker.enabled(Rule::UnsortedDunderSlots) {
-            if let Some(diagnostic) = ruff::rules::sort_dunder_slots(checker, binding) {
-                checker.report_diagnostic(diagnostic);
-            }
+            ruff::rules::sort_dunder_slots(checker, binding);
         }
         if checker.enabled(Rule::UsedDummyVariable) {
-            if let Some(diagnostic) = ruff::rules::used_dummy_variable(checker, binding, binding_id)
-            {
-                checker.report_diagnostic(diagnostic);
-            }
+            ruff::rules::used_dummy_variable(checker, binding, binding_id);
         }
         if checker.enabled(Rule::AssignmentInAssert) {
-            if let Some(diagnostic) = ruff::rules::assignment_in_assert(checker, binding) {
-                checker.report_diagnostic(diagnostic);
-            }
+            ruff::rules::assignment_in_assert(checker, binding);
         }
         if checker.enabled(Rule::PytestUnittestRaisesAssertion) {
-            if let Some(diagnostic) =
-                flake8_pytest_style::rules::unittest_raises_assertion_binding(checker, binding)
-            {
-                checker.report_diagnostic(diagnostic);
-            }
+            flake8_pytest_style::rules::unittest_raises_assertion_binding(checker, binding);
         }
         if checker.enabled(Rule::ForLoopWrites) {
-            if let Some(diagnostic) = refurb::rules::for_loop_writes_binding(checker, binding) {
-                checker.report_diagnostic(diagnostic);
-            }
+            refurb::rules::for_loop_writes_binding(checker, binding);
         }
         if checker.enabled(Rule::CustomTypeVarForSelf) {
-            if let Some(diagnostic) =
-                flake8_pyi::rules::custom_type_var_instead_of_self(checker, binding)
-            {
-                checker.report_diagnostic(diagnostic);
-            }
+            flake8_pyi::rules::custom_type_var_instead_of_self(checker, binding);
         }
         if checker.enabled(Rule::PrivateTypeParameter) {
-            if let Some(diagnostic) = pyupgrade::rules::private_type_parameter(checker, binding) {
-                checker.report_diagnostic(diagnostic);
-            }
+            pyupgrade::rules::private_type_parameter(checker, binding);
         }
     }
 }

crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs

@@ -1,4 +1,4 @@
-use ruff_diagnostics::{Diagnostic, Fix};
+use ruff_diagnostics::Fix;
 use ruff_python_semantic::analyze::visibility;
 use ruff_python_semantic::{Binding, BindingKind, Imported, ResolvedReference, ScopeKind};
 use ruff_text_size::Ranged;
@@ -112,12 +112,12 @@ pub(crate) fn deferred_scopes(checker: &Checker) {
                         .map(|id| checker.semantic.reference(*id))
                         .all(ResolvedReference::is_load)
                     {
-                        checker.report_diagnostic(Diagnostic::new(
+                        checker.report_diagnostic(
                             pylint::rules::GlobalVariableNotAssigned {
                                 name: (*name).to_string(),
                             },
                             binding.range(),
-                        ));
+                        );
                     }
                 }
             }
@@ -146,12 +146,12 @@ pub(crate) fn deferred_scopes(checker: &Checker) {
                     if scope.kind.is_generator() {
                         continue;
                     }
-                    checker.report_diagnostic(Diagnostic::new(
+                    checker.report_diagnostic(
                         pylint::rules::RedefinedArgumentFromLocal {
                             name: name.to_string(),
                         },
                         binding.range(),
-                    ));
+                    );
                 }
             }
         }
@@ -186,13 +186,13 @@ pub(crate) fn deferred_scopes(checker: &Checker) {
                         continue;
                     }
 
-                    checker.report_diagnostic(Diagnostic::new(
+                    checker.report_diagnostic(
                         pyflakes::rules::ImportShadowedByLoopVar {
                             name: name.to_string(),
                             row: checker.compute_source_row(shadowed.start()),
                         },
                         binding.range(),
-                    ));
+                    );
                 }
             }
         }
@@ -331,7 +331,7 @@ pub(crate) fn deferred_scopes(checker: &Checker) {
             // Create diagnostics for each statement.
             for (source, entries) in &redefinitions {
                 for (shadowed, binding) in entries {
-                    let mut diagnostic = Diagnostic::new(
+                    let mut diagnostic = checker.report_diagnostic(
                         pyflakes::rules::RedefinedWhileUnused {
                             name: binding.name(checker.source()).to_string(),
                             row: checker.compute_source_row(shadowed.start()),
@@ -346,8 +346,6 @@ pub(crate) fn deferred_scopes(checker: &Checker) {
                     if let Some(fix) = source.as_ref().and_then(|source| fixes.get(source)) {
                         diagnostic.set_fix(fix.clone());
                     }
-
-                    checker.report_diagnostic(diagnostic);
                 }
             }
         }

crates/ruff_linter/src/checkers/ast/analyze/except_handler.rs

@@ -17,14 +17,7 @@ pub(crate) fn except_handler(except_handler: &ExceptHandler, checker: &Checker)
             range: _,
         }) => {
             if checker.enabled(Rule::BareExcept) {
-                if let Some(diagnostic) = pycodestyle::rules::bare_except(
-                    type_.as_deref(),
-                    body,
-                    except_handler,
-                    checker.locator,
-                ) {
-                    checker.report_diagnostic(diagnostic);
-                }
+                pycodestyle::rules::bare_except(checker, type_.as_deref(), body, except_handler);
             }
             if checker.enabled(Rule::RaiseWithoutFromInsideExcept) {
                 flake8_bugbear::rules::raise_without_from_inside_except(

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

@@ -1,8 +1,6 @@
 use ruff_python_ast::{self as ast, Arguments, Expr, ExprContext, Operator};
 use ruff_python_literal::cformat::{CFormatError, CFormatErrorType};
 
-use ruff_diagnostics::Diagnostic;
-
 use ruff_python_ast::types::Node;
 use ruff_python_semantic::ScopeKind;
 use ruff_python_semantic::analyze::typing;
@@ -195,14 +193,13 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) {
                 let check_too_many_expressions = checker.enabled(Rule::ExpressionsInStarAssignment);
                 let check_two_starred_expressions =
                     checker.enabled(Rule::MultipleStarredExpressions);
-                if let Some(diagnostic) = pyflakes::rules::starred_expressions(
+                pyflakes::rules::starred_expressions(
+                    checker,
                     elts,
                     check_too_many_expressions,
                     check_two_starred_expressions,
                     expr.range(),
-                ) {
-                    checker.report_diagnostic(diagnostic);
-                }
+                );
             }
         }
         Expr::Name(ast::ExprName { id, ctx, range }) => {
@@ -527,12 +524,12 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) {
                             match pyflakes::format::FormatSummary::try_from(string_value.to_str()) {
                                 Err(e) => {
                                     if checker.enabled(Rule::StringDotFormatInvalidFormat) {
-                                        checker.report_diagnostic(Diagnostic::new(
+                                        checker.report_diagnostic(
                                             pyflakes::rules::StringDotFormatInvalidFormat {
                                                 message: pyflakes::format::error_to_string(&e),
                                             },
                                             location,
-                                        ));
+                                        );
                                     }
                                 }
                                 Ok(summary) => {
@@ -936,9 +933,7 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) {
                 pylint::rules::repeated_keyword_argument(checker, call);
             }
             if checker.enabled(Rule::PytestPatchWithLambda) {
-                if let Some(diagnostic) = flake8_pytest_style::rules::patch_with_lambda(call) {
-                    checker.report_diagnostic(diagnostic);
-                }
+                flake8_pytest_style::rules::patch_with_lambda(checker, call);
             }
             if checker.any_enabled(&[
                 Rule::PytestParametrizeNamesWrongType,
@@ -1286,22 +1281,22 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) {
                             ..
                         }) => {
                             if checker.enabled(Rule::PercentFormatUnsupportedFormatCharacter) {
-                                checker.report_diagnostic(Diagnostic::new(
+                                checker.report_diagnostic(
                                     pyflakes::rules::PercentFormatUnsupportedFormatCharacter {
                                         char: c,
                                     },
                                     location,
-                                ));
+                                );
                             }
                         }
                         Err(e) => {
                             if checker.enabled(Rule::PercentFormatInvalidFormat) {
-                                checker.report_diagnostic(Diagnostic::new(
+                                checker.report_diagnostic(
                                     pyflakes::rules::PercentFormatInvalidFormat {
                                         message: e.to_string(),
                                     },
                                     location,
-                                ));
+                                );
                             }
                         }
                         Ok(summary) => {
@@ -1365,10 +1360,7 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) {
             op: Operator::Add, ..
         }) => {
             if checker.enabled(Rule::ExplicitStringConcatenation) {
-                if let Some(diagnostic) = flake8_implicit_str_concat::rules::explicit(expr, checker)
-                {
-                    checker.report_diagnostic(diagnostic);
-                }
+                flake8_implicit_str_concat::rules::explicit(checker, expr);
             }
             if checker.enabled(Rule::CollectionLiteralConcatenation) {
                 ruff::rules::collection_literal_concatenation(checker, expr);