Conversation
|
Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices |
There was a problem hiding this comment.
Pull request overview
Adds two runtime attributes to the public System.Runtime contract to support unsafe-context analysis and module-level memory-safety metadata, with corresponding implementations in System.Private.CoreLib.
Changes:
- Exposes
System.Diagnostics.CodeAnalysis.RequiresUnsafeAttributeas a public attribute applicable to constructors/events/methods/properties. - Introduces
System.Runtime.CompilerServices.MemorySafetyRulesAttribute(module-level, editor-browsable never) with aVersionpayload. - Implements
MemorySafetyRulesAttributein System.Private.CoreLib and updatesRequiresUnsafeAttributeto be public (non-conditional) with updated documentation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.Runtime/ref/System.Runtime.cs | Adds the new public attribute declarations to the System.Runtime reference contract. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/MemorySafetyRulesAttribute.cs | Implements MemorySafetyRulesAttribute with version storage and hidden editor-browsing. |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/RequiresUnsafeAttribute.cs | Makes RequiresUnsafeAttribute public and expands its applicable targets. |
You can also share your feedback on Copilot code review. Take the survey.
...raries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/RequiresUnsafeAttribute.cs
Show resolved
Hide resolved
...raries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/RequiresUnsafeAttribute.cs
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Pull request overview
This PR exposes RequiresUnsafeAttribute publicly and introduces a new MemorySafetyRulesAttribute API, updating the reference surface and adding basic tests to validate constructor behavior.
Changes:
- Add
MemorySafetyRulesAttributetoSystem.Runtime.CompilerServices(ref + implementation) and test itsVersionroundtrip. - Make
RequiresUnsafeAttributea public attribute with broaderAttributeTargetsand add a smoke test. - Add CoreLib package-validation suppressions for the new/changed APIs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Runtime/CompilerServices/AttributesTests.cs | Adds a theory test for MemorySafetyRulesAttribute.Version. |
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Diagnostics/CodeAnalysis/RequiresUnsafeAttributeTests.cs | Adds a minimal constructor smoke test for RequiresUnsafeAttribute. |
| src/libraries/System.Runtime/ref/System.Runtime.cs | Updates the public ref surface to include RequiresUnsafeAttribute and MemorySafetyRulesAttribute. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/MemorySafetyRulesAttribute.cs | Introduces the CoreLib implementation of MemorySafetyRulesAttribute. |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/RequiresUnsafeAttribute.cs | Changes RequiresUnsafeAttribute from internal/debug-only to public and updates usage/summary. |
| src/coreclr/System.Private.CoreLib/CompatibilitySuppressions.xml | Adds compat suppressions for the new attribute and attribute-usage differences. |
| src/coreclr/nativeaot/System.Private.CoreLib/src/CompatibilitySuppressions.xml | Mirrors the compat suppressions for NativeAOT CoreLib. |
You can also share your feedback on Copilot code review. Take the survey.
...ies/System.Private.CoreLib/src/System/Runtime/CompilerServices/MemorySafetyRulesAttribute.cs
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/CompatibilitySuppressions.xml
Outdated
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/CompatibilitySuppressions.xml
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/System.Private.CoreLib/src/CompatibilitySuppressions.xml
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/System.Private.CoreLib/src/CompatibilitySuppressions.xml
Outdated
Show resolved
Hide resolved
|
Does ILLink strip these attributes? If it does - should we fix it? cc @agocke |
|
The linker doesn’t strip attributes |
Linker can strip attributes that are not needed at runtime. It is controlled by https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.Shared.xml Unless we start reading this attribute at runtime (no plans for that currently), we may want to add it to the trimmed list. |
agocke
left a comment
There was a problem hiding this comment.
LGTM, matches recent LDM decisions.
...raries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/RequiresUnsafeAttribute.cs
Show resolved
Hide resolved
|
@agocke I disabled auto-merge as I have a question. For some reason my comment didn't post earlier, but it shows up now. |
There was a problem hiding this comment.
Pull request overview
Adds the BCL surface area needed for the “unsafe evolution” feature by introducing the new attributes in the appropriate namespaces and wiring them into reference assemblies and tests.
Changes:
- Add
System.Runtime.CompilerServices.MemorySafetyRulesAttributeto System.Private.CoreLib (+ ref) and validate itsVersionround-trips in tests. - Promote
System.Diagnostics.CodeAnalysis.RequiresUnsafeAttributefrom internal/debug-only to a public attribute with updatedAttributeUsage(+ ref) and add a basic constructor test. - Ensure the new CoreLib source file is included in the shared projitems for compilation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Runtime/CompilerServices/AttributesTests.cs | Adds test coverage for MemorySafetyRulesAttribute.Version. |
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Diagnostics/CodeAnalysis/RequiresUnsafeAttributeTests.cs | New test validating RequiresUnsafeAttribute can be constructed. |
| src/libraries/System.Runtime/ref/System.Runtime.cs | Updates the reference surface to include RequiresUnsafeAttribute and MemorySafetyRulesAttribute. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/MemorySafetyRulesAttribute.cs | Introduces the new module-level attribute with Version property. |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/RequiresUnsafeAttribute.cs | Makes the attribute public and updates its intended semantics/targets. |
| src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems | Includes the new attribute source file in the CoreLib build inputs. |
You can also share your feedback on Copilot code review. Take the survey.
| AttributeTargets.Constructor | AttributeTargets.Event | AttributeTargets.Method | AttributeTargets.Property, | ||
| Inherited = false, | ||
| AllowMultiple = false)] | ||
| public sealed class RequiresUnsafeAttribute : Attribute |
There was a problem hiding this comment.
This is included from multiple project files to make the same sources files compiled downlevel. It needs to follow the same public/internal pattern as nullability attributes with CORELIB ifdef:
There was a problem hiding this comment.
Good catch, thanks. I see the RequiresUnsafeAttribute is source-shared with Microsoft.Bcl.Memory.csproj and it makes sense we don't want to have it public there; but I'm surprised no CI check would catch this public API surface change in Microsoft.Bcl.Memory?
There was a problem hiding this comment.
@ericstj Is it expected that we do not run public API surface validation for Microsoft.Bcl.Memory?
| AttributeTargets.Constructor | AttributeTargets.Event | AttributeTargets.Method | AttributeTargets.Property, | ||
| Inherited = false, | ||
| AllowMultiple = false)] | ||
| public sealed class RequiresUnsafeAttribute : Attribute |
There was a problem hiding this comment.
Another thought from reading Richard's slide deck: should RequiresUnsafe be marked as experimental for .NET 11?
There was a problem hiding this comment.
What do we think could potentially change here for the attribute that would warrant it being marked that way
There was a problem hiding this comment.
I don't expect the attribute will change since it's pretty simple, but I don't know if that should be relevant.
The whole feature is experimental (ie. not "shipped") in .NET 11. Normally, that means that users have to opt into using an explicitly experimental feature.
FWIW, for roslyn APIs, we decided all APIs added for a not-yet-shipped feature get [Experimental] (systematically, no case-by-case consideration).
There was a problem hiding this comment.
I'm not sure in this case it serves any practical purpose if we don't believe that the attribute itself will change.
Functionally this would just cause this everywhere:
#pragma warning disable DiagnosticId
[RequiresUnsafe]
#pragma warning restore DiagnosticId
public void M()
{
// ...
}or require us to globally suppress the diagnostic and hinder user adoption of the attribute as well, when user adoption will already require explicit opt-in at the language level.
There was a problem hiding this comment.
I agree that we want to streamline usage/adoption, but we also intentionally include one explicit speed bump specifically for experimental features. This feature should not be exempt.
I'm not sure what form it should take though (suppression for [Experimental] analyzer, flag given to the SDK with the word "Experimental" in it, ...).
Reframing as a more general question: what are the gestures we'll require from users to opt-in and do they clearly convey that the feature is experimental in .NET 11?
There was a problem hiding this comment.
Is the language going to require LangVersion=preview even in .NET 11?
What's the compiler behavior if it encounters an assembly with MemorySafetyRules or an API with RequiresUnsafe and the user hasn't opted in at the language level?
This attribute in particular does feel more like RequiresPreviewFeatures (in the same way as we did static virtuals in interface for language + generic math for libraries in .NET 6) than Experimental, but I could also see us just saying the attribute isn't going to change and annotating APIs as requiring unsafe is always ok. The analyzer experience may just depend on the language (you won't see any warnings in VB or F#, for example) or language version (C#'s analysis may change between C# 15 and 16, which is the same plan we had nullable on where the nullable attributes weren't annotated on the libraries side).
There was a problem hiding this comment.
Is the language going to require LangVersion=preview even in .NET 11?
I think we should, but we should confirm with the crew.
Similarly, when C# 10 shipped, we kept the feature StaticAbstractMembersInInterfaces attached to LangVersion=preview. The feature was marked as "shipped" with C# 11.
What's the compiler behavior if it encounters an assembly with MemorySafetyRules or an API with RequiresUnsafe and the user hasn't opted in at the language level?
If you turn on the new memory safety rules but use an older language version (than the version where the feature is "shipped") you get a compiler error.
This attribute in particular does feel more like RequiresPreviewFeatures
Ah, that's possible, I'm not sure between [RequiresPreviewFeatures] or [Experimental].
There was a problem hiding this comment.
Is the language going to require
LangVersion=previeweven in .NET 11?
I think we should, but we should confirm with the crew.
I think we already discussed this and yes, that's the plan. There is even a comment for it:
case MessageID.IDS_FeatureUnsafeEvolution: // https://github.com/dotnet/roslyn/issues/82546: keep this in preview until C# 16
Resolves #125134.
Relates to test plan dotnet/roslyn#81207