Use new Slice-Compiler-Definitions (Mostly Diagnostics) by InsertCreativityHere · Pull Request #4615 · icerpc/icerpc-csharp

InsertCreativityHere · 2026-05-12T21:04:31Z

This PR pulls in 2 changes from slicec:

Switching Options from a Dictionary<string, string> to a Sequence<CodeGeneratorOption>
The new API for reporting diagnostics

The first is pretty straightforward, I just updated the decoding logic and the signatures that used it.
Instead of a Dictionary<string, string>, I chose a KeyValuePair<string, string>[].
If there is a better choice of type, please let me know. I am not a master of C# type conventions!

The second involved creating a new wrapper type Diagnostic, with a handful of factory functions on it to create specific types of diagnostics. The validation (duplicate files and attributes) was updated to use these factory functions, instead of the old API which just took a string and a level.

Alot of the diff is just generated code. I promise this PR isn't as scary as it looks ;-;

Copilot

Pull request overview

This PR updates the C# Slice generator/plugin protocol to align with the new Compiler::Diagnostic/DiagnosticKind model (and option representation) introduced upstream in slicec (#776), and re-enables C# attribute validation with richer, source-aware diagnostics.

Changes:

Switch generator request “options” from Dictionary<string, string> to a sequence of key/value options and update the generator pipeline signatures accordingly.
Replace the old DiagnosticLevel/string-message diagnostics with structured compiler diagnostics (DiagnosticKind, DiagnosticNote, source, notes) and update response encoding.
Re-enable C# attribute validation and update CsAttributeValidator to emit the new structured diagnostics (with per-attribute source paths and notes).

Reviewed changes

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

Show a summary per file

File	Description
tests/ZeroC.Slice.Generator.Tests/VariantEnumTests.slice	Removes variant-enum attribute test input tied to the old behavior.
tests/ZeroC.Slice.Generator.Tests/VariantEnumTests.cs	Removes reflection-based attribute assertion that no longer applies.
src/ZeroC.Slice.Symbols/Generator.cs	Updates request decoding / response encoding for new options + diagnostics protocol.
src/ZeroC.Slice.Symbols/Diagnostic.cs	Reworks diagnostics to map to `Compiler.DiagnosticKind` + notes and adds helper factories.
src/ZeroC.Slice.Symbols/DiagnosticLevel.cs	Removes the old severity enum in favor of `DiagnosticKind`.
src/ZeroC.Slice.Symbols/Compiler/SyntaxElements.cs	Regenerated compiler model (version bump).
src/ZeroC.Slice.Symbols/Compiler/DocComment.cs	Regenerated compiler model (version bump).
src/ZeroC.Slice.Symbols/Compiler/Diagnostics.cs	Adds generated C# types for new compiler diagnostics protocol.
src/ZeroC.Slice.Symbols/Compiler/CodeGenerator.cs	Updates generated protocol types (options now `CodeGeneratorOption` sequence; old diagnostics removed).
src/ZeroC.Slice.Generator/GeneratorDriver.cs	Re-enables attribute validation and updates error handling to use new diagnostics.
src/ZeroC.Slice.Generator/CSAttributeValidator.cs	Emits structured diagnostics with sources/notes and updated validation rules.
src/IceRpc.Slice.BuildTelemetry/Program.cs	Updates generator callback signature for new options representation.
slice/Ice/ReplyStatus.ice	Vendors upstream `ReplyStatus.NotSupported`.
slice/Compiler/Diagnostics.slice	Adds Slice definitions for new compiler diagnostics.
slice/Compiler/CodeGenerator.slice	Updates options typealias and removes legacy diagnostics from this protocol file.
slice.toml	Updates sync instructions and upstream revisions for vendored Slice sources.

Copilot

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

InsertCreativityHere · 2026-05-22T18:16:04Z

This file was just synchronized with slicec; don't need to review it.

InsertCreativityHere · 2026-05-22T18:16:13Z

This file was just synchronized with slicec; don't need to review it.

InsertCreativityHere · 2026-05-22T18:16:58Z

        ["swift:identifier:invalidData"]
        InvalidData,

        /// The caller is not authorized to access the requested resource.


For completeness, I also updated the commit IDs for ice and icerpc-slice.
Apparently the *.ice files were slightly out of date.

This requires update to the C# code as well.

Okay, I will update this too while I'm at it.

You can also create an issue to make this update in a follow-up PR.

InsertCreativityHere · 2026-05-22T18:17:23Z

 static async Task<GeneratorResponse> BuildResponseAsync(
    ImmutableList<SliceFile> symbolFiles,
-    Dictionary<string, string> options)
+    KeyValuePair<string, string>[] additionalOptions)


The new mapping for the additional options you provide a plugin.

InsertCreativityHere · 2026-05-22T18:18:03Z

@@ -37,8 +37,8 @@ internal static List<Diagnostic> Validate(ImmutableList<SliceFile> files)

        foreach (SliceFile file in files)
        {


We pass along the file-name/scoped-identifier to report it back to slicec as a DiagnosticSource.

InsertCreativityHere · 2026-05-22T18:28:30Z

        }
    }
-
-    [Test]


This test was incorrectly testing that we allowed cs::attribute on variants.

InsertCreativityHere · 2026-05-22T18:29:14Z

+        Task<GeneratorResponse>
+        TransformAsync(ImmutableList<SliceFile> symbolFiles, KeyValuePair<string, string>[] additionalOptions)
        {
            // Validate CS attributes before generation.


We're actually performing the C# attribute validation now!

InsertCreativityHere · 2026-05-22T18:31:14Z

                {
-                    diagnostics.Add(new Diagnostic
-                    {
-                        Level = DiagnosticLevel.Error,
-                        Message =
-                            $"Multiple source files have the same file name '{fileName}': " +
-                            $"'{previousPath}' and '{file.Path}'. " +
-                            "Generated files are written to a common directory, so source files must have unique file names.",
-                    });
+                    string message = $"Multiple source files have the same file name '{fileName}': '{previousPath}' and '{file.Path}'. " +


For the record, this fixes the bug you had the other day @bernardnormier.

InsertCreativityHere · 2026-05-22T18:32:54Z

@@ -3,14 +3,67 @@
 namespace ZeroC.Slice.Symbols;

 /// <summary>Represents a diagnostic message produced during code generation.</summary>


The new API for Diagnostics.
This is the handwritten domain type for Symbols, not generated code.

It's no longer readonly since you can conditionally attach notes to it, further describing the error.

InsertCreativityHere · 2026-05-22T18:34:55Z

-            count => new Dictionary<string, string>(count),
-            (ref decoder) => decoder.DecodeString(),
-            (ref decoder) => decoder.DecodeString());
+        KeyValuePair<string, string>[] additionalOptions = decoder.DecodeSequence((ref decoder) =>


I settled on KeyValuePair, since that's the correct semantics here, and I used an array since that's what we used for the SliceFile arguments.

If there's a better type for me to use here, please let me know!

I would either define a new struct in Symbols or use a tuple with named fields.

Updated to (string key, string value).

pepone · 2026-05-22T19:08:31Z

+    {
+        Kind = new Compiler.DiagnosticKind.Error(message),
+        Source = source,
+        Notes = [],
+    };


I find this style confusing, I would indent this, to better distinguish from a

Suggested change

{

Kind = new Compiler.DiagnosticKind.Error(message),

Source = source,

Notes = [],

};

{

Kind = new Compiler.DiagnosticKind.Error(message),

Source = source,

Notes = [],

};

and same for others below.

Will fix!
My knowledge about C# formatting has definitely been degraded over the years...

.Net format complains about the indented syntax and tells me to dedent it:
https://github.com/icerpc/icerpc-csharp/actions/runs/26453087442/job/77879180269?pr=4615

pepone · 2026-05-22T19:12:40Z

-            // TODO: enable validation once slicec correctly handles the diagnostics reported by the
-            // generators.
-            var diagnostics = new List<Diagnostic>();
+            List<Diagnostic> diagnostics = CsAttributeValidator.Validate(symbolFiles);


I assigned #4498 to you, given that you already fixed it here!

Cool, I didn't even see it yet! 😆
Should really look through the remaining audits sometime.

bernardnormier · 2026-05-22T19:32:09Z

        ["swift:identifier:invalidData"]
        InvalidData,

        /// The caller is not authorized to access the requested resource.


This requires update to the C# code as well.

bernardnormier · 2026-05-22T19:38:09Z

-            count => new Dictionary<string, string>(count),
-            (ref decoder) => decoder.DecodeString(),
-            (ref decoder) => decoder.DecodeString());
+        KeyValuePair<string, string>[] additionalOptions = decoder.DecodeSequence((ref decoder) =>


I would either define a new struct in Symbols or use a tuple with named fields.

externl

Looks good to me!

pepone · 2026-05-26T14:22:28Z

Is better to do this in its own PR, seems totally unrelated.

I undid this and will redo it in my next PR.

But I'm not sure if I agree with the premise. It feels safer to pro-actively sync Slice sources to catch things like here where NotSupported seems to of been forgotten. If every PR only updates the minimum sources it needs, I worry we will never properly synchronize these files and things will fall out of alignment.

Maybe there's a way to update our vendored files action to better warn us about when files get out of sync?
As it stands right now, the verify check doesn't seem very useful to me TBH.

InsertCreativityHere marked this pull request as draft May 12, 2026 21:04

InsertCreativityHere mentioned this pull request May 12, 2026

Properly Handle Diagnostics From Plugins icerpc/slicec#776

Merged

InsertCreativityHere added 6 commits May 21, 2026 16:58

Updated Slice definition files.

089a22d

Re-compiled the checked-in generated code.

2fcd1b1

Implemented the 'Diagnostic' wrapper type.

8f81d57

Fixes.

b4d3c7e

Implemented the new attribute validation.

46b86a8

Fixed the tests.

4240c24

InsertCreativityHere force-pushed the use-new-slicec-diagnostics branch from d330702 to 4240c24 Compare May 22, 2026 15:47

Updated to decode options as a sequence instead of a dictionary.

fc5b99e

InsertCreativityHere requested a review from Copilot May 22, 2026 16:03

Copilot started reviewing on behalf of InsertCreativityHere May 22, 2026 16:04 View session

Copilot AI reviewed May 22, 2026

View reviewed changes

InsertCreativityHere added 2 commits May 22, 2026 13:58

Comments and generated-code cleanup.

2fc09c9

Maybe this formatting?

1e0beea

InsertCreativityHere requested a review from Copilot May 22, 2026 18:05

Copilot started reviewing on behalf of InsertCreativityHere May 22, 2026 18:05 View session

Copilot AI reviewed May 22, 2026

View reviewed changes

Comment thread src/ZeroC.Slice.Generator/GeneratorDriver.cs

Comment thread src/IceRpc.Slice.BuildTelemetry/Program.cs

InsertCreativityHere commented May 22, 2026

View reviewed changes

InsertCreativityHere marked this pull request as ready for review May 22, 2026 18:35

InsertCreativityHere requested review from bernardnormier, externl and pepone May 22, 2026 18:35

pepone approved these changes May 22, 2026

View reviewed changes

bernardnormier approved these changes May 22, 2026

View reviewed changes

externl approved these changes May 22, 2026

View reviewed changes

InsertCreativityHere added 2 commits May 26, 2026 10:02

Review fixes.

7cccb4c

dotnet wants it dedented...

0d11780

pepone reviewed May 26, 2026

View reviewed changes

InsertCreativityHere changed the title ~~Use new Slice-Compiler-Definition Diagnostics~~ Use new Slice-Compiler-Definitions (Mostly Diagnostics) May 26, 2026

InsertCreativityHere added 2 commits May 26, 2026 10:33

Fine. We'll undo this to redo this later.

853ae5f

Push us back to the old version of Ice.

df0bae9

InsertCreativityHere merged commit e185631 into icerpc:main May 26, 2026
12 checks passed

InsertCreativityHere mentioned this pull request May 26, 2026

[Audit-Medium] CsAttributeValidator.Validate is called-through a disa… #4498

Closed

		@@ -37,8 +37,8 @@ internal static List<Diagnostic> Validate(ImmutableList<SliceFile> files)

		foreach (SliceFile file in files)
		{

                       }
                   }
-                  [Test]

		@@ -3,14 +3,67 @@
		namespace ZeroC.Slice.Symbols;

		/// <summary>Represents a diagnostic message produced during code generation.</summary>

Conversation

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

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

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!

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!

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!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

InsertCreativityHere commented May 12, 2026 •

edited

Loading