Move RDJSON rendering to `ruff_db` #19293

ntBre · 2025-07-11T21:19:59Z

Summary

Another output format like #19133. This is the 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 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.

github-actions · 2025-07-11T21:30:33Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

ntBre · 2025-07-11T21:22:04Z

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

+    // The schema says this is optional, so we may be able to skip the preview check.
+    if range.is_none() && !config.preview {
+        range = Some(RdjsonRange::default());
+    }


This is the main preview check I think we could skip, based on this definition of Location:

https://github.com/reviewdog/reviewdog/blob/320a8e73a94a09248044314d8ca326a6cd710692/proto/rdf/jsonschema/DiagnosticResult.json#L126-L141

As I mention in the module comment, only the description field says "Optional," not the type field.

I suggest using a schema validator to validate our understanding, e.g. https://www.jsonschemavalidator.net/

A null range is invalid according to the validator, so this should just be unwrap_or_default irrespective of preview.

Actually, completely omitting it from the JSON also passes the validator, so I guess another option is skipping serialization if it's None.

That makes sense, we should skip it then (which should be easy enough with serde)

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

ntBre · 2025-07-11T21:30:04Z

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

+        message: diagnostic.body(),
+        location: RdjsonLocation { path, range },
+        code: RdjsonCode {
+            value: diagnostic.secondary_code(),


This one is actually not optional in the schema, but this is consistent with main. I guess we may need to unwrap_or_default here. The same might be true for url. Both are currently null for syntax errors:

ruff/crates/ruff/tests/snapshots/lint__output_format_rdjson.snap

Lines 77 to 80 in 7154b64

"code": {

"url": null,

"value": null

},

My suggestion here is to use the lint id if the secondary code is missing.

Good idea, updated! The validator is also complaining about the URL. I guess I'll just put the CARGO_PKG_HOMEPAGE if there's no rule-specific page available.

ntBre · 2025-07-11T21:37:57Z

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

+                name: "ruff",
+                url: env!("CARGO_PKG_HOMEPAGE"),
+            },
+            severity: "warning",


This is also consistent with main. We could use our real severity here, but it would change the default from warning to error for existing Ruff diagnostics.

I suggest making this change under preview

I just realized that this is a single severity for the whole collection. I guess we can use the max severity in preview.

There's also a severity field on individual diagnostics that we aren't using. I'll try that in the schema validator too.

this being optional was violating the schema. see https://www.jsonschemavalidator.net/s/uvGWl3nq

MichaReiser · 2025-07-13T14:38:09Z

@ntBre is this ready for re-review or are there more changes that you plan on making (I have no particular reason to believe that there's anything that needs changing. I just want to avoid taking a look if you plan on making changes to due the schema discussion we had)

ntBre · 2025-07-13T14:44:24Z

Yes it's ready again, I just added your suggestion about skipping the range instead of using a default.

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

MichaReiser · 2025-07-14T07:20:40Z

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

+    // The schema does _not_ say this is optional, so always unwrap to a default value.
+    let path = span
+        .map(|span| span.file().path(resolver))
+        .unwrap_or_default();


location is also an additional property. Can you verify if the JSON is valid if you omit the location?

If it can be omitted, we then shouldn't call back to unwrap_or_default and instead omit the location when a file is missing (a diagnostic should never have just a range because a range without a file is not very useful)

Yeah it's valid to omit the location. I'll do that instead.

MichaReiser · 2025-07-14T07:22:05Z

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

+        .map(|span| span.file().path(resolver))
+        .unwrap_or_default();
+
+    let severity = if config.preview {


I think we should keep setting WARNING for all diagnostics until we officially support a WARNING severity.

We weren't setting the per-diagnostic severity at all before. Would it be better to go back to that, or to set them all to WARNING?

I would probably remove it for now. We can add the severity when introducing a warning severity to ruff

MichaReiser · 2025-07-14T07:28:50Z

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

+            url: diagnostic
+                .to_ruff_url()
+                .unwrap_or_else(|| env!("CARGO_PKG_HOMEPAGE").to_string()),


I'm inclined to go with what they have in ther protobuf implementation where, as far as I can understand, it's valid to omit the url property when empty

https://github.com/reviewdog/reviewdog/blob/294118b12c6b85c28d21deb0458703cc7a878608/proto/rdf/reviewdog.pb.go#L660-L669

I'm inclined to go with that for now (the same with the range on location)

Even their own tests don't specify a url: https://github.com/reviewdog/reviewdog/blob/master/parser/rdjson_test.go

MichaReiser · 2025-07-14T07:30:23Z

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

+fn rdjson_suggestions<'a>(
+    edits: &'a [Edit],
+    source_code: Option<SourceCode>,
+) -> Vec<RdjsonSuggestion<'a>> {


I think we should add an assertion to Diagnostic::set_fix instead that asserts that it has an associated source files. Edits without a source file aren't very useful.

Added a debug_assert! and then updated some tests that were actually setting fixes on diagnostics without files. Happy to upgrade to assert if you prefer.

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

MichaReiser · 2025-07-14T07:37:37Z

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

+    // TODO(brent) this can be required after it's out of preview.
+    #[serde(skip_serializing_if = "Option::is_none")]


I'm not sure what the benefit is of setting the severity at the file level. I'm also inclined to just keep this None until we add a WARNING severity. Adding severities feels like an unrelated change

Ah okay, I'll revert the per-diagnostic severity and just keep WARNING as the global severity. I agree that this feels unrelated to the rest of the refactor.

ntBre · 2025-07-14T14:39:52Z

I think this should be ready for another look. The main thing I'm unsure about is the debug_assert in Diagnostic::set_fix combined with the runtime unwrap in rdjson_suggestions. Should they both be debug_assert or both full assert/unwrap?

MichaReiser

Thank you.

MichaReiser · 2025-07-14T15:35:37Z

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

-              ],
-              "message": null
-            },
+            "fix": null,


Do we have any other test for edits? If not, I think it's worth preserving testing fixes in some test or another.

Yes, the output and notebook_output unit tests and the CLI integration tests all have fixes/edits.

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

MichaReiser · 2025-07-14T16:22:31Z

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

+    let location = span
+        .map(|span| span.file().path(resolver))
+        .map(|path| RdjsonLocation { path, range });


Nit: Could we use source_code.name() here? It makes it clearer that range depends on the same value

At the risk of missing something obvious here, I don't see a SourceCode::name method. I think the SourceFile has the name but not the SourceCode. The Span seems like the common ancestor for both range and location, unless I factored out a shared span.file() call.

Oh, it's SourceCode and not SourceFile...

I would rewrite this to

let source_file = span.map(|span| { let file = span.file(); (file.path(resolver), file.diagnostic_source(resolver)) }); let location = source_file.as_ref().map(|(path, source)| { let range = diagnostic.range().map(|range| { let source_code = source.as_source_code(); let start = source_code.line_column(range.start()); let end = source_code.line_column(range.end()); RdjsonRange::new(start, end) }); RdjsonLocation { path, range } }); let edits = diagnostic.fix().map(Fix::edits).unwrap_or_default(); RdjsonDiagnostic { message: diagnostic.body(), location, code: RdjsonCode { value: diagnostic .secondary_code() .map_or_else(|| diagnostic.name(), |code| code.as_str()), url: diagnostic.to_ruff_url(), }, suggestions: rdjson_suggestions(edits, source_file.map(|(_, source)| source)), }

It makes it clearer that range can only be set when span isn't None.

Thanks! I like this better than the version in this PR with many separate map calls and also better than the mutable version in #19340. I'll try to use this pattern going forward.

MichaReiser · 2025-07-14T16:24:14Z

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

+        .map(|edit| {
+            // Safety: we assert that diagnostics with fixes have an associated source file in
+            // `Diagnostic::set_fix`.
+            let source_code = source_code.as_ref().unwrap();


Nit: You could make this code more error resilient by skipping all edits if source_code is empty (but use a debug_assert to make it fail if that happens).

It has the benefit that Ruff doesn't panic if we ever fail to set the source file

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

Co-authored-by: Micha Reiser <micha@reiser.io>

* main: [`pylint`] Extend invalid string character rules to include t-strings (#19355) Make TC010 docs example more realistic (#19356) Move RDJSON rendering to `ruff_db` (#19293) [`flake8-use-pathlib`] Skip single dots for `invalid-pathlib-with-suffix` (`PTH210`) on versions >= 3.14 (#19331) [`ruff`] Allow `strict` kwarg when checking for `starmap-zip` (`RUF058`) in Python 3.14+ (#19333) [ty] Reduce false positives for `TypedDict` types (#19354) [ty] Remove `ConnectionInitializer` (#19353) [ty] Use `Type::string_literal()` more (#19352) [ty] Add ecosystem-report workflow (#19349) [ty] Make use of salsa `Lookup` when interning values (#19347) [ty] Sync vendored typeshed stubs (#19345) [`pylint`] Make example error out-of-the-box (`PLE2502`) (#19272) [`pydoclint`] Fix `SyntaxError` from fixes with line continuations (`D201`, `D202`) (#19246)

* dcreager/merge-arguments: add types iterator add asserting constructor debug assert lengths remove unused From use FromIterator [`pylint`] Extend invalid string character rules to include t-strings (#19355) Make TC010 docs example more realistic (#19356) Move RDJSON rendering to `ruff_db` (#19293) [`flake8-use-pathlib`] Skip single dots for `invalid-pathlib-with-suffix` (`PTH210`) on versions >= 3.14 (#19331) [`ruff`] Allow `strict` kwarg when checking for `starmap-zip` (`RUF058`) in Python 3.14+ (#19333) [ty] Reduce false positives for `TypedDict` types (#19354) [ty] Remove `ConnectionInitializer` (#19353) [ty] Use `Type::string_literal()` more (#19352) [ty] Add ecosystem-report workflow (#19349) [ty] Make use of salsa `Lookup` when interning values (#19347) [ty] Sync vendored typeshed stubs (#19345) [`pylint`] Make example error out-of-the-box (`PLE2502`) (#19272) [`pydoclint`] Fix `SyntaxError` from fixes with line continuations (`D201`, `D202`) (#19246)

…finition * 'main' of https://github.com/astral-sh/ruff: (39 commits) [ty] Sync vendored typeshed stubs (astral-sh#19368) Fix typeshed-sync workflow (astral-sh#19367) Rework typeshed-sync workflow to also add docstrings for Windows- and MacOS-specific APIs (astral-sh#19360) [ty] Allow `-qq` for silent output mode (astral-sh#19366) [ty] Allow `-q` short alias for `--quiet` (astral-sh#19364) Add shellcheck to pre-commit (astral-sh#19361) distinguish references from definitions in `infer_nonlocal` [`pycodestyle`] Handle brace escapes for t-strings in logical lines (astral-sh#19358) [ty] Combine CallArguments and CallArgumentTypes (astral-sh#19337) Move Pylint rendering to `ruff_db` (astral-sh#19340) [`pylint`] Extend invalid string character rules to include t-strings (astral-sh#19355) Make TC010 docs example more realistic (astral-sh#19356) Move RDJSON rendering to `ruff_db` (astral-sh#19293) [`flake8-use-pathlib`] Skip single dots for `invalid-pathlib-with-suffix` (`PTH210`) on versions >= 3.14 (astral-sh#19331) [`ruff`] Allow `strict` kwarg when checking for `starmap-zip` (`RUF058`) in Python 3.14+ (astral-sh#19333) [ty] Reduce false positives for `TypedDict` types (astral-sh#19354) [ty] Remove `ConnectionInitializer` (astral-sh#19353) [ty] Use `Type::string_literal()` more (astral-sh#19352) [ty] Add ecosystem-report workflow (astral-sh#19349) [ty] Make use of salsa `Lookup` when interning values (astral-sh#19347) ... # Conflicts: # crates/ty_ide/src/goto.rs # crates/ty_server/src/server.rs

ntBre added 2 commits July 11, 2025 16:12

use types instead of json!

9f7ca9e

move rdjson to ruff_db

fe964cf

ntBre added internal An internal refactor or improvement diagnostics Related to reporting of diagnostics. labels Jul 11, 2025

ntBre commented Jul 11, 2025

View reviewed changes

ntBre marked this pull request as ready for review July 11, 2025 21:31

ntBre requested review from AlexWaygood, MichaReiser, carljm, dcreager and sharkdp as code owners July 11, 2025 21:31

ntBre removed request for AlexWaygood, carljm, dcreager and sharkdp July 11, 2025 21:31

ntBre commented Jul 11, 2025

View reviewed changes

ntBre added 7 commits July 12, 2025 16:36

use LineColumn directly

5c5fd22

use the lint name if the secondary code is unavailable

6487cf0

use CARGO_PKG_HOMEPAGE if no rule page is available

7100cd8

this being optional was violating the schema. see https://www.jsonschemavalidator.net/s/uvGWl3nq

range can't be null without violating the schema

4ddea6f

fix severity case, lowercase rejected by validator

1e7e836

use the real maximum severity in preview

8b2768f

include per-diagnostic severity in preview

e511808

skip serializing the range, if it's None

9cebd72

MichaReiser reviewed Jul 14, 2025

View reviewed changes

ntBre marked this pull request as draft July 14, 2025 12:10

remove outdated module comment

dc4b9df

ntBre added 4 commits July 14, 2025 09:44

make the diagnostic's location optional, omit if None

6284913

omit URL if None

49bab15

revert severity changes, remove now-unused config

4a240ba

debug_assert! that fixable diagnostics have a source file

d3d6bb5

ntBre marked this pull request as ready for review July 14, 2025 14:38

MichaReiser approved these changes Jul 14, 2025

View reviewed changes

ntBre and others added 3 commits July 14, 2025 12:33

delete trailing comma

55398de

Co-authored-by: Micha Reiser <micha@reiser.io>

debug_assert! in rdjson_suggestions too

3a5b7a4

tie range and location together more closely

022b6b1

Co-authored-by: Micha Reiser <micha@reiser.io>

ntBre enabled auto-merge (squash) July 15, 2025 12:36

ntBre merged commit e9b0c33 into main Jul 15, 2025
34 checks passed

ntBre deleted the brent/rdjson branch July 15, 2025 12:39

		// TODO(brent) this can be required after it's out of preview.
		#[serde(skip_serializing_if = "Option::is_none")]

Move RDJSON rendering to ruff_db #19293

Move RDJSON rendering to ruff_db #19293

Uh oh!

Conversation

ntBre commented Jul 11, 2025

Summary

Test Plan

Uh oh!

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

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

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!

Uh oh!

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!

MichaReiser commented Jul 13, 2025

Uh oh!

ntBre commented Jul 13, 2025

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntBre commented Jul 14, 2025

Uh oh!

MichaReiser 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!

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!

Move RDJSON rendering to `ruff_db` #19293

Move RDJSON rendering to `ruff_db` #19293

github-actions bot commented Jul 11, 2025 •

edited

Loading

`ruff-ecosystem` results