`slicec` Diagnostic Refactoring by InsertCreativityHere · Pull Request #772 · icerpc/slicec

InsertCreativityHere · 2026-04-16T18:55:24Z

Right now we use the same Diagnostic type to report errors as we do to emit them.

However, these are really 2 different purposes, which our API reflects. Diagnostics requires some post-processing before it's correct to emit them, so our API keeps them under lock-and-key, and has functions (like into_updated) which internally update it's fields to "convert" from one form to another.

So, instead of using 1 type for 2 purposes, with 2 different internal "states" (ready to emit vs not)... better to just have 2 different types, with a domain-separation approach. This PR replaces our current design by splitting Diagnostic in 2:

Diagnostic is what the validation/parsing creates when an error or something happens.
PrintableDiagnostic contains a patched & updated version of Diagnostic which is ready to be emitted to the user.

InsertCreativityHere · 2026-04-16T18:56:41Z

 pub fn parse_multiple_for_ast(slice: &[&str]) -> Ast {
    let compilation_state = compile_from_strings(slice, None);
-    if compilation_state.diagnostics.has_errors() {
+    if !compilation_state.diagnostics.is_empty() {


The tests should fail if there's any diagnostics, not just errors.
If there were warnings in our tests, they would be silently ignored with the old logic.

InsertCreativityHere · 2026-04-16T19:04:34Z

+        // If a scope was provided, check that it matches.
+        if expect.scope.is_some() && expect.scope != diagnostic.scope {
+            eprintln!("diagnostic scopes didn't match:");
+            eprintln!("\texpected: \"{:?}\"", expect.scope);
+            eprintln!("\t but got: \"{:?}\"", diagnostic.scope);


We weren't checking that diagnostics were thrown the Slice scope we expected, when we really should be checking this. Thankfully there were no bugs to be caught :) But should still be checking!

InsertCreativityHere · 2026-04-16T19:09:10Z

@@ -1,11 +0,0 @@
-// Copyright (c) ZeroC, Inc.


We had this outdated example laying around, from when slicec was library only. The example shows how to run slicec as a binary... But now that's actually a binary, this example is pointless.

InsertCreativityHere · 2026-04-16T19:20:10Z

 #[derive(Debug)]
 pub struct Diagnostic {
-    kind: DiagnosticKind,
-    level: DiagnosticLevel,


DIagnosticLevel is the "patched" level, which is only useful for printing the diagnostic.

i.e. a lint can become either a warning, or nothing if it's allowed.
We also plan in the future to add opt-in lints which can be upgraded to warnings.

InsertCreativityHere · 2026-04-16T19:23:40Z


        // Store any diagnostics that were emitted during parsing.
-        state.diagnostics.extend(diagnostics);
+        state.diagnostics.extend(diagnostics.into_inner());


We used to have our own Diagnostics::extend function, which I removed. Now we're just using the normal Vec::extend, but in this one spot, it requires an extra call to get the right type.

InsertCreativityHere · 2026-04-20T15:24:33Z

+pub struct DiagnosticEmitter<'a> {
    /// Reference to the output that diagnostics should be emitted to.
-    output: &'a mut T,
+    output: &'a mut dyn Write,


Switched from using a generic T: Write to a dynamic dyn Write.
This makes the typing for DiagnosticEmitter easier, since it no longer has a generic type.

And in reality, it's not like we care about DiagnosticEmitter<StdOut> vs DiagnosticEmitter<Vec<u8>>...
As long as we can write to something it's fine. No need to tie the underlying write-able thing to the type of DiagnosticEmitter.

InsertCreativityHere · 2026-04-20T15:25:18Z

-    /// If true, diagnostic output will not be styled with colors (only used in `human` format).
-    disable_color: bool,


We disable color way earlier in the program now, instead of right before diagnostic emission.
Otherwise there were other things still being emitted in color.

InsertCreativityHere · 2026-04-20T15:26:53Z

-        if self.disable_color {
-            console::set_colors_enabled(false);
-            console::set_colors_enabled_stderr(false);
+    pub fn get_totals(diagnostics: &[PrintableDiagnostic]) -> (usize, usize) {


This used to be a free function at the bottom of diagnostic.rs, now it's a static function on DiagnosticEmitter.
Just feels tidier to keep it here, since this was only used when emitting diagnostics.

InsertCreativityHere · 2026-04-20T15:29:14Z

+        (total_warnings, total_errors)
+    }
+
+    pub fn emit_totals(total_warnings: usize, total_errors: usize) -> Result<()> {


This used to be a free function at the bottom of this file, now it's a static function on DiagnosticEmitter.
Just feels tidier, and saves an import in the places it's used.

InsertCreativityHere · 2026-04-20T15:36:12Z

@@ -124,16 +126,17 @@ mod attributes {
                    message: "comment has a 'returns' tag, but only operations can return".to_owned(),
                }),
            ];


With the old test setup, "allowed" lints were removed entirely.

With the new test setup, we make sure that the lints were emitted, but that they are correctly marked as Allowed instead of Warning. This feels like a safer and more true-to-task test.

Copilot

Pull request overview

This PR refactors slicec diagnostics by separating “internal” diagnostics produced during parsing/validation from “emission-ready” diagnostics, introducing a new PrintableDiagnostic type that contains computed levels and pre-extracted snippets suitable for user output.

Changes:

Introduces PrintableDiagnostic conversion and updates CompilationState to produce printable diagnostics for emission.
Refactors DiagnosticEmitter to emit &[PrintableDiagnostic] (human + JSON) and updates CLI/tests accordingly.
Adjusts various validators/parsers/tests to work with the new diagnostic representation and APIs.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`slicec/tests/test_helpers.rs`	Updates test helpers to work with refactored diagnostics storage/access patterns.
`slicec/tests/diagnostic_output_tests.rs`	Switches output tests to use `get_printable_diagnostics` and new emitter signature.
`slicec/tests/attribute_tests.rs`	Updates allow-lint tests to assert against printable diagnostics (level changes).
`slicec/src/validators/dictionary.rs`	Adapts note/span handling to new `Diagnostic` field access patterns.
`slicec/src/validators/cycle_detection.rs`	Refactors cycle diagnostic construction without removed fluent APIs.
`slicec/src/parsers/mod.rs`	Updates diagnostics accumulation to use `into_inner()` with the refactored container.
`slicec/src/main.rs`	Refactors CLI emission flow to use printable diagnostics and updated emitter API.
`slicec/src/lib.rs`	Adds color-disabling behavior during compilation entry points.
`slicec/src/diagnostics/printable_diagnostic.rs`	New conversion logic from `Diagnostic` to `PrintableDiagnostic` (level + snippet extraction).
`slicec/src/diagnostics/mod.rs`	Exposes new module/types and relocates `DiagnosticKind`/`DiagnosticLevel`.
`slicec/src/diagnostics/diagnostic.rs`	Simplifies `Diagnostic` to “internal” form and adjusts `Diagnostics` container API.
`slicec/src/diagnostic_emitter.rs`	Refactors emitter to consume `PrintableDiagnostic` and moves totals logic into the emitter.
`slicec/src/compilation_state.rs`	Adds `get_printable_diagnostics` as the primary “ready to emit” diagnostics accessor.
`slicec/examples/parse.rs`	Removes example that relied on the removed `emit_diagnostics` API.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-20T19:28:33Z

+        if is_lint_allowed_by_attributes(file.unwrap(), lint) {
+            return DiagnosticLevel::Allowed;


file.unwrap() can panic if a diagnostic span references a file that is not present in compilation_state.files (e.g., if diagnostics are constructed from external inputs). This also removes the more informative panic message that previously existed in the old code path. Prefer handling the None case gracefully (treat as not allowed) or use expect("...") with a clear message to preserve debuggability.

Suggested change

if is_lint_allowed_by_attributes(file.unwrap(), lint) {

return DiagnosticLevel::Allowed;

if let Some(file) = file {

if is_lint_allowed_by_attributes(file, lint) {

return DiagnosticLevel::Allowed;

}

Copilot · 2026-04-20T19:28:33Z

+    // Disable colored output if the user requested we do so.
+    if options.disable_color {
+        console::set_colors_enabled(false);
+        console::set_colors_enabled_stderr(false);
+    }


Calling console::set_colors_enabled(false) here changes a global process-wide setting and is never reverted when disable_color is false in subsequent compilations. Since slicec is a library as well as a CLI, this can leak configuration across independent compile calls/tests. Consider moving color toggling back to the emission layer, or explicitly setting colors enabled/disabled based on the passed options each time.

Copilot · 2026-04-20T19:28:34Z

+    // Disable colored output if the user requested we do so.
+    if options.map(|opts| opts.disable_color).unwrap_or_default() {
+        console::set_colors_enabled(false);
+        console::set_colors_enabled_stderr(false);
+    }


compile_from_strings also disables console colors via global setters, which can unintentionally affect later tests/compilations in the same process. If global toggling is required, consider restoring the previous value or explicitly setting enable/disable based on SliceOptions for each call to avoid cross-call leakage.

externl · 2026-04-20T19:32:43Z

    pub fn new() -> Self {
        Self::default()
    }



Do we really need this extra constructor, why not just use ::default?

Do we really need this extra constructor

Honestly, no.

why not just use ::default?

It just feels more semantically correct to create a 'new' Diagnostic, instead of a 'default' Diagnostic.
That was my feeling at least. If you don't like it though, I can remove or simplify! : ^)

externl · 2026-04-20T19:36:06Z

+/// Diagnostic levels describe the severity of a diagnostic, and how the compiler should react to their emission.
+#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)]
+pub enum DiagnosticLevel {
+    #[rustfmt::skip] // See https://github.com/rust-lang/rustfmt/issues/5801


This issue is closed, presumably the fix should be in the nighty.

This was just copy-pasted. Didn't even think to check, good idea.

externl · 2026-04-20T19:37:19Z

+    #[serde(rename = "span")]
+    pub snippet: Option<Snippet>,


Why do we encode as "span" in JSON?

This makes it backwards compatible with the JSON decoding the tooling does.

But, it depended on what you answered to the TODO :) I'll update to send the full snippet.

externl · 2026-04-20T19:39:37Z

+    pub span: Span,
+    pub text: String,
+}
+// TODO should we send the text snippet in JSON diagnostic format? Or just the range?


I would just include it for simplicity. Right now all our Snippets are really just spans in JSON.

externl · 2026-04-20T19:46:05Z

-            state.serialize_field("span", &diagnostic.span())?;
-            state.serialize_field("notes", diagnostic.notes())?;
-            state.serialize_field("error_code", diagnostic.code())?;
+            state.serialize_field("span", &diagnostic.snippet)?;


I think we should either encode just the span like we did before (but as diagnostic.snippet.span`, or encode the entire snippet.

This PR currently does option 1.
If you look at the implementation where snippet is encoded, all it actually encodes is the span.

Personally, I'd lean towards option 2 (encode the full snippet (span + text),
but wasn't sure what other opinions would be.

InsertCreativityHere added 2 commits April 15, 2026 15:55

Did everything except for the emitter so far.

48560eb

Finished the refactoring portion.

db37903

InsertCreativityHere changed the title ~~`slicec Diagnostic Refactoring~~ slicec Diagnostic Refactoring Apr 16, 2026

InsertCreativityHere added 3 commits April 20, 2026 11:06

Better comment and changed the name.

02df338

Slightly easier to review this way.

d66a411

More comments in the printable_diagnostics file.

bfb0242

InsertCreativityHere commented Apr 20, 2026

View reviewed changes

InsertCreativityHere requested review from Copilot and externl April 20, 2026 19:23

InsertCreativityHere marked this pull request as ready for review April 20, 2026 19:23

Copilot started reviewing on behalf of InsertCreativityHere April 20, 2026 19:24 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

externl reviewed Apr 20, 2026

View reviewed changes

		/// If true, diagnostic output will not be styled with colors (only used in `human` format).
		disable_color: bool,

		if is_lint_allowed_by_attributes(file.unwrap(), lint) {
		return DiagnosticLevel::Allowed;

Conversation

InsertCreativityHere commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

InsertCreativityHere commented Apr 16, 2026 •

edited

Loading