Simplify Activator API (#3877)

build/CodeAnalysis.Base.globalconfig

@@ -160,6 +160,9 @@ dotnet_diagnostic.SA1000.severity = warning
 # SA1001: Commas should not be preceded by whitespace
 dotnet_diagnostic.SA1001.severity = warning
 
+# SA1010: Opening square brackets should be spaced correctly
+dotnet_diagnostic.SA1010.severity = none
+
 # SA1013: Closing brace should not be preceded by a space
 dotnet_diagnostic.SA1013.severity = warning
 

src/ZeroC.Slice/IActivator.cs

@@ -31,14 +31,12 @@ public static IActivator FromAssemblies(params Assembly[] assemblies) =>
 
     /// <summary>Creates an instance of a Slice class based on a type ID.</summary>
     /// <param name="typeId">The Slice type ID.</param>
-    /// <param name="decoder">The decoder.</param>
     /// <returns>A new instance of the class identified by <paramref name="typeId" />.</returns>
-    object? CreateClassInstance(string typeId, ref SliceDecoder decoder);
+    object? CreateClassInstance(string typeId);
 
     /// <summary>Creates an instance of a Slice exception based on a type ID.</summary>
     /// <param name="typeId">The Slice type ID.</param>
-    /// <param name="decoder">The decoder.</param>
     /// <param name="message">The exception message.</param>
     /// <returns>A new instance of the class identified by <paramref name="typeId" />.</returns>
-    object? CreateExceptionInstance(string typeId, ref SliceDecoder decoder, string? message);
+    object? CreateExceptionInstance(string typeId, string? message);
 }

src/ZeroC.Slice/Internal/Activator.cs

@@ -8,10 +8,8 @@
 
 namespace ZeroC.Slice.Internal;
 
-// Instantiates a Slice class or exception. The message is used only for exceptions.
-// Note that the decoder is actually not used by the constructor of the Slice class or exception since the actual
-// decoding takes place later on.
-internal delegate object ActivateObject(ref SliceDecoder decoder, string? message);
+// Instantiates a Slice class or exception. The message is used only for exceptions; innerException is always null.
+internal delegate object ActivateObject(string? message, Exception? innerException);
 
 /// <summary>The default implementation of <see cref="IActivator" />, which uses a dictionary.</summary>
 internal class Activator : IActivator
@@ -21,11 +19,12 @@ internal class Activator : IActivator
 
     private readonly IReadOnlyDictionary<string, Lazy<ActivateObject>> _dict;
 
-    public object? CreateClassInstance(string typeId, ref SliceDecoder decoder) =>
-        _dict.TryGetValue(typeId, out Lazy<ActivateObject>? factory) ? factory.Value(ref decoder, message: null) : null;
+    public object? CreateClassInstance(string typeId) =>
+        _dict.TryGetValue(typeId, out Lazy<ActivateObject>? factory) ?
+            factory.Value(message: null, innerException: null) : null;
 
-    public object? CreateExceptionInstance(string typeId, ref SliceDecoder decoder, string? message) =>
-        _dict.TryGetValue(typeId, out Lazy<ActivateObject>? factory) ? factory.Value(ref decoder, message) : null;
+    public object? CreateExceptionInstance(string typeId, string? message) =>
+        _dict.TryGetValue(typeId, out Lazy<ActivateObject>? factory) ? factory.Value(message, null) : null;
 
     /// <summary>Merge activators into a single activator; duplicate entries are ignored.</summary>
     internal static Activator Merge(IEnumerable<Activator> activators)
@@ -106,25 +105,22 @@ static ActivateObject CreateActivateObject(Type type)
         {
             bool isException = type.IsSubclassOf(typeof(SliceException));
 
-            Type[] types = isException ?
-                new Type[] { typeof(SliceDecoder).MakeByRefType(), typeof(string) } :
-                new Type[] { typeof(SliceDecoder).MakeByRefType() };
+            Type[] types = isException ? [typeof(string), typeof(Exception)] : [];
 
             ConstructorInfo constructor = type.GetConstructor(
                 BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic,
                 null,
                 types,
-                null) ?? throw new InvalidOperationException($"Cannot get Slice decoding constructor for '{type}'.");
-            ParameterExpression decoderParam =
-                Expression.Parameter(typeof(SliceDecoder).MakeByRefType(), "decoder");
+                null) ?? throw new InvalidOperationException($"Cannot find Slice decoding constructor for '{type}'.");
 
             ParameterExpression messageParam = Expression.Parameter(typeof(string), "message");
+            ParameterExpression innerExceptionParam = Expression.Parameter(typeof(Exception), "innerException");
 
             Expression expression = isException ?
-                Expression.New(constructor, decoderParam, messageParam) :
-                Expression.New(constructor, decoderParam);
+                Expression.New(constructor, messageParam, innerExceptionParam) :
+                Expression.New(constructor);
 
-            return Expression.Lambda<ActivateObject>(expression, decoderParam, messageParam).Compile();
+            return Expression.Lambda<ActivateObject>(expression, messageParam, innerExceptionParam).Compile();
         }
     }
 

src/ZeroC.Slice/SliceDecoder.Class.cs

@@ -50,7 +50,7 @@ public SliceException DecodeException(string? message = null)
 
             DecodeIndirectionTableIntoCurrent(); // we decode the indirection table immediately.
 
-            sliceException = activator.CreateExceptionInstance(typeId, ref this, message) as SliceException;
+            sliceException = activator.CreateExceptionInstance(typeId, message) as SliceException;
             if (sliceException is null && SkipSlice(typeId))
             {
                 // Cannot decode this exception.
@@ -261,7 +261,7 @@ private SliceClass DecodeInstance(int index)
             // not created yet.
             if (typeId is not null)
             {
-                instance = activator.CreateClassInstance(typeId, ref this) as SliceClass;
+                instance = activator.CreateClassInstance(typeId) as SliceClass;
             }
 
             if (instance is null && SkipSlice(typeId))

tests/ZeroC.Slice.Tests/ActivatorTests.cs

@@ -66,21 +66,18 @@ public static IEnumerable<TestCaseData> ReferencedAssembliesExceptionTypeIdsWith
     [Test, TestCaseSource(nameof(ReferencedAssembliesClassTypeIds))]
     public void Activator_cannot_create_instances_of_classes_defined_in_unknown_assemblies(string typeId)
     {
-        var decoder = new SliceDecoder(ReadOnlyMemory<byte>.Empty, SliceEncoding.Slice1);
         var sut = IActivator.FromAssembly(typeof(SliceDecoder).Assembly);
-
-        object? instance = sut.CreateClassInstance(typeId, ref decoder);
+        object? instance = sut.CreateClassInstance(typeId);
 
         Assert.That(instance, Is.Null);
     }
 
     [Test, TestCaseSource(nameof(ReferencedAssembliesClassTypeIds))]
     public void Activator_cannot_create_instances_of_exceptions_defined_in_unknown_assemblies(string typeId)
     {
-        var decoder = new SliceDecoder(ReadOnlyMemory<byte>.Empty, SliceEncoding.Slice1);
         var sut = IActivator.FromAssembly(typeof(SliceDecoder).Assembly);
 
-        object? instance = sut.CreateExceptionInstance(typeId, ref decoder, message: null);
+        object? instance = sut.CreateExceptionInstance(typeId, message: null);
 
         Assert.That(instance, Is.Null);
     }
@@ -91,10 +88,9 @@ public void Activator_can_create_instances_of_classes_defined_in_known_assemblie
         string typeId,
         Type expectedType)
     {
-        var decoder = new SliceDecoder(ReadOnlyMemory<byte>.Empty, SliceEncoding.Slice1);
         var sut = IActivator.FromAssembly(assembly);
 
-        object? instance = sut.CreateClassInstance(typeId, ref decoder);
+        object? instance = sut.CreateClassInstance(typeId);
 
         Assert.That(instance, Is.Not.Null);
         Assert.That(instance!.GetType(), Is.EqualTo(expectedType));
@@ -109,7 +105,7 @@ public void Activator_can_create_instances_of_exceptions_defined_in_known_assemb
         var decoder = new SliceDecoder(ReadOnlyMemory<byte>.Empty, SliceEncoding.Slice1);
         var sut = IActivator.FromAssembly(assembly);
 
-        object? instance = sut.CreateExceptionInstance(typeId, ref decoder, message: null);
+        object? instance = sut.CreateExceptionInstance(typeId, message: null);
 
         Assert.That(instance, Is.Not.Null);
         Assert.That(instance!.GetType(), Is.EqualTo(expectedType));

tests/ZeroC.Slice.Tests/SlicingTests.cs

@@ -268,10 +268,10 @@ public SlicingActivator(IActivator activator, string? excludeTypeId = null)
             _excludeTypeId = excludeTypeId;
         }
 
-        public object? CreateClassInstance(string typeId, ref SliceDecoder decoder) =>
-            _excludeTypeId == typeId ? null : _decoratee.CreateClassInstance(typeId, ref decoder);
+        public object? CreateClassInstance(string typeId) =>
+            _excludeTypeId == typeId ? null : _decoratee.CreateClassInstance(typeId);
 
-        public object? CreateExceptionInstance(string typeId, ref SliceDecoder decoder, string? message) =>
-            _excludeTypeId == typeId ? null : _decoratee.CreateExceptionInstance(typeId, ref decoder, message);
+        public object? CreateExceptionInstance(string typeId, string? message) =>
+            _excludeTypeId == typeId ? null : _decoratee.CreateExceptionInstance(typeId, message);
     }
 }

tests/ZeroC.Slice.Tests/TaggedTests.cs

@@ -538,11 +538,11 @@ private class TypeReplacementActivator : IActivator
         private readonly string _replacementTypeId;
         private readonly string _typeId;
 
-        public object? CreateClassInstance(string typeId, ref SliceDecoder decoder) =>
-            _decoratee.CreateClassInstance(typeId == _typeId ? _replacementTypeId : typeId, ref decoder);
+        public object? CreateClassInstance(string typeId) =>
+            _decoratee.CreateClassInstance(typeId == _typeId ? _replacementTypeId : typeId);
 
-        public object? CreateExceptionInstance(string typeId, ref SliceDecoder decoder, string? message) =>
-            _decoratee.CreateExceptionInstance(typeId, ref decoder, message);
+        public object? CreateExceptionInstance(string typeId, string? message) =>
+            _decoratee.CreateExceptionInstance(typeId, message);
 
         internal TypeReplacementActivator(IActivator decoratee, string typeId, string replacementTypeId)
         {

tools/slicec-cs/src/generators/class_generator.rs

@@ -14,7 +14,6 @@ use slicec::utils::code_gen_util::TypeContext;
 pub fn generate_class(class_def: &Class) -> CodeBlock {
     let class_name = class_def.escape_identifier();
     let namespace = class_def.namespace();
-    let has_base_class = class_def.base_class().is_some();
 
     let fields = class_def.fields();
     let base_fields = class_def.base_class().map_or(vec![], Class::all_fields);
@@ -95,30 +94,18 @@ pub fn generate_class(class_def: &Class) -> CodeBlock {
         ));
     }
 
-    // public constructor used for decoding
-    // the decoder parameter is used to distinguish this ctor from the parameterless ctor that
-    // users may want to add to the partial class. It's not used otherwise.
-    let mut decode_constructor = FunctionBuilder::new(access, "", &class_name, FunctionType::BlockBody);
-
-    if !has_base_class {
-        decode_constructor.add_attribute(
-            r#"global::System.Diagnostics.CodeAnalysis.SuppressMessage(
-    "Microsoft.Performance",
-    "CA1801: Review unused parameters",
-    Justification="Special constructor used for Slice decoding")"#,
+    // parameterless constructor for decoding, generated only if the preceding constructors are not parameterless
+    if non_nullable_fields.len() + non_nullable_base_fields.len() > 0 {
+        let mut decode_constructor = FunctionBuilder::new(access, "", &class_name, FunctionType::BlockBody);
+        decode_constructor.add_never_editor_browsable_attribute();
+        decode_constructor.add_comment(
+            "summary",
+            format!(r#"Constructs a new instance of <see cref="{class_name}"/> for the Slice decoder."#),
         );
+        decode_constructor.set_body(initialize_required_fields(&fields));
+        class_builder.add_block(decode_constructor.build());
     }
 
-    decode_constructor.add_parameter("ref SliceDecoder", "decoder", None, None);
-    if has_base_class {
-        decode_constructor.add_base_parameter("ref decoder");
-    }
-    decode_constructor
-        .set_body(initialize_required_fields(&fields))
-        .add_never_editor_browsable_attribute();
-
-    class_builder.add_block(decode_constructor.build());
-
     class_builder.add_block(encode_and_decode(class_def));
 
     class_builder.build()

tools/slicec-cs/src/generators/exception_generator.rs

@@ -51,25 +51,26 @@ pub fn generate_exception(exception_def: &Exception) -> CodeBlock {
 
     exception_class_builder.add_block(one_shot_constructor(exception_def));
 
-    // Hide this method because it must only be called by the Activator and not by the application or generated code.
-    exception_class_builder.add_block(if has_base {
-        FunctionBuilder::new("public", "", &exception_name, FunctionType::BlockBody)
-            .add_never_editor_browsable_attribute()
-            .add_parameter("ref SliceDecoder", "decoder", None, None)
-            .add_parameter("string?", "message", Some("null"), None)
-            .add_base_parameter("ref decoder")
-            .add_base_parameter("message")
-            .set_body(initialize_required_fields(fields))
-            .build()
-    } else {
-        FunctionBuilder::new("public", "", &exception_name, FunctionType::BlockBody)
-            .add_never_editor_browsable_attribute()
-            .add_parameter("ref SliceDecoder", "decoder", None, None)
-            .add_parameter("string?", "message", Some("null"), None)
-            .add_base_parameter("message")
-            .set_body(initialize_required_fields(fields))
-            .build()
-    });
+    // constructor for decoding, generated only if necessary
+    if !exception_def.all_fields().is_empty() {
+        exception_class_builder.add_block(
+            FunctionBuilder::new("public", "", &exception_name, FunctionType::BlockBody)
+                .add_never_editor_browsable_attribute()
+                .add_comment(
+                    "summary",
+                    format!(
+                        r#"Constructs a new instance of <see cref="{}"/> for the Slice decoder."#,
+                        &exception_name
+                    ),
+                )
+                .add_parameter("string?", "message", None, None)
+                .add_base_parameter("message")
+                .add_parameter("global::System.Exception?", "innerException", None, None)
+                .add_base_parameter("innerException")
+                .set_body(initialize_required_fields(fields))
+                .build(),
+        );
+    }
 
     exception_class_builder.add_block(
         FunctionBuilder::new("protected override", "void", "DecodeCore", FunctionType::BlockBody)