Skip to content
Open
Show file tree
Hide file tree
Changes from 10 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
50 changes: 44 additions & 6 deletions src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -935,14 +935,38 @@ private static string GetParameterizedCtorInvocationFunc(TypeGenerationSpec type

const string ArgsVarName = "args";

StringBuilder sb = new($"static {ArgsVarName} => new {typeGenerationSpec.TypeRef.FullyQualifiedName}(");
// RefKind values from Microsoft.CodeAnalysis.RefKind:
// None = 0, Ref = 1, Out = 2, In = 3, RefReadOnlyParameter = 4
bool hasRefOrRefReadonlyParams = parameters.Any(p => p.RefKind == 1 || p.RefKind == 4); // Ref or RefReadOnlyParameter
Copy link
Member

Choose a reason for hiding this comment

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

@copilot rather than hardcoding numeric values, consider defining constants only for the RefKinds not available in the current version of Roslyn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 7de1ab7. Now using RefKind.Ref and RefKind.Out directly, with only RefKindRefReadOnlyParameter (value 4) defined as a constant since it was added in Roslyn 4.4.


StringBuilder sb;

if (hasRefOrRefReadonlyParams)
{
// For ref/ref readonly parameters, we need a block lambda with temp variables
sb = new($"static {ArgsVarName} => {{ ");

// Declare temp variables for ref and ref readonly parameters
foreach (ParameterGenerationSpec param in parameters)
{
if (param.RefKind == 1 || param.RefKind == 4) // Ref or RefReadOnlyParameter
{
sb.Append($"var __temp{param.ParameterIndex} = ({param.ParameterType.FullyQualifiedName}){ArgsVarName}[{param.ParameterIndex}]; ");
}
}

sb.Append($"return new {typeGenerationSpec.TypeRef.FullyQualifiedName}(");
}
else
{
sb = new($"static {ArgsVarName} => new {typeGenerationSpec.TypeRef.FullyQualifiedName}(");
}

if (parameters.Count > 0)
{
foreach (ParameterGenerationSpec param in parameters)
{
int index = param.ParameterIndex;
sb.Append($"{GetParamUnboxing(param.ParameterType, index)}, ");
sb.Append($"{GetParamExpression(param)}, ");
}

sb.Length -= 2; // delete the last ", " token
Expand All @@ -955,17 +979,31 @@ private static string GetParameterizedCtorInvocationFunc(TypeGenerationSpec type
sb.Append("{ ");
foreach (PropertyInitializerGenerationSpec property in propertyInitializers)
{
sb.Append($"{property.Name} = {GetParamUnboxing(property.ParameterType, property.ParameterIndex)}, ");
sb.Append($"{property.Name} = ({property.ParameterType.FullyQualifiedName}){ArgsVarName}[{property.ParameterIndex}], ");
}

sb.Length -= 2; // delete the last ", " token
sb.Append(" }");
}

if (hasRefOrRefReadonlyParams)
{
sb.Append("; }");
}

return sb.ToString();

static string GetParamUnboxing(TypeRef type, int index)
=> $"({type.FullyQualifiedName}){ArgsVarName}[{index}]";
static string GetParamExpression(ParameterGenerationSpec param)
{
// RefKind values: None = 0, Ref = 1, Out = 2, In = 3, RefReadOnlyParameter = 4
return param.RefKind switch
{
1 => $"ref __temp{param.ParameterIndex}", // Ref
2 => $"out var __discard{param.ParameterIndex}", // Out - use discard pattern
4 => $"in __temp{param.ParameterIndex}", // RefReadOnlyParameter - use in keyword with temp
_ => $"({param.ParameterType.FullyQualifiedName}){ArgsVarName}[{param.ParameterIndex}]", // None or In (in doesn't require keyword at call site)
};
}
}

private static string? GetPrimitiveWriterMethod(TypeGenerationSpec type)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1515,6 +1515,7 @@ private void ProcessMember(
DefaultValue = parameterInfo.HasExplicitDefaultValue ? parameterInfo.ExplicitDefaultValue : null,
ParameterIndex = i,
IsNullable = parameterInfo.IsNullable(),
RefKind = (int)parameterInfo.RefKind,
};
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,5 +33,11 @@ public sealed record ParameterGenerationSpec
public required object? DefaultValue { get; init; }
public required int ParameterIndex { get; init; }
public required bool IsNullable { get; init; }

/// <summary>
/// The ref kind of the parameter: None (0), Ref (1), Out (2), In (3), RefReadOnlyParameter (4).
/// Using int instead of Microsoft.CodeAnalysis.RefKind to avoid dependency issues.
/// </summary>
public required int RefKind { get; init; }
}
}
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
Loading
Loading