Unsafe evolution: handle the rest of pointer-related operations #81420

jjonescz · 2025-11-24T12:55:32Z

Test plan: #81207

Everything under section Existing unsafe rules of the speclet should be now handled.

333fred · 2025-11-24T19:13:13Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

                    stackAllocType = new PointerTypeSymbol(TypeWithAnnotations.Create(elementType));
                    break;
                case ConversionKind.StackAllocToSpanType:
+                    // Under the updated memory safety rules, a stackalloc_expression is unsafe if being converted to Span/ROS,


Probably leave a PROTOTYPE to follow up with LDM on this rule. #Resolved

333fred · 2025-11-24T19:48:17Z

src/Compilers/CSharp/Test/Emit3/Semantics/UnsafeEvolutionTests.cs

+        expectedDiagnostics = PlatformInformation.IsWindows
+            ? [
+                // error CS8911: Using a function pointer type in this context is not supported.
+                Diagnostic(ErrorCode.ERR_FunctionPointerTypesInAttributeNotSupported).WithLocation(1, 1),


What is this error? It doesn't appear to have a good location or span, and and I don't know why a function pointer type would be illegal here. #Resolved

See the issue linked in the comment above - #77389.

It doesn't have a location because it's produced during emit inside TypeSerializer iirc, where there is no location information.

I could get rid of it by switching to portalepdb (instead of pdb on windows) but it seems there is no test for the bug, so I figured it would be good to have it captured in a test.

jcouv · 2025-11-25T17:41:19Z

src/Compilers/CSharp/Test/Emit3/Semantics/UnsafeEvolutionTests.cs

+        expectedDiagnostics = PlatformInformation.IsWindows
+            ? [
+                // error CS8911: Using a function pointer type in this context is not supported.
+                Diagnostic(ErrorCode.ERR_FunctionPointerTypesInAttributeNotSupported).WithLocation(1, 1),


Not related to this PR: should ERR_FunctionPointerTypesInAttributeNotSupported be marked as build-only? #Resolved

jcouv · 2025-11-25T17:57:21Z

src/Compilers/CSharp/Portable/Binder/Binder_Unsafe.cs

-        private CSDiagnosticInfo? GetUnsafeDiagnosticInfo(TypeSymbol? sizeOfTypeOpt, MemorySafetyRules disallowedUnder = MemorySafetyRules.Legacy)
+        private CSDiagnosticInfo? GetUnsafeDiagnosticInfo(TypeSymbol? sizeOfTypeOpt, MemorySafetyRules disallowedUnder = MemorySafetyRules.Legacy, ErrorCode? customErrorCode = null)
        {
+            Debug.Assert(sizeOfTypeOpt is null || disallowedUnder is MemorySafetyRules.Legacy);


Regarding first condition of the if below, do we have tests with updated safety rules for the scenarios where unsafe diagnostics are suppressed? #Resolved

That flag is used only when binding types (BindTypeArgument and BindEventType), so there is no observable difference for the updated rules, I think.

I can add tests that hit this code path anyway, though, thanks.

jcouv · 2025-11-25T18:00:55Z

src/Compilers/CSharp/Portable/Binder/Binder_Unsafe.cs

                {
-                    // PROTOTYPE: What about sizeOfTypeOpt/ERR_SizeofUnsafe?
-                    return new CSDiagnosticInfo(ErrorCode.ERR_UnsafeOperation);
+                    return new CSDiagnosticInfo(customErrorCode ?? ErrorCode.ERR_UnsafeOperation);


Should we be checking feature availability (IDS_FeatureUnsafeEvolution) here too? #Resolved

If we are here, the user has explicitly opted into the new unsafe rules. I thought we would check the langversion when validating the unsafe rules version (there is a PROTOTYPE comment for that validation). But I realize now we actually don't know the langversion in CSharpCompilationOptions, so it might be better to check it here after all. Thanks.

jcouv · 2025-11-25T18:14:19Z

src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs


-            bool hasErrors = ReportUnsafeIfNotAllowed(node, diagnostics);
+            bool hasErrors = ReportUnsafeIfNotAllowed(node, diagnostics) ||
+                ReportUnsafeIfNotAllowed(node, diagnostics, disallowedUnder: MemorySafetyRules.Updated);


Consider using explicit disallowedUnder: for the first invocation. #Resolved

There is a PROTOTYPE comment to update all callsites, but I can update this one now since I already touched it, thanks.

jcouv · 2025-11-25T18:16:44Z

src/Compilers/CSharp/Test/Emit3/Semantics/UnsafeEvolutionTests.cs

+            Diagnostic(ErrorCode.ERR_FeatureInPreview, "stackalloc int[3]").WithArguments("updated memory safety rules").WithLocation(1, 10),
+            // (8,26): error CS9501: stackalloc expression without an initializer inside SkipLocalsInit may only be used in an unsafe context
+            //     System.Span<int> a = stackalloc int[5];
+            Diagnostic(ErrorCode.ERR_UnsafeUninitializedStackAlloc, "stackalloc int[5]").WithLocation(8, 26),


Should there be an ERR_FeatureInPreview for stackalloc conversion in declaration of a? #Resolved

jcouv · 2025-11-25T18:18:32Z

src/Compilers/CSharp/Test/Emit3/Semantics/UnsafeEvolutionTests.cs

+            void M()
+            {
+                unsafe { System.Span<int> a = stackalloc int[5]; }
+                System.Span<int> b = stackalloc int[] { 1 };


Is it still useful to keep b and d in the "UnsafeContext" test, since they are not inside an unsafe context? #Resolved

Similarly, do we need to keep the top-level statements? (they seem redundant with previous test)

jcouv · 2025-11-25T18:22:37Z

src/Compilers/CSharp/Test/Emit3/Semantics/UnsafeEvolutionTests.cs

+    }
+
+    [Fact]
+    public void StackAlloc_UnsafeContext()


Consider similar test (specifically a declaration) without SkipLocalsInit #Resolved

jcouv

Done with review pass (commit 10)

jjonescz added 8 commits November 24, 2025 13:51

Add unsafe compiler test trait

b7f3dc3

Add more tests

67b08f8

Handle function pointers

d049d76

Add more tests

721e3dc

Handle sizeof

30d06d3

Handle fixed size buffers

54ba979

Handle stackalloc

d77292e

Add more tests

6770ef1

jjonescz added Area-Compilers Feature - Unsafe Evolution labels Nov 24, 2025

jjonescz changed the title ~~Unsafe evolution: require unsafe for the rest of pointer-related operations~~ Unsafe evolution: handle the rest of pointer-related operations Nov 24, 2025

Fix PDB-only expected diagnostics on Linux

d4c462b

jjonescz marked this pull request as ready for review November 24, 2025 15:23

jjonescz requested a review from a team as a code owner November 24, 2025 15:23

jjonescz requested review from 333fred and jcouv November 24, 2025 15:24

333fred reviewed Nov 24, 2025

View reviewed changes

jcouv self-assigned this Nov 24, 2025

Add PROTOTYPE comment

c13a696

jjonescz requested a review from 333fred November 25, 2025 13:08

333fred approved these changes Nov 25, 2025

View reviewed changes

jcouv reviewed Nov 25, 2025

View reviewed changes

jjonescz added 3 commits November 26, 2025 13:30

Check langversion in another place

00628b7

Improve code

fe3ddd0

Add more tests

3b86a63

jjonescz requested a review from jcouv November 26, 2025 13:41

Unsafe evolution: handle the rest of pointer-related operations #81420

Are you sure you want to change the base?

Unsafe evolution: handle the rest of pointer-related operations #81420

Conversation

jjonescz commented Nov 24, 2025

Uh oh!

333fred Nov 24, 2025 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Nov 24, 2025 • edited by jcouv 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 Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 25, 2025 • edited by jjonescz 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 Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 25, 2025 • edited by jjonescz 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 Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 25, 2025 • edited by jjonescz 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 Nov 26, 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!

jcouv Nov 25, 2025 • edited by jjonescz 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 Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 25, 2025 • edited by jjonescz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 25, 2025 • edited by jjonescz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

jcouv Nov 25, 2025 • edited by jjonescz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

333fred Nov 24, 2025 •

edited by jcouv

Loading

333fred Nov 24, 2025 •

edited by jcouv

Loading

jcouv Nov 25, 2025 •

edited by jjonescz

Loading

jcouv Nov 25, 2025 •

edited by jjonescz

Loading

jcouv Nov 25, 2025 •

edited by jjonescz

Loading

jjonescz Nov 26, 2025 •

edited

Loading

jcouv Nov 25, 2025 •

edited by jjonescz

Loading

jcouv Nov 25, 2025 •

edited by jjonescz

Loading

jcouv Nov 25, 2025 •

edited by jjonescz

Loading

jcouv Nov 25, 2025 •

edited by jjonescz

Loading