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

[manandre]

Fixes #97621

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #97621

Author:	manandre
Assignees:	-
Labels:	area-System.Text.Json
Milestone:	-

[eiriktsarpalis]

There's a test suite in the repo call PropertyVisibilityTests. As part of this change, would it be possible to evaluate the test coverage for shadowed properties (i.e. properties with the new keyword?)

[manandre]

There's a test suite in the repo call PropertyVisibilityTests. As part of this change, would it be possible to evaluate the test coverage for shadowed properties (i.e. properties with the new keyword?)

Some tests around shadowed properties already exist but do not cover combination with required and init-only properties, what should be addressed by this PR.

[eiriktsarpalis]

There's an equivalent implementation in the reflection resolver:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Helpers.cs

Line 141 in 3a382f2

PropertyIsOverriddenAndIgnored(propertyInfo, state.IgnoredProperties))

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

[manandre]

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.

[eiriktsarpalis]

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

runtime/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs

Lines 1037 to 1039 in b94a82d

	// 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.

[eiriktsarpalis]

@manandre I just pushed changes to the branch that moves the deduplication logic back to the member initializer parser method. On closer examination of the parser logic, I was reminded that no such deduplication should be happening in the property parsing logic (code for shadowed members must be generated for use by the metadata-based serializer). At the same time, property-based deduplication uses the JSON name of the property (configurable via JsonPropertyNameAttribute and property naming policies) whereas this deduplication must happen exclusively on the basis of the .NET member name, so a dedicated deduplication loop made sense.

[manandre]

@eiriktsarpalis What do we do for the metadata flow then? I had changed it to keep alignment with source generation flow...

[eiriktsarpalis]

Good catch, I've reverted the changes.

[eiriktsarpalis]

Thank you @manandre !