-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add support for "data" properties #44883
Conversation
4c64b0c
to
d599f94
Compare
<Kind Name="DataPropertyDeclaration" /> | ||
<Field Name="AttributeLists" Type="SyntaxList<AttributeListSyntax>" Override="true"/> | ||
<Field Name="Modifiers" Type="SyntaxList<SyntaxToken>" Override="true"/> | ||
<Field Name="DataKeyword" Type="SyntaxToken"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedAutoPropAccessorSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedAutoPropAccessorSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/DataPropertySymbol.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Source/DataPropertySymbol.cs
Outdated
Show resolved
Hide resolved
8db6be9
to
fa8e17e
Compare
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."
@jcouv I think this is ready for re-review. I've also re-targeted the branch. #Resolved |
@@ -245,6 +245,7 @@ private static bool IsInContextWhichNeedsDynamicAttribute(CSharpSyntaxNode node) | |||
case SyntaxKind.OperatorDeclaration: | |||
case SyntaxKind.ConversionOperatorDeclaration: | |||
case SyntaxKind.PropertyDeclaration: | |||
case SyntaxKind.DataPropertyDeclaration: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DataPropertyDeclaration [](start = 32, length = 23)
Is this code path tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
}"; | ||
|
||
var comp = CreateCompilation(src); | ||
comp.VerifyDiagnostics( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Done with review pass (iteration 26) #Closed |
@agocke It looks like there are legitimate test failures. |
@@ -174,6 +174,9 @@ public static bool IsInTypeOnlyContext(ExpressionSyntax node) | |||
case PropertyDeclaration: | |||
return ((PropertyDeclarationSyntax)parent).Type == node; | |||
|
|||
case DataPropertyDeclaration: | |||
return ((DataPropertyDeclarationSyntax)parent).Type == node; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return ((DataPropertyDeclarationSyntax)parent).Type == node; [](start = 24, length = 60)
Is this code path tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, in DataProperties12
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) |
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) |
Done with review pass (iteration 28) |
Yup, makes sense. |
@@ -2201,6 +2207,11 @@ private static bool HasInstanceData(MemberDeclarationSyntax m) | |||
!ContainsModifier(eventFieldDecl.Modifiers, SyntaxKind.ExternKeyword); | |||
default: | |||
return false; | |||
|
|||
bool propertyModifierPreventingField(SyntaxTokenList modifiers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool [](start = 20, length = 4)
static?
nit: extra empty line above Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs:3022 in 0c38cc8. [](commit_id = 0c38cc8, deletion_comment = False) |
else if ((fieldDecl = memberDecl as BaseFieldDeclarationSyntax) != null) | ||
{ | ||
memberName = null; | ||
case DataPropertyDeclarationSyntax propDecl when initializerInSpan(propDecl.Initializer, syntax): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test for this path?
break; | ||
} | ||
} | ||
static bool initializerInSpan(EqualsValueClauseSyntax? initializer, SyntaxNode syntax) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we try to group local functions at the end of methods where possible to avoid interrupting flow of logic.
{ | ||
// auto-property | ||
var propertyDecl = (DataPropertyDeclarationSyntax)m; | ||
return !propertyModifierPreventingField(propertyDecl.Modifiers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is not reachable at the moment, is that correct?
out name, | ||
out explicitInterfaceImplementations); | ||
out string name, | ||
out _); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider naming discarded argument
default: | ||
return false; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra empty line below
Assert.Equal(CodeAnalysis.NullableAnnotation.NotAnnotated, p4.Type.NullableAnnotation); | ||
typeInfo = model.GetTypeInfo(props[3].Initializer!.Value); | ||
Assert.Equal(CodeAnalysis.NullableFlowState.MaybeNull, typeInfo.Nullability.FlowState); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have follow-up work item or issue for GetOperation on initializer?
allowed, | ||
location, | ||
diagnostics, | ||
out _); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider naming discarded argument
@@ -49,7 +49,7 @@ internal static InitializerSemanticModel Create(SyntaxTreeSemanticModel containi | |||
internal static InitializerSemanticModel Create(SyntaxTreeSemanticModel containingSemanticModel, CSharpSyntaxNode syntax, PropertySymbol propertySymbol, Binder rootBinder) | |||
{ | |||
Debug.Assert(containingSemanticModel != null); | |||
Debug.Assert(syntax.IsKind(SyntaxKind.PropertyDeclaration)); | |||
Debug.Assert(syntax.IsKind(SyntaxKind.PropertyDeclaration) || syntax.IsKind(SyntaxKind.DataPropertyDeclaration)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 Can we call this a PropertyFieldDeclaration
to align with EventFieldDeclaration
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with review pass (iteration 29)
Marking as draft for now. |
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."