Skip to content

Unsafe evolution: use attribute to denote requires-unsafe members#82383

Open
jjonescz wants to merge 5 commits intodotnet:features/UnsafeEvolutionfrom
jjonescz:Unsafe-13-Attribute
Open

Unsafe evolution: use attribute to denote requires-unsafe members#82383
jjonescz wants to merge 5 commits intodotnet:features/UnsafeEvolutionfrom
jjonescz:Unsafe-13-Attribute

Conversation

@jjonescz
Copy link
Member

Test plan: #81207

@jjonescz jjonescz marked this pull request as ready for review February 12, 2026 18:05
@jjonescz jjonescz requested a review from a team as a code owner February 12, 2026 18:05
@jjonescz jjonescz requested review from 333fred and jcouv February 12, 2026 18:05
@jcouv jcouv self-assigned this Feb 12, 2026
var location = Syntax.Identifier.GetLocation();

if (CallerUnsafeMode.NeedsRequiresUnsafeAttribute())
if (NeedsSynthesizedRequiresUnsafeAttribute)
Copy link
Member

@jcouv jcouv Feb 12, 2026

Choose a reason for hiding this comment

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

I see we have an open issue related to this: https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md#extern-implicitly-unsafe
But this behavior doesn't seem mentioned in the main body of the spec. Let's make sure we capture it (can wait until the open issue is closed) #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

But this behavior doesn't seem mentioned in the main body of the spec

The extern section says "any extern method is automatically considered requires-unsafe if compiled under the updated memory safety rules (i.e., it gets the RequiresUnsafeAttribute)"

@jcouv
Copy link
Member

jcouv commented Feb 12, 2026

    object[] unsafeSymbols = ["C.E", "C.add_E", "C.remove_E"];

Should the Extern_Event test be checking extern event with and without [RequiresUnsafe] instead? Currently it is checking with and without unsafe
Comment applies to some other tests too (Extern_Indexer, ...) #Closed


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

if (ContainingModule.UseUpdatedMemorySafetyRules)
{
return IsUnsafe || IsExtern
return HasRequiresUnsafeAttribute || IsExtern || AssociatedSymbol?.CallerUnsafeMode == CallerUnsafeMode.Explicit
Copy link
Member

@jcouv jcouv Feb 12, 2026

Choose a reason for hiding this comment

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

nit: if I understood correctly, the associated symbol could not have CallerUnsafeMode.Implicit. Consider assertion as such (here and possibly other similar places)

Copy link
Member

Choose a reason for hiding this comment

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

nit: Any reason we couldn't do simpler change, just adding assertion: Debug.Assert(this.AssociatedSymbol?.CallerUnsafeMode != CallerUnsafeMode.Implicit);

{
allowedModifiers |= DeclarationModifiers.AccessibilityMask;
}
var allowedModifiers = isExplicitInterfaceImplementation ? DeclarationModifiers.None : DeclarationModifiers.AccessibilityMask;
Copy link
Member

@jcouv jcouv Feb 12, 2026

Choose a reason for hiding this comment

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

Spec: As part of the feature we'd allowed unsafe on accessors, but that's no longer the case. It's not blocking since no longer essential to using the feature, but we may want to confirm. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcouv
Copy link
Member

jcouv commented Feb 12, 2026

            $"Expected {symbol.GetType().Name} '{symbol.ToTestDisplayString()}' {(shouldHaveAttribute ? "or" : "and")} its associated symbol to{(shouldHaveAttribute ? "" : " not")} have the attribute.");

nit: it may be easier to follow with if statement. Then each branch can have a fully completed sentence #Closed


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

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (commit 1)

@jjonescz
Copy link
Member Author

            $"Expected {symbol.GetType().Name} '{symbol.ToTestDisplayString()}' {(shouldHaveAttribute ? "or" : "and")} its associated symbol to{(shouldHaveAttribute ? "" : " not")} have the attribute.");

Maybe I can simplify by having just one conditional interpolation at the beginning.


In reply to: 3894019832


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

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (commit 4)

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.

2 participants