Skip to content

Commit aa7c25c

Browse files
committed
Customize where the fix_title sub-diagnostic appears
Summary -- This is a more general alternative to #23043 that more closely follows Micha's [suggestion]. I sort of prefer this approach because it gives the diagnostic author full control of the order of sub-diagnostics. On the other hand, it feels a bit too "clever" for the current usage, which achieves the same effect as the simpler #23043. So I'm curious to get others' thoughts. I still think the ideal solution long-term would be to combine the diff rendering with the sub-diagnostic holding the fix title, but that would be much more involved than either of these PRs. As I noted in the summary for #23043, the main motivation for the defer machinery is that [RUF064] has a bunch of early returns mixed in with its `info` additions, so it was a big pain to add a `help` diagnostic before each return. We may at least want a `deferred_help` helper instead of the two closures used here, if we do take this approach. [suggestion]: https://github.com/astral-sh/ruff/pull/19900/changes/BASE..ab685c16a8c01bb33f054698d2f32bb023d07429#r2276500131 [RUF064]: https://github.com/astral-sh/ruff/blob/4a32a96740bb7327a3f554b7f6d6675cb2ea797e/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs#L120-L145 Test Plan -- Updated snapshots for `RUF064` and `ISC004`.
1 parent f1f21f7 commit aa7c25c

5 files changed

Lines changed: 121 additions & 31 deletions

File tree

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

Lines changed: 80 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ use std::path::Path;
2727
use itertools::Itertools;
2828
use log::debug;
2929
use rustc_hash::{FxHashMap, FxHashSet};
30+
use smallvec::SmallVec;
3031

3132
use ruff_db::diagnostic::{Annotation, Diagnostic, DiagnosticTag, IntoDiagnosticMessage, Span};
3233
use ruff_diagnostics::{Applicability, Fix, IsolationLevel};
@@ -421,6 +422,20 @@ impl<'a> Checker<'a> {
421422
self.context.report_diagnostic_if_enabled(kind, range)
422423
}
423424

425+
/// Return a [`DiagnosticGuard`] for reporting a diagnostic, along with its fix title to be
426+
/// attached later.
427+
///
428+
/// Prefer [`Checker::report_diagnostic`] unless you need to attach sub-diagnostics before the
429+
/// fix title. See its documentation for more details.
430+
#[must_use]
431+
pub(crate) fn report_custom_diagnostic<'chk, T: Violation>(
432+
&'chk self,
433+
kind: T,
434+
range: TextRange,
435+
) -> (DiagnosticGuard<'chk, 'a>, Option<String>) {
436+
self.context.report_custom_diagnostic(kind, range)
437+
}
438+
424439
/// Adds a [`TextRange`] to the set of ranges of variable names
425440
/// flagged in `flake8-bugbear` violations so far.
426441
///
@@ -3367,6 +3382,7 @@ impl<'a> LintContext<'a> {
33673382
context: self,
33683383
diagnostic: Some(kind.into_diagnostic(range, &self.source_file)),
33693384
rule: T::rule(),
3385+
before_drop_fns: SmallVec::new(),
33703386
}
33713387
}
33723388

@@ -3386,12 +3402,46 @@ impl<'a> LintContext<'a> {
33863402
context: self,
33873403
diagnostic: Some(kind.into_diagnostic(range, &self.source_file)),
33883404
rule,
3405+
before_drop_fns: SmallVec::new(),
33893406
})
33903407
} else {
33913408
None
33923409
}
33933410
}
33943411

3412+
/// Return a [`DiagnosticGuard`] for reporting a diagnostic, along with its fix title to be
3413+
/// attached later.
3414+
///
3415+
/// Prefer [`LintContext::report_diagnostic`] unless you need to attach sub-diagnostics before
3416+
/// the fix title. See its documentation for more details.
3417+
#[expect(clippy::needless_pass_by_value)]
3418+
#[must_use]
3419+
pub(crate) fn report_custom_diagnostic<'chk, T: Violation>(
3420+
&'chk self,
3421+
kind: T,
3422+
range: TextRange,
3423+
) -> (DiagnosticGuard<'chk, 'a>, Option<String>) {
3424+
let diagnostic = crate::message::create_lint_diagnostic(
3425+
kind.message(),
3426+
Option::<&'static str>::None,
3427+
range,
3428+
None,
3429+
None,
3430+
self.source_file.clone(),
3431+
None,
3432+
T::rule(),
3433+
);
3434+
(
3435+
DiagnosticGuard {
3436+
context: self,
3437+
diagnostic: Some(diagnostic),
3438+
rule: T::rule(),
3439+
before_drop_fns: SmallVec::new(),
3440+
},
3441+
kind.fix_title(),
3442+
)
3443+
}
3444+
33953445
#[inline]
33963446
pub(crate) const fn is_rule_enabled(&self, rule: Rule) -> bool {
33973447
self.rules.enabled(rule)
@@ -3433,6 +3483,8 @@ impl<'a> LintContext<'a> {
34333483
}
34343484
}
34353485

3486+
type BeforeDropFn = Box<dyn Fn(&mut Diagnostic)>;
3487+
34363488
/// An abstraction for mutating a diagnostic.
34373489
///
34383490
/// Callers can build this guard by starting with `Checker::report_diagnostic`.
@@ -3447,6 +3499,8 @@ pub(crate) struct DiagnosticGuard<'a, 'b> {
34473499
///
34483500
/// This is always `Some` until the `Drop` (or `defuse`) call.
34493501
diagnostic: Option<Diagnostic>,
3502+
/// Functions to call before dropping the guard and storing the diagnostic.
3503+
before_drop_fns: SmallVec<[BeforeDropFn; 1]>,
34503504
rule: Rule,
34513505
}
34523506

@@ -3508,6 +3562,28 @@ impl DiagnosticGuard<'_, '_> {
35083562
let ann = self.primary_annotation_mut().unwrap();
35093563
ann.push_tag(tag);
35103564
}
3565+
3566+
/// Register a function to call before dropping the guard.
3567+
///
3568+
/// This is intended for use with the [`Checker::report_custom_diagnostic`]
3569+
/// or [`LintContext::report_custom_diagnostic`] methods in cases where a
3570+
/// fix title should be added as a `help` diagnostic after all other
3571+
/// sub-diagnostics. For example:
3572+
///
3573+
/// ```ignore
3574+
/// let (mut diagnostic, fix_title) = checker.report_custom_diagnostic(MyViolation, range);
3575+
/// diagnostic.before_drop(move |diag| {
3576+
/// if let Some(fix_title) = &fix_title {
3577+
/// diag.help(fix_title);
3578+
/// }
3579+
/// });
3580+
/// ```
3581+
pub(crate) fn before_drop<F>(&mut self, f: F)
3582+
where
3583+
F: Fn(&mut Diagnostic) + 'static,
3584+
{
3585+
self.before_drop_fns.push(Box::new(f));
3586+
}
35113587
}
35123588

35133589
impl DiagnosticGuard<'_, '_> {
@@ -3592,7 +3668,10 @@ impl Drop for DiagnosticGuard<'_, '_> {
35923668
return;
35933669
}
35943670

3595-
if let Some(diagnostic) = self.diagnostic.take() {
3671+
if let Some(mut diagnostic) = self.diagnostic.take() {
3672+
for f in &self.before_drop_fns {
3673+
f(&mut diagnostic);
3674+
}
35963675
self.context.diagnostics.borrow_mut().push(diagnostic);
35973676
}
35983677
}

crates/ruff_linter/src/rules/flake8_implicit_str_concat/rules/collection_literal.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,10 +90,15 @@ pub(crate) fn implicit_string_concatenation_in_collection_literal(
9090
continue;
9191
}
9292

93-
let mut diagnostic = checker.report_diagnostic(
93+
let (mut diagnostic, fix_title) = checker.report_custom_diagnostic(
9494
ImplicitStringConcatenationInCollectionLiteral,
9595
string_like.range(),
9696
);
97+
diagnostic.before_drop(move |diag| {
98+
if let Some(fix_title) = &fix_title {
99+
diag.help(fix_title);
100+
}
101+
});
97102
diagnostic.help("Did you forget a comma?");
98103
diagnostic.set_fix(Fix::unsafe_edits(
99104
Edit::insertion("(".to_string(), string_like.range().start()),

crates/ruff_linter/src/rules/flake8_implicit_str_concat/snapshots/ruff_linter__rules__flake8_implicit_str_concat__tests__ISC004_ISC004.py.snap

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ ISC004 [*] Unparenthesized implicit string concatenation in collection
1111
| |______________________________________________________________^
1212
6 | )
1313
|
14-
help: Wrap implicitly concatenated strings in parentheses
1514
help: Did you forget a comma?
15+
help: Wrap implicitly concatenated strings in parentheses
1616
1 | facts = (
1717
2 | "Lobsters have blue blood.",
1818
3 | "The liver is the only human organ that can fully regenerate itself.",
@@ -35,8 +35,8 @@ ISC004 [*] Unparenthesized implicit string concatenation in collection
3535
| |______________________________________________________________^
3636
13 | ]
3737
|
38-
help: Wrap implicitly concatenated strings in parentheses
3938
help: Did you forget a comma?
39+
help: Wrap implicitly concatenated strings in parentheses
4040
8 | facts = [
4141
9 | "Lobsters have blue blood.",
4242
10 | "The liver is the only human organ that can fully regenerate itself.",
@@ -59,8 +59,8 @@ ISC004 [*] Unparenthesized implicit string concatenation in collection
5959
| |______________________________________________________________^
6060
20 | }
6161
|
62-
help: Wrap implicitly concatenated strings in parentheses
6362
help: Did you forget a comma?
63+
help: Wrap implicitly concatenated strings in parentheses
6464
15 | facts = {
6565
16 | "Lobsters have blue blood.",
6666
17 | "The liver is the only human organ that can fully regenerate itself.",
@@ -83,8 +83,8 @@ ISC004 [*] Unparenthesized implicit string concatenation in collection
8383
| |_________________________^
8484
33 | )
8585
|
86-
help: Wrap implicitly concatenated strings in parentheses
8786
help: Did you forget a comma?
87+
help: Wrap implicitly concatenated strings in parentheses
8888
27 | }
8989
28 |
9090
29 | facts = (
@@ -108,8 +108,8 @@ ISC004 [*] Unparenthesized implicit string concatenation in collection
108108
| |_________________________^
109109
39 | ]
110110
|
111-
help: Wrap implicitly concatenated strings in parentheses
112111
help: Did you forget a comma?
112+
help: Wrap implicitly concatenated strings in parentheses
113113
33 | )
114114
34 |
115115
35 | facts = [
@@ -133,8 +133,8 @@ ISC004 [*] Unparenthesized implicit string concatenation in collection
133133
| |_________________________^
134134
45 | }
135135
|
136-
help: Wrap implicitly concatenated strings in parentheses
137136
help: Did you forget a comma?
137+
help: Wrap implicitly concatenated strings in parentheses
138138
39 | ]
139139
40 |
140140
41 | facts = {

crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,13 @@ pub(crate) fn non_octal_permissions(checker: &Checker, call: &ExprCall) {
111111
return;
112112
}
113113

114-
let mut diagnostic = checker.report_diagnostic(NonOctalPermissions, mode_arg.range());
114+
let (mut diagnostic, fix_title) =
115+
checker.report_custom_diagnostic(NonOctalPermissions, mode_arg.range());
116+
diagnostic.before_drop(move |diag| {
117+
if let Some(fix_title) = &fix_title {
118+
diag.help(fix_title);
119+
}
120+
});
115121

116122
let Some(mode) = int.as_u16() else {
117123
return;

0 commit comments

Comments
 (0)