-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Disallow pointer and restricted types for record parameters #48925
Conversation
77fad72
to
d6447ea
Compare
d6447ea
to
be9a062
Compare
foreach (var parameter in ctor.Parameters) | ||
{ | ||
var parameterType = parameter.Type; | ||
if (parameterType.IsPointerOrFunctionPointer() || parameterType.IsRestrictedType()) |
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.
IsPointerOrFunctionPointer [](start = 42, length = 26)
Should we use IsUnsafe
helper instead? It looks like this is what GetAnonymousTypeFieldType
is using and it looks like IsUnsafe
flags more types. #Closed
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 question is should we align with type argument rules or anonymous type rules.
I included tests with int*[]
to highlight the difference.
Raised question in thread with LDM
In reply to: 512784802 [](ancestors = 512784802)
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.
Thread from LDM landed on disallowing unsafe types. Pushed the change to the PR. Thanks
In reply to: 513087857 [](ancestors = 513087857,512784802)
@@ -3046,6 +3046,15 @@ private void AddSynthesizedRecordMembersIfNecessary(MembersAndInitializersBuilde | |||
{ | |||
var existingOrAddedMembers = addProperties(ctor.Parameters); | |||
addDeconstruct(ctor, existingOrAddedMembers); | |||
|
|||
foreach (var parameter in ctor.Parameters) |
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.
foreach (var parameter in ctor.Parameters) [](start = 20, length = 42)
I am not sure this is the right place for the check and the right set of members to check. If I remember correctly, any field in the type is a subject for equality. #Closed
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.
static class C2 { } | ||
ref struct RefLike{} | ||
|
||
unsafe record C( // 1 |
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.
Please add similar test(s) for fields and auto-properties explicitly declared in a record. #Closed
Done with review pass (iteration 1) #Closed |
@dotnet/roslyn-compiler for review. Thanks |
var parameterType = f.Type; | ||
if (parameterType.IsUnsafe()) | ||
{ | ||
diagnostics.Add(ErrorCode.ERR_BadFieldTypeInRecord, f.Locations[0], parameterType); |
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.
[0] [](start = 91, length = 3)
Since we do not control what kind of symbols we are going to get here, consider using FirstOrNone helper here.
|
||
[ConditionalFact(typeof(DesktopOnly), Reason = ConditionalSkipReason.RestrictedTypesNeedDesktop)] | ||
[WorkItem(48115, "https://github.com/dotnet/roslyn/issues/48115")] | ||
public void RestrictedTypesAndPointerTypes_NominalMembers() |
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.
RestrictedTypesAndPointerTypes_NominalMembers [](start = 20, length = 45)
Please add similar test(s) for auto-properties explicitly declared in a record.
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 (iteration 5), modulo a test suggestion and suggestion to use FirstOrNone helper for location.
Fixes #48115
We're disallowing unsafe types and restricted types for record fields.
Spec update: dotnet/csharplang#4077