Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that types of ignored or inaccessible properties are not included by the source generator. #87383

Merged
Merged
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
12 changes: 12 additions & 0 deletions src/libraries/System.Text.Json/gen/Helpers/KnownTypeSymbols.cs
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,21 @@ public KnownTypeSymbols(Compilation compilation)
public INamedTypeSymbol? JsonUnmappedMemberHandlingAttributeType => GetOrResolveType("System.Text.Json.Serialization.JsonUnmappedMemberHandlingAttribute", ref _JsonUnmappedMemberHandlingAttributeType);
private Option<INamedTypeSymbol?> _JsonUnmappedMemberHandlingAttributeType;

public INamedTypeSymbol? JsonConstructorAttributeType => GetOrResolveType("System.Text.Json.Serialization.JsonConstructorAttribute", ref _JsonConstructorAttributeType);
private Option<INamedTypeSymbol?> _JsonConstructorAttributeType;

public INamedTypeSymbol? SetsRequiredMembersAttributeType => GetOrResolveType("System.Diagnostics.CodeAnalysis.SetsRequiredMembersAttribute", ref _SetsRequiredMembersAttributeType);
private Option<INamedTypeSymbol?> _SetsRequiredMembersAttributeType;

public INamedTypeSymbol? JsonStringEnumConverterType => GetOrResolveType("System.Text.Json.Serialization.JsonStringEnumConverter", ref _JsonStringEnumConverterType);
private Option<INamedTypeSymbol?> _JsonStringEnumConverterType;

public INamedTypeSymbol? IJsonOnSerializingType => GetOrResolveType(JsonConstants.IJsonOnSerializingFullName, ref _IJsonOnSerializingType);
private Option<INamedTypeSymbol?> _IJsonOnSerializingType;

public INamedTypeSymbol? IJsonOnSerializedType => GetOrResolveType(JsonConstants.IJsonOnSerializedFullName, ref _IJsonOnSerializedType);
private Option<INamedTypeSymbol?> _IJsonOnSerializedType;

// Unsupported types
public INamedTypeSymbol? DelegateType => _DelegateType ??= Compilation.GetSpecialType(SpecialType.System_Delegate);
private INamedTypeSymbol? _DelegateType;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ public static bool CanUseDefaultConstructorForDeserialization(this ITypeSymbol t
public static IEnumerable<IMethodSymbol> GetExplicitlyDeclaredInstanceConstructors(this INamedTypeSymbol type)
=> type.Constructors.Where(ctor => !ctor.IsStatic && !(ctor.IsImplicitlyDeclared && type.IsValueType && ctor.Parameters.Length == 0));

public static bool ContainsAttribute(this ISymbol memberInfo, string attributeFullName)
=> memberInfo.GetAttributes().Any(attr => attr.AttributeClass?.ToDisplayString() == attributeFullName);
public static bool ContainsAttribute(this ISymbol memberInfo, INamedTypeSymbol? attributeType)
=> attributeType != null && memberInfo.GetAttributes().Any(attr => SymbolEqualityComparer.Default.Equals(attr.AttributeClass, attributeType));

public static bool IsVirtual(this ISymbol symbol)
=> symbol.IsVirtual || symbol.IsOverride || symbol.IsAbstract;
Expand Down
287 changes: 104 additions & 183 deletions src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs

Large diffs are not rendered by default.

431 changes: 231 additions & 200 deletions src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs

Large diffs are not rendered by default.

42 changes: 42 additions & 0 deletions src/libraries/System.Text.Json/gen/Model/PropertyGenerationSpec.cs
Original file line number Diff line number Diff line change
Expand Up @@ -137,5 +137,47 @@ public sealed record PropertyGenerationSpec
/// Design-time specified custom converter type.
/// </summary>
public required TypeRef? ConverterType { get; init; }

/// <summary>
/// Determines if the specified property should be included in the fast-path method body.
/// </summary>
public bool ShouldIncludePropertyForFastPath(ContextGenerationSpec contextSpec)
{
// Discard ignored properties
if (DefaultIgnoreCondition is JsonIgnoreCondition.Always)
{
return false;
}

// Discard properties without getters
if (!CanUseGetter)
{
return false;
}

// Discard fields when JsonInclude or IncludeFields aren't enabled.
if (!IsProperty && !HasJsonInclude && !contextSpec.IncludeFields)
{
return false;
}

// Ignore read-only properties/fields if enabled in configuration.
if (IsReadOnly)
{
if (IsProperty)
{
if (contextSpec.IgnoreReadOnlyProperties)
{
return false;
}
}
else if (contextSpec.IgnoreReadOnlyFields)
{
return false;
}
}

return true;
}
}
}
50 changes: 46 additions & 4 deletions src/libraries/System.Text.Json/gen/Model/TypeGenerationSpec.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Diagnostics;
using System.Text.Json.Serialization;
using Microsoft.CodeAnalysis;

namespace System.Text.Json.SourceGeneration
{
Expand Down Expand Up @@ -55,11 +56,11 @@ public sealed record TypeGenerationSpec
public required JsonUnmappedMemberHandling? UnmappedMemberHandling { get; init; }
public required JsonObjectCreationHandling? PreferredPropertyObjectCreationHandling { get; init; }

public required ImmutableEquatableArray<PropertyGenerationSpec>? PropertyGenSpecs { get; init; }
public required ImmutableEquatableArray<PropertyGenerationSpec> PropertyGenSpecs { get; init; }

public required ImmutableEquatableArray<ParameterGenerationSpec>? CtorParamGenSpecs { get; init; }
public required ImmutableEquatableArray<ParameterGenerationSpec> CtorParamGenSpecs { get; init; }

public required ImmutableEquatableArray<PropertyInitializerGenerationSpec>? PropertyInitializerSpecs { get; init; }
public required ImmutableEquatableArray<PropertyInitializerGenerationSpec> PropertyInitializerSpecs { get; init; }

public required CollectionType CollectionType { get; init; }

Expand All @@ -79,10 +80,51 @@ public sealed record TypeGenerationSpec
/// </summary>
public required TypeRef? RuntimeTypeRef { get; init; }

public required TypeRef? ExtensionDataPropertyType { get; init; }
public required bool HasExtensionDataPropertyType { get; init; }

public required TypeRef? ConverterType { get; init; }

public required string? ImmutableCollectionFactoryMethod { get; init; }

public bool IsFastPathSupported()
{
if (IsPolymorphic)
{
return false;
}

switch (ClassType)
{
case ClassType.Object:
if (HasExtensionDataPropertyType)
{
return false;
}

foreach (PropertyGenerationSpec property in PropertyGenSpecs)
{
if (property.PropertyType.SpecialType is SpecialType.System_Object ||
property.NumberHandling is JsonNumberHandling.AllowNamedFloatingPointLiterals
or JsonNumberHandling.WriteAsString ||
property.ConverterType != null)
{
return false;
}
}

return true;

case ClassType.Enumerable:
return CollectionType != CollectionType.IAsyncEnumerableOfT &&
CollectionValueType!.SpecialType is not SpecialType.System_Object;

case ClassType.Dictionary:
return CollectionKeyType!.SpecialType is SpecialType.System_String &&
CollectionValueType!.SpecialType is not SpecialType.System_Object;

default:
return false;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -123,15 +123,11 @@ private static void AddMembersDeclaredBySuperType(
BindingFlags.NonPublic |
BindingFlags.DeclaredOnly;

PropertyInfo[] properties = currentType.GetProperties(BindingFlags);

foreach (PropertyInfo propertyInfo in properties)
foreach (PropertyInfo propertyInfo in currentType.GetProperties(BindingFlags))
{
string propertyName = propertyInfo.Name;

// Ignore indexers and virtual properties that have overrides that were [JsonIgnore]d.
if (propertyInfo.GetIndexParameters().Length > 0 ||
PropertyIsOverriddenAndIgnored(propertyName, propertyInfo.PropertyType, propertyInfo.IsVirtual(), state.IgnoredProperties))
PropertyIsOverriddenAndIgnored(propertyInfo, state.IgnoredProperties))
{
continue;
}
Expand All @@ -151,7 +147,7 @@ private static void AddMembersDeclaredBySuperType(
{
if (propertyInfo.GetCustomAttribute<JsonIncludeAttribute>(inherit: false) != null)
{
ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(propertyName, currentType);
ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(propertyInfo.Name, currentType);
}

// Non-public properties should not be included for (de)serialization.
Expand All @@ -160,13 +156,6 @@ private static void AddMembersDeclaredBySuperType(

foreach (FieldInfo fieldInfo in currentType.GetFields(BindingFlags))
{
string fieldName = fieldInfo.Name;

if (PropertyIsOverriddenAndIgnored(fieldName, fieldInfo.FieldType, currentMemberIsVirtual: false, state.IgnoredProperties))
{
continue;
}

bool hasJsonInclude = fieldInfo.GetCustomAttribute<JsonIncludeAttribute>(inherit: false) != null;

if (fieldInfo.IsPublic)
Expand All @@ -185,7 +174,7 @@ private static void AddMembersDeclaredBySuperType(
{
if (hasJsonInclude)
{
ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(fieldName, currentType);
ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(fieldInfo.Name, currentType);
}

// Non-public fields should not be included for (de)serialization.
Expand Down Expand Up @@ -267,20 +256,12 @@ private static void AddMember(
return numberHandlingAttribute?.UnmappedMemberHandling;
}

private static bool PropertyIsOverriddenAndIgnored(
string currentMemberName,
Type currentMemberType,
bool currentMemberIsVirtual,
Dictionary<string, JsonPropertyInfo>? ignoredMembers)
private static bool PropertyIsOverriddenAndIgnored(PropertyInfo propertyInfo, Dictionary<string, JsonPropertyInfo>? ignoredMembers)
{
if (ignoredMembers == null || !ignoredMembers.TryGetValue(currentMemberName, out JsonPropertyInfo? ignoredMember))
{
return false;
}

return currentMemberType == ignoredMember.PropertyType &&
currentMemberIsVirtual &&
ignoredMember.IsVirtual;
return propertyInfo.IsVirtual() &&
ignoredMembers?.TryGetValue(propertyInfo.Name, out JsonPropertyInfo? ignoredMember) == true &&
ignoredMember.IsVirtual &&
propertyInfo.PropertyType == ignoredMember.PropertyType;
}

private static void PopulateParameterInfoValues(JsonTypeInfo typeInfo)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,8 @@ internal static void PopulateProperties(JsonTypeInfo typeInfo, JsonTypeInfo.Json
JsonSerializerContext? context = typeInfo.Options.TypeInfoResolver as JsonSerializerContext;
JsonPropertyInfo[] properties = propInitFunc(context!);

// TODO update the source generator so that all property
// hierarchy resolution is happening at compile time.
// Regardless of the source generator we need to re-run the naming conflict resolution algorithm
// at run time since it is possible that the naming policy or other configs can be different then.
JsonTypeInfo.PropertyHierarchyResolutionState state = new();

foreach (JsonPropertyInfo jsonPropertyInfo in properties)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1347,6 +1347,8 @@ public void AddPropertyWithConflictResolution(JsonPropertyInfo jsonPropertyInfo,
{
Debug.Assert(!_jsonTypeInfo.IsConfigured);
Debug.Assert(jsonPropertyInfo.MemberName != null, "MemberName can be null in custom JsonPropertyInfo instances and should never be passed in this method");

// Algorithm should be kept in sync with the Roslyn equivalent in JsonSourceGenerator.Emitter.cs
string memberName = jsonPropertyInfo.MemberName;

if (state.AddedProperties.TryAdd(jsonPropertyInfo.Name, (jsonPropertyInfo, Count)))
Expand Down Expand Up @@ -1405,8 +1407,7 @@ public void AddPropertyWithConflictResolution(JsonPropertyInfo jsonPropertyInfo,

if (jsonPropertyInfo.IsIgnored)
{
state.IgnoredProperties ??= new();
state.IgnoredProperties[memberName] = jsonPropertyInfo;
(state.IgnoredProperties ??= new())[memberName] = jsonPropertyInfo;
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Text.Json.Serialization.Metadata;
using System.Threading.Tasks;
using Xunit;

Expand Down Expand Up @@ -395,5 +396,66 @@ public class ClassWithProtected_InitOnlyProperty_WithJsonIncludeProperty
[JsonInclude]
protected string MyString { get; init; }
}

[Fact]
public async Task CanAlwaysRoundtripInternalJsonIncludeProperties()
{
// Internal JsonInclude properties should be honored
// by both reflection and the source generator.

var value = new ClassWithInternalJsonIncludeProperties { X = 1, Y = 2 };
string json = await Serializer.SerializeWrapper(value);
Assert.Equal("""{"X":1,"Y":2}""", json);

value = await Serializer.DeserializeWrapper<ClassWithInternalJsonIncludeProperties>(json);
Assert.Equal(1, value.X);
Assert.Equal(2, value.Y);
}

public class ClassWithInternalJsonIncludeProperties
{
[JsonInclude]
public int X { get; internal set; }
[JsonInclude]
public int Y { internal get; set; }
}

[Fact]
public virtual async Task ClassWithIgnoredAndPrivateMembers_DoesNotIncludeIgnoredMetadata()
{
JsonSerializerOptions options = Serializer.CreateOptions(includeFields: true);

JsonTypeInfo typeInfo = options.GetTypeInfo(typeof(ClassWithIgnoredAndPrivateMembers));

// The contract surfaces the ignored properties but not the private ones
Assert.Equal(2, typeInfo.Properties.Count);
Assert.Contains(typeInfo.Properties, prop => prop.Name == "PublicIgnoredField");
Assert.Contains(typeInfo.Properties, prop => prop.Name == "PublicIgnoredProperty");

// The ignored properties included in the contract do not specify any accessor delegates.
Assert.All(typeInfo.Properties, prop => Assert.True(prop.Get is null));
Assert.All(typeInfo.Properties, prop => Assert.True(prop.Set is null));

string json = await Serializer.SerializeWrapper(new ClassWithIgnoredAndPrivateMembers(), options);
Assert.Equal("{}", json);
}

public class ClassWithIgnoredAndPrivateMembers
{
[JsonIgnore]
public TypeThatShouldNotBeGenerated PublicIgnoredField = new();

private TypeThatShouldNotBeGenerated PrivateField = new();

[JsonIgnore]
public TypeThatShouldNotBeGenerated PublicIgnoredProperty { get; set; } = new();

private TypeThatShouldNotBeGenerated PrivateProperty { get; set; } = new();
}

public class TypeThatShouldNotBeGenerated
{
private protected object _thisLock = new object();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,17 @@ public override async Task TestDictionaryWithPrivateKeyAndValueType()
await Assert.ThrowsAsync<NotSupportedException>(() => Serializer.DeserializeWrapper<DictionaryWithPrivateKeyAndValueType>(json));
}

[Fact]
public override async Task ClassWithIgnoredAndPrivateMembers_DoesNotIncludeIgnoredMetadata()
{
// The type referenced by the ignored/private properties should
// not be included in the supported types by the generator.
JsonSerializerOptions options = Serializer.DefaultOptions;
Assert.Null(options.TypeInfoResolver.GetTypeInfo(typeof(TypeThatShouldNotBeGenerated), options));

await base.ClassWithIgnoredAndPrivateMembers_DoesNotIncludeIgnoredMetadata();
}

[JsonSourceGenerationOptions(GenerationMode = JsonSourceGenerationMode.Metadata)]
[JsonSerializable(typeof(ClassWithNewSlotField))]
[JsonSerializable(typeof(int))]
Expand Down Expand Up @@ -320,7 +331,9 @@ public override async Task TestDictionaryWithPrivateKeyAndValueType()
[JsonSerializable(typeof(IDiamondInterfaceHierarchyWithNamingConflict.IJoinInterface), TypeInfoPropertyName = "IDiamondInterfaceHierarchyWithNamingConflictIJoinInterface")]
[JsonSerializable(typeof(IDiamondInterfaceHierarchyWithNamingConflictUsingAttribute.IJoinInterface), TypeInfoPropertyName = "IDiamondInterfaceHierarchyWithNamingConflictUsingAttributeIJoinInterface")]
[JsonSerializable(typeof(CollectionWithPrivateElementType))]
[JsonSerializable(typeof(DictionaryWithPrivateKeyAndValueType))]
[JsonSerializable(typeof(DictionaryWithPrivateKeyAndValueType))][JsonSerializable(typeof(ClassWithIgnoredAndPrivateMembers))]
[JsonSerializable(typeof(ClassWithInternalJsonIncludeProperties))]
[JsonSerializable(typeof(ClassWithIgnoredAndPrivateMembers))]
internal sealed partial class PropertyVisibilityTestsContext_Metadata : JsonSerializerContext
{
}
Expand Down Expand Up @@ -578,6 +591,8 @@ public void PublicContextAndJsonSerializerOptions()
[JsonSerializable(typeof(IDiamondInterfaceHierarchyWithNamingConflictUsingAttribute.IJoinInterface), TypeInfoPropertyName = "IDiamondInterfaceHierarchyWithNamingConflictUsingAttributeIJoinInterface")]
[JsonSerializable(typeof(CollectionWithPrivateElementType))]
[JsonSerializable(typeof(DictionaryWithPrivateKeyAndValueType))]
[JsonSerializable(typeof(ClassWithInternalJsonIncludeProperties))]
[JsonSerializable(typeof(ClassWithIgnoredAndPrivateMembers))]
internal sealed partial class PropertyVisibilityTestsContext_Default : JsonSerializerContext
{
}
Expand Down