Skip to content

Append the fix title to a Diagnostic on drop#23043

Closed
ntBre wants to merge 1 commit intomainfrom
brent/fix-title
Closed

Append the fix title to a Diagnostic on drop#23043
ntBre wants to merge 1 commit intomainfrom
brent/fix-title

Conversation

@ntBre
Copy link
Contributor

@ntBre ntBre commented Feb 2, 2026

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.

I also remembered that @MichaReiser and I had discussed this a bit before and found the comment. Micha's suggestion was to add something like a Checker::report_custom_diagnostic method that leaves out the fix title, expecting the diagnostic author to attach it later. I also tried this approach but found it a bit awkward, at least with the current version of RUF064. For example, RUF064 has several early returns intermixed with the info additions, so you have to be sure to attach the help in each of those cases. Then I tried to get really cute with a defer method that would run a closure before drop so that you could do something like this:

let mut diagnostic = checker.report_custom_diagnostic(...);
diagnostic.defer(|diag| diag.help(...));

but that also became a bit painful with all of the generic/lifetime changes. Maybe I picked a bad type for the deferred function field, though. Anyway, I think the approach in this PR is a reasonable improvement for now.

Test Plan

Existing tests for ISC004 and from #22972 showing the help message at the end.

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.
@AlexWaygood AlexWaygood added the diagnostics Related to reporting of diagnostics. label Feb 2, 2026
ntBre added a commit that referenced this pull request Feb 2, 2026
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`.
ntBre added a commit that referenced this pull request Feb 2, 2026
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`.
@astral-sh-bot
Copy link

astral-sh-bot bot commented Feb 3, 2026

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@ntBre
Copy link
Contributor Author

ntBre commented Feb 3, 2026

Ahhh, these test failures are showing a case where we actually want the info diagnostic second, so simply attaching the fix title last isn't going to work, even for our few existing sub-diagnostics. I'll close this in favor of #23044.

@ntBre ntBre closed this Feb 3, 2026
ntBre added a commit that referenced this pull request Feb 3, 2026
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 closure
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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

diagnostics Related to reporting of diagnostics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants