-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Add unsafe evolution attributes #125721
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add unsafe evolution attributes #125721
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,16 +4,17 @@ | |
| namespace System.Diagnostics.CodeAnalysis | ||
| { | ||
| /// <summary> | ||
| /// Indicates that the specified method requires unsafe code that may not be available | ||
| /// in all execution environments. | ||
| /// Indicates that the specified member requires the caller to be in an unsafe context. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This allows tools to understand which methods are unsafe to call when targeting | ||
| /// environments that do not support unsafe code. | ||
| /// </remarks> | ||
| [AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor, Inherited = false)] | ||
| [Conditional("DEBUG")] | ||
| internal sealed class RequiresUnsafeAttribute : Attribute | ||
| [AttributeUsage( | ||
| AttributeTargets.Constructor | AttributeTargets.Event | AttributeTargets.Method | AttributeTargets.Property, | ||
| Inherited = false, | ||
| AllowMultiple = false)] | ||
| public sealed class RequiresUnsafeAttribute : Attribute | ||
| { | ||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="RequiresUnsafeAttribute"/> class. | ||
| /// </summary> | ||
| public RequiresUnsafeAttribute() { } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I'm pretty sure we have really basic attribute tests that just validate the attribute can be constructed without throwing, or in the case of something like MemorySafetyRulesAttribute, that ctor arguments are properly roundtripped through properties. Could you do a quick search and add a couple tests in the right place? |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.ComponentModel; | ||
|
|
||
| namespace System.Runtime.CompilerServices | ||
| { | ||
| /// <summary>Indicates the version of the memory safety rules used when the module was compiled.</summary> | ||
| [EditorBrowsable(EditorBrowsableState.Never)] | ||
| [AttributeUsage(AttributeTargets.Module, Inherited = false, AllowMultiple = false)] | ||
| public sealed class MemorySafetyRulesAttribute : Attribute | ||
| { | ||
| /// <summary>Initializes a new instance of the <see cref="MemorySafetyRulesAttribute"/> class.</summary> | ||
| /// <param name="version">The version of the memory safety rules used when the module was compiled.</param> | ||
| public MemorySafetyRulesAttribute(int version) => Version = version; | ||
|
|
||
| /// <summary>Gets the version of the memory safety rules used when the module was compiled.</summary> | ||
| public int Version { get; } | ||
jjonescz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using Xunit; | ||
|
|
||
| namespace System.Diagnostics.CodeAnalysis.Tests; | ||
|
|
||
| public class RequiresUnsafeAttributeTests | ||
| { | ||
| [Fact] | ||
| public static void TestConstructor() | ||
| { | ||
| new RequiresUnsafeAttribute(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inherited=false is interesting. Is that correct? From a reflection standpoint, we wouldn't expect a derived type to inherit the requires unsafe nature of its base?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compiler doesn't consider inherited attributes though, so it seems better to disallow in reflection too, for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, as part of the language feature, we require that a derived API is not RequiresUnsafe if the base API is not RequiresUnsafe too.