Skip to content
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

Merged
merged 7 commits into from
Oct 31, 2020

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Oct 26, 2020

Fixes #48115
We're disallowing unsafe types and restricted types for record fields.

Spec update: dotnet/csharplang#4077

@jcouv jcouv marked this pull request as ready for review October 26, 2020 23:41
@jcouv jcouv requested a review from a team as a code owner October 26, 2020 23:41
@jcouv jcouv added this to the 16.9 milestone Oct 27, 2020
foreach (var parameter in ctor.Parameters)
{
var parameterType = parameter.Type;
if (parameterType.IsPointerOrFunctionPointer() || parameterType.IsRestrictedType())
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 27, 2020

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

Copy link
Member Author

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)

Copy link
Member Author

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)
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 27, 2020

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Thanks


In reply to: 512787196 [](ancestors = 512787196)

static class C2 { }
ref struct RefLike{}

unsafe record C( // 1
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 27, 2020

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

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Oct 27, 2020

Done with review pass (iteration 1) #Closed

@jcouv jcouv requested review from AlekseyTs and a team October 29, 2020 17:50
@jcouv
Copy link
Member Author

jcouv commented Oct 30, 2020

@dotnet/roslyn-compiler for review. Thanks

var parameterType = f.Type;
if (parameterType.IsUnsafe())
{
diagnostics.Add(ErrorCode.ERR_BadFieldTypeInRecord, f.Locations[0], parameterType);
Copy link
Contributor

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()
Copy link
Contributor

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.

Copy link
Contributor

@AlekseyTs AlekseyTs left a 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.

@jcouv jcouv merged commit 7e828c8 into dotnet:master Oct 31, 2020
@jcouv jcouv deleted the restricted-types branch October 31, 2020 06:56
@ghost ghost modified the milestones: 16.9, Next Oct 31, 2020
@allisonchou allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeLoadException when pointer is used as member of Records
3 participants