Add metadata rules for unsafe #9814
Conversation
| { | ||
| /// <summary>Indicates the language version of the memory safety rules used when the module was compiled.</summary> | ||
| [AttributeUsage(AttributeTargets.Module, Inherited = false)] | ||
| public sealed class MemorySafetyRulesAttribute : Attribute |
There was a problem hiding this comment.
Will this play well with other languages? Suppose that C# 15 adds new safety rules. Now also suppose that F# adds safety rules in version 8. Does Roslyn understand which langauge version the number is related to? Are attributes under System.Runtime.CompilerServices assumed to be limited to C#?
There was a problem hiding this comment.
It's based on the RefSafetyRulesAttribute scheme, which also used language version. Perhaps @jaredpar or @RikkiGibson could comment on that.
There was a problem hiding this comment.
It's documented here: https://github.com/dotnet/csharplang/blob/main/proposals/csharp-11.0/low-level-struct-improvements.md#refsafetyrulesattribute. The version is actually ignored, but it could be considered in the future if there was another "ref safety rules" version.
There was a problem hiding this comment.
@jjonescz Thanks. I don't see anything specific about the attribute and other languages though. It seems like it is implicitly a C# only attribute. This gets back to my question of, what if F# has their own ref-safety rules? or another .NET compiler decides to express their own memory safety rules?
I'm not saying we should throw it all away or even add a new API or change the existing one. I think I would like to see some clear documentation on the API though say "is specifically for Roslyn" or "is specifically for C#". I'm not going to quibble about potentially other non-Roslyn C# compilers.
There was a problem hiding this comment.
The attribute is essentially for C#. The version number in the attribute is always a C# major language version. Languages that want to interop with C#'s unsafe code, will need to recognize and understand it, and possibly even emit it themselves if they want their "unsafe methods", etc. to be understood and treated similarly as C#'s.
This gets back to my question of, what if F# has their own ref-safety rules? or another .NET compiler decides to express their own memory safety rules?
Other languages/compilers can simply be oblivious to this attribute. That is the default state of affairs. If another language decided to implement a divergent concept of unsafe, then, we would likely remain in that situation of: we are oblivious to their unsafe mechanics, and they are oblivious to ours. That seems like something we should avoid, and also something which is unlikely to arise inadvertently.
In the past, F# did some work to check ref safety for ref structs, etc. It doesn't look like they recognize RefSafetyRulesAttribute, though, they might wish to do so in the future. That's the nature of the impact I would anticipate on other major .NET languages for this feature as well.
There was a problem hiding this comment.
The attribute is essentially for C#.
Then that should be documented cleared on the API.
| public int Version { get; } | ||
| } | ||
|
|
||
| [AttributeUsage(AttributeTargets.Field | AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Delegate | AttributeTargets.Constructor, AllowMultiple = false, Inherited = true)] |
There was a problem hiding this comment.
| [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)] |
As discussed offline, we may want to mark delegate Invoke method as unsafe so that delegate types are not special. AttributeTargets.Delegate is not needed in that case.
| } | ||
| ``` | ||
|
|
||
| #### Compat mode |
There was a problem hiding this comment.
I think this compat behavior is fine to reduce virality of the feature, but we should consider offering a facility to tighten our enforcement.
The scenario is: I'm enabling the new rules for my project but want to ensure my references all using the new rules too.
Maybe that's an analyzer or maybe that's a different value to the project-level flag that turns on the new rules.
There was a problem hiding this comment.
That feels like the job of either an analyzer or even an SDK build task, imo.
There was a problem hiding this comment.
Agreed, I'm not saying it would be implemented in the compiler.
In LDM last week, we discussed the need to review the full picture with all the pieces: compiler+analyzers+guidelines+SDK to see how it all fits.
If this spec is only meant to cover the language/compiler portion, where would be the overarching design at?
There was a problem hiding this comment.
@agocke Either you or JanK at LDM last week mentioned that the language rules would not be sufficient by themselves to deliver on the memory safety claim we're trying to offer as part of the feature, and we may need analyzers and coding guidelines to shore up some gaps.
Will you be on point to update the overall design doc with things that fall outside of language/compiler?
- coding guidelines for proper usage of the feature
- analyzers or additional SDK tooling
The reason I'm asking is I'd like to capture a open question: if we design the language/compiler for more compatibility (older compilers and not-opted-into-new-rules compiler are oblivious to new "unsafecaller" modifier) then should some other part of the system allow the user to crank the enforcement to "strict"?
The scenario is: I'm enabling the new rules for my project but want to ensure my references all using the new rules too (no "oblivious" code here).
Before discussing how this would be implemented, my question is whether that's a scenario we're trying to support.
|
|
||
| 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 |
There was a problem hiding this comment.
Unrelated to this PR, but:
unsafeon a member will mean that that member isunsafe, and the body of that member is considered anunsafecontext.
Was it considered to change this so that unsafe on a member has no impact on the body? That's what rust did and their motivation seems to apply to us too (reducing unsafe code): https://github.com/rust-lang/rfcs/blob/master/text/2585-unsafe-block-in-unsafe-fn.md#motivation
There was a problem hiding this comment.
Yes, we discussed this heavily in the most recent LDM. It's not currently in the plan, but we haven't ruled it out.
There was a problem hiding this comment.
nit: I assume you're tracking those offline, but consider adding an open issues section to this doc.
There was a problem hiding this comment.
It will be added when I get the notes from yesterday typed up 🙂.
6 participants
Expands rules for how metadata will be emitted for the new unsafe feature