Skip to content

Unsafe evolution: add csc flag and msbuild property - #85980

Merged
jjonescz merged 5 commits into
dotnet:mainfrom
jjonescz:Unsafe-58-MsbuildProperty
Oct 27, 2026
Merged

jjonescz merged 5 commits into
dotnet:mainfrom
jjonescz:Unsafe-58-MsbuildProperty

Conversation

@jjonescz

@jjonescz jjonescz commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Test plan: #81207
Part of #82791.
Follow up on #84860 - previously I didn't want to add these "APIs" because they can't be marked as experimental, but since then I've realized they are gated on LangVersion=preview anyway which I think is equivalent to experimental.

Microsoft Reviewers: Open in CodeFlow

@azure-pipelines

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The public naming needs API alignment, and the option must propagate through the legacy compiler-host path.

2 open findings
What changed in this PR

Adds compiler and MSBuild configuration for selecting unsafe-evolution memory-safety rules.

Changes:

  • Adds the compiler switch and MSBuild task property.
  • Normalizes the legacy feature flag into compilation options.
  • Adds command-line, compiler, and SDK integration tests.

Holistic assessment: The implementation is well-tested, but the public option name conflicts with API review and the legacy host path omits the setting.

File Description
src/​Compilers/​CSharp/​Test/​CSharp15/​UnsafeEvolutionTests.cs Expands option and language-version tests.
src/​Compilers/​CSharp/​Test/​CommandLine/​CommandLineTests.cs Tests command-line parsing and feature compatibility.
src/​Compilers/​CSharp/​Portable/​xlf/​CSharpResources.zh-Hant.xlf Updates localized help source.
src/​Compilers/​CSharp/​Portable/​xlf/​CSharpResources.zh-Hans.xlf Updates localized help source.
src/​Compilers/​CSharp/​Portable/​xlf/​CSharpResources.tr.xlf Updates localized help source.
src/​Compilers/​CSharp/​Portable/​xlf/​CSharpResources.ru.xlf Updates localized help source.
src/​Compilers/​CSharp/​Portable/​xlf/​CSharpResources.pt-BR.xlf Updates localized help source.
src/​Compilers/​CSharp/​Portable/​xlf/​CSharpResources.pl.xlf Updates localized help source.
src/​Compilers/​CSharp/​Portable/​xlf/​CSharpResources.ko.xlf Updates localized help source.
src/​Compilers/​CSharp/​Portable/​xlf/​CSharpResources.ja.xlf Updates localized help source.
src/​Compilers/​CSharp/​Portable/​xlf/​CSharpResources.it.xlf Updates localized help source.
src/​Compilers/​CSharp/​Portable/​xlf/​CSharpResources.fr.xlf Updates localized help source.
src/​Compilers/​CSharp/​Portable/​xlf/​CSharpResources.es.xlf Updates localized help source.
src/​Compilers/​CSharp/​Portable/​xlf/​CSharpResources.de.xlf Updates localized help source.
src/​Compilers/​CSharp/​Portable/​xlf/​CSharpResources.cs.xlf Updates localized help source.
src/​Compilers/​CSharp/​Portable/​Symbols/​Source/​SourceModuleSymbol.cs Reads normalized compilation options.
src/​Compilers/​CSharp/​Portable/​CSharpResources.resx Documents the new compiler switch.
src/​Compilers/​CSharp/​Portable/​Compilation/​CSharpCompilation.cs Normalizes the legacy feature flag.
src/​Compilers/​CSharp/​Portable/​CommandLine/​CSharpCommandLineParser.cs Parses the memory-safety rules switch.
src/​Compilers/​Core/​MSBuildTaskTests/​SdkIntegrationTests.cs Tests SDK property propagation.
src/​Compilers/​Core/​MSBuildTaskTests/​CscTests.cs Tests the task default.
src/​Compilers/​Core/​MSBuildTask/​Microsoft.CSharp.Core.targets Passes the MSBuild property to Csc.
src/​Compilers/​Core/​MSBuildTask/​Csc.cs Adds task storage and switch emission.

🧠 Review effort: Balanced

Comment thread src/Compilers/Core/MSBuildTask/Csc.cs Resolved
@jjonescz
jjonescz marked this pull request as ready for review October 13, 2026 11:37
@jjonescz
jjonescz requested a review from a team as a code owner October 13, 2026 11:37
Copilot AI balanced review requested due to automatic review settings October 13, 2026 11:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Legacy feature promotion becomes sticky across immutable syntax-tree updates, changing compilation behavior after the feature is removed.

0 open findings

2 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Preserve syntax-tree feature changes across immutable compilation updates

src/​Compilers/​CSharp/​Portable/​Compilation/​CSharpCompilation.cs:570

Promoting the legacy syntax-tree feature into _options makes the opt-in sticky across immutable compilation updates. For example, create a compilation whose tree has updated-memory-safety-rules, then call RemoveAllSyntaxTrees() (or replace it with a tree without the feature): Update passes the already-promoted Version2 options back into this constructor, so the new compilation still uses Version2 even though SyntaxTreeCommonFeatures is now empty. Before this change, SourceModuleSymbol evaluated the current compilation feature and reverted to Version1. Please preserve the originally supplied option and derive the legacy fallback from the current syntax-tree features (including for the language-version diagnostic), and add a remove/replace-tree regression test.

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 13, 2026 12:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It establishes durable compiler and MSBuild contracts whose naming and legacy compatibility warrant final maintainer review.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Low severity Place MemorySafetyRulesVersion in alphabetical order

src/​Compilers/​Core/​MSBuildTask/​Csc.cs:173

The class explicitly requires C#-specific task parameters to stay alphabetized (Csc.cs:32-34). MemorySafetyRulesVersion should be placed between GenerateFullPaths and ModuleAssemblyName, rather than after Nullable, so future task-property maintenance follows the documented layout.

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 13, 2026 12:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It introduces a durable compiler/MSBuild contract, and most current CI validation remains in progress.

0 open findings

🧠 Review effort: Balanced

if (value == null)
{
AddDiagnostic(diagnostics, ErrorCode.ERR_SwitchNeedsNumber, name);
continue;

@AlekseyTs AlekseyTs Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continue;

Are we testing this code path? #Closed

if (string.IsNullOrEmpty(value) ||
!int.TryParse(value, NumberStyles.Integer, CultureInfo.InvariantCulture, out var newMemorySafetyRulesVersion))
{
AddDiagnostic(diagnostics, ErrorCode.ERR_SwitchNeedsNumber, name);

@AlekseyTs AlekseyTs Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AddDiagnostic

Are we testing this code path? #Closed

@AlekseyTs AlekseyTs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 4)

@jjonescz
jjonescz requested a review from 333fred October 13, 2026 19:12
@jjonescz

Copy link
Copy Markdown
Member Author

@333fred or @RikkiGibson for a second review, thanks


internal bool UseUpdatedMemorySafetyRules =>
Options.UseUpdatedMemorySafetyRules ||
(Options.MemorySafetyRulesVersion == MemorySafetyRulesVersion.Version1 && HasFeature(CodeAnalysis.Feature.UpdatedMemorySafetyRules));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider dropping support for the flag as we move to the command line option. Perhaps emit a warning that it will be removed soon?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'd rather look into that in a follow up PR though if that's okay.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine.

@jjonescz
jjonescz requested a review from 333fred October 26, 2026 10:32

@333fred 333fred left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with followup

Copilot AI balanced review requested due to automatic review settings October 27, 2026 10:24
@jjonescz
jjonescz enabled auto-merge (squash) October 27, 2026 10:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

Static review supports consistent option wiring and preview gating, with focused test coverage and no unresolved blocking findings.

No open issues 🎉

Review effort: Balanced

@jjonescz
jjonescz merged commit 0f7bcef into dotnet:main Oct 27, 2026
21 of 22 checks passed
@jjonescz
jjonescz deleted the Unsafe-58-MsbuildProperty branch October 27, 2026 11:54
@dotnet-policy-service dotnet-policy-service Bot added this to the Next milestone Oct 27, 2026
EgorBo added a commit to EgorBo/runtime-1 that referenced this pull request Oct 29, 2026
… rules

- An assembly opts in with `<MemorySafetyRulesVersion>2</MemorySafetyRulesVersion>`, the compiler's own property (dotnet/roslyn#85980). `eng/memorySafetyRules.targets` maps it to the `updated-memory-safety-rules` feature flag until that compiler flows in; 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` for `dotnet format`, and `UnsafeMigrationProject`/`UnsafeMigrationTargetFramework` let a migration driver load one target framework of the migrated project. A driver script is at https://gist.github.com/EgorBo/456f9e7d3680a5f9c8e54a168a907187.

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
… rules

- An assembly opts in with `<MemorySafetyRulesVersion>2</MemorySafetyRulesVersion>`, the compiler's own property (dotnet/roslyn#85980). `eng/memorySafetyRules.targets` maps it to the `updated-memory-safety-rules` feature flag until that compiler flows in; 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` for `dotnet format`, and `UnsafeMigrationProject`/`UnsafeMigrationTargetFramework` let a migration driver load one target framework of the migrated project. A driver script is at https://gist.github.com/EgorBo/456f9e7d3680a5f9c8e54a168a907187.

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
… rules

- An assembly opts in with `<MemorySafetyRulesVersion>2</MemorySafetyRulesVersion>`, the compiler's own property (dotnet/roslyn#85980). `eng/memorySafetyRules.targets` maps it to the `updated-memory-safety-rules` feature flag until that compiler flows in; 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` for `dotnet format`, and `UnsafeMigrationProject`/`UnsafeMigrationTargetFramework` let a migration driver load one target framework of the migrated project. A driver script is at https://gist.github.com/EgorBo/456f9e7d3680a5f9c8e54a168a907187.

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
… rules

- An assembly opts in with `<MemorySafetyRulesVersion>2</MemorySafetyRulesVersion>`, the compiler's own property (dotnet/roslyn#85980). `eng/memorySafetyRules.targets` maps it to the `updated-memory-safety-rules` feature flag until that compiler flows in; 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` for `dotnet format`, and `UnsafeMigrationProject`/`UnsafeMigrationTargetFramework` let a migration driver load one target framework of the migrated project. A driver script is at https://gist.github.com/EgorBo/456f9e7d3680a5f9c8e54a168a907187.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: cf859571-aaeb-4ade-b35e-bf2acc951c30
EgorBo added a commit to dotnet/runtime that referenced this pull request Oct 29, 2026
…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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants