Unsafe evolution: handle extern members by jjonescz · Pull Request #81786 · dotnet/roslyn

jjonescz · 2025-12-22T11:54:52Z

Test plan: #81207

Follow up on #81581 (i.e., still only for methods and properties).

Implements this speclet section: https://github.com/dotnet/csharplang/blob/d4a7252e082927c9e6d6f1386d0bb46fa690db8b/proposals/unsafe-evolution.md#extern

AlekseyTs · 2025-12-23T16:15:55Z

    private void ReportDiagnosticsIfUnsafeMemberAccess(BindingDiagnosticBag diagnostics, Symbol symbol, SyntaxNode node)

Diagnostic bag is usually the last parameter. #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Unsafe.cs:24 in 8569af4. [](commit_id = 8569af4, deletion_comment = False)

AlekseyTs · 2025-12-23T16:27:54Z

+    /// The member is explicitly marked as <see langword="extern"/> under the updated memory safety rules.
+    /// This can only appear for source symbols (after metadata roundtrip, this turns into <see cref="Explicit"/>).
+    /// </summary>
+    Extern,


Extern

In general, we prefer to treat source and metadata symbols uniformly. There are exceptions, but they usually happen when an information isn't preserved in metadata. In this case, however, it looks like the information is preserved "The member is explicitly marked", whether by unsafe or extern modifier. It is not clear to me why do we want to separate extern, this looks fragile and bug prone. I think explicit in source should get CallerUnsafeMode.Explicit value as it gets it when imported from metadata. #Closed

when an information isn't preserved in metadata

See the speclet section and the corresponding discussion about this. extern is not preserved in ref assemblies apparently.

It is not clear to me why do we want to separate extern

I guess I can merge the enum members (I originally separated them to have a different error message, but got rid of that since then for uniformity as you are saying).

See the speclet section and the corresponding discussion about this. extern is not preserved in ref assemblies apparently.

I am not saying that extern members should be treated as safe. I am saying that the CallerUnsafeMode API should produce the same value from source and metadata.

Yes, I agree. That part was replying to "There are exceptions, but they usually happen when an information isn't preserved in metadata" which technically is the case here (extern is not preserved in metadata)

which technically is the case here (extern is not preserved in metadata)

No, this is not the case. The information is preserved, there is an attribute on the member, it really doesn't matter what syntax triggered its application.

If there were no attribute, then the safety information wouldn't be preserved.

Right, I will unify the Extern and Explicit members, thanks.

AlekseyTs · 2025-12-23T16:31:13Z

Done with a quick glance over the changes (commit 1) #Closed

AlekseyTs · 2025-12-23T16:37:58Z

Please make sure we have tests that consume APIs through compilation references from a compilation that is using old rules and doesn't emit new attributes etc. The expectation is that the experience should match the experience of consuming metadata references produced from the same compilation. Retargeting scenarios are probably interesting too.

To clarify, "from a compilation that is using old rules" refers to the referenced compilation rather than to the consuming compilation. #Closed

jjonescz · 2025-12-23T16:40:52Z

consume APIs through compilation references

We have that. All the tests use common helpers that always test both image and compilation references.

Retargeting scenarios are probably interesting too.

I can look into that (or maybe I'll write it to test plan), thanks. #Resolved

AlekseyTs · 2025-12-23T16:46:31Z

We have that.

Could you confirm that the safety mode is different between compilations? Perhaps you could point me to an example. #Closed

jjonescz · 2025-12-23T16:51:20Z

Could you confirm that the safety mode is different between compilations? Perhaps you could point me to an example.

roslyn/src/Compilers/CSharp/Test/CSharp15/UnsafeEvolutionTests.cs

Line 3589 in 8569af4

// When compiling the lib under legacy rules, extern members are not unsafe.

(This isn't using a common helper after all since it's sufficiently different from the common scenarios, but still verifies both types of references.) #Resolved

AlekseyTs · 2025-12-23T16:58:52Z

The scenario in referenced test doesn't look like one I have in mind. I am talking about scenarios where consumer uses new rules and the library uses old rules. It looks like the referenced test does the opposite. And such tests should not be limited to extern members. #Closed

AlekseyTs · 2025-12-23T17:02:37Z

I guess my original comment was hard to "parse". I tried to clarify it #Closed

AlekseyTs · 2025-12-23T18:20:33Z

private void CompileAndVerify(

Overloading existing common APIs hurts readability of tests. Instead of overloading consider using distinct name #Closed

Refers to: src/Compilers/CSharp/Test/CSharp15/UnsafeEvolutionTests.cs:23 in 8569af4. [](commit_id = 8569af4, deletion_comment = False)

AlekseyTs · 2025-12-23T18:22:52Z

        ReadOnlySpan<string> additionalSources = default,
-        Verification verify = default)
+        Verification verify = default,
+        CallerUnsafeMode expectedUnsafeMode = CallerUnsafeMode.Explicit)


CallerUnsafeMode expectedUnsafeMode = CallerUnsafeMode.Explicit

I think that new optional parameters hurt readability of tests, especially when the API is used a lot. No one remembers the defaults. #Closed

I believe the default here makes sense, it should be rarely different.

jjonescz · 2026-01-05T10:42:46Z

private void CompileAndVerify(

~~This is a pre-existing helper and I'd rather not bloat the diff by renaming it here, I can do it in a follow up PR.~~ Nevermind, I guess I can rename to help clarify the other comments too.

In reply to: 3687547136

Refers to: src/Compilers/CSharp/Test/CSharp15/UnsafeEvolutionTests.cs:23 in 8569af4. [](commit_id = 8569af4, deletion_comment = False)

jjonescz · 2026-01-05T10:50:11Z

I am talking about scenarios where consumer uses new rules and the library uses old rules. It looks like the referenced test does the opposite. And such tests should not be limited to extern members.

I think the example I posted does exactly that - the library uses old rules (old rules are the default and the compilation in var lib = CreateCompilation(...) doesn't overwrite that) and the consumer uses new rules (via options: TestOptions.UnsafeReleaseExe.WithUpdatedMemorySafetyRules()). We test this in many places (not just for extern members) via the custom CompileAndVerify helper (now renamed to CompileAndVerifyUnsafe) defined at the top of the file.

In reply to: 3687334849

jjonescz · 2026-01-05T10:55:43Z

    private void ReportDiagnosticsIfUnsafeMemberAccess(BindingDiagnosticBag diagnostics, Symbol symbol, SyntaxNode node)

This was modeled after ReportDiagnosticsIfObsolete for consistency (it should be also called at the same callsites).

In reply to: 3687194228

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Unsafe.cs:24 in 8569af4. [](commit_id = 8569af4, deletion_comment = False)

AlekseyTs

LGTM (commit 3)

Make extern members caller-unsafe

8569af4

jjonescz added Area-Compilers Feature - Unsafe Evolution labels Dec 22, 2025

jjonescz changed the title ~~Make extern members caller-unsafe~~ Unsafe evolution: handle extern members Dec 22, 2025

jjonescz marked this pull request as ready for review December 22, 2025 13:05

jjonescz requested a review from a team as a code owner December 22, 2025 13:05

jjonescz requested review from 333fred and jcouv December 22, 2025 13:05

333fred approved these changes Dec 22, 2025

View reviewed changes

jcouv self-assigned this Dec 22, 2025

AlekseyTs reviewed Dec 23, 2025

View reviewed changes

Unify explicit and extern unsafe modes

c7f5d8b

Rename helper

0009241

AlekseyTs approved these changes Jan 6, 2026

View reviewed changes

jjonescz merged commit 24f9053 into dotnet:features/UnsafeEvolution Jan 7, 2026
25 checks passed

jjonescz deleted the Unsafe-09-Extern branch January 7, 2026 09:56

dotnet-maestro Bot mentioned this pull request Feb 27, 2026

[release/10.0.3xx] Source code updates from dotnet/roslyn dotnet/dotnet#5091

Merged

dotnet-maestro Bot mentioned this pull request Feb 28, 2026

[main] Source code updates from dotnet/roslyn dotnet/dotnet#5006

Merged

dotnet-bot mentioned this pull request Mar 3, 2026

[Automated] PRs inserted in VS build main-11602.225 #82592

Closed

dotnet-maestro Bot mentioned this pull request Mar 5, 2026

[release/11.0.1xx-preview2] Source code updates from dotnet/roslyn dotnet/dotnet#5237

Merged

dotnet-bot mentioned this pull request Mar 14, 2026

[Automated] PRs inserted in VS build feature.debugger.wix5-11613.132 #82758

Closed

Conversation

jjonescz commented Dec 22, 2025

Uh oh!

AlekseyTs commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

jjonescz Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

jjonescz Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjonescz commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjonescz commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

jjonescz commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjonescz commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjonescz commented Jan 5, 2026

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AlekseyTs commented Dec 23, 2025 •

edited

Loading

AlekseyTs Dec 23, 2025 •

edited

Loading

jjonescz Dec 23, 2025 •

edited

Loading

AlekseyTs commented Dec 23, 2025 •

edited

Loading

AlekseyTs commented Dec 23, 2025 •

edited

Loading

jjonescz commented Dec 23, 2025 •

edited

Loading

AlekseyTs commented Dec 23, 2025 •

edited

Loading

jjonescz commented Dec 23, 2025 •

edited

Loading

AlekseyTs commented Dec 23, 2025 •

edited

Loading

AlekseyTs commented Dec 23, 2025 •

edited

Loading

AlekseyTs commented Dec 23, 2025 •

edited

Loading

AlekseyTs Dec 23, 2025 •

edited

Loading

jjonescz commented Jan 5, 2026 •

edited

Loading

jjonescz commented Jan 5, 2026 •

edited

Loading