Unsafe evolution: use attribute to denote requires-unsafe members#82383
Unsafe evolution: use attribute to denote requires-unsafe members#82383jjonescz wants to merge 5 commits intodotnet:features/UnsafeEvolutionfrom
Conversation
| var location = Syntax.Identifier.GetLocation(); | ||
|
|
||
| if (CallerUnsafeMode.NeedsRequiresUnsafeAttribute()) | ||
| if (NeedsSynthesizedRequiresUnsafeAttribute) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)"
Should the 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 |
There was a problem hiding this comment.
nit: if I understood correctly, the associated symbol could not have CallerUnsafeMode.Implicit. Consider assertion as such (here and possibly other similar places)
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
nit: it may be easier to follow with Refers to: src/Compilers/CSharp/Test/CSharp15/UnsafeEvolutionTests.cs:284 in cd0ec37. [](commit_id = cd0ec37, deletion_comment = False) |
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (commit 1)
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) |
Test plan: #81207