Skip to content

Add support for nominal records#44774

Merged
agocke merged 1 commit intodotnet:features/recordsfrom
agocke:nominal-records
Jun 2, 2020
Merged

Add support for nominal records#44774
agocke merged 1 commit intodotnet:features/recordsfrom
agocke:nominal-records

Conversation

@agocke
Copy link
Member

@agocke agocke commented Jun 1, 2020

Removes the requirement for a record to have a parameter list

@agocke agocke requested a review from a team as a code owner June 1, 2020 23:27
hasInstanceConstructor = true;
hasParameterlessInstanceConstructor = hasParameterlessInstanceConstructor || method.ParameterCount == 0;
// Ignore the record copy constructor
if (!(method is SynthesizedRecordCopyCtor))
Copy link
Member

Choose a reason for hiding this comment

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

Can we check 'method.IsImplicitlyDeclared' here instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true for all SynthesizedInstanceMethodSymbols, including the record primary constructor, unfortunately

@RikkiGibson RikkiGibson requested a review from a team June 1, 2020 23:43
@agocke agocke requested review from cston, gafter and jcouv June 2, 2020 18:09
{
var comp = CreateCompilation(@"
#nullable enable
data class C
Copy link
Member

@jcouv jcouv Jun 2, 2020

Choose a reason for hiding this comment

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

Please add a test for data class C(). From your PR description, I'm not sure what is expected (allowed or not?)

Update: I see that is covered in EmptyRecord in the other test file. #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.

Right the absence of a record parameter list is not the same as an empty record parameter list. Right now it's an error.

}

var ctor = addCtor(paramList);
// Positional record
Copy link
Member

Choose a reason for hiding this comment

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

Positional record [](start = 15, length = 17)

nit: positional members?

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 1)

@jcouv jcouv self-assigned this Jun 2, 2020
@agocke agocke merged commit eebb3e4 into dotnet:features/records Jun 2, 2020
@agocke agocke deleted the nominal-records branch June 2, 2020 21:21
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.

3 participants