Skip to content

Commit 6def73f

Browse files
committed
Append the fix title to a Diagnostic on drop
Summary -- As noted in #22972 (comment), attaching sub-diagnostics to Ruff's diagnostics can interact a bit awkwardly with how we attach the `Violation::fix_title` and then render a fix diff. In particular, the fix title is currently attached as a `help` sub-diagnostic when the diagnostic is originally created, meaning that any additional sub-diagnostics appear between the fix title and the rendered fix: ``` RUF064 [*] Non-octal mode --> RUF064.py:6:17 | 4 | from pathlib import Path 5 | 6 | os.chmod("foo", 444) # Error | ^^^ 7 | os.chmod("foo", 0o444) # OK 8 | os.chmod("foo", 7777) # Error | help: Replace with octal literal info: Current value of mode 444 (0o674) sets permissions: u=rw-, g=rwx, o=r--) info: Suggested value of 292 sets permissions: u=r--, g=r--, o=r-- 3 | import os 4 | from pathlib import Path 5 | - os.chmod("foo", 444) # Error 6 + os.chmod("foo", 0o444) # Error 7 | os.chmod("foo", 0o444) # OK 8 | os.chmod("foo", 7777) # Error 9 | os.chmod("foo", 10000) # Error note: This is an unsafe fix and may change runtime behavior ``` Instead of adding the fix title immediately, this PR stores it on our `DiagnosticGuard` type to be added just before the guard is dropped and the diagnostic is stored. I think a better long-term fix would be to attach the diff to the sub-diagnostic with the `help` message/fix title somehow and render these `info` sub-diagnostics _after_ the diff (or let the diagnostic author choose the order), but this seemed like an easy improvement over the current approach, at least. Test Plan -- Existing tests for ISC004 and from #22972 showing the `help` message at the end.
1 parent f1f21f7 commit 6def73f

5 files changed

Lines changed: 44 additions & 43 deletions

File tree

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

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3365,6 +3365,7 @@ impl<'a> LintContext<'a> {
33653365
) -> DiagnosticGuard<'chk, 'a> {
33663366
DiagnosticGuard {
33673367
context: self,
3368+
fix_title: kind.fix_title(),
33683369
diagnostic: Some(kind.into_diagnostic(range, &self.source_file)),
33693370
rule: T::rule(),
33703371
}
@@ -3384,6 +3385,7 @@ impl<'a> LintContext<'a> {
33843385
if self.is_rule_enabled(rule) {
33853386
Some(DiagnosticGuard {
33863387
context: self,
3388+
fix_title: kind.fix_title(),
33873389
diagnostic: Some(kind.into_diagnostic(range, &self.source_file)),
33883390
rule,
33893391
})
@@ -3447,6 +3449,11 @@ pub(crate) struct DiagnosticGuard<'a, 'b> {
34473449
///
34483450
/// This is always `Some` until the `Drop` (or `defuse`) call.
34493451
diagnostic: Option<Diagnostic>,
3452+
/// The optional help message to display.
3453+
///
3454+
/// This is stored here so that it can be added to the `Diagnostic` in the `Drop` impl and
3455+
/// render just above the diff, even if other sub-diagnostics have been added.
3456+
fix_title: Option<String>,
34503457
rule: Rule,
34513458
}
34523459

@@ -3592,7 +3599,10 @@ impl Drop for DiagnosticGuard<'_, '_> {
35923599
return;
35933600
}
35943601

3595-
if let Some(diagnostic) = self.diagnostic.take() {
3602+
if let Some(mut diagnostic) = self.diagnostic.take() {
3603+
if let Some(fix_title) = self.fix_title.take() {
3604+
diagnostic.help(fix_title);
3605+
}
35963606
self.context.diagnostics.borrow_mut().push(diagnostic);
35973607
}
35983608
}

crates/ruff_linter/src/message/mod.rs

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -75,10 +75,8 @@ pub fn create_panic_diagnostic(error: &PanicError, path: Option<&Path>) -> Diagn
7575
diagnostic
7676
}
7777

78-
#[expect(clippy::too_many_arguments)]
79-
pub fn create_lint_diagnostic<B, S>(
78+
pub fn create_lint_diagnostic<B>(
8079
body: B,
81-
suggestion: Option<S>,
8280
range: TextRange,
8381
fix: Option<Fix>,
8482
parent: Option<TextSize>,
@@ -88,7 +86,6 @@ pub fn create_lint_diagnostic<B, S>(
8886
) -> Diagnostic
8987
where
9088
B: Display,
91-
S: Display,
9289
{
9390
let mut diagnostic = Diagnostic::new(
9491
DiagnosticId::Lint(LintName::of(rule.into())),
@@ -108,10 +105,6 @@ where
108105
}
109106
diagnostic.annotate(annotation);
110107

111-
if let Some(suggestion) = suggestion {
112-
diagnostic.help(suggestion);
113-
}
114-
115108
if let Some(fix) = fix {
116109
diagnostic.set_fix(fix);
117110
}
@@ -278,9 +271,8 @@ def fibonacci(n):
278271
let fib_source = SourceFileBuilder::new("fib.py", fib).finish();
279272

280273
let unused_import_start = TextSize::from(7);
281-
let unused_import = create_lint_diagnostic(
274+
let mut unused_import = create_lint_diagnostic(
282275
"`os` imported but unused",
283-
Some("Remove unused import: `os`"),
284276
TextRange::new(unused_import_start, TextSize::from(9)),
285277
Some(Fix::unsafe_edit(Edit::range_deletion(TextRange::new(
286278
TextSize::from(0),
@@ -291,11 +283,11 @@ def fibonacci(n):
291283
Some(unused_import_start),
292284
Rule::UnusedImport,
293285
);
286+
unused_import.help("Remove unused import: `os`");
294287

295288
let unused_variable_start = TextSize::from(94);
296-
let unused_variable = create_lint_diagnostic(
289+
let mut unused_variable = create_lint_diagnostic(
297290
"Local variable `x` is assigned to but never used",
298-
Some("Remove assignment to unused variable `x`"),
299291
TextRange::new(unused_variable_start, TextSize::from(95)),
300292
Some(Fix::unsafe_edit(Edit::deletion(
301293
TextSize::from(94),
@@ -306,13 +298,13 @@ def fibonacci(n):
306298
Some(unused_variable_start),
307299
Rule::UnusedVariable,
308300
);
301+
unused_variable.help("Remove assignment to unused variable `x`");
309302

310303
let file_2 = r"if a == 1: pass";
311304

312305
let undefined_name_start = TextSize::from(3);
313306
let undefined_name = create_lint_diagnostic(
314307
"Undefined name `a`",
315-
Option::<&'static str>::None,
316308
TextRange::new(undefined_name_start, TextSize::from(4)),
317309
None,
318310
None,

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 = {

0 commit comments

Comments
 (0)