Add support for "data" properties by agocke · Pull Request #44883 · dotnet/roslyn

agocke · 2020-06-05T09:34:17Z

A "data" property is a short-form syntax for a particular type of
property declaration, similar to events. A data property has no legal
modifiers and always means "public init-only auto-property."

jcouv · 2020-06-07T23:11:07Z

src/Compilers/CSharp/Portable/Syntax/Syntax.xml

I understand that we have a principle that our grammar should result in non-overlapping productions. But I wonder if we could achieve that without hard-coding the position of the data keyword.

I guess I'm proposing that we treat data as one of the modifiers (and so could be re-ordered). That seems doable from a parser perspective, but I'm not sure if we could make that work from a language perspective.

Not blocking this PR or 16.7p3, but may be worth further discussion. #Closed

I'm not opposed to it, but I found it somewhat easier to think about as closer to the "event" syntax than a modifier, because if we ever did allow another modifier in the declaration I'm not sure I would like the potential reordering. #Closed

Added a note to test plan

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

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedAutoPropAccessorSymbol.cs

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

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

src/Compilers/CSharp/Test/Symbol/Symbols/Source/RecordTests.cs

A "data" property is a short-form syntax for a particular type of tproperty declaration, similar to events. A data property has no legal modifiers and always means "public init-only auto-property."

agocke · 2020-06-12T20:55:04Z

@jcouv I think this is ready for re-review. I've also re-targeted the branch. #Resolved

AlekseyTs · 2020-06-30T13:26:33Z

src/Compilers/CSharp/Portable/Lowering/Instrumentation/DynamicAnalysisInjector.cs

+                case SyntaxKind.DataPropertyDeclaration:
+                    {
+                        var dataProp = (DataPropertyDeclarationSyntax)syntax;
+                        return SkipAttributes(syntax, dataProp.AttributeLists, dataProp.Modifiers, dataProp.DataKeyword, dataProp.Type);


return SkipAttributes(syntax, dataProp.AttributeLists, dataProp.Modifiers, dataProp.DataKeyword, dataProp.Type); [](start = 24, length = 112)

Is this code path tested? #Closed

AlekseyTs · 2020-06-30T13:27:41Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs

-                                else if ((fieldDecl = memberDecl as BaseFieldDeclarationSyntax) != null)
-                                {
-                                    memberName = null;
+                                case DataPropertyDeclarationSyntax propDecl when initializerInSpan(propDecl.Initializer, syntax):


DataPropertyDeclarationSyntax [](start = 37, length = 29)

Is this code path tested?

AlekseyTs · 2020-06-30T13:28:59Z

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

                case SyntaxKind.OperatorDeclaration:
                case SyntaxKind.ConversionOperatorDeclaration:
                case SyntaxKind.PropertyDeclaration:
+                case SyntaxKind.DataPropertyDeclaration:


DataPropertyDeclaration [](start = 32, length = 23)

Is this code path tested?

AlekseyTs · 2020-06-30T13:43:25Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/RecordTests.cs

+}";
+
+            var comp = CreateCompilation(src);
+            comp.VerifyDiagnostics(


comp.VerifyDiagnostics( [](start = 12, length = 23)

We should have tests for abstract data properties, i.e. verify that they are indeed abstract. Should also test success scenarios for overriding, including executing and observing expected runtime behavior.

Also, tests with sealed.

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

Added test for sealed, which wasn't one of the approved modifiers in LDM. Added a symbol test for abstract to DataProperties11. I don't think special execution for data properties tests is particularly interesting (since they're just syntax sugar), unless there's something different in the lowering for data properties.

don't think special execution for data properties tests is particularly interesting (since they're just syntax sugar), unless there's something different in the lowering for data properties.

Well, there is a dedicated code that processes modifiers, etc.. In general we don't assume that things would work just because we expect them to. In fact, we almost always expect them to work, but that isn't always the reality. I am not saying we have to test full test matrix as for regular properties, but to have at least one success case verifying runtime behavior for each distinct flavor, is a must, in my opinion. Especially that overriding is such an interesting case, takes unique code paths, etc.

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

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

AlekseyTs · 2020-06-30T14:03:44Z

Done with review pass (iteration 26) #Closed

AlekseyTs · 2020-07-02T16:56:35Z

@agocke It looks like there are legitimate test failures.

AlekseyTs · 2020-07-02T17:16:53Z

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

                        return ((PropertyDeclarationSyntax)parent).Type == node;

+                    case DataPropertyDeclaration:
+                        return ((DataPropertyDeclarationSyntax)parent).Type == node;


return ((DataPropertyDeclarationSyntax)parent).Type == node; [](start = 24, length = 60)

Is this code path tested?

It is, in DataProperties12

AlekseyTs · 2020-07-02T17:28:14Z

                        var property = new DataPropertySymbol(this, bodyBinder, propertySyntax, diagnostics);

Dropping the binder feels suspicious. How would we be able to deal with overriding without it?

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs:3497 in 8844778. [](commit_id = 8844778, deletion_comment = True)

AlekseyTs · 2020-07-02T17:54:21Z

                        var property = new DataPropertySymbol(this, bodyBinder, propertySyntax, diagnostics);

I understand that the code is written in a way that we would create a new binder if one wasn't passed in, but, I think, it would be more efficient to reuse the binder across multiple declarations.

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

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs:3497 in 8844778. [](commit_id = 8844778, deletion_comment = True)

AlekseyTs · 2020-07-02T17:59:52Z

Done with review pass (iteration 28)

agocke · 2020-07-02T23:39:08Z

I understand that the code is written in a way that we would create a new binder if one wasn't passed in, but, I think, it would be more efficient to reuse the binder across multiple declarations.

Yup, makes sense.

jcouv · 2020-07-08T17:58:35Z

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

                default:
                    return false;
+
+                    bool propertyModifierPreventingField(SyntaxTokenList modifiers)


bool [](start = 20, length = 4)

static?

jcouv · 2020-07-08T17:59:41Z

        // We put synthesized record members first so that errors about conflicts show up on user-defined members rather than all

nit: extra empty line above

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs:3022 in 0c38cc8. [](commit_id = 0c38cc8, deletion_comment = False)

jcouv · 2020-07-08T18:09:07Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs

-                                else if ((fieldDecl = memberDecl as BaseFieldDeclarationSyntax) != null)
-                                {
-                                    memberName = null;
+                                case DataPropertyDeclarationSyntax propDecl when initializerInSpan(propDecl.Initializer, syntax):


Do we have a test for this path?

jcouv · 2020-07-08T18:09:21Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs

-                                        break;
-                                    }
-                                }
+                                    static bool initializerInSpan(EqualsValueClauseSyntax? initializer, SyntaxNode syntax)


nit: we try to group local functions at the end of methods where possible to avoid interrupting flow of logic.

jcouv · 2020-07-08T18:13:50Z

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

+                    {
+                        // auto-property
+                        var propertyDecl = (DataPropertyDeclarationSyntax)m;
+                        return !propertyModifierPreventingField(propertyDecl.Modifiers);


It looks like this is not reachable at the moment, is that correct?

jcouv · 2020-07-08T18:14:29Z

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

-                out name,
-                out explicitInterfaceImplementations);
+                out string name,
+                out _);


nit: consider naming discarded argument

jcouv · 2020-07-08T18:17:37Z

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

+                default:
+                    return false;
+            }
+        }


nit: extra empty line below

jcouv · 2020-07-08T18:34:53Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/RecordTests.cs

+            Assert.Equal(CodeAnalysis.NullableAnnotation.NotAnnotated, p4.Type.NullableAnnotation);
+            typeInfo = model.GetTypeInfo(props[3].Initializer!.Value);
+            Assert.Equal(CodeAnalysis.NullableFlowState.MaybeNull, typeInfo.Nullability.FlowState);
+        }


Do we have follow-up work item or issue for GetOperation on initializer?

jcouv · 2020-07-08T18:38:13Z

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

+                    allowed,
+                    location,
+                    diagnostics,
+                    out _);


nit: consider naming discarded argument

sharwell · 2020-07-08T18:45:49Z

src/Compilers/CSharp/Portable/Compilation/InitializerSemanticModel.cs

        {
            Debug.Assert(containingSemanticModel != null);
-            Debug.Assert(syntax.IsKind(SyntaxKind.PropertyDeclaration));
+            Debug.Assert(syntax.IsKind(SyntaxKind.PropertyDeclaration) || syntax.IsKind(SyntaxKind.DataPropertyDeclaration));


💭 Can we call this a PropertyFieldDeclaration to align with EventFieldDeclaration?

jcouv

Done with review pass (iteration 29)

jcouv · 2020-08-11T19:01:42Z

Marking as draft for now.

agocke force-pushed the data-properties branch 2 times, most recently from 4c64b0c to d599f94 Compare June 5, 2020 23:41

agocke marked this pull request as ready for review June 6, 2020 18:58

agocke requested a review from a team as a code owner June 6, 2020 18:58

agocke force-pushed the data-properties branch from d599f94 to 1ee3f19 Compare June 6, 2020 20:04

jcouv added Area-Compilers Feature - Records Records labels Jun 7, 2020

jcouv reviewed Jun 7, 2020

View reviewed changes