Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,14 @@ public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializer
foreach (ParameterInfo parameter in parameters)
{
// Every argument must be of supported type.
JsonTypeInfo.ValidateType(parameter.ParameterType);
// For byref parameters (in/ref/out), validate the underlying element type.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot, are there tests covering in, out, and ref?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests currently cover in parameters only. I investigated adding tests for ref and out:

  • ref: Requires source generator changes to emit ref keyword at call sites (currently generates new Type(arg1, arg2) but needs new Type(ref arg1, ref arg2))
  • out: Not meaningful for deserialization - out parameters provide output, not input, so the caller can't provide values from JSON

The in keyword is the most common use case and works because it doesn't require a keyword at call sites (it's the callee that specifies in). Support for ref parameters would require a separate change to the source generator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot can you add tests for ref and out that at least lock in the current behaviors, rather than leaking them totally untested?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 316c65d. The tests lock in current behavior:

  • ref: Works with reflection, skipped for source gen (would need ref keyword at call sites)
  • out: Constructor receives default values (99, "default") since out is for output, not input. Test verifies this expected limitation.

Type parameterType = parameter.ParameterType;
if (parameterType.IsByRef)
{
parameterType = parameterType.GetElementType()!;
}

JsonTypeInfo.ValidateType(parameterType);
}

if (parameterCount <= JsonConstants.UnboxedParameterCountThreshold)
Expand All @@ -75,7 +82,15 @@ public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializer
{
if (i < parameterCount)
{
typeArguments[i + 1] = parameters[i].ParameterType;
// For byref parameters (in/ref/out), use the underlying element type
// since byref types cannot be used as generic type arguments.
Type parameterType = parameters[i].ParameterType;
if (parameterType.IsByRef)
{
parameterType = parameterType.GetElementType()!;
}

typeArguments[i + 1] = parameterType;
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,10 +301,17 @@ private static void PopulateParameterInfoValues(JsonTypeInfo typeInfo, Nullabili
ThrowHelper.ThrowNotSupportedException_ConstructorContainsNullParameterNames(typeInfo.Converter.ConstructorInfo.DeclaringType);
}

// For byref parameters (in/ref/out), use the underlying element type.
Type parameterType = reflectionInfo.ParameterType;
if (parameterType.IsByRef)
{
parameterType = parameterType.GetElementType()!;
}

JsonParameterInfoValues jsonInfo = new()
{
Name = reflectionInfo.Name,
ParameterType = reflectionInfo.ParameterType,
ParameterType = parameterType,
Position = reflectionInfo.Position,
HasDefaultValue = reflectionInfo.HasDefaultValue,
DefaultValue = reflectionInfo.GetDefaultValue(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,44 @@ private static DynamicMethod CreateParameterizedConstructor(ConstructorInfo cons

ILGenerator generator = dynamicMethod.GetILGenerator();

// For byref parameters, we need to store values in local variables and pass addresses.
LocalBuilder?[] locals = new LocalBuilder?[parameterCount];
for (int i = 0; i < parameterCount; i++)
{
Type paramType = parameters[i].ParameterType;
if (paramType.IsByRef)
{
// Declare a local for the underlying type.
Type elementType = paramType.GetElementType()!;
locals[i] = generator.DeclareLocal(elementType);

// Load value from object array, unbox it, and store in the local.
generator.Emit(OpCodes.Ldarg_0);
generator.Emit(OpCodes.Ldc_I4, i);
generator.Emit(OpCodes.Ldelem_Ref);
generator.Emit(OpCodes.Unbox_Any, elementType);
generator.Emit(OpCodes.Stloc, locals[i]!);
}
}

// Now push all arguments onto the stack.
for (int i = 0; i < parameterCount; i++)
{
Type paramType = parameters[i].ParameterType;

generator.Emit(OpCodes.Ldarg_0);
generator.Emit(OpCodes.Ldc_I4, i);
generator.Emit(OpCodes.Ldelem_Ref);
generator.Emit(OpCodes.Unbox_Any, paramType);
if (paramType.IsByRef)
{
// Load address of the local variable.
generator.Emit(OpCodes.Ldloca, locals[i]!);
}
else
{
// Load value from object array and unbox.
generator.Emit(OpCodes.Ldarg_0);
generator.Emit(OpCodes.Ldc_I4, i);
generator.Emit(OpCodes.Ldelem_Ref);
generator.Emit(OpCodes.Unbox_Any, paramType);
}
}

generator.Emit(OpCodes.Newobj, constructor);
Expand Down Expand Up @@ -136,15 +166,25 @@ public override JsonTypeInfo.ParameterizedConstructorDelegate<T, TArg0, TArg1, T
{
Debug.Assert(index <= JsonConstants.UnboxedParameterCountThreshold);

generator.Emit(
index switch
{
0 => OpCodes.Ldarg_0,
1 => OpCodes.Ldarg_1,
2 => OpCodes.Ldarg_2,
3 => OpCodes.Ldarg_3,
_ => throw new InvalidOperationException()
});
// For byref parameters (in/ref/out), load the address of the argument instead of the value.
bool isByRef = parameters[index].ParameterType.IsByRef;

if (isByRef)
{
generator.Emit(OpCodes.Ldarga_S, index);
}
else
{
generator.Emit(
index switch
{
0 => OpCodes.Ldarg_0,
1 => OpCodes.Ldarg_1,
2 => OpCodes.Ldarg_2,
3 => OpCodes.Ldarg_3,
_ => throw new InvalidOperationException()
});
}
}

generator.Emit(OpCodes.Newobj, constructor);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1841,5 +1841,83 @@ public class ClassWithRequiredProperty
{
public required string? Bar { get; set; }
}

[Fact]
public async Task DeserializeType_WithInParameters()
{
string json = @"{""DateTime"":""2020-12-15T00:00:00"",""TimeSpan"":""01:02:03""}";
TypeWith_InParameters result = await Serializer.DeserializeWrapper<TypeWith_InParameters>(json);
Assert.Equal(new DateTime(2020, 12, 15), result.DateTime);
Assert.Equal(new TimeSpan(1, 2, 3), result.TimeSpan);
}

public class TypeWith_InParameters
{
public TypeWith_InParameters(in DateTime dateTime, in TimeSpan timeSpan)
{
DateTime = dateTime;
TimeSpan = timeSpan;
}

public DateTime DateTime { get; set; }
public TimeSpan TimeSpan { get; set; }
}

[Fact]
public async Task DeserializeType_WithMixedByRefParameters()
{
string json = @"{""Value1"":42,""Value2"":""hello"",""Value3"":3.14,""Value4"":true}";
TypeWith_MixedByRefParameters result = await Serializer.DeserializeWrapper<TypeWith_MixedByRefParameters>(json);
Assert.Equal(42, result.Value1);
Assert.Equal("hello", result.Value2);
Assert.Equal(3.14, result.Value3);
Assert.True(result.Value4);
}

public class TypeWith_MixedByRefParameters
{
public TypeWith_MixedByRefParameters(in int value1, string value2, in double value3, bool value4)
{
Value1 = value1;
Value2 = value2;
Value3 = value3;
Value4 = value4;
}

public int Value1 { get; set; }
public string Value2 { get; set; }
public double Value3 { get; set; }
public bool Value4 { get; set; }
}

[Fact]
public async Task DeserializeType_WithLargeInParameters()
{
string json = @"{""A"":1,""B"":2,""C"":3,""D"":4,""E"":5}";
TypeWith_LargeInParameters result = await Serializer.DeserializeWrapper<TypeWith_LargeInParameters>(json);
Assert.Equal(1, result.A);
Assert.Equal(2, result.B);
Assert.Equal(3, result.C);
Assert.Equal(4, result.D);
Assert.Equal(5, result.E);
}

public class TypeWith_LargeInParameters
{
public TypeWith_LargeInParameters(in int a, in int b, in int c, in int d, in int e)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please add tests covering all possible permutations of ref, in, out, and ref readonly applied to all possible types: primitives, structs, reference types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in c3a2b48. Tests now cover all permutations of ref, in, out, and ref readonly with primitives (int), structs (DateTime), and reference types (string).

Modifier Primitive Struct Reference Source Gen
in
ref ❌ (skip)
out ❌ (skip)
ref readonly ❌ (skip)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot make sure the test matrix as visualized above has been updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 7d164fd. The test matrix now shows all tests passing for both reflection and source generation:

Modifier Primitive Struct Reference Source Gen
in
ref
out
ref readonly

{
A = a;
B = b;
C = c;
D = d;
E = e;
}

public int A { get; set; }
public int B { get; set; }
public int C { get; set; }
public int D { get; set; }
public int E { get; set; }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,9 @@ protected ConstructorTests_Metadata(JsonSerializerWrapper stringWrapper)
[JsonSerializable(typeof(Class_ExtraProperty_JsonElementDictionaryExtData))]
[JsonSerializable(typeof(Class_ManyParameters_ExtraProperty_ExtData))]
[JsonSerializable(typeof(ClassWithRequiredProperty))]
[JsonSerializable(typeof(TypeWith_InParameters))]
[JsonSerializable(typeof(TypeWith_MixedByRefParameters))]
[JsonSerializable(typeof(TypeWith_LargeInParameters))]
internal sealed partial class ConstructorTestsContext_Metadata : JsonSerializerContext
{
}
Expand Down Expand Up @@ -313,6 +316,9 @@ public ConstructorTests_Default(JsonSerializerWrapper jsonSerializer) : base(jso
[JsonSerializable(typeof(Class_ExtraProperty_JsonElementDictionaryExtData))]
[JsonSerializable(typeof(Class_ManyParameters_ExtraProperty_ExtData))]
[JsonSerializable(typeof(ClassWithRequiredProperty))]
[JsonSerializable(typeof(TypeWith_InParameters))]
[JsonSerializable(typeof(TypeWith_MixedByRefParameters))]
[JsonSerializable(typeof(TypeWith_LargeInParameters))]
internal sealed partial class ConstructorTestsContext_Default : JsonSerializerContext
{
}
Expand Down
Loading