Ensure source generated metadata properties are read-only.

[eiriktsarpalis]

Fixes #76535. Note that this introduces the following API that is used by the source generator.

namespace System.Text.Json;

public partial class JsonTypeInfo
{
    public bool IsReadOnly { get; }
    public static void MakeReadOnly();
}

While we don't expect user code will directly invoke either of these APIs, their presence is beneficial. When the metadata is locked, mutation members throw exceptions, and customers can anticipate that possibility by seeing IsReadOnly and inspecting its value and when it changes. There is a possibility user code will begin referencing MakeReadOnly in scenarios we don't anticipate, and/or that we'll want to introduce scoped or validated read-only states in the future. If those scenarios arise, it's possible the shape introduced here won't be ideal. But we have mitigation approaches if needed.

Should be backported to release/7.0.

Fix #76893.

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[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

---

Proposed fix for #76535.

Author:	eiriktsarpalis
Assignees:	-
Labels:	area-System.Text.Json, new-api-needs-documentation
Milestone:	-

[eiriktsarpalis]

Marking as ready-for-review, since we're probably favoring this approach over #76683.

[eiriktsarpalis]

FYI @ericstj @jeffhandley

[eiriktsarpalis]

/backport to release/7.0

[github-actions]

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3229345791

[github-actions]

@eiriktsarpalis backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Ensure source generated metadata properties are read-only.
Applying: Use RemoteExecutor when modifying static metadata.
Applying: Add testing validating that interface metadata is mutable.
Applying: Update error messages
Using index info to reconstruct a base tree...
M	src/libraries/System.Text.Json/src/Resources/Strings.resx
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Text.Json/src/Resources/Strings.resx
CONFLICT (content): Merge conflict in src/libraries/System.Text.Json/src/Resources/Strings.resx
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0004 Update error messages
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

[eiriktsarpalis]

Rebased & squashed changes to ensure the changes from #76869 are incorporated in CI runs.

[jeffhandley]

Per API review, we altered this fix to move MakeReadOnly() to JsonTypeInfo and expose IsReadOnly there too. The description has been updated to reflect that change.

The APIs are now more discoverable by users

• This is a mixed bag.
• We won’t expect user code to call MakeReadOnly(), but it could, and we can contrive scenarios where this causes us grief in the future.
• The visibility of the APIs sheds light on why some mutation APIs may throw exceptions when the instance is readonly, and it can help users troubleshoot by observing IsReadOnly.

Other than where this surfaces in the API, the implementation of the fix remains the same. This ensures users don’t stumble upon mutations when we don’t expect it, leading to unexpected results that are difficult to diagnose.

[jeffhandley]

@bartonjs Can you take a look at the X509 test failures on this please? I can't imagine the failures are related, but I also don't see any existing issues for these specific failures. On the surface, I wonder if this is one of those situations where Window is spontaneously failing and we've only been able to repro this on CI machines.

Note that the release/7.0 version of the PR had solid green on the CI and it was merged.

[GSPP]

MakeReadOnly is a kind of informal name. How about SetReadOnly or Freeze? "Freezing" is the terminology that WPF has chosen.

[eiriktsarpalis]

It's based on precedent from equivalent APIs in JsonSerializerOptions:

#54482 (comment)

[eiriktsarpalis]

@bartonjs Jeremy Barton FTE Can you take a look at the X509 test failures on this please? I can't imagine the failures are related, but I also don't see any existing issues for these specific failures. On the surface, I wonder if this is one of those situations where Window is spontaneously failing and we've only been able to repro this on CI machines.

Note that the release/7.0 version of the PR had solid green on the CI and it was merged.

Appears to be this error. I'm guessing it's caused by a service not running on the particular test machine.