-
Notifications
You must be signed in to change notification settings - Fork 1.1k
More unsafe spec updates
#9831
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?
More unsafe spec updates
#9831
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -134,9 +134,16 @@ It is a memory safety error to convert an `unsafe` member to a delegate type tha | |||||
| is updated to include whether the _anonymous function_ has the `unsafe` keyword, or the _method group_ is to a member that is marked `unsafe`. If it is, an anonymous function type is created, just as | ||||||
| it would be if any parameter were a by-`ref`, optional, or `params`. | ||||||
|
|
||||||
| It is a memory safety error convert a delegate type that is marked as `unsafe` to `System.Delegate`/`System.Linq.Expressions.Expression`/`System.Linq.Expressions.Expression<T>`. | ||||||
| It is a memory safety error convert a delegate type that is marked as `unsafe` to `System.Delegate`/`System.Linq.Expressions.Expression`/`System.Linq.Expressions.Expression<T>`, or any interface those | ||||||
| types implement or base type of those types. | ||||||
|
|
||||||
| A delegate type that is marked `unsafe` can only be invoked in an `unsafe` context. If a delegate type is `unsafe`, then its `Invoke`, `BeginInvoke`, and `EndInvoke` methods are also marked as `unsafe`. | ||||||
| A delegate type that is marked `unsafe` can only be invoked in an `unsafe` context. If a delegate type is `unsafe`, then its `Invoke`, `BeginInvoke`, and `EndInvoke` methods are marked as `unsafe`. | ||||||
|
|
||||||
| > [!NOTE] | ||||||
| > We don't actually attribute the delegate type itself, just the `Invoke`, `BeginInvoke`, and `EndInvoke` methods. Determining whether a delegate type is `unsafe` is done by examining those 3 methods. | ||||||
| > If all are marked as `unsafe`, the delegate type is considered `unsafe`. If only some are marked as `unsafe`, then it is presumed that calling the others is safe and only calling the member that is | ||||||
| > marked as `unsafe` will cause a memory safety error. It will be a memory safety error to convert an `unsafe` lambda or method group to a delegate type that is does not have all of `Invoke`, `BeginInvoke`, | ||||||
|
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.
Suggested change
|
||||||
| > and `EndInvoke` marked as `unsafe`. | ||||||
|
|
||||||
| ### `extern` | ||||||
|
|
||||||
|
|
@@ -146,39 +153,26 @@ value cannot be safely called by C#, as the calling convention used for the meth | |||||
| ### Unsafe modifiers and contexts | ||||||
|
|
||||||
| Today, as covered by the [unsafe context specification][unsafe-context-spec], `unsafe` behaves in a lexical manner, marking the entire textual body contained by the `unsafe` block as an `unsafe` context | ||||||
| (except for iterator bodies). We propose changing this definition from textual to sematic. `unsafe` on a type will now mean that all members declared by that type are considered `unsafe`, and all of the | ||||||
| member bodies of that type are considered an `unsafe` context. `unsafe` on a member will mean that that member is `unsafe`, and the body of that member is considered an `unsafe` context. For existing code | ||||||
| moving to the new definition of `unsafe`, this may produce a number of false positives for methods that don't need to be considered `unsafe`; we believe this better than false positives around not doing | ||||||
| this, or making it an error to put `unsafe` on a type which would easily be the largest breaking change that we've ever introduced in C#. | ||||||
| (except for iterator bodies). We propose changing this definition from textual to sematic. `unsafe` on a member will mean that that member is `unsafe`, and the body of that member is considered an `unsafe` | ||||||
|
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. Regarding the syntactical meaning of unsafe on bodies, there is no change, right? So for example, iterators still escape out of the unsafe context.
Member
Author
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. There's an open question on that.
Member
Author
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. There's an open question on that. |
||||||
| context. `unsafe` on a type (other than delegate types) will be permitted for source compatibility purposes only; it will have no meaning, and the compiler will produce a warning informing the user that it | ||||||
| does not have any effect. | ||||||
|
|
||||||
| `unsafe` on a member is _not_ applied to any nested anonymous or local functions inside the member. To mark a anonymous or local function as `unsafe`, it must manually be marked as `unsafe`. The same goes for | ||||||
| anonymous and local functions declared inside of an `unsafe` block. | ||||||
|
|
||||||
| When a type is `partial`, `unsafe` on a part of that type marks everything declared or defined in that part as `unsafe`. If a `partial` member is declared or defined in an `unsafe partial` part, that member | ||||||
| is considered `unsafe`, even if it is also declared or defined in a part that not marked as `unsafe`. If one part of a member is declared as `unsafe`, then all parts of that member are considered unsafe. | ||||||
| When a member is `partial`, both parts must agree on the `unsafe` modifier, unchanged from C# rules today. | ||||||
|
|
||||||
| ```cs | ||||||
| new C1().M1(); // Error, M1() must be called in an `unsafe` context | ||||||
| new C2().M2(); // Error, M2() must be called in an `unsafe` context | ||||||
|
|
||||||
| unsafe partial class C1 | ||||||
| { | ||||||
| public partial void M1(); | ||||||
| } | ||||||
|
|
||||||
| partial class C1 | ||||||
| { | ||||||
| public partial void M1() => Console.WriteLine("hello world"); | ||||||
| } | ||||||
|
|
||||||
| partial class C2 | ||||||
| { | ||||||
| public partial void M1(); // Error: both parts must be unsafe, or neither can be | ||||||
| public partial unsafe void M2(); | ||||||
| } | ||||||
|
|
||||||
| partial class C2 | ||||||
| partial class C1 | ||||||
| { | ||||||
| public partial void M2() => Console.WriteLine("hello world"); | ||||||
| public unsafe partial void M1() => Console.WriteLine("hello world"); | ||||||
| public partial void M2() => Console.WriteLine("hello world"); // Error: both parts must be unsafe, or neither can be | ||||||
| } | ||||||
| ``` | ||||||
|
|
||||||
|
|
@@ -209,7 +203,7 @@ namespace System.Runtime.CompilerServices | |||||
| public int Version { get; } | ||||||
| } | ||||||
|
|
||||||
| [AttributeUsage(AttributeTargets.Field | AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Delegate | AttributeTargets.Constructor, AllowMultiple = false, Inherited = true)] | ||||||
| [AttributeUsage(AttributeTargets.Field | AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Constructor, AllowMultiple = false, Inherited = true)] | ||||||
| public sealed class RequiresUnsafeAttribute : Attribute | ||||||
| { | ||||||
| } | ||||||
|
|
@@ -234,6 +228,16 @@ anonymous and local functions do not keep the unsafe context of their containing | |||||
| Is conversion of a lambda or method group marked `unsafe` to a non-unsafe delegate type permitted without warning or error in an `unsafe` context? If we don't do this, then it could be fairly painful | ||||||
| for various parts of the ecosystem, particularly any enumerables that are passed through LINQ queries. | ||||||
|
|
||||||
| ### Delegate type `unsafe`ty | ||||||
|
|
||||||
| We could remove the ability to make delegate types as `unsafe` entirely, and simply require that all conversions of `unsafe` lambdas or method groups to a delegate type occur inside an `unsafe` context. | ||||||
| This could simplify the model around `unsafe` in C#, but at the risk of forcing `unsafe` annotations in the wrong spot and having an area where the real area of `unsafe`ty isn't properly called out. | ||||||
|
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. If we started without unsafe delegates, can they be added later as an incremental non-breaking change once/if proven necessary? #Resolved
Member
Author
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. Yes, I expect so. |
||||||
|
|
||||||
| ### Lambda/method group natural types | ||||||
|
|
||||||
| Today, the only real impact on codegen (besides additional metadata) is changing the *function_type* of a lambda or method group when `unsafe` is in the signature. If we were to avoid doing this, then | ||||||
|
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. This is not impacting just codegen, it's impacting semantics too since different delegate types cannot be converted to each other, so for example this snippet will stop working: unsafe void M() { }
var x = M;
System.Action y = x; |
||||||
| there would be no real impact to codegen, which could give adopters more confidence that behavior has not subtly changed under the hood. | ||||||
|
|
||||||
| ### `stackalloc` as initialized | ||||||
|
|
||||||
| Today, [the spec](https://github.com/dotnet/csharpstandard/blob/draft-v9/standard/expressions.md#12822-stack-allocation) always considers `stackalloc` memory as uninitialized, and says that the contents | ||||||
|
|
@@ -266,6 +270,12 @@ unsafe | |||||
| Console.WriteLine(result); | ||||||
| ``` | ||||||
|
|
||||||
| ### `unsafe` context defaults in members | ||||||
|
|
||||||
| We could consider not automatically making the entire body of an `unsafe` method an `unsafe` context. Rust did this in [RFC 2585](https://github.com/rust-lang/rfcs/blob/master/text/2585-unsafe-block-in-unsafe-fn.md), | ||||||
| with the motivation that it helps reduce the scope of `unsafe` blocks to the locations in which `unsafe` is actually used. We could do the same thing in C#, either as a warning or an error, with similar | ||||||
| motivations. | ||||||
|
|
||||||
| ### `unsafe` fields | ||||||
|
|
||||||
| Today, no proposal is made around `unsafe` on a field. We may need to add it though, such that any read from or write to a field marked as `unsafe` must be in an `unsafe` context. This would | ||||||
|
|
||||||
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.
Does it mean that the unsafe delegates cannot be used as generic arguments (in safe contexts)?
If they can be used as generic arguments, one can do the offending memory unsafe cast indirectly by doing e..g.:
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.
@333fred Let's capture this in open issues
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.
Would it be better to postpone the whole unsafe delegate idea until we have evidence that it is actually worth it to deal with all the complex corner cases that it comes with?