Add safety documentation analyzers by EgorBo · Pull Request #131484 · dotnet/runtime

EgorBo · 2026-07-28T18:17:02Z

This PR prototypes the safety documentation part of the unsafe-v2 migration tooling (tracking issue §5), continuing #131002, #131245 and #131454. Everything here is non-shipping (#if DEBUG) and disabled by default.

Background

Passing an obligation to a caller comes with a responsibility to say what that obligation is, and discharging one comes with a responsibility to say how. IL5005 from #131002 covers the first half: an unsafe member must carry a <safety> XML comment. Two gaps remain.

The first is the other side of the same contract. An unsafe region is where the obligations are actually met, and that reasoning is invisible unless it is written down. The speclet recommends Rust-style // SAFETY comments and LDM 2026-05-27 left "compiler or analyzer?" open, so an analyzer is the natural home.

The second is safe itself. It is a hand-written assertion the compiler cannot verify, and it is arguably the annotation that most needs a recorded audit, yet nothing checks it.

Changes in this PR

🔍 UnsafeBlockMissingSafetyCommentAnalyzer (IL5009) reports an unsafe block or unsafe(...) expression with no // SAFETY: comment. A region nested inside an already-documented one is not reported, since the outer comment covers it.
🔧 AddSafetyCommentCodeFixProvider fixes IL5009 by inserting a // SAFETY: TODO stub, matching the /* SAFETY: Audit */ convention Add unsafe modifier migration code fixer #131002 already uses. The stub only marks the region for review; describing the reasoning is the developer's job.
🔍 SafeModifierMissingJustificationAnalyzer (IL5010) reports an explicit safe modifier with no <safety> documentation, wherever safe can appear: extern members and [LibraryImport] methods, where it claims the native boundary upholds its contract, and fields in explicit or extended layouts, where it claims the overlap cannot be used to type-pun. No code fix, since only a developer can write the justification.

The `SAFETY:` marker

Recognized by \bSAFETY\s*:, case-sensitive, matched anywhere inside a comment.

Accepted: // SAFETY:, //SAFETY:, // Reads one element. SAFETY: ..., /* SAFETY: ... */, and a marker on an inner line of a block comment. Rejected: // UNSAFETY: and // SAFETYNET: (whole word), // SAFETY concerns are handled elsewhere (no colon), and // safety: or // Safety:.

Case sensitivity is deliberate: it keeps git grep 'SAFETY:' authoritative as an audit tool, at the cost of a mildly confusing report for someone who writes // Safety:. Both that and the leniency about spacing before the colon are pinned by tests, so they are easy to revisit.

Notable case handled

unsafe(...) expressions are newer than the Roslyn these analyzers compile against, so UnsafeExpressionSyntax and SyntaxKind.UnsafeExpression are unavailable at build time even though they exist at run time. Detecting the expression form as "an unsafe keyword followed by (" is wrong: the modifier is also followed by ( on any member whose type is a tuple.

public static unsafe (int, int) GetPair() => default;   // modifier, not a region
int x = unsafe(Read());                                 // the actual expression

That shape is pervasive in the hardware intrinsics (AdvSimd.cs alone has around 15), and every one of them would have been reported as an undocumented unsafe region, with the fixer offering to stamp a // SAFETY: TODO on it. The analyzer instead tests token.Parent is ExpressionSyntax, which is version-safe because ExpressionSyntax predates the feature.

Diagnostics IDs

Just for reference

IL5005 [Add unsafe modifier migration code fixer #131002] - An unsafe member has no <safety> XML documentation.
IL5009 [This PR] - An unsafe region has no // SAFETY: comment.
IL5010 [This PR] - An explicit safe modifier has no justification.

Known limitations / follow ups

IL5009 treats any // SAFETY: comment as sufficient. It cannot judge whether the reasoning is correct, or whether it still applies after the region is edited.
The nesting exemption is deliberately coarse: a documented outer region silences every region inside it, even a much later one that may warrant its own note.
This branch adds UnsafeMigrationSyntaxHelpers.SafeKeywordKind, which Add unsafe contract consistency code fixes #131454 also adds. Whichever merges second will need a trivial conflict resolution.

Testing

ILLink.RoslynAnalyzer.Tests: 1238 passed. New coverage for both region forms, the accepted and rejected marker spellings, nesting in both directions, unsafe expressions in field initializers where a block is not valid syntax, the tuple-returning modifier shapes above, and safe on both extern members and explicit-layout fields.

Note

This description and the changes in this PR were generated with GitHub Copilot.

Adds the safety documentation tier of the unsafe-v2 migration tooling. IL5009 reports an unsafe block or unsafe(...) expression that records no // SAFETY: comment explaining how its obligations are discharged, and AddSafetyCommentCodeFixProvider inserts a stub for review. IL5005 already covers the signature side of the contract; this covers the place where the obligations are actually met. IL5010 reports an explicit safe modifier with no <safety> documentation. It is the symmetric hole to IL5005: safe is a hand-written assertion the compiler cannot verify, on a native boundary or on a field whose overlap must not be used to type-pun. No fixer, since only a developer can write the justification. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5

Replace the substring check with a case-sensitive whole-word regex so that '// UNSAFETY:' and '// SAFETYNET' no longer count as safety comments, and a marker is only recognized when it is followed by a colon. Keeping the marker case-sensitive keeps the convention greppable across a code base. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5

azure-pipelines · 2026-07-28T18:17:55Z

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

dotnet-policy-service · 2026-07-28T18:18:31Z

Tagging subscribers to this area: @agocke, @dotnet/illink
See info in area-owners.md if you want to be subscribed.

Copilot

Pull request overview

This PR extends the ILLink unsafe-v2 migration tooling prototype by adding analyzers and a code fix to enforce and scaffold safety documentation for unsafe regions (// SAFETY:) and explicit safe modifiers (<safety> XML docs). The additions are gated behind #if DEBUG and are disabled by default, aligning with the existing non-shipping migration tooling approach in this area.

Changes:

Add UnsafeBlockMissingSafetyCommentAnalyzer (IL5009) to report undocumented unsafe { } blocks and unsafe(...) expressions, with nesting suppression.
Add AddSafetyCommentCodeFixProvider to insert a // SAFETY: TODO stub for IL5009.
Add SafeModifierMissingJustificationAnalyzer (IL5010) to require <safety> documentation for explicit safe modifiers, plus corresponding resources/tests and diagnostic wiring.

Reviewed changes

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

Show a summary per file

File	Description
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMigrationTestHelpers.cs	Enables the new diagnostics as warnings in the analyzer/code-fix test harness.
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeBlockMissingSafetyCommentAnalyzerTests.cs	Adds coverage for `IL5009` across block/expression forms, marker spellings, and nesting.
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SafeModifierMissingJustificationAnalyzerTests.cs	Adds coverage for `IL5010` on `safe extern` and explicit-layout `safe` fields.
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddSafetyCommentCodeFixTests.cs	Verifies the `IL5009` code fix inserts `// SAFETY: TODO` in the expected location and preserves existing trivia.
src/tools/illink/src/ILLink.Shared/SharedStrings.resx	Adds title/message strings for `IL5009` and `IL5010`.
src/tools/illink/src/ILLink.Shared/DiagnosticId.cs	Introduces new diagnostic IDs and broadens “Safety” categorization to the `5000–5099` range.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs	Exposes `SafeKeywordKind` and adds a helper to identify `unsafe(...)` expression keywords safely across Roslyn versions.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeBlockMissingSafetyCommentAnalyzer.cs	Implements `IL5009` detection for `unsafe` statements and `unsafe(...)` expressions, including nesting and trivia scanning rules.
src/tools/illink/src/ILLink.RoslynAnalyzer/SafeModifierMissingJustificationAnalyzer.cs	Implements `IL5010` detection for explicit `safe` modifiers lacking `<safety>` docs.
src/tools/illink/src/ILLink.CodeFix/Resources.resx	Adds localized title string for the new code fix.
src/tools/illink/src/ILLink.CodeFix/AddSafetyCommentCodeFixProvider.cs	Implements the `IL5009` code fix that inserts a `// SAFETY: TODO` stub above the appropriate syntax node.

The fixer inserted the comment above all existing leading trivia, which contradicted its own doc comment and pushed the marker away from the code it explains. Insert it directly above the target instead, so XML documentation keeps its place on top and the marker stays adjacent to the region, matching the Rust convention the marker is modeled on. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

src/tools/illink/src/ILLink.CodeFix/AddSafetyCommentCodeFixProvider.cs:12

The using ILLink.RoslynAnalyzer; directive is unused in this file and may produce CS8019/IDE0005 warnings (and can become build-breaking if warnings are treated as errors). Remove it to keep the code fix project warning-clean.

using ILLink.CodeFixProvider;
using ILLink.RoslynAnalyzer;
using ILLink.Shared;

The nesting check asked whether an ancestor's first token was an unsafe expression keyword, but a node's first token can belong to a descendant, so the binary expression in 'unsafe(a) + b' also starts with one. Every unsafe expression in such a statement was therefore treated as nested and silently skipped. Require the keyword to be owned by the ancestor being tested. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5

Copilot

Pull request overview

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

EgorBo · 2026-10-01T18:37:46Z

Since Roslyn already added similar analyzers, I'm going to close this

…iler - Build the migration analyzers and fixers in every configuration but Release (`UNSAFE_MIGRATION`), so Checked has them too, not just the unsafe-context fixer. - Add a fix-all provider that applies each diagnostic's own fix. The batch fixer takes the equivalence key from one arbitrary diagnostic, so under `dotnet format` a single unfixable `IL5005` (e.g. on a pointer signature) turned the whole pass into a no-op and made the result depend on diagnostic order. `SynchronizeUnsafeContract`'s "remove" and "replace with safe" actions now share one key. - `IL5005`: keep `unsafe` on `extern` and `[LibraryImport]` methods (removing it oscillated with the `CS9389`/`SYSLIB1064` fixers) and on members whose inactive `#if` branches contain pointer operations, which may still rely on its lexical context in other configurations or in other assemblies compiling the same file. - Unsafe-context fixer: also run under the legacy rules, to repair other consumers of a shared file, and fix `CS9402` (address of an unassigned variable, dotnet/roslyn#86017). - `CS9392` also applies to `StructLayout` with a nonzero `Pack` or `Size` (dotnet/roslyn#85921). - Detect the updated rules through `IModuleSymbol.MemorySafetyRulesVersion` (feature flag as fallback) in the ILLink analyzers and the interop generators, so the `MemorySafetyRulesVersion` property works too. - Restore `IL5009`/`IL5010` and the add-safety-comment fixer from dotnet#131484; Roslyn's IDE0380 change (dotnet/roslyn#85842) only covers member modifiers. The stub is the unsafe-context fixer's `// SAFETY: To be audited` placeholder, and inline `/* SAFETY: */` markers are recognized. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf859571-aaeb-4ade-b35e-bf2acc951c30

…iler - Build the migration analyzers and fixers in every configuration but Release (`UNSAFE_MIGRATION`), so Checked has them too, not just the unsafe-context fixer. - Add a fix-all provider that applies each diagnostic's own fix. The batch fixer takes the equivalence key from one arbitrary diagnostic, so under `dotnet format` a single unfixable `IL5005` (e.g. on a pointer signature) turned the whole pass into a no-op and made the result depend on diagnostic order. `SynchronizeUnsafeContract`'s "remove" and "replace with safe" actions now share one key. - `IL5005`: keep `unsafe` on `extern` and `[LibraryImport]` methods (removing it oscillated with the `CS9389`/`SYSLIB1064` fixers) and on members whose inactive `#if` branches contain pointer operations, which may still rely on its lexical context in other configurations or in other assemblies compiling the same file. - Unsafe-context fixer: also run under the legacy rules, to repair other consumers of a shared file, and fix `CS9402` (address of an unassigned variable, dotnet/roslyn#86017). - `CS9392` also applies to `StructLayout` with a nonzero `Pack` or `Size` (dotnet/roslyn#85921). - Detect the updated rules through `IModuleSymbol.MemorySafetyRulesVersion` in the ILLink analyzers and the interop and System.Text.Json generators, so the `MemorySafetyRulesVersion` property works too. A shared `Common/src/Roslyn/MemorySafetyRulesPolyfills.cs` extension property stands in for the API (reflection, with the feature flag as fallback) and replaces three reflection-based copies; once our Roslyn reference has the API, call sites bind to it and the shim can be deleted. - Restore `IL5009`/`IL5010` and the add-safety-comment fixer from dotnet#131484; Roslyn's IDE0380 change (dotnet/roslyn#85842) only covers member modifiers. The stub is the unsafe-context fixer's `// SAFETY: To be audited` placeholder, and inline `/* SAFETY: */` markers are recognized. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf859571-aaeb-4ade-b35e-bf2acc951c30

…ollment (#136621) Contributes to #131451. Infrastructure only: no library is migrated here. ## Changes **Migration tooling (`src/tools/illink`, interop generators)** - **`UNSAFE_MIGRATION` instead of `#if DEBUG`.** The analyzers and most fixers were compiled out of Checked, so a Checked toolset ran only the unsafe-context fixer. They're now built in every configuration but Release. - **`UnsafeMigrationFixAllProvider`.** The batch fixer takes the equivalence key from one arbitrary diagnostic. Under `dotnet format`, a single unfixable `IL5005` (e.g. on a pointer signature) turned the whole pass into a silent no-op, and results depended on diagnostic order. Each diagnostic now gets its own fix. `SynchronizeUnsafeContract`'s remove and replace-with-`safe` actions share one key. - **`IL5005` keeps `unsafe` in two more cases.** - On `extern`/`[LibraryImport]` methods: removing it oscillated with the `CS9389`/`SYSLIB1064` fixers. - On members whose inactive `#if` branches contain pointer operations: those branches can rely on the modifier's lexical context in other configurations or in other assemblies compiling the same file (e.g. `Marvin.cs` in CoreLib). - **Unsafe-context fixer.** It also runs under the legacy rules, to repair other consumers of a shared file, and it fixes `CS9402` (address of an unassigned variable, dotnet/roslyn#86017). - **`CS9392` also covers `StructLayout` `Pack`/`Size`** (dotnet/roslyn#85921). Otherwise `IL5005` and the field fixer undo each other. - **v2 detection through `IModuleSymbol.MemorySafetyRulesVersion`** in the ILLink analyzers and the interop and System.Text.Json generators. Reading only the feature flag breaks once `MemorySafetyRulesVersion` is used. A shared `Common/src/Roslyn/MemorySafetyRulesPolyfills.cs` extension property stands in for the API (reflection, with the feature flag as fallback) and replaces three reflection-based copies. Once our Roslyn reference has the API, call sites bind to it and the shim can be deleted. - **Restored `IL5009`/`IL5010` and the add-safety-comment fixer from #131484.** dotnet/roslyn#85842 only changes IDE0380, which covers member modifiers (not blocks, expressions or `safe`), and it isn't in our SDK. The stub is the unsafe-context fixer's `// SAFETY: To be audited` placeholder, and inline `/* SAFETY: */` markers are recognized. **Enrollment (`eng/memorySafetyRules.targets`)** - **Opt in with `<MemorySafetyRulesVersion>2</MemorySafetyRulesVersion>`,** the compiler's own property (dotnet/roslyn#85980). Until that compiler flows in, it maps to the `updated-memory-safety-rules` feature flag. `AllowUnsafeBlocks` stays each library's own decision, since a library without unsafe operations needs neither it nor any `safe`/`unsafe` annotations. Delete the mapping then, since the flag will warn (dotnet/roslyn#86224). The existing ref and System.Text.Json test opt-ins move to the property. - **`EnableUnsafeMigrationAnalyzers=true` enables `IL5005`-`IL5010`** through `eng/unsafeMigration.globalconfig`; they are disabled by default and `dotnet format --diagnostics` doesn't enable them. - **`UnsafeMigrationProject`/`UnsafeMigrationTargetFramework` narrow the migrated project to one target framework.** `dotnet format -f` can't be used: it retargets every referenced project too, which fails for platform-specific TFMs. ## Enrolling a library The driver lives in a gist: https://gist.github.com/EgorBo/456f9e7d3680a5f9c8e54a168a907187. Each step runs `dotnet format analyzers`, and the whole sequence repeats until nothing changes: 1. `CS9377`/`CS0106`: remove invalid `unsafe`. 2. `IL5005`: remove undocumented `unsafe`. 3. `IL5006`/`SYSLIB1064`/`CS9389`/`CS9392`: add the contracts that have to be explicit. 4. `CS9364`-`CS9366`/`CS0764`/`CS9390`: synchronize overrides and partial parts. 5. `CS9360`-`CS9363`/`CS9376`/`CS9402`: introduce unsafe contexts. 6. `IL5009`: add safety comments. ```powershell # 1. Opt in: add <MemorySafetyRulesVersion>2</MemorySafetyRulesVersion> to src\libraries\Foo\src\Foo.csproj, # then build it once so its references exist. .\build.cmd libs -c Release # 2. Migrate, one pass per target framework. .\migrate-unsafe-v2.ps1 src\libraries\Foo\src\Foo.csproj -TargetFrameworks net11.0-windows,net11.0-unix,net11.0,netstandard2.0 # 3. Build every target framework, plus the other consumers of the shared files the script lists. .\dotnet.cmd build src\libraries\Foo\src\Foo.csproj -c Release ``` 4. Audit the result: - Replace each `// SAFETY: To be audited` with the actual reasoning. - Change `unsafe` to `safe` on `[LibraryImport]`/`extern` members whose native side is safe, with a `<safety>` doc. - Write a `<safety>` doc for every `unsafe` member that stays. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf859571-aaeb-4ade-b35e-bf2acc951c30

EgorBo and others added 2 commits July 28, 2026 14:28

Copilot AI review requested due to automatic review settings July 28, 2026 18:17

EgorBo requested a review from sbomer as a code owner July 28, 2026 18:17

github-actions Bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Jul 28, 2026

dotnet-policy-service Bot added the linkable-framework Issues associated with delivering a linker friendly framework label Jul 28, 2026

dotnet-policy-service Bot assigned EgorBo Jul 28, 2026

Copilot started reviewing on behalf of EgorBo July 28, 2026 18:17

View session

EgorBo mentioned this pull request Jul 28, 2026

[Tracking] Unsafe-v2 migration tooling (analyzers and code fixers) #131451

Open

28 of 36 tasks

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Comment thread src/tools/illink/src/ILLink.CodeFix/AddSafetyCommentCodeFixProvider.cs Outdated

Copilot AI review requested due to automatic review settings July 28, 2026 19:14

Copilot started reviewing on behalf of EgorBo July 28, 2026 19:15

View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

build-analysis Bot mentioned this pull request Jul 28, 2026

Test Failure: System.Text.Json.Serialization.Tests.StreamTests_Sync.VeryLargeJsonFileTest #130783

Open

Copilot AI review requested due to automatic review settings July 30, 2026 07:51

Copilot started reviewing on behalf of EgorBo July 30, 2026 07:51

View session

Copilot AI reviewed Jul 30, 2026

View reviewed changes

EgorBo mentioned this pull request Jul 30, 2026

Add IntroduceUnsafeContextCodeFixProvider for unsafe-v2 call sites #131581

Closed

richlander mentioned this pull request Aug 30, 2026

Define memory-safety models and evidence richlander/dotnet-inspect#5226

Merged

EgorBo closed this Oct 1, 2026

EgorBo mentioned this pull request Oct 29, 2026

Unsafe-v2: fix migration tooling and add MemorySafetyRulesVersion enrollment #136621

Merged

Navigation Menu

Repository navigation

Add safety documentation analyzers - #131484

Add safety documentation analyzers#131484
EgorBo wants to merge 4 commits into
dotnet:mainfrom
EgorBo:egorbo/unsafe-v2-safety-docs

EgorBo commented Jul 28, 2026

Uh oh!

azure-pipelines Bot commented Jul 28, 2026

Uh oh!

dotnet-policy-service Bot commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

EgorBo commented Oct 1, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

EgorBo commented Jul 28, 2026

Background

Changes in this PR

The SAFETY: marker

Notable case handled

Diagnostics IDs

Known limitations / follow ups

Testing

Uh oh!

azure-pipelines Bot commented Jul 28, 2026

Uh oh!

dotnet-policy-service Bot commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

EgorBo commented Oct 1, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

The `SAFETY:` marker