Render Azure, JSON, and JSON lines output with the new diagnostics by ntBre · Pull Request #19133 · astral-sh/ruff

ntBre · 2025-07-03T22:02:07Z

Summary

This was originally stacked on #19129, but some of the changes I made for JSON also impacted the Azure format, so I went ahead and combined them. The main changes here are:

Implementing FileResolver for Ruff's EmitterContext
Adding FileResolver::notebook_index and FileResolver::is_notebook methods
Adding a DisplayDiagnostics (with an "s") type for rendering a group of diagnostics at once
Adding Azure, Json, and JsonLines as new DiagnosticFormats

I tried a couple of alternatives to the FileResolver::notebook methods like passing down the NotebookIndex separately and trying to reparse a Notebook from Ruff's SourceFile. The latter seemed promising, but the SourceFile only stores the concatenated plain text of the notebook, not the re-parsable JSON. I guess the current version is just a variation on passing the NotebookIndex, but at least we can reuse the existing resolver argument. I think a lot of this can be cleaned up once Ruff has its own actual file resolver.

As suggested, I also tried deleting the corresponding Emitter files in ruff_linter, but it doesn't look like git was able to follow this as a rename. It did, however, track that the tests were moved, so the snapshots should be easy to review.

Test Plan

Existing Ruff tests ported to tests in ruff_db. I think some other existing ruff tests also cover parts of this refactor.

github-actions · 2025-07-03T22:05:38Z

`mypy_primer` results

No ecosystem changes detected ✅
No memory usage changes detected ✅

github-actions · 2025-07-03T22:21:57Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

This reverts commit 5d918e9.

ntBre · 2025-07-10T16:15:13Z

I introduced some intentional typos to see if any integration tests fail, and we have one for JSON (ruff::integration_test::stdin_json) but not for JSON lines or Azure. I'll write some CLI tests.

Summary -- I spun this off from #19133 to be sure to get an accurate baseline before modifying any of the formats. I picked the code snippet to include a lint diagnostic with a fix, one without a fix, and one syntax error. I'm happy to expand it if there are any other kinds we want to test. Test Plan -- New CLI tests

BurntSushi

Nice! Feel free to ignore/disagree with my nits about json!. :-)

BurntSushi · 2025-07-10T18:28:50Z

crates/ruff_db/src/diagnostic/render/json.rs

+            "end_location": location_to_json(end_location.unwrap_or_default()),
+            "filename": filename.unwrap_or_default(),
+            "noqa_row": noqa_location.map(|location| location.line)
+        })


Out of curiosity, how come the json! macro instead of types? I'd imagine json! to be slower since it has to shove everything into the serde_json::Value type, but I guess I'd also imagine it probably doesn't matter here.

I just pulled these over from the current Ruff version, so I'm not too sure. @MichaReiser might know, it looks like there were types deriving Serialize before #3895.

json! is kind of handy here for the preview behavior, but otherwise I'd lean toward types too.

I don't. Wasn't it already like this before my PR and all I did was splitting the code into different modules.

I'm generally in favor of structs. It makes accidental schema changes easier to spot

Ah yeah sorry, I didn't realize this was copied code. I thought it was newly written.

No worries, I should have pointed it out to help with the review. I opened #19270 to explore using structs.

BurntSushi · 2025-07-10T18:39:52Z

crates/ruff_db/src/diagnostic/render/json.rs

+
+        s.end()
+    }
+}


I feel like if you're using the json! macro everywhere already, it's probably not worth a hand-impl of the Serialize trait? There's a bit more ceremony with it compared to just building a Value::Array(Vec<Value>) directly.

Summary -- I spun this off from #19133 to be sure to get an accurate baseline before modifying any of the formats. I picked the code snippet to include a lint diagnostic with a fix, one without a fix, and one syntax error. I'm happy to expand it if there are any other kinds we want to test. I initially passed `CONTENT` on stdin, but I was a bit surprised to notice that some of our output formats include an absolute path to the file. I switched to a `TempDir` to use the `tempdir_filter`. Test Plan -- New CLI tests

## Summary See #19133 (comment) for recent discussion. This PR moves to using structs for the types in our JSON output format instead of the `json!` macro. I didn't rename any of the `message` references because that should be handled when rebasing #19133 onto this. My plan for handling the `preview` behavior with the new diagnostics is to use a wrapper enum. Something like: ```rust #[derive(Serialize)] #[serde(untagged)] pub(crate) enum JsonDiagnostic<'a> { Old(OldJsonDiagnostic<'a>), } #[derive(Serialize)] pub(crate) struct OldJsonDiagnostic<'a> { // ... } ``` Initially I thought I could use a `&dyn Serialize` for the affected fields, but I see that `Serialize` isn't dyn-compatible in testing this now. ## Test Plan Existing tests. One quirk of the new types is that their fields are in alphabetical order. I guess `json!` sorts the fields alphabetically? The tests were failing before I sorted the struct fields. ## Other formats It looks like the `rdjson`, `sarif`, and `gitlab` formats also use `json!`, so if we decide to merge this, I can do something similar for those before moving them to the new diagnostic format.

the remaining `message` fields are in the actual JSON schema and seem appropriate. they aren't referring to the old internal `Message` implemenation, that's just a reasonable field name for the diagnostic's main message.

ntBre · 2025-07-11T15:21:25Z

crates/ruff_db/src/diagnostic/render/json.rs

+        }
+    };
+
+    json!(value)


This feels a bit silly, but callers of this function expect a Display type. This avoids having to update the callers to use serde_json::to_writer, which in turn avoids updating their arguments from std::fmt::Formatters to something implementing std::io::Write and their return types to something that can accommodate a serde_json::Error instead of a std::fmt::Error.

We could also just call serde_json::to_value and unwrap it here or in the callers, which is all this macro expands into.

I would probably move the json! call to where you need the Display implementation.

ntBre · 2025-07-11T15:30:35Z

...c/diagnostic/render/snapshots/ruff_db__diagnostic__render__json__tests__notebook_output.snap

+            "column": 1,
+            "row": 5


This is actually correct based on testing a released version of Ruff against the real notebook. I think the manual notebook cell index was outdated.

MichaReiser · 2025-07-11T15:42:26Z

crates/ruff_db/src/diagnostic/render/json.rs

+enum JsonDiagnostic<'a> {
+    Old {
+        cell: Option<OneIndexed>,
+        code: Option<&'a SecondaryCode>,
+        end_location: JsonLocation,
+        filename: &'a str,
+        fix: Option<JsonFix<'a>>,
+        location: JsonLocation,
+        message: &'a str,
+        noqa_row: Option<OneIndexed>,
+        url: Option<String>,
+    },
+    New {
+        cell: Option<OneIndexed>,
+        code: Option<&'a SecondaryCode>,
+        end_location: Option<JsonLocation>,
+        filename: Option<&'a str>,
+        fix: Option<JsonFix<'a>>,
+        location: Option<JsonLocation>,
+        message: &'a str,
+        noqa_row: Option<OneIndexed>,
+        url: Option<String>,
+    },


Using an enum here strikes me a bit weird. I would either use two separate structs or just ignore the old entirely and ensure that the values are all Some where the change isn't backwards compatible.

Oh, for some reason just wrapping the unwrap_or_default calls in Some didn't occur to me. That makes a lot more sense. Thanks!

ntBre · 2025-07-11T18:50:26Z

I'm going to merge this and start working on more output formats, happy to follow up if I missed anything. Thanks for the reviews!

## Summary Another output format like #19133. This is the [reviewdog](https://github.com/reviewdog/reviewdog) output format, which is somewhat similar to regular JSON. Like #19270, in the first commit I converted from using `json!` to `Serialize` structs, then in the second commit I moved the module to `ruff_db`. The reviewdog [schema](https://github.com/reviewdog/reviewdog/blob/320a8e73a94a09248044314d8ca326a6cd710692/proto/rdf/jsonschema/DiagnosticResult.json) seems a bit more flexible than our JSON schema, so I'm not sure if we need any preview checks here. I'll flag the places I wasn't sure about as review comments. ## Test Plan New tests in `rdjson.rs`, ported from the old `rjdson.rs` module, as well as the new CLI output tests. --------- Co-authored-by: Micha Reiser <[email protected]>

ntBre added ty Multi-file analysis & type inference diagnostics Related to reporting of diagnostics. labels Jul 3, 2025

ntBre force-pushed the brent/json-rendering branch from 6601b4c to 200c1a7 Compare July 4, 2025 16:03

ntBre added 21 commits July 4, 2025 12:04

add DummyFileResolver for ruff

ddba240

port azure output format

8bc5b71

update schema

c885225

use severity and report diagnostics even without a span or range

28c27fa

add ty cli test

232efc3

suppress summary messages for structured formats

6917667

render json, except for notebooks

0021a7e

move json module to its own file

4ccc7e3

give up and pass down notebook_indexes

305075c

add serde feature for ruff_diagnostics

2f730a5

Revert "give up and pass down notebook_indexes"

af7137d

This reverts commit 5d918e9.

implement FileResolver for EmitterContext instead of separate type

7547863

add FileResolver::notebook_index and pass JSON tests

bcab772

update schema

9648bee

add JsonLines support

ae467c3

add json and json-lines to cli, add cli tests

c67a898

update Diagnostic::to_url for ty docs

8a9a1bf

LazyLock homepage replacement

99bfb55

add FileResolver::is_notebook, restore fallback location for azure

70b226f

use DisplayDiagnostics in MainLoop, fix JSON snapshot

8c905be

update option docs

c3e3077

ntBre force-pushed the brent/json-rendering branch from 200c1a7 to c3e3077 Compare July 4, 2025 16:04

ntBre changed the base branch from brent/diagnostic-rendering to main July 4, 2025 16:05

ntBre changed the title ~~[WIP] Render JSON output with the new diagnostics~~ Render Azure, JSON, and JSON lines output with the new diagnostics Jul 4, 2025

ntBre marked this pull request as ready for review July 4, 2025 16:19

ntBre mentioned this pull request Jul 10, 2025

Stabilize the new output formats #19263

Open

ntBre added 4 commits July 10, 2025 12:04

fix missing serde feature for json imports

fe375db

replace some serde gates with temporary #[allow(dead_code)]

38fb7d8

fix serde feature for ruff_diagnostics

6dbfd61

delete unused From impl

6c470c2

use a real notebook, revert related changes to TestEnvironment

32f485d

ntBre mentioned this pull request Jul 10, 2025

Add simple integration tests for all output formats #19265

Merged

BurntSushi approved these changes Jul 10, 2025

View reviewed changes

ntBre mentioned this pull request Jul 10, 2025

Use structs for JSON serialization #19270

Merged

Merge branch 'main' into brent/json-rendering

a3509f3

ntBre added 3 commits July 11, 2025 11:06

Merge branch 'main' into brent/json-rendering

e7d2080

message -> diagnostic

fbf339f

remove last message variable name

a30f772

the remaining `message` fields are in the actual JSON schema and seem appropriate. they aren't referring to the old internal `Message` implemenation, that's just a reasonable field name for the diagnostic's main message.

ntBre commented Jul 11, 2025

View reviewed changes

ntBre added 2 commits July 11, 2025 11:41

clean up some docs

950e256

revert lifetimes that ended up unused

c0e453c

MichaReiser reviewed Jul 11, 2025

View reviewed changes

ntBre added 2 commits July 11, 2025 11:53

avoid confusing enums

b4ad384

move json! to caller

880a9a5

ntBre merged commit b5c5f71 into main Jul 11, 2025
37 checks passed

ntBre deleted the brent/json-rendering branch July 11, 2025 19:04

ntBre mentioned this pull request Jul 11, 2025

Move RDJSON rendering to ruff_db #19293

Merged

ntBre mentioned this pull request Sep 11, 2025

[ty] Add GitHub output format #20358

Merged

Comments

Conversation

ntBre commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mypy_primer results

Uh oh!

github-actions bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

ntBre commented Jul 10, 2025

Uh oh!

BurntSushi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntBre commented Jul 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ntBre commented Jul 3, 2025 •

edited

Loading

github-actions bot commented Jul 3, 2025 •

edited

Loading

`mypy_primer` results

github-actions bot commented Jul 3, 2025 •

edited

Loading

`ruff-ecosystem` results