Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
69 changes: 60 additions & 9 deletions src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Linq;
using System.Reflection;
using System.Text.Json.Serialization;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.Text;
using SourceGenerators;
Expand Down Expand Up @@ -729,8 +730,11 @@ private static void GenerateCtorParamMetadataInitFunc(SourceWriter writer, strin
{
ImmutableEquatableArray<ParameterGenerationSpec> parameters = typeGenerationSpec.CtorParamGenSpecs;
ImmutableEquatableArray<PropertyInitializerGenerationSpec> propertyInitializers = typeGenerationSpec.PropertyInitializerSpecs;
int paramCount = parameters.Count + propertyInitializers.Count(propInit => !propInit.MatchesConstructorParameter);
Debug.Assert(paramCount > 0);

// out parameters don't appear in metadata - they don't receive values from JSON.
int nonOutParamCount = parameters.Count(p => p.RefKind != (int)RefKind.Out);
int paramCount = nonOutParamCount + propertyInitializers.Count(propInit => !propInit.MatchesConstructorParameter);
Debug.Assert(paramCount > 0 || parameters.Any(p => p.RefKind == (int)RefKind.Out));

writer.WriteLine($"private static {JsonParameterInfoValuesTypeRef}[] {ctorParamMetadataInitMethodName}() => new {JsonParameterInfoValuesTypeRef}[]");
writer.WriteLine('{');
Expand All @@ -739,12 +743,19 @@ private static void GenerateCtorParamMetadataInitFunc(SourceWriter writer, strin
int i = 0;
foreach (ParameterGenerationSpec spec in parameters)
{
// Skip out parameters - they don't receive values from JSON deserialization.
if (spec.RefKind == (int)RefKind.Out)
{
continue;
}

Debug.Assert(spec.ArgsIndex >= 0);
writer.WriteLine($$"""
new()
{
Name = {{FormatStringLiteral(spec.Name)}},
ParameterType = typeof({{spec.ParameterType.FullyQualifiedName}}),
Position = {{spec.ParameterIndex}},
Position = {{spec.ArgsIndex}},
HasDefaultValue = {{FormatBoolLiteral(spec.HasDefaultValue)}},
DefaultValue = {{(spec.HasDefaultValue ? CSharpSyntaxUtilities.FormatLiteral(spec.DefaultValue, spec.ParameterType) : "null")}},
IsNullable = {{FormatBoolLiteral(spec.IsNullable)}},
Expand Down Expand Up @@ -928,21 +939,47 @@ static void ThrowPropertyNullException(string propertyName)
writer.WriteLine('}');
}

// RefKind.RefReadOnlyParameter was added in Roslyn 4.4
private const int RefKindRefReadOnlyParameter = 4;

private static string GetParameterizedCtorInvocationFunc(TypeGenerationSpec typeGenerationSpec)
{
ImmutableEquatableArray<ParameterGenerationSpec> parameters = typeGenerationSpec.CtorParamGenSpecs;
ImmutableEquatableArray<PropertyInitializerGenerationSpec> propertyInitializers = typeGenerationSpec.PropertyInitializerSpecs;

const string ArgsVarName = "args";

StringBuilder sb = new($"static {ArgsVarName} => new {typeGenerationSpec.TypeRef.FullyQualifiedName}(");
bool hasRefOrRefReadonlyParams = parameters.Any(p => p.RefKind == (int)RefKind.Ref || p.RefKind == RefKindRefReadOnlyParameter);

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 == (int)RefKind.Ref || param.RefKind == RefKindRefReadOnlyParameter)
{
// Use ArgsIndex to access the args array (out params don't have entries in args)
sb.Append($"var __temp{param.ParameterIndex} = ({param.ParameterType.FullyQualifiedName}){ArgsVarName}[{param.ArgsIndex}]; ");
}
}

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, ArgsVarName)}, ");
}

sb.Length -= 2; // delete the last ", " token
Expand All @@ -955,17 +992,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, string argsVarName)
{
return param.RefKind switch
{
(int)RefKind.Ref => $"ref __temp{param.ParameterIndex}",
(int)RefKind.Out => $"out var __discard{param.ParameterIndex}",
RefKindRefReadOnlyParameter => $"in __temp{param.ParameterIndex}",
// Use ArgsIndex to access the args array (out params don't have entries in args)
_ => $"({param.ParameterType.FullyQualifiedName}){argsVarName}[{param.ArgsIndex}]", // None or In (in doesn't require keyword at call site)
};
}
}

private static string? GetPrimitiveWriterMethod(TypeGenerationSpec type)
Expand Down
19 changes: 16 additions & 3 deletions src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1494,6 +1494,9 @@ private void ProcessMember(
constructionStrategy = ObjectConstructionStrategy.ParameterizedConstructor;
constructorParameters = new ParameterGenerationSpec[paramCount];

// Compute ArgsIndex for each parameter.
// out parameters don't have entries in the args array.
int argsIndex = 0;
for (int i = 0; i < paramCount; i++)
{
IParameterSymbol parameterInfo = constructor.Parameters[i];
Expand All @@ -1507,14 +1510,19 @@ private void ProcessMember(

TypeRef parameterTypeRef = EnqueueType(parameterInfo.Type, typeToGenerate.Mode);

// out parameters don't receive values from JSON, so they have ArgsIndex = -1.
int currentArgsIndex = parameterInfo.RefKind == RefKind.Out ? -1 : argsIndex++;

constructorParameters[i] = new ParameterGenerationSpec
{
ParameterType = parameterTypeRef,
Name = parameterInfo.Name,
HasDefaultValue = parameterInfo.HasExplicitDefaultValue,
DefaultValue = parameterInfo.HasExplicitDefaultValue ? parameterInfo.ExplicitDefaultValue : null,
ParameterIndex = i,
ArgsIndex = currentArgsIndex,
IsNullable = parameterInfo.IsNullable(),
RefKind = (int)parameterInfo.RefKind,
};
}
}
Expand All @@ -1535,7 +1543,9 @@ private void ProcessMember(

HashSet<string>? memberInitializerNames = null;
List<PropertyInitializerGenerationSpec>? propertyInitializers = null;
int paramCount = constructorParameters?.Length ?? 0;

// Count non-out constructor parameters - out params don't have entries in the args array.
int paramCount = constructorParameters?.Count(p => p.RefKind != (int)RefKind.Out) ?? 0;

// Determine potential init-only or required properties that need to be part of the constructor delegate signature.
foreach (PropertyGenerationSpec property in properties)
Expand Down Expand Up @@ -1575,7 +1585,8 @@ private void ProcessMember(
Name = property.NameSpecifiedInSourceCode,
ParameterType = property.PropertyType,
MatchesConstructorParameter = matchingConstructorParameter is not null,
ParameterIndex = matchingConstructorParameter?.ParameterIndex ?? paramCount++,
// Use ArgsIndex for matching ctor params (excludes out params), or paramCount++ for new ones
ParameterIndex = matchingConstructorParameter?.ArgsIndex ?? paramCount++,
IsNullable = property.PropertyType.CanBeNull && !property.IsSetterNonNullableAnnotation,
};

Expand All @@ -1587,7 +1598,9 @@ private void ProcessMember(
return paramGenSpecs?.FirstOrDefault(MatchesConstructorParameter);

bool MatchesConstructorParameter(ParameterGenerationSpec paramSpec)
=> propSpec.MemberName.Equals(paramSpec.Name, StringComparison.OrdinalIgnoreCase);
// Don't match out parameters - they don't receive values from JSON.
=> paramSpec.RefKind != (int)RefKind.Out &&
propSpec.MemberName.Equals(paramSpec.Name, StringComparison.OrdinalIgnoreCase);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,24 @@ public sealed record ParameterGenerationSpec
// The default value of a constructor parameter can only be a constant
// so it always satisfies the structural equality requirement for the record.
public required object? DefaultValue { get; init; }

/// <summary>
/// The zero-based position of the parameter in the constructor's formal parameter list.
/// </summary>
public required int ParameterIndex { get; init; }

/// <summary>
/// The zero-based index into the args array for this parameter.
/// For out parameters, this is -1 since they don't receive values from the args array.
/// </summary>
public required int ArgsIndex { 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 @@ -287,31 +287,56 @@ private static void PopulateParameterInfoValues(JsonTypeInfo typeInfo, Nullabili
{
Debug.Assert(typeInfo.Converter.ConstructorInfo != null);
ParameterInfo[] parameters = typeInfo.Converter.ConstructorInfo.GetParameters();
int parameterCount = parameters.Length;
JsonParameterInfoValues[] jsonParameters = new JsonParameterInfoValues[parameterCount];

for (int i = 0; i < parameterCount; i++)
// Count non-out parameters - out parameters don't receive values from JSON.
int nonOutParameterCount = 0;
foreach (ParameterInfo param in parameters)
{
if (!param.IsOut)
{
nonOutParameterCount++;
}
}

JsonParameterInfoValues[] jsonParameters = new JsonParameterInfoValues[nonOutParameterCount];

int jsonParamIndex = 0;
for (int i = 0; i < parameters.Length; i++)
{
ParameterInfo reflectionInfo = parameters[i];

// Skip out parameters - they don't receive values from JSON deserialization.
if (reflectionInfo.IsOut)
{
continue;
}

// Trimmed parameter names are reported as null in CoreCLR or "" in Mono.
if (string.IsNullOrEmpty(reflectionInfo.Name))
{
Debug.Assert(typeInfo.Converter.ConstructorInfo.DeclaringType != null);
ThrowHelper.ThrowNotSupportedException_ConstructorContainsNullParameterNames(typeInfo.Converter.ConstructorInfo.DeclaringType);
}

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

JsonParameterInfoValues jsonInfo = new()
{
Name = reflectionInfo.Name,
ParameterType = reflectionInfo.ParameterType,
Position = reflectionInfo.Position,
ParameterType = parameterType,
Position = jsonParamIndex, // Use the position in the args array, not the constructor parameter index
HasDefaultValue = reflectionInfo.HasDefaultValue,
DefaultValue = reflectionInfo.GetDefaultValue(),
IsNullable = DetermineParameterNullability(reflectionInfo, nullabilityCtx) is not NullabilityState.NotNull,
};

jsonParameters[i] = jsonInfo;
jsonParameters[jsonParamIndex] = jsonInfo;
jsonParamIndex++;
}

typeInfo.PopulateParameterInfoValues(jsonParameters);
Expand Down
Loading
Loading