Add unsafe evolution migration code fixers

[EgorBo]

Spec: https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md

What it does:

• Adds IL5005 to remove legacy or invalid unsafe modifiers.
  • Type declarations such as classes, structs, interfaces, records, record structs, and enums.
  • Delegate declarations, static constructors, and destructors.
  • Methods, instance constructors, operators, conversion operators, local functions, properties, indexers, and events when:
    • no pointer or function pointer in the signature (we keep those caller-unsafe)
    • no <safety> comment. It's a sign that this unsafe was from unsafe-v1.
  • Keeps unsafe when required by interop, partial members, overrides, or interface contracts.
• Adds IL5006 to add required unsafe blocks, expressions, and contract modifiers.
• Adds unsafe and <safety> placeholders to unannotated interop declarations.
• Covers partial members, interfaces, accessors, stackalloc, async code, top-level statements, and scoped refs.

How to run it from the repo root:

C:\prj\dotnet-latest\dotnet.cmd build .\src\tools\illink\src\ILLink.CodeFix\ILLink.CodeFixProvider.csproj -c Debug --no-restore
$env:EnableUnsafeMigration='true'
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5005 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd build .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0-c Debug --no-restore -p:BuildProjectReferences=false -p:EnableUnsafeMigration=true

I was using the latest daily SDK in C:\prj\dotnet-latest, not sure the bootstrapped one contains all the new C# features.

[Copilot]

Pull request overview

This PR adds a DEBUG-only “unsafe evolution” migration analyzer + code fixers to ILLink, introducing two new IL* diagnostics (IL5005/IL5006) to (1) remove legacy/invalid unsafe modifiers and (2) migrate unsafe usages by adding explicit unsafe contexts and safety placeholders. It also wires up additional MSBuild properties and live-ILLink behavior to enable the migration experience, and adds/extends Roslyn analyzer/codefix tests.

Changes:

• Add UnsafeMigrationAnalyzer + UnsafeMigrationAnalysis to detect modifier removals, required declaration updates, and unsafe-operation sites (behind #if DEBUG).
• Add two new code fix providers (modifier removal + usage migration) and supporting document/AST rewrite helpers (behind #if DEBUG).
• Add new diagnostic strings/IDs and update build/targets/tests to enable and validate the migration workflow.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs	Adds test helpers and extensive test coverage for IL5005/IL5006 codefix behavior.
src/tools/illink/src/ILLink.Shared/SharedStrings.resx	Adds user-facing title/message strings for the new unsafe migration diagnostics.
src/tools/illink/src/ILLink.Shared/DiagnosticId.cs	Adds DEBUG-only diagnostic IDs and introduces an “Unsafe” category range mapping.
src/tools/illink/src/ILLink.Shared/DiagnosticCategory.cs	Adds DEBUG-only Unsafe category constant.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalyzer.cs	New analyzer that reports file-level migration diagnostics when migration is enabled.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalysis.cs	New analysis engine for modifier removals, contract/declaration updates, and unsafe operation locations.
src/tools/illink/src/ILLink.RoslynAnalyzer/MSBuildPropertyOptionNames.cs	Adds DEBUG-only MSBuild property names used to toggle unsafe migration behavior.
src/tools/illink/src/ILLink.RoslynAnalyzer/build/Microsoft.NET.ILLink.Analyzers.props	Exposes additional compiler-visible MSBuild properties used by the migration flow.
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageMigrationCodeFixProvider.cs	New code fix provider that migrates unsafe usage sites (wrap blocks/expressions, etc.).
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageDocumentFixer.cs	New document fixer implementing the migration transformations and heuristics.
src/tools/illink/src/ILLink.CodeFix/UnsafeModifierMigrationCodeFixProvider.cs	New code fix provider that removes legacy unsafe modifiers when eligible.
src/tools/illink/src/ILLink.CodeFix/UnsafeCodeFixHelpers.cs	New shared helpers for adding unsafe contexts and safety documentation.
src/tools/illink/src/ILLink.CodeFix/Resources.resx	Adds code fix titles for the new migration fixers.
eng/liveILLink.targets	Updates live-ILLink wiring to support enabling unsafe migration and design-time analyzer referencing.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_reviewed_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_recorded_worker_run_id": "29686415576",
"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": "74d7482707561a9b391043d905abee5cc4c733c9",
"review_id": 4730737351
}
]
}

[github-actions]

Holistic Review

Motivation: The change implements automated migration tooling (analyzer IL5005/IL5006 plus code-fix providers) for the unsafe-evolution C# language proposal, letting the runtime repo mechanically strip legacy/invalid unsafe modifiers and add required unsafe blocks, expressions, and <safety> documentation. This is a real, well-motivated need: migrating a large codebase to the new memory-safety rules by hand would be impractical and error-prone.

Approach: The approach is sound and appropriately scoped. All new code is wrapped in #if DEBUG and gated behind the opt-in EnableUnsafeMigration MSBuild property, so it adds no shipping public API surface and cannot affect production builds or normal analyzer runs. It reuses the existing ILLink analyzer/code-fix infrastructure (diagnostic IDs, categories, resx strings, CSharpCodeFixVerifier) rather than inventing parallel machinery, and the IL5005/5006 IDs and Unsafe category are correctly slotted into DiagnosticId/DiagnosticCategory without disturbing the existing AOT range mapping. The logic handles a broad matrix of C# constructs (partial members, interfaces, accessors, stackalloc, async, top-level statements, scoped refs), which the extensive test file exercises.

Summary: ⚠️ Needs Human Review. The code is well-structured, self-contained, and thoroughly tested for a migration tool, and it carries essentially zero production risk because it is DEBUG-only and opt-in. I have no blocking findings. However, this is a large (2,661-line) body of intricate syntax/semantic-model manipulation whose correctness is best judged by the ILLink area owners against the actual proposal semantics and by running it over real libraries — a level of validation this static review cannot fully substitute for. Human reviewers should focus on the correctness of the modifier-removal/retention heuristics (interop, overrides, interface contracts, pointer-in-signature detection) and the <safety>/skiplocalsinit handling, and confirm the tool's output on representative source is what the proposal intends.

---

Detailed Findings

✅ Scope & Risk — Correctly isolated tooling change

Every new type and every touched enum/category/resx addition is under #if DEBUG and further gated by the EnableUnsafeMigration compiler-visible property (verified in Microsoft.NET.ILLink.Analyzers.props, MSBuildPropertyOptionNames.cs, and both code-fix providers' RegisterCodeFixesAsync). No ref/ assembly or shipping public API is introduced, so the API-approval gate does not apply. Production analyzer behavior is unchanged.

✅ Codebase Fit — Consistent with existing ILLink patterns

Diagnostic IDs (5005/5006), the new Unsafe category, and the resx entries follow existing conventions. The GetDiagnosticCategory switch inserts the two new IDs before the >= 3050 and <= 6000 => AOT arm so they resolve to Unsafe rather than being swallowed by the AOT range — this ordering is correct and covered by the UnsafeMigrationDiagnostics_HaveUnsafeCategoryWithoutHelpLinks test.

✅ Test Quality — Broad behavioral coverage

The new RequiresUnsafeCodeFixTests.cs adds ~44 focused test methods covering modifier removal/retention, whole-body vs. statement vs. expression wrapping, forward declarations, ref locals, async/yield exclusions, and the migration-disabled case. Using CSharpCodeFixVerifier with before/after sources is the right harness for code-fix validation. Note: I could not build or run these tests in this environment; the human reviewer should confirm they pass in CI.

💡 Brittle syntax-node detection (inline)

One low-severity, non-blocking maintainability note is left inline in UnsafeMigrationAnalysis.cs regarding the GetType().Name == "UnsafeExpressionSyntax" string check used to recognize the preview unsafe(...) expression node.

Note

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

Generated by Holistic Review · 192.5 AIC · ⌖ 12.1 AIC · ⊞ 10K

[github-actions]

💡 This uses a stringly-typed check (ancestor.GetType().Name == "UnsafeExpressionSyntax") to detect the preview unsafe(...) expression node, presumably because the strongly-typed syntax type isn't available in the referenced Roslyn version. That's a reasonable pragmatic workaround, but it is brittle: it silently breaks if the node type is renamed, and it won't match derived/wrapped forms. Consider adding a short comment explaining why the reflection-by-name check is used and, if feasible, guarding it with a SyntaxKind check so the intent survives a Roslyn bump. Non-blocking given this is DEBUG-only tooling.

[EgorBo]

decided to split into many small analyzers/fixers and the first batch is in #131002

[EgorBo]

Spec: https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md

What it does:

• Adds IL5005 to remove legacy or invalid unsafe modifiers.
  • Type declarations such as classes, structs, interfaces, records, record structs, and enums.
  • Delegate declarations, static constructors, and destructors.
  • Methods, instance constructors, operators, conversion operators, local functions, properties, indexers, and events when:
    • no pointer or function pointer in the signature (we keep those caller-unsafe)
    • no <safety> comment. It's a sign that this unsafe was from unsafe-v1.
  • Keeps unsafe when required by interop, partial members, overrides, or interface contracts.
• Adds IL5006 to add required unsafe blocks, expressions, and contract modifiers.
• Adds unsafe and <safety> placeholders to unannotated interop declarations.
• Covers partial members, interfaces, accessors, stackalloc, async code, top-level statements, and scoped refs.

How to run it from the repo root:

C:\prj\dotnet-latest\dotnet.cmd build .\src\tools\illink\src\ILLink.CodeFix\ILLink.CodeFixProvider.csproj -c Debug --no-restore
$env:EnableUnsafeMigration='true'
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5005 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd build .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0-c Debug --no-restore -p:BuildProjectReferences=false -p:EnableUnsafeMigration=true

I was using the latest daily SDK in C:\prj\dotnet-latest, not sure the bootstrapped one contains all the new C# features.

[Copilot]

Pull request overview

This PR adds a DEBUG-only “unsafe evolution” migration analyzer + code fixers to ILLink, introducing two new IL* diagnostics (IL5005/IL5006) to (1) remove legacy/invalid unsafe modifiers and (2) migrate unsafe usages by adding explicit unsafe contexts and safety placeholders. It also wires up additional MSBuild properties and live-ILLink behavior to enable the migration experience, and adds/extends Roslyn analyzer/codefix tests.

Changes:

• Add UnsafeMigrationAnalyzer + UnsafeMigrationAnalysis to detect modifier removals, required declaration updates, and unsafe-operation sites (behind #if DEBUG).
• Add two new code fix providers (modifier removal + usage migration) and supporting document/AST rewrite helpers (behind #if DEBUG).
• Add new diagnostic strings/IDs and update build/targets/tests to enable and validate the migration workflow.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs	Adds test helpers and extensive test coverage for IL5005/IL5006 codefix behavior.
src/tools/illink/src/ILLink.Shared/SharedStrings.resx	Adds user-facing title/message strings for the new unsafe migration diagnostics.
src/tools/illink/src/ILLink.Shared/DiagnosticId.cs	Adds DEBUG-only diagnostic IDs and introduces an “Unsafe” category range mapping.
src/tools/illink/src/ILLink.Shared/DiagnosticCategory.cs	Adds DEBUG-only Unsafe category constant.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalyzer.cs	New analyzer that reports file-level migration diagnostics when migration is enabled.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalysis.cs	New analysis engine for modifier removals, contract/declaration updates, and unsafe operation locations.
src/tools/illink/src/ILLink.RoslynAnalyzer/MSBuildPropertyOptionNames.cs	Adds DEBUG-only MSBuild property names used to toggle unsafe migration behavior.
src/tools/illink/src/ILLink.RoslynAnalyzer/build/Microsoft.NET.ILLink.Analyzers.props	Exposes additional compiler-visible MSBuild properties used by the migration flow.
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageMigrationCodeFixProvider.cs	New code fix provider that migrates unsafe usage sites (wrap blocks/expressions, etc.).
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageDocumentFixer.cs	New document fixer implementing the migration transformations and heuristics.
src/tools/illink/src/ILLink.CodeFix/UnsafeModifierMigrationCodeFixProvider.cs	New code fix provider that removes legacy unsafe modifiers when eligible.
src/tools/illink/src/ILLink.CodeFix/UnsafeCodeFixHelpers.cs	New shared helpers for adding unsafe contexts and safety documentation.
src/tools/illink/src/ILLink.CodeFix/Resources.resx	Adds code fix titles for the new migration fixers.
eng/liveILLink.targets	Updates live-ILLink wiring to support enabling unsafe migration and design-time analyzer referencing.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_reviewed_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_recorded_worker_run_id": "29686415576",
"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": "74d7482707561a9b391043d905abee5cc4c733c9",
"review_id": 4730737351
}
]
}

[github-actions]

Holistic Review

Motivation: The change implements automated migration tooling (analyzer IL5005/IL5006 plus code-fix providers) for the unsafe-evolution C# language proposal, letting the runtime repo mechanically strip legacy/invalid unsafe modifiers and add required unsafe blocks, expressions, and <safety> documentation. This is a real, well-motivated need: migrating a large codebase to the new memory-safety rules by hand would be impractical and error-prone.

Approach: The approach is sound and appropriately scoped. All new code is wrapped in #if DEBUG and gated behind the opt-in EnableUnsafeMigration MSBuild property, so it adds no shipping public API surface and cannot affect production builds or normal analyzer runs. It reuses the existing ILLink analyzer/code-fix infrastructure (diagnostic IDs, categories, resx strings, CSharpCodeFixVerifier) rather than inventing parallel machinery, and the IL5005/5006 IDs and Unsafe category are correctly slotted into DiagnosticId/DiagnosticCategory without disturbing the existing AOT range mapping. The logic handles a broad matrix of C# constructs (partial members, interfaces, accessors, stackalloc, async, top-level statements, scoped refs), which the extensive test file exercises.

Summary: ⚠️ Needs Human Review. The code is well-structured, self-contained, and thoroughly tested for a migration tool, and it carries essentially zero production risk because it is DEBUG-only and opt-in. I have no blocking findings. However, this is a large (2,661-line) body of intricate syntax/semantic-model manipulation whose correctness is best judged by the ILLink area owners against the actual proposal semantics and by running it over real libraries — a level of validation this static review cannot fully substitute for. Human reviewers should focus on the correctness of the modifier-removal/retention heuristics (interop, overrides, interface contracts, pointer-in-signature detection) and the <safety>/skiplocalsinit handling, and confirm the tool's output on representative source is what the proposal intends.

---

Detailed Findings

✅ Scope & Risk — Correctly isolated tooling change

Every new type and every touched enum/category/resx addition is under #if DEBUG and further gated by the EnableUnsafeMigration compiler-visible property (verified in Microsoft.NET.ILLink.Analyzers.props, MSBuildPropertyOptionNames.cs, and both code-fix providers' RegisterCodeFixesAsync). No ref/ assembly or shipping public API is introduced, so the API-approval gate does not apply. Production analyzer behavior is unchanged.

✅ Codebase Fit — Consistent with existing ILLink patterns

Diagnostic IDs (5005/5006), the new Unsafe category, and the resx entries follow existing conventions. The GetDiagnosticCategory switch inserts the two new IDs before the >= 3050 and <= 6000 => AOT arm so they resolve to Unsafe rather than being swallowed by the AOT range — this ordering is correct and covered by the UnsafeMigrationDiagnostics_HaveUnsafeCategoryWithoutHelpLinks test.

✅ Test Quality — Broad behavioral coverage

The new RequiresUnsafeCodeFixTests.cs adds ~44 focused test methods covering modifier removal/retention, whole-body vs. statement vs. expression wrapping, forward declarations, ref locals, async/yield exclusions, and the migration-disabled case. Using CSharpCodeFixVerifier with before/after sources is the right harness for code-fix validation. Note: I could not build or run these tests in this environment; the human reviewer should confirm they pass in CI.

💡 Brittle syntax-node detection (inline)

One low-severity, non-blocking maintainability note is left inline in UnsafeMigrationAnalysis.cs regarding the GetType().Name == "UnsafeExpressionSyntax" string check used to recognize the preview unsafe(...) expression node.

Note

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

Generated by Holistic Review · 192.5 AIC · ⌖ 12.1 AIC · ⊞ 10K

[github-actions]

💡 This uses a stringly-typed check (ancestor.GetType().Name == "UnsafeExpressionSyntax") to detect the preview unsafe(...) expression node, presumably because the strongly-typed syntax type isn't available in the referenced Roslyn version. That's a reasonable pragmatic workaround, but it is brittle: it silently breaks if the node type is renamed, and it won't match derived/wrapped forms. Consider adding a short comment explaining why the reflection-by-name check is used and, if feasible, guarding it with a SyntaxKind check so the intent survives a Roslyn bump. Non-blocking given this is DEBUG-only tooling.

[EgorBo]

decided to split into many small analyzers/fixers and the first batch is in #131002

[EgorBo]

Spec: https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md

What it does:

• Adds IL5005 to remove legacy or invalid unsafe modifiers.
  • Type declarations such as classes, structs, interfaces, records, record structs, and enums.
  • Delegate declarations, static constructors, and destructors.
  • Methods, instance constructors, operators, conversion operators, local functions, properties, indexers, and events when:
    • no pointer or function pointer in the signature (we keep those caller-unsafe)
    • no <safety> comment. It's a sign that this unsafe was from unsafe-v1.
  • Keeps unsafe when required by interop, partial members, overrides, or interface contracts.
• Adds IL5006 to add required unsafe blocks, expressions, and contract modifiers.
• Adds unsafe and <safety> placeholders to unannotated interop declarations.
• Covers partial members, interfaces, accessors, stackalloc, async code, top-level statements, and scoped refs.

How to run it from the repo root:

C:\prj\dotnet-latest\dotnet.cmd build .\src\tools\illink\src\ILLink.CodeFix\ILLink.CodeFixProvider.csproj -c Debug --no-restore
$env:EnableUnsafeMigration='true'
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5005 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd build .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0-c Debug --no-restore -p:BuildProjectReferences=false -p:EnableUnsafeMigration=true

I was using the latest daily SDK in C:\prj\dotnet-latest, not sure the bootstrapped one contains all the new C# features.

[Copilot]

Pull request overview

This PR adds a DEBUG-only “unsafe evolution” migration analyzer + code fixers to ILLink, introducing two new IL* diagnostics (IL5005/IL5006) to (1) remove legacy/invalid unsafe modifiers and (2) migrate unsafe usages by adding explicit unsafe contexts and safety placeholders. It also wires up additional MSBuild properties and live-ILLink behavior to enable the migration experience, and adds/extends Roslyn analyzer/codefix tests.

Changes:

• Add UnsafeMigrationAnalyzer + UnsafeMigrationAnalysis to detect modifier removals, required declaration updates, and unsafe-operation sites (behind #if DEBUG).
• Add two new code fix providers (modifier removal + usage migration) and supporting document/AST rewrite helpers (behind #if DEBUG).
• Add new diagnostic strings/IDs and update build/targets/tests to enable and validate the migration workflow.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs	Adds test helpers and extensive test coverage for IL5005/IL5006 codefix behavior.
src/tools/illink/src/ILLink.Shared/SharedStrings.resx	Adds user-facing title/message strings for the new unsafe migration diagnostics.
src/tools/illink/src/ILLink.Shared/DiagnosticId.cs	Adds DEBUG-only diagnostic IDs and introduces an “Unsafe” category range mapping.
src/tools/illink/src/ILLink.Shared/DiagnosticCategory.cs	Adds DEBUG-only Unsafe category constant.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalyzer.cs	New analyzer that reports file-level migration diagnostics when migration is enabled.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalysis.cs	New analysis engine for modifier removals, contract/declaration updates, and unsafe operation locations.
src/tools/illink/src/ILLink.RoslynAnalyzer/MSBuildPropertyOptionNames.cs	Adds DEBUG-only MSBuild property names used to toggle unsafe migration behavior.
src/tools/illink/src/ILLink.RoslynAnalyzer/build/Microsoft.NET.ILLink.Analyzers.props	Exposes additional compiler-visible MSBuild properties used by the migration flow.
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageMigrationCodeFixProvider.cs	New code fix provider that migrates unsafe usage sites (wrap blocks/expressions, etc.).
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageDocumentFixer.cs	New document fixer implementing the migration transformations and heuristics.
src/tools/illink/src/ILLink.CodeFix/UnsafeModifierMigrationCodeFixProvider.cs	New code fix provider that removes legacy unsafe modifiers when eligible.
src/tools/illink/src/ILLink.CodeFix/UnsafeCodeFixHelpers.cs	New shared helpers for adding unsafe contexts and safety documentation.
src/tools/illink/src/ILLink.CodeFix/Resources.resx	Adds code fix titles for the new migration fixers.
eng/liveILLink.targets	Updates live-ILLink wiring to support enabling unsafe migration and design-time analyzer referencing.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_reviewed_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_recorded_worker_run_id": "29686415576",
"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": "74d7482707561a9b391043d905abee5cc4c733c9",
"review_id": 4730737351
}
]
}

[github-actions]

Holistic Review

Motivation: The change implements automated migration tooling (analyzer IL5005/IL5006 plus code-fix providers) for the unsafe-evolution C# language proposal, letting the runtime repo mechanically strip legacy/invalid unsafe modifiers and add required unsafe blocks, expressions, and <safety> documentation. This is a real, well-motivated need: migrating a large codebase to the new memory-safety rules by hand would be impractical and error-prone.

Approach: The approach is sound and appropriately scoped. All new code is wrapped in #if DEBUG and gated behind the opt-in EnableUnsafeMigration MSBuild property, so it adds no shipping public API surface and cannot affect production builds or normal analyzer runs. It reuses the existing ILLink analyzer/code-fix infrastructure (diagnostic IDs, categories, resx strings, CSharpCodeFixVerifier) rather than inventing parallel machinery, and the IL5005/5006 IDs and Unsafe category are correctly slotted into DiagnosticId/DiagnosticCategory without disturbing the existing AOT range mapping. The logic handles a broad matrix of C# constructs (partial members, interfaces, accessors, stackalloc, async, top-level statements, scoped refs), which the extensive test file exercises.

Summary: ⚠️ Needs Human Review. The code is well-structured, self-contained, and thoroughly tested for a migration tool, and it carries essentially zero production risk because it is DEBUG-only and opt-in. I have no blocking findings. However, this is a large (2,661-line) body of intricate syntax/semantic-model manipulation whose correctness is best judged by the ILLink area owners against the actual proposal semantics and by running it over real libraries — a level of validation this static review cannot fully substitute for. Human reviewers should focus on the correctness of the modifier-removal/retention heuristics (interop, overrides, interface contracts, pointer-in-signature detection) and the <safety>/skiplocalsinit handling, and confirm the tool's output on representative source is what the proposal intends.

---

Detailed Findings

✅ Scope & Risk — Correctly isolated tooling change

Every new type and every touched enum/category/resx addition is under #if DEBUG and further gated by the EnableUnsafeMigration compiler-visible property (verified in Microsoft.NET.ILLink.Analyzers.props, MSBuildPropertyOptionNames.cs, and both code-fix providers' RegisterCodeFixesAsync). No ref/ assembly or shipping public API is introduced, so the API-approval gate does not apply. Production analyzer behavior is unchanged.

✅ Codebase Fit — Consistent with existing ILLink patterns

Diagnostic IDs (5005/5006), the new Unsafe category, and the resx entries follow existing conventions. The GetDiagnosticCategory switch inserts the two new IDs before the >= 3050 and <= 6000 => AOT arm so they resolve to Unsafe rather than being swallowed by the AOT range — this ordering is correct and covered by the UnsafeMigrationDiagnostics_HaveUnsafeCategoryWithoutHelpLinks test.

✅ Test Quality — Broad behavioral coverage

The new RequiresUnsafeCodeFixTests.cs adds ~44 focused test methods covering modifier removal/retention, whole-body vs. statement vs. expression wrapping, forward declarations, ref locals, async/yield exclusions, and the migration-disabled case. Using CSharpCodeFixVerifier with before/after sources is the right harness for code-fix validation. Note: I could not build or run these tests in this environment; the human reviewer should confirm they pass in CI.

💡 Brittle syntax-node detection (inline)

One low-severity, non-blocking maintainability note is left inline in UnsafeMigrationAnalysis.cs regarding the GetType().Name == "UnsafeExpressionSyntax" string check used to recognize the preview unsafe(...) expression node.

Note

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

Generated by Holistic Review · 192.5 AIC · ⌖ 12.1 AIC · ⊞ 10K

[github-actions]

💡 This uses a stringly-typed check (ancestor.GetType().Name == "UnsafeExpressionSyntax") to detect the preview unsafe(...) expression node, presumably because the strongly-typed syntax type isn't available in the referenced Roslyn version. That's a reasonable pragmatic workaround, but it is brittle: it silently breaks if the node type is renamed, and it won't match derived/wrapped forms. Consider adding a short comment explaining why the reflection-by-name check is used and, if feasible, guarding it with a SyntaxKind check so the intent survives a Roslyn bump. Non-blocking given this is DEBUG-only tooling.

[EgorBo]

decided to split into many small analyzers/fixers and the first batch is in #131002

[EgorBo]

Spec: https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md

What it does:

• Adds IL5005 to remove legacy or invalid unsafe modifiers.
  • Type declarations such as classes, structs, interfaces, records, record structs, and enums.
  • Delegate declarations, static constructors, and destructors.
  • Methods, instance constructors, operators, conversion operators, local functions, properties, indexers, and events when:
    • no pointer or function pointer in the signature (we keep those caller-unsafe)
    • no <safety> comment. It's a sign that this unsafe was from unsafe-v1.
  • Keeps unsafe when required by interop, partial members, overrides, or interface contracts.
• Adds IL5006 to add required unsafe blocks, expressions, and contract modifiers.
• Adds unsafe and <safety> placeholders to unannotated interop declarations.
• Covers partial members, interfaces, accessors, stackalloc, async code, top-level statements, and scoped refs.

How to run it from the repo root:

C:\prj\dotnet-latest\dotnet.cmd build .\src\tools\illink\src\ILLink.CodeFix\ILLink.CodeFixProvider.csproj -c Debug --no-restore
$env:EnableUnsafeMigration='true'
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5005 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd build .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0-c Debug --no-restore -p:BuildProjectReferences=false -p:EnableUnsafeMigration=true

I was using the latest daily SDK in C:\prj\dotnet-latest, not sure the bootstrapped one contains all the new C# features.

[Copilot]

Pull request overview

This PR adds a DEBUG-only “unsafe evolution” migration analyzer + code fixers to ILLink, introducing two new IL* diagnostics (IL5005/IL5006) to (1) remove legacy/invalid unsafe modifiers and (2) migrate unsafe usages by adding explicit unsafe contexts and safety placeholders. It also wires up additional MSBuild properties and live-ILLink behavior to enable the migration experience, and adds/extends Roslyn analyzer/codefix tests.

Changes:

• Add UnsafeMigrationAnalyzer + UnsafeMigrationAnalysis to detect modifier removals, required declaration updates, and unsafe-operation sites (behind #if DEBUG).
• Add two new code fix providers (modifier removal + usage migration) and supporting document/AST rewrite helpers (behind #if DEBUG).
• Add new diagnostic strings/IDs and update build/targets/tests to enable and validate the migration workflow.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs	Adds test helpers and extensive test coverage for IL5005/IL5006 codefix behavior.
src/tools/illink/src/ILLink.Shared/SharedStrings.resx	Adds user-facing title/message strings for the new unsafe migration diagnostics.
src/tools/illink/src/ILLink.Shared/DiagnosticId.cs	Adds DEBUG-only diagnostic IDs and introduces an “Unsafe” category range mapping.
src/tools/illink/src/ILLink.Shared/DiagnosticCategory.cs	Adds DEBUG-only Unsafe category constant.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalyzer.cs	New analyzer that reports file-level migration diagnostics when migration is enabled.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalysis.cs	New analysis engine for modifier removals, contract/declaration updates, and unsafe operation locations.
src/tools/illink/src/ILLink.RoslynAnalyzer/MSBuildPropertyOptionNames.cs	Adds DEBUG-only MSBuild property names used to toggle unsafe migration behavior.
src/tools/illink/src/ILLink.RoslynAnalyzer/build/Microsoft.NET.ILLink.Analyzers.props	Exposes additional compiler-visible MSBuild properties used by the migration flow.
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageMigrationCodeFixProvider.cs	New code fix provider that migrates unsafe usage sites (wrap blocks/expressions, etc.).
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageDocumentFixer.cs	New document fixer implementing the migration transformations and heuristics.
src/tools/illink/src/ILLink.CodeFix/UnsafeModifierMigrationCodeFixProvider.cs	New code fix provider that removes legacy unsafe modifiers when eligible.
src/tools/illink/src/ILLink.CodeFix/UnsafeCodeFixHelpers.cs	New shared helpers for adding unsafe contexts and safety documentation.
src/tools/illink/src/ILLink.CodeFix/Resources.resx	Adds code fix titles for the new migration fixers.
eng/liveILLink.targets	Updates live-ILLink wiring to support enabling unsafe migration and design-time analyzer referencing.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_reviewed_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_recorded_worker_run_id": "29686415576",
"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": "74d7482707561a9b391043d905abee5cc4c733c9",
"review_id": 4730737351
}
]
}

[github-actions]

Holistic Review

Motivation: The change implements automated migration tooling (analyzer IL5005/IL5006 plus code-fix providers) for the unsafe-evolution C# language proposal, letting the runtime repo mechanically strip legacy/invalid unsafe modifiers and add required unsafe blocks, expressions, and <safety> documentation. This is a real, well-motivated need: migrating a large codebase to the new memory-safety rules by hand would be impractical and error-prone.

Approach: The approach is sound and appropriately scoped. All new code is wrapped in #if DEBUG and gated behind the opt-in EnableUnsafeMigration MSBuild property, so it adds no shipping public API surface and cannot affect production builds or normal analyzer runs. It reuses the existing ILLink analyzer/code-fix infrastructure (diagnostic IDs, categories, resx strings, CSharpCodeFixVerifier) rather than inventing parallel machinery, and the IL5005/5006 IDs and Unsafe category are correctly slotted into DiagnosticId/DiagnosticCategory without disturbing the existing AOT range mapping. The logic handles a broad matrix of C# constructs (partial members, interfaces, accessors, stackalloc, async, top-level statements, scoped refs), which the extensive test file exercises.

Summary: ⚠️ Needs Human Review. The code is well-structured, self-contained, and thoroughly tested for a migration tool, and it carries essentially zero production risk because it is DEBUG-only and opt-in. I have no blocking findings. However, this is a large (2,661-line) body of intricate syntax/semantic-model manipulation whose correctness is best judged by the ILLink area owners against the actual proposal semantics and by running it over real libraries — a level of validation this static review cannot fully substitute for. Human reviewers should focus on the correctness of the modifier-removal/retention heuristics (interop, overrides, interface contracts, pointer-in-signature detection) and the <safety>/skiplocalsinit handling, and confirm the tool's output on representative source is what the proposal intends.

---

Detailed Findings

✅ Scope & Risk — Correctly isolated tooling change

Every new type and every touched enum/category/resx addition is under #if DEBUG and further gated by the EnableUnsafeMigration compiler-visible property (verified in Microsoft.NET.ILLink.Analyzers.props, MSBuildPropertyOptionNames.cs, and both code-fix providers' RegisterCodeFixesAsync). No ref/ assembly or shipping public API is introduced, so the API-approval gate does not apply. Production analyzer behavior is unchanged.

✅ Codebase Fit — Consistent with existing ILLink patterns

Diagnostic IDs (5005/5006), the new Unsafe category, and the resx entries follow existing conventions. The GetDiagnosticCategory switch inserts the two new IDs before the >= 3050 and <= 6000 => AOT arm so they resolve to Unsafe rather than being swallowed by the AOT range — this ordering is correct and covered by the UnsafeMigrationDiagnostics_HaveUnsafeCategoryWithoutHelpLinks test.

✅ Test Quality — Broad behavioral coverage

The new RequiresUnsafeCodeFixTests.cs adds ~44 focused test methods covering modifier removal/retention, whole-body vs. statement vs. expression wrapping, forward declarations, ref locals, async/yield exclusions, and the migration-disabled case. Using CSharpCodeFixVerifier with before/after sources is the right harness for code-fix validation. Note: I could not build or run these tests in this environment; the human reviewer should confirm they pass in CI.

💡 Brittle syntax-node detection (inline)

One low-severity, non-blocking maintainability note is left inline in UnsafeMigrationAnalysis.cs regarding the GetType().Name == "UnsafeExpressionSyntax" string check used to recognize the preview unsafe(...) expression node.

Note

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

Generated by Holistic Review · 192.5 AIC · ⌖ 12.1 AIC · ⊞ 10K

[github-actions]

💡 This uses a stringly-typed check (ancestor.GetType().Name == "UnsafeExpressionSyntax") to detect the preview unsafe(...) expression node, presumably because the strongly-typed syntax type isn't available in the referenced Roslyn version. That's a reasonable pragmatic workaround, but it is brittle: it silently breaks if the node type is renamed, and it won't match derived/wrapped forms. Consider adding a short comment explaining why the reflection-by-name check is used and, if feasible, guarding it with a SyntaxKind check so the intent survives a Roslyn bump. Non-blocking given this is DEBUG-only tooling.

[EgorBo]

decided to split into many small analyzers/fixers and the first batch is in #131002

[EgorBo]

Spec: https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md

What it does:

• Adds IL5005 to remove legacy or invalid unsafe modifiers.
  • Type declarations such as classes, structs, interfaces, records, record structs, and enums.
  • Delegate declarations, static constructors, and destructors.
  • Methods, instance constructors, operators, conversion operators, local functions, properties, indexers, and events when:
    • no pointer or function pointer in the signature (we keep those caller-unsafe)
    • no <safety> comment. It's a sign that this unsafe was from unsafe-v1.
  • Keeps unsafe when required by interop, partial members, overrides, or interface contracts.
• Adds IL5006 to add required unsafe blocks, expressions, and contract modifiers.
• Adds unsafe and <safety> placeholders to unannotated interop declarations.
• Covers partial members, interfaces, accessors, stackalloc, async code, top-level statements, and scoped refs.

How to run it from the repo root:

C:\prj\dotnet-latest\dotnet.cmd build .\src\tools\illink\src\ILLink.CodeFix\ILLink.CodeFixProvider.csproj -c Debug --no-restore
$env:EnableUnsafeMigration='true'
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5005 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd build .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0-c Debug --no-restore -p:BuildProjectReferences=false -p:EnableUnsafeMigration=true

I was using the latest daily SDK in C:\prj\dotnet-latest, not sure the bootstrapped one contains all the new C# features.

[Copilot]

Pull request overview

This PR adds a DEBUG-only “unsafe evolution” migration analyzer + code fixers to ILLink, introducing two new IL* diagnostics (IL5005/IL5006) to (1) remove legacy/invalid unsafe modifiers and (2) migrate unsafe usages by adding explicit unsafe contexts and safety placeholders. It also wires up additional MSBuild properties and live-ILLink behavior to enable the migration experience, and adds/extends Roslyn analyzer/codefix tests.

Changes:

• Add UnsafeMigrationAnalyzer + UnsafeMigrationAnalysis to detect modifier removals, required declaration updates, and unsafe-operation sites (behind #if DEBUG).
• Add two new code fix providers (modifier removal + usage migration) and supporting document/AST rewrite helpers (behind #if DEBUG).
• Add new diagnostic strings/IDs and update build/targets/tests to enable and validate the migration workflow.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs	Adds test helpers and extensive test coverage for IL5005/IL5006 codefix behavior.
src/tools/illink/src/ILLink.Shared/SharedStrings.resx	Adds user-facing title/message strings for the new unsafe migration diagnostics.
src/tools/illink/src/ILLink.Shared/DiagnosticId.cs	Adds DEBUG-only diagnostic IDs and introduces an “Unsafe” category range mapping.
src/tools/illink/src/ILLink.Shared/DiagnosticCategory.cs	Adds DEBUG-only Unsafe category constant.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalyzer.cs	New analyzer that reports file-level migration diagnostics when migration is enabled.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalysis.cs	New analysis engine for modifier removals, contract/declaration updates, and unsafe operation locations.
src/tools/illink/src/ILLink.RoslynAnalyzer/MSBuildPropertyOptionNames.cs	Adds DEBUG-only MSBuild property names used to toggle unsafe migration behavior.
src/tools/illink/src/ILLink.RoslynAnalyzer/build/Microsoft.NET.ILLink.Analyzers.props	Exposes additional compiler-visible MSBuild properties used by the migration flow.
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageMigrationCodeFixProvider.cs	New code fix provider that migrates unsafe usage sites (wrap blocks/expressions, etc.).
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageDocumentFixer.cs	New document fixer implementing the migration transformations and heuristics.
src/tools/illink/src/ILLink.CodeFix/UnsafeModifierMigrationCodeFixProvider.cs	New code fix provider that removes legacy unsafe modifiers when eligible.
src/tools/illink/src/ILLink.CodeFix/UnsafeCodeFixHelpers.cs	New shared helpers for adding unsafe contexts and safety documentation.
src/tools/illink/src/ILLink.CodeFix/Resources.resx	Adds code fix titles for the new migration fixers.
eng/liveILLink.targets	Updates live-ILLink wiring to support enabling unsafe migration and design-time analyzer referencing.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_reviewed_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_recorded_worker_run_id": "29686415576",
"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": "74d7482707561a9b391043d905abee5cc4c733c9",
"review_id": 4730737351
}
]
}

[github-actions]

Holistic Review

Motivation: The change implements automated migration tooling (analyzer IL5005/IL5006 plus code-fix providers) for the unsafe-evolution C# language proposal, letting the runtime repo mechanically strip legacy/invalid unsafe modifiers and add required unsafe blocks, expressions, and <safety> documentation. This is a real, well-motivated need: migrating a large codebase to the new memory-safety rules by hand would be impractical and error-prone.

Approach: The approach is sound and appropriately scoped. All new code is wrapped in #if DEBUG and gated behind the opt-in EnableUnsafeMigration MSBuild property, so it adds no shipping public API surface and cannot affect production builds or normal analyzer runs. It reuses the existing ILLink analyzer/code-fix infrastructure (diagnostic IDs, categories, resx strings, CSharpCodeFixVerifier) rather than inventing parallel machinery, and the IL5005/5006 IDs and Unsafe category are correctly slotted into DiagnosticId/DiagnosticCategory without disturbing the existing AOT range mapping. The logic handles a broad matrix of C# constructs (partial members, interfaces, accessors, stackalloc, async, top-level statements, scoped refs), which the extensive test file exercises.

Summary: ⚠️ Needs Human Review. The code is well-structured, self-contained, and thoroughly tested for a migration tool, and it carries essentially zero production risk because it is DEBUG-only and opt-in. I have no blocking findings. However, this is a large (2,661-line) body of intricate syntax/semantic-model manipulation whose correctness is best judged by the ILLink area owners against the actual proposal semantics and by running it over real libraries — a level of validation this static review cannot fully substitute for. Human reviewers should focus on the correctness of the modifier-removal/retention heuristics (interop, overrides, interface contracts, pointer-in-signature detection) and the <safety>/skiplocalsinit handling, and confirm the tool's output on representative source is what the proposal intends.

---

Detailed Findings

✅ Scope & Risk — Correctly isolated tooling change

Every new type and every touched enum/category/resx addition is under #if DEBUG and further gated by the EnableUnsafeMigration compiler-visible property (verified in Microsoft.NET.ILLink.Analyzers.props, MSBuildPropertyOptionNames.cs, and both code-fix providers' RegisterCodeFixesAsync). No ref/ assembly or shipping public API is introduced, so the API-approval gate does not apply. Production analyzer behavior is unchanged.

✅ Codebase Fit — Consistent with existing ILLink patterns

Diagnostic IDs (5005/5006), the new Unsafe category, and the resx entries follow existing conventions. The GetDiagnosticCategory switch inserts the two new IDs before the >= 3050 and <= 6000 => AOT arm so they resolve to Unsafe rather than being swallowed by the AOT range — this ordering is correct and covered by the UnsafeMigrationDiagnostics_HaveUnsafeCategoryWithoutHelpLinks test.

✅ Test Quality — Broad behavioral coverage

The new RequiresUnsafeCodeFixTests.cs adds ~44 focused test methods covering modifier removal/retention, whole-body vs. statement vs. expression wrapping, forward declarations, ref locals, async/yield exclusions, and the migration-disabled case. Using CSharpCodeFixVerifier with before/after sources is the right harness for code-fix validation. Note: I could not build or run these tests in this environment; the human reviewer should confirm they pass in CI.

💡 Brittle syntax-node detection (inline)

One low-severity, non-blocking maintainability note is left inline in UnsafeMigrationAnalysis.cs regarding the GetType().Name == "UnsafeExpressionSyntax" string check used to recognize the preview unsafe(...) expression node.

Note

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

Generated by Holistic Review · 192.5 AIC · ⌖ 12.1 AIC · ⊞ 10K

[github-actions]

💡 This uses a stringly-typed check (ancestor.GetType().Name == "UnsafeExpressionSyntax") to detect the preview unsafe(...) expression node, presumably because the strongly-typed syntax type isn't available in the referenced Roslyn version. That's a reasonable pragmatic workaround, but it is brittle: it silently breaks if the node type is renamed, and it won't match derived/wrapped forms. Consider adding a short comment explaining why the reflection-by-name check is used and, if feasible, guarding it with a SyntaxKind check so the intent survives a Roslyn bump. Non-blocking given this is DEBUG-only tooling.

[EgorBo]

decided to split into many small analyzers/fixers and the first batch is in #131002

[EgorBo]

Spec: https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md

What it does:

• Adds IL5005 to remove legacy or invalid unsafe modifiers.
  • Type declarations such as classes, structs, interfaces, records, record structs, and enums.
  • Delegate declarations, static constructors, and destructors.
  • Methods, instance constructors, operators, conversion operators, local functions, properties, indexers, and events when:
    • no pointer or function pointer in the signature (we keep those caller-unsafe)
    • no <safety> comment. It's a sign that this unsafe was from unsafe-v1.
  • Keeps unsafe when required by interop, partial members, overrides, or interface contracts.
• Adds IL5006 to add required unsafe blocks, expressions, and contract modifiers.
• Adds unsafe and <safety> placeholders to unannotated interop declarations.
• Covers partial members, interfaces, accessors, stackalloc, async code, top-level statements, and scoped refs.

How to run it from the repo root:

C:\prj\dotnet-latest\dotnet.cmd build .\src\tools\illink\src\ILLink.CodeFix\ILLink.CodeFixProvider.csproj -c Debug --no-restore
$env:EnableUnsafeMigration='true'
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5005 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd build .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0-c Debug --no-restore -p:BuildProjectReferences=false -p:EnableUnsafeMigration=true

I was using the latest daily SDK in C:\prj\dotnet-latest, not sure the bootstrapped one contains all the new C# features.

[Copilot]

Pull request overview

This PR adds a DEBUG-only “unsafe evolution” migration analyzer + code fixers to ILLink, introducing two new IL* diagnostics (IL5005/IL5006) to (1) remove legacy/invalid unsafe modifiers and (2) migrate unsafe usages by adding explicit unsafe contexts and safety placeholders. It also wires up additional MSBuild properties and live-ILLink behavior to enable the migration experience, and adds/extends Roslyn analyzer/codefix tests.

Changes:

• Add UnsafeMigrationAnalyzer + UnsafeMigrationAnalysis to detect modifier removals, required declaration updates, and unsafe-operation sites (behind #if DEBUG).
• Add two new code fix providers (modifier removal + usage migration) and supporting document/AST rewrite helpers (behind #if DEBUG).
• Add new diagnostic strings/IDs and update build/targets/tests to enable and validate the migration workflow.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs	Adds test helpers and extensive test coverage for IL5005/IL5006 codefix behavior.
src/tools/illink/src/ILLink.Shared/SharedStrings.resx	Adds user-facing title/message strings for the new unsafe migration diagnostics.
src/tools/illink/src/ILLink.Shared/DiagnosticId.cs	Adds DEBUG-only diagnostic IDs and introduces an “Unsafe” category range mapping.
src/tools/illink/src/ILLink.Shared/DiagnosticCategory.cs	Adds DEBUG-only Unsafe category constant.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalyzer.cs	New analyzer that reports file-level migration diagnostics when migration is enabled.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalysis.cs	New analysis engine for modifier removals, contract/declaration updates, and unsafe operation locations.
src/tools/illink/src/ILLink.RoslynAnalyzer/MSBuildPropertyOptionNames.cs	Adds DEBUG-only MSBuild property names used to toggle unsafe migration behavior.
src/tools/illink/src/ILLink.RoslynAnalyzer/build/Microsoft.NET.ILLink.Analyzers.props	Exposes additional compiler-visible MSBuild properties used by the migration flow.
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageMigrationCodeFixProvider.cs	New code fix provider that migrates unsafe usage sites (wrap blocks/expressions, etc.).
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageDocumentFixer.cs	New document fixer implementing the migration transformations and heuristics.
src/tools/illink/src/ILLink.CodeFix/UnsafeModifierMigrationCodeFixProvider.cs	New code fix provider that removes legacy unsafe modifiers when eligible.
src/tools/illink/src/ILLink.CodeFix/UnsafeCodeFixHelpers.cs	New shared helpers for adding unsafe contexts and safety documentation.
src/tools/illink/src/ILLink.CodeFix/Resources.resx	Adds code fix titles for the new migration fixers.
eng/liveILLink.targets	Updates live-ILLink wiring to support enabling unsafe migration and design-time analyzer referencing.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_reviewed_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_recorded_worker_run_id": "29686415576",
"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": "74d7482707561a9b391043d905abee5cc4c733c9",
"review_id": 4730737351
}
]
}

[github-actions]

Holistic Review

Motivation: The change implements automated migration tooling (analyzer IL5005/IL5006 plus code-fix providers) for the unsafe-evolution C# language proposal, letting the runtime repo mechanically strip legacy/invalid unsafe modifiers and add required unsafe blocks, expressions, and <safety> documentation. This is a real, well-motivated need: migrating a large codebase to the new memory-safety rules by hand would be impractical and error-prone.

Approach: The approach is sound and appropriately scoped. All new code is wrapped in #if DEBUG and gated behind the opt-in EnableUnsafeMigration MSBuild property, so it adds no shipping public API surface and cannot affect production builds or normal analyzer runs. It reuses the existing ILLink analyzer/code-fix infrastructure (diagnostic IDs, categories, resx strings, CSharpCodeFixVerifier) rather than inventing parallel machinery, and the IL5005/5006 IDs and Unsafe category are correctly slotted into DiagnosticId/DiagnosticCategory without disturbing the existing AOT range mapping. The logic handles a broad matrix of C# constructs (partial members, interfaces, accessors, stackalloc, async, top-level statements, scoped refs), which the extensive test file exercises.

Summary: ⚠️ Needs Human Review. The code is well-structured, self-contained, and thoroughly tested for a migration tool, and it carries essentially zero production risk because it is DEBUG-only and opt-in. I have no blocking findings. However, this is a large (2,661-line) body of intricate syntax/semantic-model manipulation whose correctness is best judged by the ILLink area owners against the actual proposal semantics and by running it over real libraries — a level of validation this static review cannot fully substitute for. Human reviewers should focus on the correctness of the modifier-removal/retention heuristics (interop, overrides, interface contracts, pointer-in-signature detection) and the <safety>/skiplocalsinit handling, and confirm the tool's output on representative source is what the proposal intends.

---

Detailed Findings

✅ Scope & Risk — Correctly isolated tooling change

Every new type and every touched enum/category/resx addition is under #if DEBUG and further gated by the EnableUnsafeMigration compiler-visible property (verified in Microsoft.NET.ILLink.Analyzers.props, MSBuildPropertyOptionNames.cs, and both code-fix providers' RegisterCodeFixesAsync). No ref/ assembly or shipping public API is introduced, so the API-approval gate does not apply. Production analyzer behavior is unchanged.

✅ Codebase Fit — Consistent with existing ILLink patterns

Diagnostic IDs (5005/5006), the new Unsafe category, and the resx entries follow existing conventions. The GetDiagnosticCategory switch inserts the two new IDs before the >= 3050 and <= 6000 => AOT arm so they resolve to Unsafe rather than being swallowed by the AOT range — this ordering is correct and covered by the UnsafeMigrationDiagnostics_HaveUnsafeCategoryWithoutHelpLinks test.

✅ Test Quality — Broad behavioral coverage

The new RequiresUnsafeCodeFixTests.cs adds ~44 focused test methods covering modifier removal/retention, whole-body vs. statement vs. expression wrapping, forward declarations, ref locals, async/yield exclusions, and the migration-disabled case. Using CSharpCodeFixVerifier with before/after sources is the right harness for code-fix validation. Note: I could not build or run these tests in this environment; the human reviewer should confirm they pass in CI.

💡 Brittle syntax-node detection (inline)

One low-severity, non-blocking maintainability note is left inline in UnsafeMigrationAnalysis.cs regarding the GetType().Name == "UnsafeExpressionSyntax" string check used to recognize the preview unsafe(...) expression node.

Note

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

Generated by Holistic Review · 192.5 AIC · ⌖ 12.1 AIC · ⊞ 10K

[github-actions]

💡 This uses a stringly-typed check (ancestor.GetType().Name == "UnsafeExpressionSyntax") to detect the preview unsafe(...) expression node, presumably because the strongly-typed syntax type isn't available in the referenced Roslyn version. That's a reasonable pragmatic workaround, but it is brittle: it silently breaks if the node type is renamed, and it won't match derived/wrapped forms. Consider adding a short comment explaining why the reflection-by-name check is used and, if feasible, guarding it with a SyntaxKind check so the intent survives a Roslyn bump. Non-blocking given this is DEBUG-only tooling.

[EgorBo]

decided to split into many small analyzers/fixers and the first batch is in #131002

[EgorBo]

Spec: https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md

What it does:

• Adds IL5005 to remove legacy or invalid unsafe modifiers.
  • Type declarations such as classes, structs, interfaces, records, record structs, and enums.
  • Delegate declarations, static constructors, and destructors.
  • Methods, instance constructors, operators, conversion operators, local functions, properties, indexers, and events when:
    • no pointer or function pointer in the signature (we keep those caller-unsafe)
    • no <safety> comment. It's a sign that this unsafe was from unsafe-v1.
  • Keeps unsafe when required by interop, partial members, overrides, or interface contracts.
• Adds IL5006 to add required unsafe blocks, expressions, and contract modifiers.
• Adds unsafe and <safety> placeholders to unannotated interop declarations.
• Covers partial members, interfaces, accessors, stackalloc, async code, top-level statements, and scoped refs.

How to run it from the repo root:

C:\prj\dotnet-latest\dotnet.cmd build .\src\tools\illink\src\ILLink.CodeFix\ILLink.CodeFixProvider.csproj -c Debug --no-restore
$env:EnableUnsafeMigration='true'
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5005 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd build .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0-c Debug --no-restore -p:BuildProjectReferences=false -p:EnableUnsafeMigration=true

I was using the latest daily SDK in C:\prj\dotnet-latest, not sure the bootstrapped one contains all the new C# features.

[Copilot]

Pull request overview

This PR adds a DEBUG-only “unsafe evolution” migration analyzer + code fixers to ILLink, introducing two new IL* diagnostics (IL5005/IL5006) to (1) remove legacy/invalid unsafe modifiers and (2) migrate unsafe usages by adding explicit unsafe contexts and safety placeholders. It also wires up additional MSBuild properties and live-ILLink behavior to enable the migration experience, and adds/extends Roslyn analyzer/codefix tests.

Changes:

• Add UnsafeMigrationAnalyzer + UnsafeMigrationAnalysis to detect modifier removals, required declaration updates, and unsafe-operation sites (behind #if DEBUG).
• Add two new code fix providers (modifier removal + usage migration) and supporting document/AST rewrite helpers (behind #if DEBUG).
• Add new diagnostic strings/IDs and update build/targets/tests to enable and validate the migration workflow.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs	Adds test helpers and extensive test coverage for IL5005/IL5006 codefix behavior.
src/tools/illink/src/ILLink.Shared/SharedStrings.resx	Adds user-facing title/message strings for the new unsafe migration diagnostics.
src/tools/illink/src/ILLink.Shared/DiagnosticId.cs	Adds DEBUG-only diagnostic IDs and introduces an “Unsafe” category range mapping.
src/tools/illink/src/ILLink.Shared/DiagnosticCategory.cs	Adds DEBUG-only Unsafe category constant.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalyzer.cs	New analyzer that reports file-level migration diagnostics when migration is enabled.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalysis.cs	New analysis engine for modifier removals, contract/declaration updates, and unsafe operation locations.
src/tools/illink/src/ILLink.RoslynAnalyzer/MSBuildPropertyOptionNames.cs	Adds DEBUG-only MSBuild property names used to toggle unsafe migration behavior.
src/tools/illink/src/ILLink.RoslynAnalyzer/build/Microsoft.NET.ILLink.Analyzers.props	Exposes additional compiler-visible MSBuild properties used by the migration flow.
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageMigrationCodeFixProvider.cs	New code fix provider that migrates unsafe usage sites (wrap blocks/expressions, etc.).
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageDocumentFixer.cs	New document fixer implementing the migration transformations and heuristics.
src/tools/illink/src/ILLink.CodeFix/UnsafeModifierMigrationCodeFixProvider.cs	New code fix provider that removes legacy unsafe modifiers when eligible.
src/tools/illink/src/ILLink.CodeFix/UnsafeCodeFixHelpers.cs	New shared helpers for adding unsafe contexts and safety documentation.
src/tools/illink/src/ILLink.CodeFix/Resources.resx	Adds code fix titles for the new migration fixers.
eng/liveILLink.targets	Updates live-ILLink wiring to support enabling unsafe migration and design-time analyzer referencing.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_reviewed_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_recorded_worker_run_id": "29686415576",
"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": "74d7482707561a9b391043d905abee5cc4c733c9",
"review_id": 4730737351
}
]
}

[github-actions]

Holistic Review

Motivation: The change implements automated migration tooling (analyzer IL5005/IL5006 plus code-fix providers) for the unsafe-evolution C# language proposal, letting the runtime repo mechanically strip legacy/invalid unsafe modifiers and add required unsafe blocks, expressions, and <safety> documentation. This is a real, well-motivated need: migrating a large codebase to the new memory-safety rules by hand would be impractical and error-prone.

Approach: The approach is sound and appropriately scoped. All new code is wrapped in #if DEBUG and gated behind the opt-in EnableUnsafeMigration MSBuild property, so it adds no shipping public API surface and cannot affect production builds or normal analyzer runs. It reuses the existing ILLink analyzer/code-fix infrastructure (diagnostic IDs, categories, resx strings, CSharpCodeFixVerifier) rather than inventing parallel machinery, and the IL5005/5006 IDs and Unsafe category are correctly slotted into DiagnosticId/DiagnosticCategory without disturbing the existing AOT range mapping. The logic handles a broad matrix of C# constructs (partial members, interfaces, accessors, stackalloc, async, top-level statements, scoped refs), which the extensive test file exercises.

Summary: ⚠️ Needs Human Review. The code is well-structured, self-contained, and thoroughly tested for a migration tool, and it carries essentially zero production risk because it is DEBUG-only and opt-in. I have no blocking findings. However, this is a large (2,661-line) body of intricate syntax/semantic-model manipulation whose correctness is best judged by the ILLink area owners against the actual proposal semantics and by running it over real libraries — a level of validation this static review cannot fully substitute for. Human reviewers should focus on the correctness of the modifier-removal/retention heuristics (interop, overrides, interface contracts, pointer-in-signature detection) and the <safety>/skiplocalsinit handling, and confirm the tool's output on representative source is what the proposal intends.

---

Detailed Findings

✅ Scope & Risk — Correctly isolated tooling change

Every new type and every touched enum/category/resx addition is under #if DEBUG and further gated by the EnableUnsafeMigration compiler-visible property (verified in Microsoft.NET.ILLink.Analyzers.props, MSBuildPropertyOptionNames.cs, and both code-fix providers' RegisterCodeFixesAsync). No ref/ assembly or shipping public API is introduced, so the API-approval gate does not apply. Production analyzer behavior is unchanged.

✅ Codebase Fit — Consistent with existing ILLink patterns

Diagnostic IDs (5005/5006), the new Unsafe category, and the resx entries follow existing conventions. The GetDiagnosticCategory switch inserts the two new IDs before the >= 3050 and <= 6000 => AOT arm so they resolve to Unsafe rather than being swallowed by the AOT range — this ordering is correct and covered by the UnsafeMigrationDiagnostics_HaveUnsafeCategoryWithoutHelpLinks test.

✅ Test Quality — Broad behavioral coverage

The new RequiresUnsafeCodeFixTests.cs adds ~44 focused test methods covering modifier removal/retention, whole-body vs. statement vs. expression wrapping, forward declarations, ref locals, async/yield exclusions, and the migration-disabled case. Using CSharpCodeFixVerifier with before/after sources is the right harness for code-fix validation. Note: I could not build or run these tests in this environment; the human reviewer should confirm they pass in CI.

💡 Brittle syntax-node detection (inline)

One low-severity, non-blocking maintainability note is left inline in UnsafeMigrationAnalysis.cs regarding the GetType().Name == "UnsafeExpressionSyntax" string check used to recognize the preview unsafe(...) expression node.

Note

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

Generated by Holistic Review · 192.5 AIC · ⌖ 12.1 AIC · ⊞ 10K

[github-actions]

💡 This uses a stringly-typed check (ancestor.GetType().Name == "UnsafeExpressionSyntax") to detect the preview unsafe(...) expression node, presumably because the strongly-typed syntax type isn't available in the referenced Roslyn version. That's a reasonable pragmatic workaround, but it is brittle: it silently breaks if the node type is renamed, and it won't match derived/wrapped forms. Consider adding a short comment explaining why the reflection-by-name check is used and, if feasible, guarding it with a SyntaxKind check so the intent survives a Roslyn bump. Non-blocking given this is DEBUG-only tooling.

[EgorBo]

decided to split into many small analyzers/fixers and the first batch is in #131002

[EgorBo]

Spec: https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md

What it does:

• Adds IL5005 to remove legacy or invalid unsafe modifiers.
  • Type declarations such as classes, structs, interfaces, records, record structs, and enums.
  • Delegate declarations, static constructors, and destructors.
  • Methods, instance constructors, operators, conversion operators, local functions, properties, indexers, and events when:
    • no pointer or function pointer in the signature (we keep those caller-unsafe)
    • no <safety> comment. It's a sign that this unsafe was from unsafe-v1.
  • Keeps unsafe when required by interop, partial members, overrides, or interface contracts.
• Adds IL5006 to add required unsafe blocks, expressions, and contract modifiers.
• Adds unsafe and <safety> placeholders to unannotated interop declarations.
• Covers partial members, interfaces, accessors, stackalloc, async code, top-level statements, and scoped refs.

How to run it from the repo root:

C:\prj\dotnet-latest\dotnet.cmd build .\src\tools\illink\src\ILLink.CodeFix\ILLink.CodeFixProvider.csproj -c Debug --no-restore
$env:EnableUnsafeMigration='true'
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5005 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd format analyzers .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0--diagnostics IL5006 --severity info --no-restore
C:\prj\dotnet-latest\dotnet.cmd build .\src\libraries\System.Text.Json\src\System.Text.Json.csproj -f net11.0-c Debug --no-restore -p:BuildProjectReferences=false -p:EnableUnsafeMigration=true

I was using the latest daily SDK in C:\prj\dotnet-latest, not sure the bootstrapped one contains all the new C# features.

[Copilot]

Pull request overview

This PR adds a DEBUG-only “unsafe evolution” migration analyzer + code fixers to ILLink, introducing two new IL* diagnostics (IL5005/IL5006) to (1) remove legacy/invalid unsafe modifiers and (2) migrate unsafe usages by adding explicit unsafe contexts and safety placeholders. It also wires up additional MSBuild properties and live-ILLink behavior to enable the migration experience, and adds/extends Roslyn analyzer/codefix tests.

Changes:

• Add UnsafeMigrationAnalyzer + UnsafeMigrationAnalysis to detect modifier removals, required declaration updates, and unsafe-operation sites (behind #if DEBUG).
• Add two new code fix providers (modifier removal + usage migration) and supporting document/AST rewrite helpers (behind #if DEBUG).
• Add new diagnostic strings/IDs and update build/targets/tests to enable and validate the migration workflow.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs	Adds test helpers and extensive test coverage for IL5005/IL5006 codefix behavior.
src/tools/illink/src/ILLink.Shared/SharedStrings.resx	Adds user-facing title/message strings for the new unsafe migration diagnostics.
src/tools/illink/src/ILLink.Shared/DiagnosticId.cs	Adds DEBUG-only diagnostic IDs and introduces an “Unsafe” category range mapping.
src/tools/illink/src/ILLink.Shared/DiagnosticCategory.cs	Adds DEBUG-only Unsafe category constant.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalyzer.cs	New analyzer that reports file-level migration diagnostics when migration is enabled.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalysis.cs	New analysis engine for modifier removals, contract/declaration updates, and unsafe operation locations.
src/tools/illink/src/ILLink.RoslynAnalyzer/MSBuildPropertyOptionNames.cs	Adds DEBUG-only MSBuild property names used to toggle unsafe migration behavior.
src/tools/illink/src/ILLink.RoslynAnalyzer/build/Microsoft.NET.ILLink.Analyzers.props	Exposes additional compiler-visible MSBuild properties used by the migration flow.
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageMigrationCodeFixProvider.cs	New code fix provider that migrates unsafe usage sites (wrap blocks/expressions, etc.).
src/tools/illink/src/ILLink.CodeFix/UnsafeUsageDocumentFixer.cs	New document fixer implementing the migration transformations and heuristics.
src/tools/illink/src/ILLink.CodeFix/UnsafeModifierMigrationCodeFixProvider.cs	New code fix provider that removes legacy unsafe modifiers when eligible.
src/tools/illink/src/ILLink.CodeFix/UnsafeCodeFixHelpers.cs	New shared helpers for adding unsafe contexts and safety documentation.
src/tools/illink/src/ILLink.CodeFix/Resources.resx	Adds code fix titles for the new migration fixers.
eng/liveILLink.targets	Updates live-ILLink wiring to support enabling unsafe migration and design-time analyzer referencing.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_reviewed_commit": "74d7482707561a9b391043d905abee5cc4c733c9",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "53a3837195f7d8e7d714306fab5679562b2261d2",
"last_recorded_worker_run_id": "29686415576",
"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": "74d7482707561a9b391043d905abee5cc4c733c9",
"review_id": 4730737351
}
]
}

[github-actions]

Holistic Review

Motivation: The change implements automated migration tooling (analyzer IL5005/IL5006 plus code-fix providers) for the unsafe-evolution C# language proposal, letting the runtime repo mechanically strip legacy/invalid unsafe modifiers and add required unsafe blocks, expressions, and <safety> documentation. This is a real, well-motivated need: migrating a large codebase to the new memory-safety rules by hand would be impractical and error-prone.

Approach: The approach is sound and appropriately scoped. All new code is wrapped in #if DEBUG and gated behind the opt-in EnableUnsafeMigration MSBuild property, so it adds no shipping public API surface and cannot affect production builds or normal analyzer runs. It reuses the existing ILLink analyzer/code-fix infrastructure (diagnostic IDs, categories, resx strings, CSharpCodeFixVerifier) rather than inventing parallel machinery, and the IL5005/5006 IDs and Unsafe category are correctly slotted into DiagnosticId/DiagnosticCategory without disturbing the existing AOT range mapping. The logic handles a broad matrix of C# constructs (partial members, interfaces, accessors, stackalloc, async, top-level statements, scoped refs), which the extensive test file exercises.

Summary: ⚠️ Needs Human Review. The code is well-structured, self-contained, and thoroughly tested for a migration tool, and it carries essentially zero production risk because it is DEBUG-only and opt-in. I have no blocking findings. However, this is a large (2,661-line) body of intricate syntax/semantic-model manipulation whose correctness is best judged by the ILLink area owners against the actual proposal semantics and by running it over real libraries — a level of validation this static review cannot fully substitute for. Human reviewers should focus on the correctness of the modifier-removal/retention heuristics (interop, overrides, interface contracts, pointer-in-signature detection) and the <safety>/skiplocalsinit handling, and confirm the tool's output on representative source is what the proposal intends.

---

Detailed Findings

✅ Scope & Risk — Correctly isolated tooling change

Every new type and every touched enum/category/resx addition is under #if DEBUG and further gated by the EnableUnsafeMigration compiler-visible property (verified in Microsoft.NET.ILLink.Analyzers.props, MSBuildPropertyOptionNames.cs, and both code-fix providers' RegisterCodeFixesAsync). No ref/ assembly or shipping public API is introduced, so the API-approval gate does not apply. Production analyzer behavior is unchanged.

✅ Codebase Fit — Consistent with existing ILLink patterns

Diagnostic IDs (5005/5006), the new Unsafe category, and the resx entries follow existing conventions. The GetDiagnosticCategory switch inserts the two new IDs before the >= 3050 and <= 6000 => AOT arm so they resolve to Unsafe rather than being swallowed by the AOT range — this ordering is correct and covered by the UnsafeMigrationDiagnostics_HaveUnsafeCategoryWithoutHelpLinks test.

✅ Test Quality — Broad behavioral coverage

The new RequiresUnsafeCodeFixTests.cs adds ~44 focused test methods covering modifier removal/retention, whole-body vs. statement vs. expression wrapping, forward declarations, ref locals, async/yield exclusions, and the migration-disabled case. Using CSharpCodeFixVerifier with before/after sources is the right harness for code-fix validation. Note: I could not build or run these tests in this environment; the human reviewer should confirm they pass in CI.

💡 Brittle syntax-node detection (inline)

One low-severity, non-blocking maintainability note is left inline in UnsafeMigrationAnalysis.cs regarding the GetType().Name == "UnsafeExpressionSyntax" string check used to recognize the preview unsafe(...) expression node.

Note

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

Generated by Holistic Review · 192.5 AIC · ⌖ 12.1 AIC · ⊞ 10K

[github-actions]

💡 This uses a stringly-typed check (ancestor.GetType().Name == "UnsafeExpressionSyntax") to detect the preview unsafe(...) expression node, presumably because the strongly-typed syntax type isn't available in the referenced Roslyn version. That's a reasonable pragmatic workaround, but it is brittle: it silently breaks if the node type is renamed, and it won't match derived/wrapped forms. Consider adding a short comment explaining why the reflection-by-name check is used and, if feasible, guarding it with a SyntaxKind check so the intent survives a Roslyn bump. Non-blocking given this is DEBUG-only tooling.

[EgorBo]

decided to split into many small analyzers/fixers and the first batch is in #131002