mirror of
https://github.com/astral-sh/ruff.git
synced 2025-10-29 19:17:20 +00:00
Display diffs for ruff format --check and add support for different output formats (#20443)
## Summary This PR uses the new `Diagnostic` type for rendering formatter diagnostics. This allows the formatter to inherit all of the output formats already implemented in the linter and ty. For example, here's the new `full` output format, with the formatting diff displayed using the same infrastructure as the linter: <img width="592" height="364" alt="image" src="https://github.com/user-attachments/assets/6d09817d-3f27-4960-aa8b-41ba47fb4dc0" /> <details><summary>Resolved TODOs</summary> <p> ~~There are several limitiations/todos here still, especially around the `OutputFormat` type~~: - [x] A few literal `todo!`s for the remaining `OutputFormat`s without matching `DiagnosticFormat`s - [x] The default output format is `full` instead of something more concise like the current output - [x] Some of the output formats (namely JSON) have information that doesn't make much sense for these diagnostics The first of these is definitely resolved, and I think the other two are as well, based on discussion on the design document. In brief, we're okay inheriting the default `OutputFormat` and can separate the global option into `lint.output-format` and `format.output-format` in the future, if needed; and we're okay including redundant information in the non-human-readable output formats. My last major concern is with the performance of the new code, as discussed in the `Benchmarks` section below. A smaller question is whether we should use `Diagnostic`s for formatting errors too. I think the answer to this is yes, in line with changes we're making in the linter too. I still need to implement that here. </p> </details> <details><summary>Benchmarks</summary> <p> The values in the table are from a large benchmark on the CPython 3.10 code base, which involves checking 2011 files, 1872 of which need to be reformatted. `stable` corresponds to the same code used on `main`, while `preview-full` and `preview-concise` use the new `Diagnostic` code gated behind `--preview` for the `full` and `concise` output formats, respectively. `stable-diff` uses the `--diff` to compare the two diff rendering approaches. See the full hyperfine command below for more details. For a sense of scale, the `stable` output format produces 1873 lines on stdout, compared to 855,278 for `preview-full` and 857,798 for `stable-diff`. | Command | Mean [ms] | Min [ms] | Max [ms] | Relative | |:------------------|--------------:|---------:|---------:|-------------:| | `stable` | 201.2 ± 6.8 | 192.9 | 220.6 | 1.00 | | `preview-full` | 9113.2 ± 31.2 | 9076.1 | 9152.0 | 45.29 ± 1.54 | | `preview-concise` | 214.2 ± 1.4 | 212.0 | 217.6 | 1.06 ± 0.04 | | `stable-diff` | 3308.6 ± 20.2 | 3278.6 | 3341.8 | 16.44 ± 0.56 | In summary, the `preview-concise` diagnostics are ~6% slower than the stable output format, increasing the average runtime from 201.2 ms to 214.2 ms. The `full` preview diagnostics are much more expensive, taking over 9113.2 ms to complete, which is ~3x more expensive even than the stable diffs produced by the `--diff` flag. My main takeaways here are: 1. Rendering `Edit`s is much more expensive than rendering the diffs from `--diff` 2. Constructing `Edit`s actually isn't too bad ### Constructing `Edit`s I also took a closer look at `Edit` construction by modifying the code and repeating the `preview-concise` benchmark and found that the main issue is constructing a `SourceFile` for use in the `Edit` rendering. Commenting out the `Edit` construction itself has basically no effect: | Command | Mean [ms] | Min [ms] | Max [ms] | Relative | |:----------|------------:|---------:|---------:|------------:| | `stable` | 197.5 ± 1.6 | 195.0 | 200.3 | 1.00 | | `no-edit` | 208.9 ± 2.2 | 204.8 | 212.2 | 1.06 ± 0.01 | However, also omitting the source text from the `SourceFile` construction resolves the slowdown compared to `stable`. So it seems that copying the full source text into a `SourceFile` is the main cause of the slowdown for non-`full` diagnostics. | Command | Mean [ms] | Min [ms] | Max [ms] | Relative | |:-----------------|------------:|---------:|---------:|------------:| | `stable` | 202.4 ± 2.9 | 197.6 | 207.9 | 1.00 | | `no-source-text` | 202.7 ± 3.3 | 196.3 | 209.1 | 1.00 ± 0.02 | ### Rendering diffs The main difference between `stable-diff` and `preview-full` seems to be the diffing strategy we use from `similar`. Both versions use the same algorithm, but in the existing [`CodeDiff`](https://github.com/astral-sh/ruff/blob/main/crates/ruff_linter/src/source_kind.rs#L259) rendering for the `--diff` flag, we only do line-level diffing, whereas for `Diagnostic`s we use `TextDiff::iter_inline_changes` to highlight word-level changes too. Skipping the word diff for `Diagnostic`s closes most of the gap: | Command | Mean [s] | Min [s] | Max [s] | Relative | |:---|---:|---:|---:|---:| | `stable-diff` | 3.323 ± 0.015 | 3.297 | 3.341 | 1.00 | | `preview-full` | 3.654 ± 0.019 | 3.618 | 3.682 | 1.10 ± 0.01 | (In some repeated runs, I've seen as small as a ~5% difference, down from 10% in the table) This doesn't actually change any of our snapshots, but it would obviously change the rendered result in a terminal since we wouldn't highlight the specific words that changed within a line. Another much smaller change that we can try is removing the deadline from the `iter_inline_changes` call. It looks like there's a fair amount of overhead from the default 500 ms deadline for computing these, and using `iter_inline_changes(op, None)` (`None` for the optional deadline argument) improves the runtime quite a bit: | Command | Mean [s] | Min [s] | Max [s] | Relative | |:---|---:|---:|---:|---:| | `stable-diff` | 3.322 ± 0.013 | 3.298 | 3.341 | 1.00 | | `preview-full` | 5.296 ± 0.030 | 5.251 | 5.366 | 1.59 ± 0.01 | <hr> <details><summary>hyperfine command</summary> ```shell cargo build --release --bin ruff && hyperfine --ignore-failure --warmup 10 --export-markdown /tmp/table.md \ -n stable -n preview-full -n preview-concise -n stable-diff \ "./target/release/ruff format --check ./crates/ruff_linter/resources/test/cpython/ --no-cache" \ "./target/release/ruff format --check ./crates/ruff_linter/resources/test/cpython/ --no-cache --preview --output-format=full" \ "./target/release/ruff format --check ./crates/ruff_linter/resources/test/cpython/ --no-cache --preview --output-format=concise" \ "./target/release/ruff format --check ./crates/ruff_linter/resources/test/cpython/ --no-cache --diff" ``` </details> </p> </details> ## Test Plan Some new CLI tests and manual testing
This commit is contained in:
parent
b483d3b0b9
commit
2b1d3c60fa
32 changed files with 1226 additions and 134 deletions
|
|
@ -69,6 +69,7 @@ impl Diagnostic {
|
|||
parent: None,
|
||||
noqa_offset: None,
|
||||
secondary_code: None,
|
||||
header_offset: 0,
|
||||
});
|
||||
Diagnostic { inner }
|
||||
}
|
||||
|
|
@ -432,14 +433,23 @@ impl Diagnostic {
|
|||
|
||||
/// Returns the URL for the rule documentation, if it exists.
|
||||
pub fn to_ruff_url(&self) -> Option<String> {
|
||||
if self.is_invalid_syntax() {
|
||||
None
|
||||
} else {
|
||||
Some(format!(
|
||||
"{}/rules/{}",
|
||||
env!("CARGO_PKG_HOMEPAGE"),
|
||||
self.name()
|
||||
))
|
||||
match self.id() {
|
||||
DiagnosticId::Panic
|
||||
| DiagnosticId::Io
|
||||
| DiagnosticId::InvalidSyntax
|
||||
| DiagnosticId::RevealedType
|
||||
| DiagnosticId::UnknownRule
|
||||
| DiagnosticId::InvalidGlob
|
||||
| DiagnosticId::EmptyInclude
|
||||
| DiagnosticId::UnnecessaryOverridesSection
|
||||
| DiagnosticId::UselessOverridesSection
|
||||
| DiagnosticId::DeprecatedSetting
|
||||
| DiagnosticId::Unformatted
|
||||
| DiagnosticId::InvalidCliOption
|
||||
| DiagnosticId::InternalError => None,
|
||||
DiagnosticId::Lint(lint_name) => {
|
||||
Some(format!("{}/rules/{lint_name}", env!("CARGO_PKG_HOMEPAGE")))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -512,6 +522,11 @@ impl Diagnostic {
|
|||
|
||||
a.cmp(&b)
|
||||
}
|
||||
|
||||
/// Add an offset for aligning the header sigil with the line number separators in a diff.
|
||||
pub fn set_header_offset(&mut self, offset: usize) {
|
||||
Arc::make_mut(&mut self.inner).header_offset = offset;
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Eq, PartialEq, Hash, get_size2::GetSize)]
|
||||
|
|
@ -525,6 +540,7 @@ struct DiagnosticInner {
|
|||
parent: Option<TextSize>,
|
||||
noqa_offset: Option<TextSize>,
|
||||
secondary_code: Option<SecondaryCode>,
|
||||
header_offset: usize,
|
||||
}
|
||||
|
||||
struct RenderingSortKey<'a> {
|
||||
|
|
@ -742,11 +758,11 @@ pub struct Annotation {
|
|||
is_primary: bool,
|
||||
/// The diagnostic tags associated with this annotation.
|
||||
tags: Vec<DiagnosticTag>,
|
||||
/// Whether this annotation is a file-level or full-file annotation.
|
||||
/// Whether the snippet for this annotation should be hidden.
|
||||
///
|
||||
/// When set, rendering will only include the file's name and (optional) range. Everything else
|
||||
/// is omitted, including any file snippet or message.
|
||||
is_file_level: bool,
|
||||
hide_snippet: bool,
|
||||
}
|
||||
|
||||
impl Annotation {
|
||||
|
|
@ -765,7 +781,7 @@ impl Annotation {
|
|||
message: None,
|
||||
is_primary: true,
|
||||
tags: Vec::new(),
|
||||
is_file_level: false,
|
||||
hide_snippet: false,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -782,7 +798,7 @@ impl Annotation {
|
|||
message: None,
|
||||
is_primary: false,
|
||||
tags: Vec::new(),
|
||||
is_file_level: false,
|
||||
hide_snippet: false,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -849,19 +865,20 @@ impl Annotation {
|
|||
self.tags.push(tag);
|
||||
}
|
||||
|
||||
/// Set whether or not this annotation is file-level.
|
||||
/// Set whether or not the snippet on this annotation should be suppressed when rendering.
|
||||
///
|
||||
/// File-level annotations are only rendered with their file name and range, if available. This
|
||||
/// is intended for backwards compatibility with Ruff diagnostics, which historically used
|
||||
/// Such annotations are only rendered with their file name and range, if available. This is
|
||||
/// intended for backwards compatibility with Ruff diagnostics, which historically used
|
||||
/// `TextRange::default` to indicate a file-level diagnostic. In the new diagnostic model, a
|
||||
/// [`Span`] with a range of `None` should be used instead, as mentioned in the `Span`
|
||||
/// documentation.
|
||||
///
|
||||
/// TODO(brent) update this usage in Ruff and remove `is_file_level` entirely. See
|
||||
/// <https://github.com/astral-sh/ruff/issues/19688>, especially my first comment, for more
|
||||
/// details.
|
||||
pub fn set_file_level(&mut self, yes: bool) {
|
||||
self.is_file_level = yes;
|
||||
/// details. As of 2025-09-26 we also use this to suppress snippet rendering for formatter
|
||||
/// diagnostics, which also need to have a range, so we probably can't eliminate this entirely.
|
||||
pub fn hide_snippet(&mut self, yes: bool) {
|
||||
self.hide_snippet = yes;
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -1016,6 +1033,17 @@ pub enum DiagnosticId {
|
|||
|
||||
/// Use of a deprecated setting.
|
||||
DeprecatedSetting,
|
||||
|
||||
/// The code needs to be formatted.
|
||||
Unformatted,
|
||||
|
||||
/// Use of an invalid command-line option.
|
||||
InvalidCliOption,
|
||||
|
||||
/// An internal assumption was violated.
|
||||
///
|
||||
/// This indicates a bug in the program rather than a user error.
|
||||
InternalError,
|
||||
}
|
||||
|
||||
impl DiagnosticId {
|
||||
|
|
@ -1055,6 +1083,9 @@ impl DiagnosticId {
|
|||
DiagnosticId::UnnecessaryOverridesSection => "unnecessary-overrides-section",
|
||||
DiagnosticId::UselessOverridesSection => "useless-overrides-section",
|
||||
DiagnosticId::DeprecatedSetting => "deprecated-setting",
|
||||
DiagnosticId::Unformatted => "unformatted",
|
||||
DiagnosticId::InvalidCliOption => "invalid-cli-option",
|
||||
DiagnosticId::InternalError => "internal-error",
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -208,6 +208,7 @@ struct ResolvedDiagnostic<'a> {
|
|||
message: String,
|
||||
annotations: Vec<ResolvedAnnotation<'a>>,
|
||||
is_fixable: bool,
|
||||
header_offset: usize,
|
||||
}
|
||||
|
||||
impl<'a> ResolvedDiagnostic<'a> {
|
||||
|
|
@ -258,7 +259,8 @@ impl<'a> ResolvedDiagnostic<'a> {
|
|||
id,
|
||||
message: diag.inner.message.as_str().to_string(),
|
||||
annotations,
|
||||
is_fixable: diag.has_applicable_fix(config),
|
||||
is_fixable: config.show_fix_status && diag.has_applicable_fix(config),
|
||||
header_offset: diag.inner.header_offset,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -288,6 +290,7 @@ impl<'a> ResolvedDiagnostic<'a> {
|
|||
message: diag.inner.message.as_str().to_string(),
|
||||
annotations,
|
||||
is_fixable: false,
|
||||
header_offset: 0,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -385,6 +388,7 @@ impl<'a> ResolvedDiagnostic<'a> {
|
|||
message: &self.message,
|
||||
snippets_by_input,
|
||||
is_fixable: self.is_fixable,
|
||||
header_offset: self.header_offset,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -404,7 +408,7 @@ struct ResolvedAnnotation<'a> {
|
|||
line_end: OneIndexed,
|
||||
message: Option<&'a str>,
|
||||
is_primary: bool,
|
||||
is_file_level: bool,
|
||||
hide_snippet: bool,
|
||||
notebook_index: Option<NotebookIndex>,
|
||||
}
|
||||
|
||||
|
|
@ -452,7 +456,7 @@ impl<'a> ResolvedAnnotation<'a> {
|
|||
line_end,
|
||||
message: ann.get_message(),
|
||||
is_primary: ann.is_primary,
|
||||
is_file_level: ann.is_file_level,
|
||||
hide_snippet: ann.hide_snippet,
|
||||
notebook_index: resolver.notebook_index(&ann.span.file),
|
||||
})
|
||||
}
|
||||
|
|
@ -492,6 +496,11 @@ struct RenderableDiagnostic<'r> {
|
|||
///
|
||||
/// This is rendered as a `[*]` indicator after the diagnostic ID.
|
||||
is_fixable: bool,
|
||||
/// Offset to align the header sigil (`-->`) with the subsequent line number separators.
|
||||
///
|
||||
/// This is only needed for formatter diagnostics where we don't render a snippet via
|
||||
/// `annotate-snippets` and thus the alignment isn't computed automatically.
|
||||
header_offset: usize,
|
||||
}
|
||||
|
||||
impl RenderableDiagnostic<'_> {
|
||||
|
|
@ -504,7 +513,11 @@ impl RenderableDiagnostic<'_> {
|
|||
.iter()
|
||||
.map(|snippet| snippet.to_annotate(path))
|
||||
});
|
||||
let mut message = self.level.title(self.message).is_fixable(self.is_fixable);
|
||||
let mut message = self
|
||||
.level
|
||||
.title(self.message)
|
||||
.is_fixable(self.is_fixable)
|
||||
.lineno_offset(self.header_offset);
|
||||
if let Some(id) = self.id {
|
||||
message = message.id(id);
|
||||
}
|
||||
|
|
@ -709,8 +722,8 @@ struct RenderableAnnotation<'r> {
|
|||
message: Option<&'r str>,
|
||||
/// Whether this annotation is considered "primary" or not.
|
||||
is_primary: bool,
|
||||
/// Whether this annotation applies to an entire file, rather than a snippet within it.
|
||||
is_file_level: bool,
|
||||
/// Whether the snippet for this annotation should be hidden instead of rendered.
|
||||
hide_snippet: bool,
|
||||
}
|
||||
|
||||
impl<'r> RenderableAnnotation<'r> {
|
||||
|
|
@ -732,7 +745,7 @@ impl<'r> RenderableAnnotation<'r> {
|
|||
range,
|
||||
message: ann.message,
|
||||
is_primary: ann.is_primary,
|
||||
is_file_level: ann.is_file_level,
|
||||
hide_snippet: ann.hide_snippet,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -758,7 +771,7 @@ impl<'r> RenderableAnnotation<'r> {
|
|||
if let Some(message) = self.message {
|
||||
ann = ann.label(message);
|
||||
}
|
||||
ann.is_file_level(self.is_file_level)
|
||||
ann.hide_snippet(self.hide_snippet)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -366,6 +366,7 @@ mod tests {
|
|||
fn hide_severity_output() {
|
||||
let (mut env, diagnostics) = create_diagnostics(DiagnosticFormat::Full);
|
||||
env.hide_severity(true);
|
||||
env.show_fix_status(true);
|
||||
env.fix_applicability(Applicability::DisplayOnly);
|
||||
|
||||
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r#"
|
||||
|
|
@ -572,7 +573,7 @@ print()
|
|||
let mut diagnostic = env.err().build();
|
||||
let span = env.path("example.py").with_range(TextRange::default());
|
||||
let mut annotation = Annotation::primary(span);
|
||||
annotation.set_file_level(true);
|
||||
annotation.hide_snippet(true);
|
||||
diagnostic.annotate(annotation);
|
||||
|
||||
insta::assert_snapshot!(env.render(&diagnostic), @r"
|
||||
|
|
@ -584,7 +585,8 @@ print()
|
|||
/// Check that ranges in notebooks are remapped relative to the cells.
|
||||
#[test]
|
||||
fn notebook_output() {
|
||||
let (env, diagnostics) = create_notebook_diagnostics(DiagnosticFormat::Full);
|
||||
let (mut env, diagnostics) = create_notebook_diagnostics(DiagnosticFormat::Full);
|
||||
env.show_fix_status(true);
|
||||
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
|
||||
error[unused-import][*]: `os` imported but unused
|
||||
--> notebook.ipynb:cell 1:2:8
|
||||
|
|
@ -698,6 +700,7 @@ print()
|
|||
fn notebook_output_with_diff() {
|
||||
let (mut env, diagnostics) = create_notebook_diagnostics(DiagnosticFormat::Full);
|
||||
env.show_fix_diff(true);
|
||||
env.show_fix_status(true);
|
||||
env.fix_applicability(Applicability::DisplayOnly);
|
||||
|
||||
insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
|
||||
|
|
@ -752,6 +755,7 @@ print()
|
|||
fn notebook_output_with_diff_spanning_cells() {
|
||||
let (mut env, mut diagnostics) = create_notebook_diagnostics(DiagnosticFormat::Full);
|
||||
env.show_fix_diff(true);
|
||||
env.show_fix_status(true);
|
||||
env.fix_applicability(Applicability::DisplayOnly);
|
||||
|
||||
// Move all of the edits from the later diagnostics to the first diagnostic to simulate a
|
||||
|
|
@ -928,6 +932,7 @@ line 10
|
|||
env.add("example.py", contents);
|
||||
env.format(DiagnosticFormat::Full);
|
||||
env.show_fix_diff(true);
|
||||
env.show_fix_status(true);
|
||||
env.fix_applicability(Applicability::DisplayOnly);
|
||||
|
||||
let mut diagnostic = env.err().primary("example.py", "3", "3", "label").build();
|
||||
|
|
|
|||
|
|
@ -31,6 +31,29 @@ impl Payload {
|
|||
}
|
||||
}
|
||||
|
||||
impl PanicError {
|
||||
pub fn to_diagnostic_message(&self, path: Option<impl std::fmt::Display>) -> String {
|
||||
use std::fmt::Write;
|
||||
|
||||
let mut message = String::new();
|
||||
message.push_str("Panicked");
|
||||
|
||||
if let Some(location) = &self.location {
|
||||
let _ = write!(&mut message, " at {location}");
|
||||
}
|
||||
|
||||
if let Some(path) = path {
|
||||
let _ = write!(&mut message, " when checking `{path}`");
|
||||
}
|
||||
|
||||
if let Some(payload) = self.payload.as_str() {
|
||||
let _ = write!(&mut message, ": `{payload}`");
|
||||
}
|
||||
|
||||
message
|
||||
}
|
||||
}
|
||||
|
||||
impl std::fmt::Display for PanicError {
|
||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
write!(f, "panicked at")?;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue