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

STJ: Avoid duplicate initialization of required or init-only properties #97726

Merged
merged 12 commits into from
Feb 5, 2024
Merged
11 changes: 10 additions & 1 deletion src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -891,7 +891,9 @@ private List<PropertyGenerationSpec> ParsePropertyGenerationSpecs(
// property is static or an indexer
propertyInfo.IsStatic || propertyInfo.Parameters.Length > 0 ||
// It is overridden by a derived property
PropertyIsOverriddenAndIgnored(propertyInfo, state.IgnoredMembers))
PropertyIsOverriddenAndIgnored(propertyInfo, state.IgnoredMembers) ||
Copy link
Member

@eiriktsarpalis eiriktsarpalis Feb 2, 2024

Choose a reason for hiding this comment

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

There's an equivalent implementation in the reflection resolver:

PropertyIsOverriddenAndIgnored(propertyInfo, state.IgnoredProperties))

Should this be updated as well? Do we know the tests are not failing in that case as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we should keep both implementations aligned. For reflection flow, the deduplication is done at one place for both get and set operations, whereas for the source generation flow the fast-path and initializers deduplication logic were split. With this PR it is handled at the same place for all flows.

Copy link
Member

Choose a reason for hiding this comment

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

Taking a closer look at the implementation -- this method ultimately calls into the AddPropertyWithConflictResolution method which handles conflict resolution (including shadowed properties):

// Is the current property hidden by the previously cached property
// (with `new` keyword, or by overriding)?
memberInfo.IsOverriddenOrShadowedBy(otherSymbol) ||

My impression is that the bug is caused by a missed case in that logic, so it seems to me that it's this particular logic that should be debugged.

If you don't know how to debug source generator code, the easiest way to do that is by adding a unit test in System.Text.Json.SourceGeneration.Roslyn*.Unit.Tests and debugging them.

// It is shadowed by a derived property
PropertyIsShadowed(propertyInfo, state.AddedProperties))
{
continue;
}
Expand Down Expand Up @@ -980,6 +982,13 @@ bool PropertyIsOverriddenAndIgnored(IPropertySymbol property, Dictionary<string,
ignoredMember.IsVirtual() &&
SymbolEqualityComparer.Default.Equals(property.Type, ignoredMember.GetMemberType());
}

bool PropertyIsShadowed(IPropertySymbol propertyInfo, Dictionary<string, (PropertyGenerationSpec, ISymbol, int index)> addedProperties)
{
return addedProperties.TryGetValue(propertyInfo.Name, out (PropertyGenerationSpec propertySpec, ISymbol symbol, int index) propertyItem) &&
propertyInfo.IsOverriddenOrShadowedBy(propertyItem.symbol) &&
propertyItem.propertySpec.DefaultIgnoreCondition != JsonIgnoreCondition.Always;
}
}

private ref struct PropertyHierarchyResolutionState(SourceGenerationOptionsSpec? options)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,35 @@ public class ClassWithCustomRequiredPropertyName
public required int PropertyWithInitOnlySetter { get; init; }
}

[Fact]
public async Task DerivedClassWithRequiredProperty()
{
var value = new DerivedClassWithRequiredInitOnlyProperty { MyInt = 42, MyBool = true, MyLong = 4242 };
string json = await Serializer.SerializeWrapper(value);
Assert.Equal("""{"MyInt":42,"MyBool":true,"MyString":null,"MyLong":4242}""", json);
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved

value = await Serializer.DeserializeWrapper<DerivedClassWithRequiredInitOnlyProperty>(json);
Assert.Equal(42, value.MyInt);
Assert.True(value.MyBool);
Assert.Null(value.MyString);
Assert.Equal(4242, value.MyLong);
}

public class BaseClassWithInitOnlyProperty
{
public int MyInt { get; init; }
public bool MyBool { get; init; }
public string MyString { get; set; }
}

public class DerivedClassWithRequiredInitOnlyProperty : BaseClassWithInitOnlyProperty
{
public new required int MyInt { get; init; }
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
public new required bool MyBool { get; set; }
public new string MyString { get; init; }
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
public required long MyLong { get; init; }
}

public static IEnumerable<object[]> InheritedPersonWithRequiredMembersSetsRequiredMembersWorksAsExpectedSources()
{
yield return new object[]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ public RequiredKeywordTests_SourceGen()
[JsonSerializable(typeof(ClassWithRequiredExtensionDataProperty))]
[JsonSerializable(typeof(ClassWithRequiredKeywordAndJsonRequiredCustomAttribute))]
[JsonSerializable(typeof(ClassWithCustomRequiredPropertyName))]
[JsonSerializable(typeof(DerivedClassWithRequiredInitOnlyProperty))]
internal sealed partial class RequiredKeywordTestsContext : JsonSerializerContext
{
}
Expand Down
Loading