Unsafe evolution: relax `safe` modifier placement restrictions by jjonescz · Pull Request #84602 · dotnet/roslyn

jjonescz · 2026-07-23T08:50:54Z

Closes #84555.
LDM decision: https://github.com/dotnet/csharplang/blob/main/meetings/2026/LDM-2026-07-22.md#allowing-safe-on-non-extern-members.
Relevant speclet section: safe keyword - see also dotnet/csharplang#10289:

The safe modifier can be applied to all declarations which allow unsafe to mark them as requires-unsafe. It is disallowed to apply both the safe and unsafe modifier on the same declaration.

The compiler requires an explicit safe or unsafe modifier on extern members and fields in explicit layout. Allowing safe even on declarations where it is not required (and hence has no effect for the compiler) is motivated by source generators, e.g., LibraryImport.

The safe modifier only marks the declaration as not requires-unsafe, it does not introduce a safe context. There is also no safe block or expression form.

Test plan: #81207

Microsoft Reviewers: Open in CodeFlow

azure-pipelines · 2026-07-23T08:53:24Z

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

Pull request overview

This PR updates the C# compiler’s handling of the safe modifier as part of Unsafe Evolution, removing prior semantic restrictions that limited safe mostly to extern members and certain layout-backed members. It also adjusts diagnostics/messages accordingly and updates compiler tests and localized resources.

Changes:

Removes compiler checks that rejected safe on many non-extern declarations, effectively allowing broader safe usage.
Re-scopes ERR_SafeModifierUnsupportedTarget to specifically report the invalid combination of safe + unsafe.
Updates Unsafe Evolution tests and all localized resource entries for the changed diagnostic message.

Reviewed changes

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

Show a summary per file

File	Description
src/Compilers/CSharp/Test/CSharp15/UnsafeEvolutionTests.cs	Updates diagnostics expectations and adds coverage for `safe` on type declarations under Unsafe Evolution gating.
src/Compilers/CSharp/Portable/CSharpResources.resx	Updates the user-facing text for `ERR_SafeModifierUnsupportedTarget` to the new meaning (safe+unsafe cannot be combined).
src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf	Updates localized string source for `ERR_SafeModifierUnsupportedTarget` and marks translation as needing review.
src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf	Same as above (German).
src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf	Same as above (Spanish).
src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf	Same as above (French).
src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf	Same as above (Italian).
src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf	Same as above (Japanese).
src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf	Same as above (Korean).
src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf	Same as above (Polish).
src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf	Same as above (Portuguese - Brazil).
src/Compilers/CSharp/Portable/xlf/CSharpResources.ru.xlf	Same as above (Russian).
src/Compilers/CSharp/Portable/xlf/CSharpResources.tr.xlf	Same as above (Turkish).
src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hans.xlf	Same as above (Chinese Simplified).
src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hant.xlf	Same as above (Chinese Traditional).
src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs	Relaxes local function rejection to only disallow `safe` when combined with `unsafe`.
src/Compilers/CSharp/Portable/Symbols/Source/ModifierUtils.cs	Moves/centralizes `safe` feature gating and adds `safe`+`unsafe` combination error emission during modifier checking.
src/Compilers/CSharp/Portable/Symbols/Source/SourceEventSymbol.cs	Allows `safe` as an event modifier broadly and removes `extern`-gated allowance and post-check rejection.
src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs	Allows `safe` on more type declarations (class/struct/interface/delegate) by adding it to allowed modifier sets.
src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberFieldSymbol.cs	Ensures `safe` is treated like other invalid modifiers in contexts like `const` fields (CS0106).
src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs	Removes the post-check that rejected `safe` on non-`extern` methods (except accessor/event-special cases handled elsewhere).
src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol.cs	Removes the post-check that rejected `safe` on certain fields based on layout/static/const/unsafe combinations.
src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs	Removes the post-check that rejected `safe` on properties based on `extern`/layout/unsafe constraints.

azure-pipelines · 2026-07-23T11:01:03Z

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.

jjonescz · 2026-07-24T07:49:28Z

@333fred @AlekseyTs for reviews, thanks

333fred

Do we have tests for public unsafe int Prop { safe get; safe set; }, or the inverse?

333fred · 2026-07-24T18:08:11Z

@@ -8136,8 +8136,8 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
    <comment>'RequiresUnsafeAttribute' and 'unsafe' should not be localized.</comment>
  </data>
  <data name="ERR_SafeModifierUnsupportedTarget" xml:space="preserve">


Consider renaming the error code for clarity.

AlekseyTs · 2026-07-27T06:58:07Z

Done with review pass (commit 1)

Copilot

Pull request overview

Copilot reviewed 28 out of 28 changed files in this pull request and generated 13 comments.

Copilot

Pull request overview

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

Comments suppressed due to low confidence (2)

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs:1019

Same as above for the single-accessor path: if the property/indexer already has a safe/unsafe modifier, then an accessor-level safe/unsafe should be diagnosed via CS9396 only. The current check will also report CS9397, causing duplicate/cascading diagnostics.

                            if (accessor.HasUnsafeModifier ^ accessor.HasSafeModifier)
                            {
                                // Cannot specify the same 'unsafe' or 'safe' modifier on all accessors of property or indexer '{0}'. Instead, put that modifier on the property itself.
                                diagnostics.Add(ErrorCode.ERR_SamePropertyUnsafeAccessorMods, Location, this);
                            }

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs:966

ERR_SamePropertyUnsafeAccessorMods should only be reported when the property/indexer itself does not already have a safe/unsafe modifier. As written, a declaration like unsafe int P { unsafe get; unsafe set; } will also satisfy this condition, producing an extra (and misleading) CS9397 in addition to the intended CS9396 from the accessor checks.

This issue also appears on line 1015 of the same file.

                    else if ((_getMethod.HasUnsafeModifier && _setMethod.HasUnsafeModifier && !_getMethod.HasSafeModifier && !_setMethod.HasSafeModifier) ||
                        (_getMethod.HasSafeModifier && _setMethod.HasSafeModifier && !_getMethod.HasUnsafeModifier && !_setMethod.HasUnsafeModifier))
                    {
                        // Cannot specify the same 'unsafe' or 'safe' modifier on all accessors of property or indexer '{0}'. Instead, put that modifier on the property itself.
                        diagnostics.Add(ErrorCode.ERR_SamePropertyUnsafeAccessorMods, Location, this);

Copilot

Pull request overview

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

AlekseyTs

LGTM (commit 16)

jjonescz · 2026-07-30T09:08:09Z

@333fred for a second review, thanks

@jkotas

Tracking issue: #131451 This PR makes `[LibraryImport]` participate in the new unsafe-v2 rules ([unsafe evolution](https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md)), plus tooling to migrate existing code. Nothing changes under unsafe-v1: - An analyzer + code-fixer to prepare codebase to unsafe-v2 by adding `unsafe` on all `[LibraryImport]` if they had no safety keyword on them (migration). - Changed LibraryImportGenerator (to be precise: `LibraryImportDiagnosticsAnalyzer` and `DownlevelLibraryImportDiagnosticsAnalyzer`) to explicitly require `safe` or `unsafe` under the new rules with human-readable message. ### Background The compiler requires an explicit `safe`/`unsafe` modifier on `extern` members (`CS9389`). But a `[LibraryImport]` method is only implemented by an `extern` forwarder when its signature needs no marshalling - otherwise the generator emits a managed wrapper around a private `extern` local function. So today the requirement fires for some P/Invokes and not others, based on marshalling alone. The speclet calls this out ([`safe` on non-`extern` members](https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md#answered-allow-safe-on-non-extern-members-libraryimport)): > Scenarios that need to require an explicit modifier when the language does not, such as a `LibraryImport` that generates a non-`extern` wrapper, will need an analyzer to enforce the presence of `safe` or `unsafe`. ### Changes in this PR 1. [analyzer] **`SYSLIB1064`** (_see 'Diagnostics IDs' below_) requires an explicit `safe`/`unsafe` modifier on every method with `LibraryImportAttribute` when the updated rules are enabled - for every shape, so adding a `string` parameter never silently changes a P/Invoke's safety obligations. Reported by both `LibraryImportDiagnosticsAnalyzer` and its downlevel counterpart. 2. [tests] **The private `extern` stays caller-unsafe.** No codegen change here: `main` already emits the inner `__PInvoke` local function as `static extern unsafe` and wraps stub bodies in an explicit `unsafe` block. An earlier revision of this PR mirrored the user-facing modifier onto that local function, which [@jkotas pointed out](#131245 (comment)) violates the model - the raw P/Invoke taking `char*` is obviously unsafe, while the wrapper is what discharges the obligation - so it was dropped and replaced with tests that lock the behavior in. The user's modifier is still mirrored onto the generated *wrapper*, so both halves of the partial agree. 3. [analyzer] **`IL5007`** reports methods with `LibraryImportAttribute` that declare no safety contract. Unlike `SYSLIB1064` it fires regardless of the opt-in, so a code base can be annotated *before* the switch is flipped. 4. [fixer] **`AddUnsafeToLibraryImportCodeFixProvider`** fixes `IL5007` by marking the method `unsafe` by default; developers can replace it with `safe` after auditing the boundary. This is the `[LibraryImport]` counterpart of `AddUnsafeToExternCodeFixProvider` from #131002 and, like the rest of that tooling, is not shipping (`#if DEBUG`) and off by default. Since `CSharpCompilationOptions.MemorySafetyRules` is still `internal` (dotnet/roslyn#82546), the opt-in is detected via the `updated-memory-safety-rules` feature flag - the same fallback Roslyn's own `SourceModuleSymbol.UseUpdatedMemorySafetyRules` uses. ### Diagnostics IDs Just for reference - `CS9389` [**Roslyn**] - An `extern` member must be explicitly marked `unsafe` or `safe`. - `CS9388` [**Roslyn**] - The `safe` modifier is only valid on non-unsafe `extern` members or field-like members of explicit or extended-layout types. - `CS0764` [**Roslyn**] - Both partial member declarations must be `unsafe`, or neither may be `unsafe`. - `CS9390` [**Roslyn**] - Both partial member declarations must be marked `safe`, or neither may be marked `safe`. - `SYSLIB1064` [**This PR**] - A method with `LibraryImportAttribute` must be marked `safe` or `unsafe` under the updated rules. - `IL5007` [**This PR**] - A method with `LibraryImportAttribute` has no explicit safety contract (migration only, needed for the code-fixer). ### Alternative design Instead of a new analyzer, the generator could emit its own part as `unsafe` and let the language enforce the rest: the user gets `CS0764` until they write `unsafe` too, or they write `safe` and we regenerate to match. Tempting - no new diagnostic ID, and no `CS9389`+`SYSLIB1064` doubling up in the forwarder shape. It was rejected because: - **The error lands in generated code.** `SourceOrdinaryMethodSymbol.PartialMethodChecks` reports both `CS0764` and `CS9390` at `implementation.GetFirstLocation()`, i.e. inside `LibraryImports.g.cs`. No code fix can be offered there, and the message never mentions P/Invoke. `CS9389` by contrast *does* land on the user's declaration, so the two shapes would report in different files with different wording. - **It only helps the wrapper shape** - the forwarder shape is already covered by `CS9389`. - **Generator output would start depending on the opt-in**, which has to be threaded through the incremental pipeline or every existing unsafe-v1 P/Invoke gets `CS0764`. - **`CS0764` is suppressed when `AllowUnsafeBlocks` is off** (`&& definition.CompilationAllowsUnsafe()`), so enforcement would silently disappear in that configuration. The speclet asks the same question ("should the language provide a narrower rule for partial members implemented by source generators?") and the working group answered: use an analyzer. ### Known limitations / follow ups - Roslyn does not allow `safe` on non-`extern` members yet (`CS9388`); dotnet/roslyn#84602 lifts this and is motivated by exactly this scenario. Until then `safe` can only be spelled on P/Invokes whose generated implementation is a forwarder, so tests only exercise `safe` in that shape. - `ConvertToLibraryImportFixer` preserves `unsafe` when rewriting a `[DllImport]` (new test) but drops `safe`, since `SyntaxGenerator` does not model the modifier and carrying it over today would produce code hitting `CS9388`. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5

Unsafe evolution: relax safe modifier placement restrictions

f751e57

jjonescz added Area-Compilers Feature - Unsafe Evolution labels Jul 23, 2026

jjonescz requested a review from Copilot July 23, 2026 09:46

Copilot started reviewing on behalf of jjonescz July 23, 2026 09:47 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

jjonescz marked this pull request as ready for review July 23, 2026 11:00

jjonescz requested a review from a team as a code owner July 23, 2026 11:00

jjonescz requested review from 333fred and AlekseyTs July 23, 2026 11:00

333fred reviewed Jul 24, 2026

View reviewed changes

AlekseyTs reviewed Jul 27, 2026

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Symbols/Source/ModifierUtils.cs

AlekseyTs reviewed Jul 27, 2026

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Symbols/Source/ModifierUtils.cs

AlekseyTs reviewed Jul 27, 2026

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

AlekseyTs reviewed Jul 27, 2026

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Symbols/Source/ModifierUtils.cs

jjonescz added 4 commits July 27, 2026 11:52

Rename error code

8966712

Fix when accessors differ from associated symbol

e9b10aa

Remove conflicting declaration modifier

015069c

Remove unused method

c0a0203

Copilot AI review requested due to automatic review settings July 27, 2026 10:10

Copilot started reviewing on behalf of jjonescz July 27, 2026 10:11 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

jjonescz added 2 commits July 27, 2026 12:25

Test compat mode and safe modifier

afd0e8d

Fix translations

fde30d0

Copilot AI review requested due to automatic review settings July 27, 2026 10:29

Copilot started reviewing on behalf of jjonescz July 27, 2026 10:31 View session

Copilot started reviewing on behalf of jjonescz July 28, 2026 16:01 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes