-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Unsafe evolution: add MemorySafetyRules attribute
#81441
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: features/UnsafeEvolution
Are you sure you want to change the base?
Unsafe evolution: add MemorySafetyRules attribute
#81441
Conversation
Inspired by RefSafetyRules attribute.
1 similar comment
333fred
left a comment
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.
Didn't fully review the tests, too many questions on the first couple for me to continue.
...s/CSharp/Portable/Symbols/Synthesized/SynthesizedEmbeddedMemorySafetyRulesAttributeSymbol.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit3/Semantics/UnsafeEvolutionTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit3/Semantics/UnsafeEvolutionTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit3/Semantics/UnsafeEvolutionTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit3/Semantics/UnsafeEvolutionTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit3/Semantics/UnsafeEvolutionTests.cs
Outdated
Show resolved
Hide resolved
| [assembly: MemorySafetyRules(2)] | ||
| [module: MemorySafetyRules(2)] |
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.
Is there a version of this test that doesn't manually apply MemorySafetyRules? IE, gets the rules from a reference (as it will from the BCL), and compiles successfully?
It would also be good to have some tests with multiple visible definitions of the attribute:
- No definition in the BCL, visible from 1 reference (I would expect this to succeed).
- No definition in the BCL, visible from 2 references (I would expect this to error).
- No definition in the BCL, visible from 2 references, plus defined in the current compilation (I would expect this to succeed).
- Definition in the BCL, visible from 2 references (I would expect this to succeed). #Resolved
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.
Is there a version of this test that doesn't manually apply MemorySafetyRules? IE, gets the rules from a reference (as it will from the BCL), and compiles successfully?
I'm not sure I understand. A compilation cannot get the rules from a reference, it only gets them if it sets compilationOptions.WithMemorySafetyRules(2). The attribute itself can come from a reference, yes, that's tested in RulesAttribute_FromMetadata.
As for the suggestions, if I understand correctly:
- tested in
RulesAttribute_FromMetadata, - adding
RulesAttribute_FromMetadata_Multiple(it doesn't fail, we synthesize our own attribute, RefSafetyRules behaves in the same way, adding a test for it too), - included in
RulesAttribute_FromMetadata_Multiple, - seems equivalent to (1) - compiler doesn't distinguish between BCL and other references in this regard.
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.
I'm specifically referring to the attribute. I agree 1-3 are covered, but 4 is not equivalent to 1. WellKnownTypes will prefer from the BCL over references.
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.
I see. Added RulesAttribute_FromMetadata_Multiple_AndCorLib. Thanks.
src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEModuleSymbol.cs
Outdated
Show resolved
Hide resolved
...s/CSharp/Portable/Symbols/Synthesized/SynthesizedEmbeddedMemorySafetyRulesAttributeSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/SourceModuleSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit3/Semantics/UnsafeEvolutionTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit3/Semantics/UnsafeEvolutionTests.cs
Outdated
Show resolved
Hide resolved
jcouv
left a comment
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.
Done with review pass (commit 6)
...s/CSharp/Portable/Symbols/Synthesized/SynthesizedEmbeddedMemorySafetyRulesAttributeSymbol.cs
Outdated
Show resolved
Hide resolved
jcouv
left a comment
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.
LGTM Thanks (commit 10) with minor feedback on LINQ usage
|
@333fred for another look, thanks |
Test plan: #81207
Inspired by RefSafetyRules attribute.
Relevant speclet section: https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md#metadata