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

Do we want to put this behind some sort of opt-in? The feature is useful, but generally the use of unsafe code should come with some sort of user acknowledgment

Do we want to put this behind some sort of opt-in? The feature is useful, but generally the use of unsafe code should come with some sort of user acknowledgment

It should be already. Only members explicit opted in via annotation (or setters of init-only properties) will prompt unsafe accessor generation.

🤖 Copilot Code Review — PR #126507

Holistic Assessment

Motivation: Justified — generic UnsafeAccessor wrapper classes without their type parameter constraints produce a C# compile error (CS0314/CS0315) when constraints are required, so this is a real correctness bug for users with constrained generic types.

Approach: The final approach (using Roslyn's SymbolDisplayGenericsOptions.IncludeTypeConstraints + string parsing) is simpler and more maintainable than the intermediate manually-built approach that was refactored away. The string parsing is sound (see below). The integration and unit tests cover roundtrip behavior. However the unit tests don't assert on generated source content, and allows ref struct was explicitly claimed (in commit ead15ae6) but is unverified after the subsequent refactor (a04c4845).

Summary: ⚠️ Needs Human Review. The code is correct for the tested scenarios, but there's an unverified allows ref struct claim and the unit tests validate compilation success only, not that the where clause text is actually emitted.

Detailed Findings

✅ String parsing — IndexOf(" where ") is robust

GetTypeParameterConstraintClauses finds the first " where " in the Roslyn display string to extract constraint clauses. This is safe because:

• C# identifiers cannot contain spaces, so neither namespace segments (global::My.Where.Namespace) nor type parameter names (TWhere) can produce the literal " where " (space-where-space) in the display string.
• The verbatim identifier @where renders as @where (with the @ prefix), which would not match.
• The Substring(whereIndex + 1) correctly trims the leading space to produce "where T : ...", and the emitter then prepends a space with $" {c}", giving valid C#.

✅ Generated code format — valid C#

private static class __GenericAccessors_Foo<T> where T : notnull, global::NS.MyBase is syntactically correct. The space separator between > and where is correctly inserted. Multi-parameter cases like where T : notnull where U : class are also handled correctly since the entire substring from the first where to the end is preserved.

✅ Incremental caching — correct

DeclaringTypeParameterConstraintClauses is string? added to a sealed record. string participates in structural equality, so the incremental source generator caching contract is preserved.

✅ namedType2 — necessary

namedType is a pattern variable declared in the DeclaringTypeParameterNames initializer expression (line 1407). In C# object initializers, pattern variables share the scope of the entire object creation expression, so namedType is already in scope for line 1409. Redeclaring it would be a compile error — hence namedType2 is required. The existing style is consistent (lines 1403–1405 also repeat the same pattern without capturing).

⚠️ allows ref struct — claimed but unverified

The commit history shows an explicit allows ref struct implementation was added (ead15ae6) via reflection on ITypeParameterSymbol.AllowsRefLikeType, then removed in the subsequent refactor (a04c4845) with the rationale that IncludeTypeConstraints is "future-proof" for new constraint kinds. However:

• Neither commit included an allows ref struct test case.
• The final PR has no test verifying that where T : allows ref struct is emitted correctly.

While IncludeTypeConstraints very likely renders allows ref struct (it's part of the Roslyn constraint display pipeline), the claim is unverified. A [InlineData] entry for "allows ref struct" in the VariousConstraints theory would close this gap — though note that allows ref struct types cannot actually be serialized by STJ, so verifying at the compilation level only is acceptable.

⚠️ Unit tests assert compilation success, not generated output

All four new unit tests (GenericTypeWithConstrainedTypeParameters_*) call RunJsonSourceGenerator(compilation, logger: logger) and discard the result. RunJsonSourceGenerator asserts no diagnostic above DiagnosticSeverity.Info, which verifies the generated code compiles — but does not assert that the where clause is actually present in the emitted source.

Before this PR, the generator would emit private static class __GenericAccessors_Foo<T> (missing the constraint), and that also compiles fine when the constraint doesn't affect the class body (e.g., where T : notnull alone is not enforced at the call site of the generated private class). The tests may therefore pass even without the fix for some constraint variants.

The InitOnlyProperties_GenericTypeWithConstraints_CanRoundtrip integration test is more meaningful because it exercises the actual generated class, but consider adding at least one unit test that asserts the generated source contains the expected constraint text:

var result = CompilationHelper.RunJsonSourceGenerator(compilation, logger: logger);
var generatedSource = result.NewCompilation.SyntaxTrees
    .Select(t => t.GetText().ToString())
    .FirstOrDefault(s => s.Contains("__GenericAccessors_"));
Assert.Contains("where T : ", generatedSource);

💡 Missing blank line (minor style)

The new s_fullyQualifiedWithConstraints field at line 34 is immediately followed by private const string JsonExtensionDataAttributeFullName without a blank line, whereas other field/constant groups in the class have separating blank lines.

Generated by Code Review for issue #126507 · ◷

/ba-g wasm coreclr timeout is non-blocking