Skip to content

Unsafe evolution: use safety documentation for unnecessary unsafe - #85842

Merged
jjonescz merged 9 commits into
dotnet:mainfrom
CyrusNajmabadi:fix-85756-safety-doc-comments
Oct 1, 2026
Merged

Unsafe evolution: use safety documentation for unnecessary unsafe#85842
jjonescz merged 9 commits into
dotnet:mainfrom
CyrusNajmabadi:fix-85756-safety-doc-comments

Conversation

@CyrusNajmabadi

@CyrusNajmabadi CyrusNajmabadi commented Sep 30, 2026 •
edited by dotnet-policy-service Bot
Loading

Copy link
Copy Markdown
Contributor

Builds on #85756 and addresses the review feedback there.

Under memory safety rules v2, keep a removable unsafe modifier when its declaration has a <safety> documentation element. Otherwise, continue reporting IDE0380.

The fixer offers both choices under v2:

  • Remove unsafe.
  • Add a <safety> documentation element, preserving existing single-line or multiline documentation style.

The tests cover methods, constructors, fields, properties, indexers, events, operators, conversions, local functions, existing documentation, both multiline documentation styles, v1 behavior, and both fix-all actions.

Fixes #85732.

Microsoft Reviewers: Open in CodeFlow

@CyrusNajmabadi
CyrusNajmabadi requested a review from a team as a code owner September 30, 2026 12:07
@azure-pipelines

azure-pipelines Bot commented Sep 30, 2026

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 2 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service dotnet-policy-service Bot added Community The pull request was submitted by a contributor who is not a Microsoft employee. VSCode labels Sep 30, 2026
@CyrusNajmabadi

CyrusNajmabadi commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

@jjonescz ptal.

@jjonescz

jjonescz commented Sep 30, 2026

Copy link
Copy Markdown
Member

@EgorBo FYI

@build-analysis build-analysis Bot mentioned this pull request Sep 30, 2026
Open
2 tasks
@jjonescz
jjonescz merged commit a1bbcb8 into dotnet:main Oct 1, 2026
23 checks passed
@dotnet-policy-service dotnet-policy-service Bot added this to the Next milestone Oct 1, 2026
@jjonescz jjonescz mentioned this pull request Oct 1, 2026
Closed
This was referenced Oct 1, 2026
Merged
Merged
@dibarbet dibarbet mentioned this pull request Oct 2, 2026
Merged
333fred added a commit that referenced this pull request Oct 2, 2026
@333fred
Merge remote-tracking branch 'upstream/main' into merge-main
e9bba31 
# By Sam Harwell (331) and others
# Via GitHub (414) and others
* upstream/main: (1352 commits)
  Document known issue workarounds inline (#85896)
  Restore automatic HTML test results for local runs (#85892)
  Report actual errors when strong-name HRESULT assertions fail (#85894)
  Restore main insertions after the VS 18.12 snap (#85824)
  Add "edit criteria" for AI doc updates (#85893)
  Handle cref type parameters in symbol lookup (#85840)
  Fix build warnings (#85754)
  Improve our Copilot instructions for testing
  [main] Update dependencies from dotnet/arcade (#85885)
  Rewrite 'router' phrasing
  Address review feedback
  Add option to disable cref paste fix
  Prefer repository terminology in generated output (#85878)
  Use cref for workspace extension handler
  Remove unnecessary static diagnostic registration
  Unsafe evolution: use safety documentation for unnecessary unsafe (#85842)
  Add Razor compiler team to code owners
  Unsafe evolution: avoid constructor recursion (#85773)
  Add roslyn-ide to Razor code owners
  Fix unexpected System.ArrayTypeMismatchException for captured method call receivers (#85811)
  ...

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fd482dbd-c828-466b-87c6-6b2103be2089

# Conflicts:
#	src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
#	src/Compilers/CSharp/Portable/Errors/MessageID.cs
@dotnet-bot dotnet-bot mentioned this pull request Oct 7, 2026
Closed
@akhera99 akhera99 modified the milestones: Next, 18.13 Oct 26, 2026
@dotnet-maestro dotnet-maestro Bot mentioned this pull request Oct 27, 2026
Closed
@EgorBo EgorBo mentioned this pull request Oct 29, 2026
Merged
EgorBo added a commit to EgorBo/runtime-1 that referenced this pull request Oct 29, 2026
@EgorBo
Fix unsafe-v2 migration tooling for dotnet format and the latest comp…
e771ed0 
…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
EgorBo added a commit to EgorBo/runtime-1 that referenced this pull request Oct 29, 2026
@EgorBo
Fix unsafe-v2 migration tooling for dotnet format and the latest comp…
021422e 
…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
@EgorBo EgorBo mentioned this pull request Oct 29, 2026
Open
28 of 36 tasks
EgorBo added a commit to dotnet/runtime that referenced this pull request Oct 29, 2026
@EgorBo
Unsafe-v2: fix migration tooling and add MemorySafetyRulesVersion enr…
e925f49 
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jjonescz jjonescz jjonescz approved these changes

Assignees

No one assigned

Labels

Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. VSCode

Projects

None yet

Milestone

18.13

Development

Successfully merging this pull request may close these issues.

IDE0380 False Positive in New C# 16 Memory Model

3 participants

@CyrusNajmabadi @jjonescz @akhera99