From f14ee9edd5e0f98261db0b781a2c68991fa1d567 Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Fri, 11 Jul 2025 09:37:44 -0400 Subject: [PATCH] Use structs for JSON serialization (#19270) ## Summary See https://github.com/astral-sh/ruff/pull/19133#discussion_r2198413586 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. --- crates/ruff_db/src/diagnostic/mod.rs | 5 +- crates/ruff_linter/src/message/diff.rs | 2 +- crates/ruff_linter/src/message/json.rs | 108 +++++++++++++++++-------- 3 files changed, 78 insertions(+), 37 deletions(-) diff --git a/crates/ruff_db/src/diagnostic/mod.rs b/crates/ruff_db/src/diagnostic/mod.rs index 9b449ee6f8..c7a7c080f6 100644 --- a/crates/ruff_db/src/diagnostic/mod.rs +++ b/crates/ruff_db/src/diagnostic/mod.rs @@ -432,8 +432,9 @@ impl Diagnostic { /// Returns the [`SourceFile`] which the message belongs to. /// /// Panics if the diagnostic has no primary span, or if its file is not a `SourceFile`. - pub fn expect_ruff_source_file(&self) -> SourceFile { - self.expect_primary_span().expect_ruff_file().clone() + pub fn expect_ruff_source_file(&self) -> &SourceFile { + self.ruff_source_file() + .expect("Expected a ruff source file") } /// Returns the [`TextRange`] for the diagnostic. diff --git a/crates/ruff_linter/src/message/diff.rs b/crates/ruff_linter/src/message/diff.rs index 289a4beffe..437fc0a830 100644 --- a/crates/ruff_linter/src/message/diff.rs +++ b/crates/ruff_linter/src/message/diff.rs @@ -21,7 +21,7 @@ use crate::{Applicability, Fix}; /// * Compute the diff from the [`Edit`] because diff calculation is expensive. pub(super) struct Diff<'a> { fix: &'a Fix, - source_code: SourceFile, + source_code: &'a SourceFile, } impl<'a> Diff<'a> { diff --git a/crates/ruff_linter/src/message/json.rs b/crates/ruff_linter/src/message/json.rs index 8fd2a20003..8425975ea5 100644 --- a/crates/ruff_linter/src/message/json.rs +++ b/crates/ruff_linter/src/message/json.rs @@ -1,10 +1,10 @@ use std::io::Write; +use ruff_diagnostics::Applicability; use serde::ser::SerializeSeq; use serde::{Serialize, Serializer}; -use serde_json::{Value, json}; -use ruff_db::diagnostic::Diagnostic; +use ruff_db::diagnostic::{Diagnostic, SecondaryCode}; use ruff_notebook::NotebookIndex; use ruff_source_file::{LineColumn, OneIndexed, SourceCode}; use ruff_text_size::Ranged; @@ -55,20 +55,15 @@ impl Serialize for ExpandedMessages<'_> { } } -pub(crate) fn message_to_json_value(message: &Diagnostic, context: &EmitterContext) -> Value { +pub(crate) fn message_to_json_value<'a>( + message: &'a Diagnostic, + context: &'a EmitterContext<'a>, +) -> JsonDiagnostic<'a> { let source_file = message.expect_ruff_source_file(); let source_code = source_file.to_source_code(); let filename = message.expect_ruff_filename(); let notebook_index = context.notebook_index(&filename); - let fix = message.fix().map(|fix| { - json!({ - "applicability": fix.applicability(), - "message": message.suggestion(), - "edits": &ExpandedEdits { edits: fix.edits(), source_code: &source_code, notebook_index }, - }) - }); - let mut start_location = source_code.line_column(message.expect_range().start()); let mut end_location = source_code.line_column(message.expect_range().end()); let mut noqa_location = message @@ -88,29 +83,32 @@ pub(crate) fn message_to_json_value(message: &Diagnostic, context: &EmitterConte noqa_location.map(|location| notebook_index.translate_line_column(&location)); } - json!({ - "code": message.secondary_code(), - "url": message.to_url(), - "message": message.body(), - "fix": fix, - "cell": notebook_cell_index, - "location": location_to_json(start_location), - "end_location": location_to_json(end_location), - "filename": filename, - "noqa_row": noqa_location.map(|location| location.line) - }) -} + let fix = message.fix().map(|fix| JsonFix { + applicability: fix.applicability(), + message: message.suggestion(), + edits: ExpandedEdits { + edits: fix.edits(), + source_code, + notebook_index, + }, + }); -fn location_to_json(location: LineColumn) -> serde_json::Value { - json!({ - "row": location.line, - "column": location.column - }) + JsonDiagnostic { + code: message.secondary_code(), + url: message.to_url(), + message: message.body(), + fix, + cell: notebook_cell_index, + location: start_location.into(), + end_location: end_location.into(), + filename, + noqa_row: noqa_location.map(|location| location.line), + } } struct ExpandedEdits<'a> { edits: &'a [Edit], - source_code: &'a SourceCode<'a, 'a>, + source_code: SourceCode<'a, 'a>, notebook_index: Option<&'a NotebookIndex>, } @@ -169,11 +167,11 @@ impl Serialize for ExpandedEdits<'_> { location = notebook_index.translate_line_column(&location); } - let value = json!({ - "content": edit.content().unwrap_or_default(), - "location": location_to_json(location), - "end_location": location_to_json(end_location) - }); + let value = JsonEdit { + content: edit.content().unwrap_or_default(), + location: location.into(), + end_location: end_location.into(), + }; s.serialize_element(&value)?; } @@ -182,6 +180,48 @@ impl Serialize for ExpandedEdits<'_> { } } +#[derive(Serialize)] +pub(crate) struct JsonDiagnostic<'a> { + cell: Option, + code: Option<&'a SecondaryCode>, + end_location: JsonLocation, + filename: String, + fix: Option>, + location: JsonLocation, + message: &'a str, + noqa_row: Option, + url: Option, +} + +#[derive(Serialize)] +struct JsonFix<'a> { + applicability: Applicability, + edits: ExpandedEdits<'a>, + message: Option<&'a str>, +} + +#[derive(Serialize)] +struct JsonLocation { + column: OneIndexed, + row: OneIndexed, +} + +impl From for JsonLocation { + fn from(location: LineColumn) -> Self { + JsonLocation { + row: location.line, + column: location.column, + } + } +} + +#[derive(Serialize)] +struct JsonEdit<'a> { + content: &'a str, + end_location: JsonLocation, + location: JsonLocation, +} + #[cfg(test)] mod tests { use insta::assert_snapshot;