Unsafe evolution: use attribute to denote requires-unsafe members#82383
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) #Closed
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) |
|
@333fred for another review, thanks |
|
@333fred for second review. Thanks |
| /// </summary> | ||
| internal abstract bool IsUnsafe { get; } | ||
|
|
||
| internal bool HasRequiresUnsafeAttribute => GetDecodedWellKnownAttributeData()?.HasRequiresUnsafeAttribute == true; |
There was a problem hiding this comment.
I'm vaguely concerned this may get pulled on in such a way that we need to move it to Early well-known attributes. Probably best to leave a prototype to see if we can devise a test that will do so; it may be best to check with @AlekseyTs for suggestions on how to hit it.
There was a problem hiding this comment.
I'm vaguely concerned this may get pulled on in such a way that we need to move it to Early well-known attributes.
If this attribute isn't used during conversion classification or overload resolution, we should be good, I think.
There was a problem hiding this comment.
Agreed I don't expect this to be a problem; it's very similar to other "normal" attributes like UnscopedRef.
Test plan: #81207