Record structs: Bind type declaration #51494

jcouv · 2021-02-25T21:21:13Z

Test plan: #51199
Spec: https://github.com/dotnet/csharplang/blob/master/proposals/record-structs.md

This PR implements the following sections, but the parts related to the primary constructor parameters will be handled in the follow-up PR (which synthesizes positional members and primary constructor).

Record struct types are value types, like other struct types. They implicitly inherit from the class System.ValueType.
The modifiers and members of a record struct are subject to the same restrictions as those of structs
(accessibility on type, modifiers on members, base(...) instance constructor initializers,
definite assignment for this in constructor, destructors, ...).
Record structs will also follow the same rules as structs for parameterless instance constructors and field initializers,
but this document assumes that we will lift those restrictions for structs generally.

See https://github.com/dotnet/csharplang/blob/master/spec/structs.md

Record structs cannot use ref modifier.

At most one partial type declaration of a partial record struct may provide a parameter_list.
The parameter_list may not be empty.

Record struct parameters cannot use ref, out or this modifiers (but in and params are allowed).

and

Allow record class

The existing syntax for record types allows record class with the same meaning as record:
record_declaration
   : attributes? class_modifier* 'partial'? 'record' 'class'? identifier type_parameter_list?
     parameter_list? record_base? type_parameter_constraints_clause* record_body
    ;

jcouv · 2021-03-03T15:36:01Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs

 internal abstract bool IsRecord { get; }
+
+ internal abstract bool IsRecordStruct { get; }


📝 Alternatively, we could re-use and extend IsRecord. Some places that previously checked IsRecord would need to be updated to also check IsReferenceType. This probably should be discussed more in context of public API we want to expose (tracked in test plan). #Resolved

If it is convenient to have IsRecordStruct, it might make sense to have the following set of properties

// new property internal abstract bool IsRecord { get; } // current IsRecord property renamed (with usages audited to determine what property they should be changed to) internal bool IsRecordClass => IsRecord && TypeKind == TypeKind.Class; internal bool IsRecordStruct => IsRecord && TypeKind == TypeKind.Struct; ``` #Resolved

Right, that's what I was considering. I have a follow-up in test plan and also PROTOTYPE above to track.

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

jcouv · 2021-03-03T15:41:16Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

@@ -1593,185 +1784,6 @@ record R()
 );
 }

- [Fact(Skip = "record struct")]


📝 All the skipped tests were moved to new file (still skipped for now) #Resolved

jcouv · 2021-03-03T15:42:34Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

+ public void EmptyRecord_01_RecordClass()
+ {
+ var src = @"
+record class C();


📝 The class keyword is ignored by the compiler. So I just picked a couple of tests and duplicated them to give record class some coverage. #Resolved

AlekseyTs · 2021-03-05T01:06:24Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs

+ }
+
+ [Fact]
+ public void TypeDeclaration_AllowedModifiers()


TypeDeclaration_AllowedModifiers [](start = 20, length = 32)

Are we testing an unsafe modifier? #Closed

AlekseyTs · 2021-03-05T01:16:57Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs

+ }
+
+ [Fact]
+ public void TypeDeclaration_DifferentPartials()


TypeDeclaration_DifferentPartials [](start = 20, length = 33)

It feels like there are more permutations that we can test. Perhaps we should cover record struct declaration paired with all other kinds of declarations, the same for record class. #Closed

AlekseyTs

LGTM (commit 1), with test suggestions.

AlekseyTs · 2021-03-05T15:05:21Z

Compilers.sln

@@ -182,6 +182,7 @@ Global
 src\Analyzers\VisualBasic\Analyzers\VisualBasicAnalyzers.projitems*{2531a8c4-97dd-47bc-a79c-b7846051e137}*SharedItemsImports = 5
 src\Workspaces\SharedUtilitiesAndExtensions\Compiler\VisualBasic\VisualBasicCompilerExtensions.projitems*{2531a8c4-97dd-47bc-a79c-b7846051e137}*SharedItemsImports = 5
 src\Analyzers\Core\Analyzers\Analyzers.projitems*{275812ee-dedb-4232-9439-91c9757d2ae4}*SharedItemsImports = 5
+ src\Dependencies\Collections\Microsoft.CodeAnalysis.Collections.projitems*{275812ee-dedb-4232-9439-91c9757d2ae4}*SharedItemsImports = 5


src\Dependencies\Collections\Microsoft.CodeAnalysis.Collections.projitems*{275812ee-dedb-4232-9439-91c9757d2ae4}*SharedItemsImports = 5 [](start = 2, length = 135)

Is this an intentional change? #Closed

IDE has tended to add this implicitly when using certain refactorings in recent weeks. IIRC we saw this change come in another PR and decided to just merge it. Merging latest main into the feature branch will probably cause this diff to go away. #Resolved

Thanks. Reverted

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

AlekseyTs · 2021-03-05T15:06:53Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs

+
+ var comp = CreateCompilation(src);
+ comp.VerifyDiagnostics(
+ // (16,22): error CS0227: Unsafe code may only appear if compiling with /unsafe


// (16,22): error CS0227: Unsafe code may only appear if compiling with /unsafe [](start = 16, length = 79)

It looks like this error is covered in the test below. I was looking for a success case. #Closed

Ah. Changed to use "unsafe" compilation option

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

RikkiGibson · 2021-03-05T23:20:38Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs

+
+ var comp = CreateCompilation(src1, parseOptions: TestOptions.Regular9, options: TestOptions.ReleaseDll);
+ comp.VerifyDiagnostics(
+ // (2,13): error CS1514: { expected


I'm starting to wonder whether such type declarations should instead be treated as records with a missing 'record' keyword. Not asking you to do it in this PR but I'm curious what you think. #Resolved

It's not obvious. If someone is making the mistake of thinking that primary constructors are generally a thing, then they really don't want to add "record" to fix this. I'll add a note for follow-up discussion.

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

RikkiGibson · 2021-03-05T23:21:37Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs

+ var src3 = @"
+struct E
+{
+ record struct Point(int x, int y);


For completeness consider also nesting this in a namespace #Resolved

jcouv added Area-Compilers New Language Feature - Records Records labels Feb 25, 2021

jcouv self-assigned this Feb 25, 2021

jcouv added this to the C# 10 milestone Feb 25, 2021

jcouv force-pushed the rs-symbol1 branch from b10baf5 to bd79af5 Compare March 3, 2021 06:28

Record structs: Bind type declaration

4d58725

jcouv force-pushed the rs-symbol1 branch from bd79af5 to 4d58725 Compare March 3, 2021 07:24

jcouv commented Mar 3, 2021

View reviewed changes

jcouv marked this pull request as ready for review March 3, 2021 15:43

jcouv requested a review from a team as a code owner March 3, 2021 15:43

AlekseyTs reviewed Mar 5, 2021

View reviewed changes

AlekseyTs approved these changes Mar 5, 2021

View reviewed changes

RikkiGibson self-assigned this Mar 5, 2021

Address feedback

fa68fe9

AlekseyTs reviewed Mar 5, 2021

View reviewed changes

Address feedback

2962645

jcouv requested a review from RikkiGibson March 5, 2021 18:54

RikkiGibson reviewed Mar 5, 2021

View reviewed changes

RikkiGibson approved these changes Mar 5, 2021

View reviewed changes

Address feedback

49cb630

jcouv merged commit a137cbd into dotnet:features/record-structs Mar 6, 2021

jcouv deleted the rs-symbol1 branch March 6, 2021 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record structs: Bind type declaration #51494

Record structs: Bind type declaration #51494

jcouv commented Feb 25, 2021 •

edited

Loading

jcouv Mar 3, 2021 •

edited

Loading

RikkiGibson Mar 5, 2021 •

edited by jcouv

Loading

jcouv Mar 6, 2021

jcouv Mar 3, 2021 •

edited

Loading

jcouv Mar 3, 2021 •

edited

Loading

AlekseyTs Mar 5, 2021 •

edited

Loading

AlekseyTs Mar 5, 2021 •

edited

Loading

AlekseyTs left a comment

AlekseyTs Mar 5, 2021 •

edited

Loading

RikkiGibson Mar 5, 2021 •

edited by jcouv

Loading

jcouv Mar 5, 2021

AlekseyTs Mar 5, 2021 •

edited

Loading

jcouv Mar 5, 2021

RikkiGibson Mar 5, 2021 •

edited by jcouv

Loading

jcouv Mar 6, 2021

RikkiGibson Mar 5, 2021 •

edited by jcouv

Loading

		internal abstract bool IsRecord { get; }

		internal abstract bool IsRecordStruct { get; }

Record structs: Bind type declaration #51494

Record structs: Bind type declaration #51494

Conversation

jcouv commented Feb 25, 2021 • edited Loading

Allow record class

jcouv Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Mar 5, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Mar 6, 2021

Choose a reason for hiding this comment

jcouv Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

AlekseyTs Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Mar 5, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Mar 5, 2021

Choose a reason for hiding this comment

AlekseyTs Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv Mar 5, 2021

Choose a reason for hiding this comment

RikkiGibson Mar 5, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Mar 6, 2021

Choose a reason for hiding this comment

RikkiGibson Mar 5, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented Feb 25, 2021 •

edited

Loading

Allow `record class`

jcouv Mar 3, 2021 •

edited

Loading

RikkiGibson Mar 5, 2021 •

edited by jcouv

Loading

jcouv Mar 3, 2021 •

edited

Loading

jcouv Mar 3, 2021 •

edited

Loading

AlekseyTs Mar 5, 2021 •

edited

Loading

AlekseyTs Mar 5, 2021 •

edited

Loading

AlekseyTs Mar 5, 2021 •

edited

Loading

RikkiGibson Mar 5, 2021 •

edited by jcouv

Loading

AlekseyTs Mar 5, 2021 •

edited

Loading

RikkiGibson Mar 5, 2021 •

edited by jcouv

Loading

RikkiGibson Mar 5, 2021 •

edited by jcouv

Loading