Remove dead net8 code

[xtqqczze]

Since #121853, NetCoreAppMinimum is net10.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[jeffhandley]

Note

This review was generated by the holistic code review workflow being iterated on as part of #130339. Please treat its findings as assistive input for human review.

Holistic Review

Motivation: Removing target-framework-only dead code is justified here: the PR targets NET8_0 branches after the repo moved NetCoreAppMinimum to net10.0 and NetCoreAppCurrent to net11.0. The cleanup reduces maintenance surface without changing shipped code paths.

Approach: The change is narrowly scoped to deleting code that was only compiled for NET8_0 in Base64Helper and DefaultJsonTypeInfoResolver.Helpers. I verified the current project target sets and remaining references rather than relying solely on the PR description.

Summary: ✅ LGTM. I did not find a live target framework that still compiles the removed #if NET8_0 blocks, and the remaining Base64 call sites resolve to System.Text.Ascii helpers rather than the deleted Base64Helper members. No public API surface appears to be removed.

---

Detailed Findings

✅ Target framework coverage — removed branches are no longer live

src/libraries/System.Text.Json/src/System.Text.Json.csproj:4 and src/libraries/System.Text.Json/ref/System.Text.Json.csproj:4 target $(NetCoreAppCurrent);$(NetCoreAppPrevious);$(NetCoreAppMinimum);netstandard2.0;$(NetFrameworkMinimum). In the PR head, Directory.Build.props:71-87 defines NetCoreAppCurrent as net11.0, leaves NetCoreAppPrevious empty except source-build, and sets NetCoreAppMinimum to net10.0. That means the deleted #if NET8_0 workaround in DefaultJsonTypeInfoResolver.Helpers.cs is not compiled by the live STJ source/ref TFMs.

For src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Helper/Base64Helper.cs, the deleted members were also entirely guarded by #if NET8_0; the CoreLib ref project targets $(NetCoreAppCurrent), so the removal does not affect the current CoreLib build path.

✅ Reference and behavior check — remaining call sites use the shared ASCII helpers

I grepped the PR head for the deleted helper names. The remaining Base64 decoder references in src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Base64Helper/Base64DecoderHelper.cs:1656-1718 are explicitly Ascii.VectorContainsNonAsciiChar(...) / Ascii.ExtractAsciiVector(...), and those implementations remain in src/libraries/System.Private.CoreLib/src/System/Text/Ascii.Utility.cs:1549-1704. The STJ nullability path now goes directly through NullabilityInfoContext.Create(parameterInfo) in DefaultJsonTypeInfoResolver.Helpers.cs:802-810, which is the non-NET8 path that was already used for current .NET and the polyfilled non-.NETCoreApp targets.

✅ Public API surface — no public removal detected

The PR does not touch ref/ API sources, and the removed members are private implementation code: Base64Helper is internal and the STJ helper logic was inside a private method. I did not find any removed public members or parameter/name changes that would require API approval or breaking-change handling.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "25b85eda75d970fe239338a0c5a058db8f3b28b4",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "3233f0d0f89402e3be32708772cebaa0f92316f1",
  "last_reviewed_commit": "25b85eda75d970fe239338a0c5a058db8f3b28b4",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "3233f0d0f89402e3be32708772cebaa0f92316f1",
  "last_recorded_worker_run_id": "29755336330",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "a3c2f628f2995931491a6bd848320bab8ff4b335",
      "review_id": 4730925384
    },
    {
      "commit": "25b85eda75d970fe239338a0c5a058db8f3b28b4",
      "review_id": 4736511323
    }
  ]
}

[github-actions]

Holistic Review

Motivation: Since #121853 raised NetCoreAppMinimum to net10, any #if NET8_0 code paths in the shared source can never be compiled and are dead. This PR removes two such blocks: the net8-specific ASCII vector helpers in Base64Helper.cs and the net8 nullability workaround (for #92487) in DefaultJsonTypeInfoResolver.Helpers.cs. This is a clean maintenance change with no behavioral impact on supported targets.

Approach: Straightforward deletion of #if NET8_0 ... #endif regions (168 deletions, 0 additions). The removed VectorContainsNonAsciiChar/ExtractAsciiVector overloads in Base64Helper.cs were net8-only duplicates; all live callers in Base64DecoderHelper.cs, Ascii.Utility.cs, and Ascii.CaseConversion.cs use the Ascii.-prefixed implementations from Ascii.Utility.cs, which are unaffected. The JSON block was a self-contained net8 branch (guarded by #92487's fix landing in net9+), and removing it leaves the net9+ nullabilityCtx.Create(parameterInfo) path intact. No remaining NET8_0 references exist in either touched file, and no unguarded call sites depended on the removed members.

Summary: LGTM. The removals are correct and target genuinely unreachable code given the net10 minimum. One minor, non-blocking observation is noted below. Verdict: approve.

Detailed Findings

• Orphaned helper methods (non-blocking, minor). The only caller of ReflectionExtensions.GetGenericParameterDefinition (src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs) was the deleted net8 nullability block. After this PR it has no callers, and it in turn is the sole caller of GetGenericMemberDefinition, leaving both internal extension methods dead. Since these are internal static members the compiler will not flag them, so this does not break the build, but the cleanup is arguably incomplete — consider removing both methods (or confirming they are intended to remain as general-purpose utilities). This concerns unchanged lines, so it is reported here rather than inline.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 58.7 AIC · ⌖ 14.6 AIC · ⊞ 10K

[xtqqczze]

• Orphaned helper methods (non-blocking, minor). The only caller of ReflectionExtensions.GetGenericParameterDefinition (src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs) was the deleted net8 nullability block. After this PR it has no callers, and it in turn is the sole caller of GetGenericMemberDefinition, leaving both internal extension methods dead. Since these are internal static members the compiler will not flag them, so this does not break the build, but the cleanup is arguably incomplete — consider removing both methods (or confirming they are intended to remain as general-purpose utilities). This concerns unchanged lines, so it is reported here rather than inline.

@eiriktsarpalis can we also remove these dead methods?

[eiriktsarpalis]

If it's dead code it must go.

[xtqqczze]

@ericstj Could you please double check this.

[ericstj]

Seems not necessary to change. I believe that this was meant to indicate the threshold where this would be a full-facade, so by removing that specific version you omit that information should anyone ever decide to bring frameworks back to the package. Your call.

[xtqqczze]

OK, I could just revert a77c8a5?

[github-actions]

Holistic Review

Motivation: Since #121853 raised NetCoreAppMinimum to net10, #if NET8_0 code paths in shared source are dead and can be removed. This re-review covers two new commits added since the initial review: a target-framework condition cleanup in Microsoft.Bcl.Memory.csproj and removal of the now-orphaned ReflectionExtensions.GetGenericParameterDefinition/GetGenericMemberDefinition helpers whose only caller was the net8 nullability block deleted earlier.

Approach: The new ReflectionExtensions.cs change deletes both internal extension methods (48 deletions, 0 additions). This directly resolves the non-blocking cleanup gap flagged in the initial review — I confirmed there are no remaining references to either method anywhere in src/. The Microsoft.Bcl.Memory.csproj change replaces IsTargetFrameworkCompatible(..., 'net9.0') guards with .NETCoreApp identifier checks (GetTargetFrameworkIdentifier(...) == '.NETCoreApp' for OmitResources, and '$(TargetFrameworkIdentifier)' != '.NETCoreApp' for the polyfill/shared-source ItemGroup). With a net10 minimum, every .NETCoreApp target is net9.0-compatible, so these are semantically equivalent. The two previously-separate !net9.0 and != '.NETCoreApp' ItemGroups are correctly merged since their conditions now coincide.

Summary: LGTM. The incremental commits are a clean, correct continuation of the dead-code removal and additionally close the loop on the earlier orphaned-helper observation. No new actionable findings.

Assessment History

• review 4730925384 reviewed commit a3c2f628 with verdict approve/LGTM. Current verdict is unchanged (approve/LGTM). Motivation, approach, and risk remain the same; the new commits merely extend the same low-risk cleanup and resolve that review's lone non-blocking observation.

Detailed Findings

None. The removed helpers now have no callers, and the csproj condition rewrites preserve existing evaluation behavior.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 53.2 AIC · ⌖ 14.6 AIC · ⊞ 10K