Records: allow positional members to be implemented with fields #52480

jcouv · 2021-04-08T03:44:28Z

Test plan: #51199

Opened issue #52630 for disallowing hidden members to be picked for positional usage.
Spec change: dotnet/csharplang#4673

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

src/Compilers/CSharp/Portable/Syntax/TypeDeclarationSyntax.cs

RikkiGibson · 2021-04-13T02:19:14Z

Was this test judged to no longer be useful? I expected the behavior to be unchanged here since we consider consts to be static.

    const int P = 4;

src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs (Line 1911)

edit: This seems to be how CodeStream allows commenting on unchanged lines. Seeing it for the first time. Interesting. #Resolved

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

jcouv · 2021-04-13T02:45:26Z

I'd removed it because I thought it was duplicate and wrong name. But FieldAsPositionalMember_Const is on record classes. I'll restore

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

RikkiGibson · 2021-04-13T17:16:19Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

+
+ if (!memberSignatures.TryGetValue(targetField, out existingMember))
+ {
+ existingMember = OverriddenOrHiddenMembersHelpers.FindFirstHiddenMemberIfAny(targetField, memberIsFromSomeCompilation: true);


Does this method return a different value when called with this targetField than when it is called with the targetProperty above?
#Resolved

RikkiGibson · 2021-04-13T17:19:06Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

+ }
+ else
+ {
+ isInherited = true;


I didn't understand why we avoid setting this for fields. In this case, the field is also inherited, right? We don't use it, but at the same time it seems simpler if we set it unconditionally. #Resolved

We shouldn't set isInherited to true when a field was found from base types here, because we may find a field on the current type (which would be preferred and is not inherited). #Resolved

AlekseyTs · 2021-04-13T22:16:30Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

@@ -3466,7 +3466,6 @@ private void AddSynthesizedRecordMembersIfNecessary(MembersAndInitializersBuilde
 {
 switch (member)
 {
- case FieldSymbol:


case FieldSymbol: [](start = 19, length = 18)

It looks like for fields the only thing we are going to compare is the Name. I think we should consider simply using a separate dictionary for that and avoiding an introduction of SignatureOnlyFieldSymbol with the only purpose to lookup a field by name. #Closed

AlekseyTs · 2021-04-13T22:17:33Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

 if (existingMember is null)
 {
 addProperty(new SynthesizedRecordPropertySymbol(this, syntax, param, isOverride: false, diagnostics));
 }
+ else if (existingMember is FieldSymbol { IsStatic: false } field)


existingMember is FieldSymbol { IsStatic: false } field [](start = 29, length = 55)

Are we making sure the type matches? Where? #Closed

That was missing. Thanks

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

AlekseyTs · 2021-04-13T22:27:09Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

@@ -3743,12 +3744,42 @@ ImmutableArray<PropertySymbol> addProperties(ImmutableArray<ParameterSymbol> rec
 if (!memberSignatures.TryGetValue(targetProperty, out var existingMember))
 {
 existingMember = OverriddenOrHiddenMembersHelpers.FindFirstHiddenMemberIfAny(targetProperty, memberIsFromSomeCompilation: true);
- isInherited = true;
+ if (existingMember is FieldSymbol)


if (existingMember is FieldSymbol) [](start = 24, length = 34)

The logic feels too convoluted and I think it can be significantly simplified as follows:

First, we look for a matching property in the current record;

Then we look for a matching field in the current record;

Then we look for a first hidden member in base #Closed

We shouldn't look at the base last though. A property from the base should win over a field from the current type, to avoid breaking existing scenario (below).
That said, I think we can achieve that by calling FindFirstHiddenMemberIfAny only once and keeping the result.

public record Base { public int X { get; set; } } public record C(int X) : Base { public new int X; }

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

We shouldn't look at the base last though. A property from the base should win over a field from the current type, to avoid breaking existing scenario (below).

I don't think this behavior is sound, even though it avoids a breaking change.

In reply to: 612935164 [](ancestors = 612935164,612815137)

AlekseyTs · 2021-04-13T22:30:10Z

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

+ /// A representation of a field symbol that is intended only to be used for comparison purposes
+ /// (esp in MemberSignatureComparer).
+ /// </summary>
+ internal sealed class SignatureOnlyFieldSymbol : FieldSymbol


SignatureOnlyFieldSymbol [](start = 26, length = 24)

It feels like we can achieve the goal without this type. #Closed

AlekseyTs · 2021-04-13T22:46:20Z

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

- comp.VerifyDiagnostics(
- // (6,9): error CS0102: The type 'C' already contains a definition for 'X'
+ comp.VerifyEmitDiagnostics(
+ // (4,10): error CS8652: The feature 'positional fields in records' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version.


// (4,10): error CS8652: The feature 'positional fields in records' is currently in Preview and unsupported. To use Preview features, use the 'preview' language version. [](start = 16, length = 171)

Should we use explicit language version for this compilation so that the test doesn't break once default version is changed? It looks like this comment is applicable to other tests too. #Closed

I checked a few such tests and they all use the CreateCompilation defined in this file with explicit language version 9.

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

AlekseyTs · 2021-04-13T22:50:51Z

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

+ .maxstack 2
+ IL_0000: ldarg.1
+ IL_0001: ldarg.0
+ IL_0002: call ""int Base.I.get""


IL_0002: call ""int Base.I.get"" [](start = 2, length = 39)

This looks unexpected. I think C.I should be used here. #Closed

And that is probably going to be a breaking change.

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

It looks like you intentionally tried to preserve previous behavior, but I am not sure if the proposed behavior is the right one to have and whether we explicitly approved it at LDM.

In reply to: 612824890 [](ancestors = 612824890,612824515)

Added an open LDM issue to test plan.
#51199

I'll start email thread and also put record structs on the agenda for next Monday, since multiple small questions remain.

In reply to: 612828289 [](ancestors = 612828289,612824890,612824515)

Added an open LDM issue to test plan.

Untill this is settled, I do not think we should merge this sub-feature.

In reply to: 612941755 [](ancestors = 612941755,612828289,612824890,612824515)

AlekseyTs · 2021-04-13T22:53:53Z

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

+
+public record Base
+{
+ public int I = 42;


int [](start = 11, length = 3)

Are we testing scenario with a type mismatch? #Closed

AlekseyTs · 2021-04-13T23:02:11Z

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

+
+public record Base
+{
+ public int I { get; set; } = 42;


public int I { get; set; } = 42; [](start = 4, length = 32)

Are we testing scenario when base has a field and derived has a property? #Closed

AlekseyTs · 2021-04-13T23:04:23Z

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

+}
+record C3(int P)
+{
+ internal int P = P;


int [](start = 13, length = 3)

Are we testing type mismatch scenario? #Closed

AlekseyTs · 2021-04-13T23:07:37Z

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

+
+record struct A(int X)
+{
+ public int X = X;


int [](start = 11, length = 3)

Are we testing type mismatch case? #Closed

AlekseyTs · 2021-04-13T23:08:13Z

Done with review pass (commit 4) #Closed

AlekseyTs

LGTM (commit 6)

RikkiGibson · 2021-04-15T00:46:45Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

- existingMember = inheritedMember;
- }
+ // There should be an error if we picked a member that is hidden
+ // This will be fixed in C# 9 as part of 16.10. Tracked by https://github.com/dotnet/roslyn/issues/52630


Do we plan to do this after feature merge?

Yes. I'll make the breaking change in 16.10 and both changes will meet in features/compiler.

RikkiGibson · 2021-04-15T00:51:37Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

+ if (!fieldsByName.ContainsKey(fieldName))
+ {
+ fieldsByName.Add(fieldName, member);
+ }


nit: it feels like this could be replaced with fieldsByName[fieldName] = member; (in error scenarios with multiple identically named fields, which one we pick feels unimportant).

If it's okay, I'll leave as-is. Thanks

jcouv added Area-Compilers New Language Feature - Records Records labels Apr 8, 2021

jcouv added this to the C# 10 milestone Apr 8, 2021

jcouv self-assigned this Apr 8, 2021

jcouv force-pushed the record-fields branch from 066d1fc to 5a7b50f Compare April 8, 2021 19:09

Records: allow positional members to be implemented with fields

fb4765c

jcouv force-pushed the record-fields branch from 5a7b50f to fb4765c Compare April 8, 2021 21:41

jcouv marked this pull request as ready for review April 9, 2021 00:36

jcouv requested a review from a team as a code owner April 9, 2021 00:36

jcouv mentioned this pull request Apr 10, 2021

Test plan for "record structs" #51199

Closed

92 tasks

jcouv added 2 commits April 12, 2021 15:19

typo

54271fa

Address remaining PROTOTYPE markers

5a9bec3

RikkiGibson self-assigned this Apr 12, 2021

RikkiGibson reviewed Apr 13, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs Outdated Show resolved Hide resolved

RikkiGibson reviewed Apr 13, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Syntax/TypeDeclarationSyntax.cs Outdated Show resolved Hide resolved

RikkiGibson reviewed Apr 13, 2021

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs Outdated Show resolved Hide resolved

jcouv requested review from a team and AlekseyTs April 13, 2021 02:42

Address feedback

1f026b2

RikkiGibson reviewed Apr 13, 2021

View reviewed changes

AlekseyTs reviewed Apr 13, 2021

View reviewed changes

jcouv added 2 commits April 13, 2021 22:21

Address feedback

cddf8ab

Adjust for LDM decision

cda3204

jcouv requested review from RikkiGibson and AlekseyTs April 14, 2021 21:46

AlekseyTs approved these changes Apr 14, 2021

View reviewed changes

RikkiGibson reviewed Apr 15, 2021

View reviewed changes

RikkiGibson approved these changes Apr 15, 2021

View reviewed changes

RikkiGibson reviewed Apr 15, 2021

View reviewed changes

RikkiGibson approved these changes Apr 15, 2021

View reviewed changes

jcouv merged commit d6a3c91 into dotnet:features/record-structs Apr 15, 2021

jcouv deleted the record-fields branch April 15, 2021 01:03

jcouv mentioned this pull request Apr 19, 2021

Update docs for positional fields and hidden positional members dotnet/csharplang#4673

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Records: allow positional members to be implemented with fields #52480

Records: allow positional members to be implemented with fields #52480

jcouv commented Apr 8, 2021 •

edited

Loading

RikkiGibson commented Apr 13, 2021 •

edited by jcouv

Loading

jcouv commented Apr 13, 2021

RikkiGibson Apr 13, 2021 •

edited by jcouv

Loading

RikkiGibson Apr 13, 2021 •

edited by jcouv

Loading

jcouv Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

jcouv Apr 14, 2021

AlekseyTs Apr 13, 2021 •

edited

Loading

jcouv Apr 14, 2021

AlekseyTs Apr 14, 2021

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

jcouv Apr 14, 2021

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

jcouv Apr 14, 2021

AlekseyTs Apr 14, 2021

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs commented Apr 13, 2021 •

edited

Loading

AlekseyTs left a comment

RikkiGibson Apr 15, 2021

jcouv Apr 15, 2021

RikkiGibson Apr 15, 2021

jcouv Apr 15, 2021

Records: allow positional members to be implemented with fields #52480

Records: allow positional members to be implemented with fields #52480

Conversation

jcouv commented Apr 8, 2021 • edited Loading

RikkiGibson commented Apr 13, 2021 • edited by jcouv Loading

jcouv commented Apr 13, 2021

RikkiGibson Apr 13, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

RikkiGibson Apr 13, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Apr 13, 2021 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Apr 8, 2021 •

edited

Loading

RikkiGibson commented Apr 13, 2021 •

edited by jcouv

Loading

RikkiGibson Apr 13, 2021 •

edited by jcouv

Loading

RikkiGibson Apr 13, 2021 •

edited by jcouv

Loading

jcouv Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs Apr 13, 2021 •

edited

Loading

AlekseyTs commented Apr 13, 2021 •

edited

Loading