-
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
Record-structs: Address some PROTOTYPE markers #52256
Conversation
case SyntaxKind.RecordStructDeclaration: | ||
{ | ||
if (associatedSymbol is IMethodSymbol ctor) | ||
{ |
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 didn't manage to cover this if
and couldn't find coverage for above if
either. #Resolved
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 didn't manage to cover this if and couldn't find coverage for above if either.
Really? I acn easily hit code paths in the previous case by running existing test in src\Compilers\CSharp\Test\Semantic\Semantics\RecordTests.cs
In reply to: 604290986 [](ancestors = 604290986)
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.
Thanks. You're right. Both if
blocks are visited by the AnalyzerActions_XX
tests in RecordTests
and RecordStructTests
.
In reply to: 605097075 [](ancestors = 605097075,604290986)
@@ -2253,7 +2253,6 @@ ITypeSymbol ITypeSymbolInternal.GetITypeSymbol() | |||
return GetITypeSymbol(DefaultNullableAnnotation); | |||
} | |||
|
|||
// PROTOTYPE(record-structs): rename to IsRecordClass? |
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.
📝 not planning to rename the internal API #Resolved
@@ -795,7 +795,6 @@ public static SyntaxKind GetTypeDeclarationKind(SyntaxKind kind) | |||
case SyntaxKind.InterfaceKeyword: | |||
return SyntaxKind.InterfaceDeclaration; | |||
case SyntaxKind.RecordKeyword: | |||
// PROTOTYPE(record-structs): anything we can do? |
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 don't see anything we could do given the shape of the API. We'll see if that causes problems during IDE work #Resolved
@@ -63,7 +63,6 @@ private static SyntaxKind GetTypeDeclarationKeywordKind(SyntaxKind kind) | |||
return SyntaxKind.InterfaceKeyword; | |||
case SyntaxKind.RecordDeclaration: | |||
return SyntaxKind.RecordKeyword; | |||
// PROTOTYPE(record-structs): update for record structs |
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.
📝 We'll see if something is needed here during IDE work #Resolved
@@ -2983,7 +2983,6 @@ void verifyParsedAsRecord() | |||
[Fact, CompilerTrait(CompilerFeature.RecordStructs)] | |||
public void RecordInterfaceParsing() | |||
{ | |||
// PROTOTYPE(record-structs): should we treat this as a binding error instead, for better error recovery? |
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 a bullet to test plan #Resolved
} | ||
|
||
[Fact] | ||
public void RecordStructLanguageVersion() | ||
{ | ||
// PROTOTYPE(record-structs): can we improve the error recovery, maybe treating this as a record struct with missing `record`? |
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 a bullet to test plan #Resolved
} | ||
|
||
return createMethodBodySemanticModel(node, symbol); | ||
return symbol is null ? null : createMethodBodySemanticModel(node, symbol); |
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 symbol is null ? null : createMethodBodySemanticModel(node, symbol); [](start = 24, length = 75)
Please revert style change. #Closed
@@ -2427,6 +2420,7 @@ internal override Symbol RemapSymbolIfNecessaryCore(Symbol symbol) | |||
case SymbolKind.NamedType: | |||
Debug.Assert((object)declaredSymbol.GetSymbol() == (object)ctor.ContainingSymbol); | |||
// Accept nodes that do not match a 'parameter list'/'base arguments list'. | |||
//return (node) => true; |
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 (node) => true; [](start = 28, length = 25)
We do not commit commented out code without a PROTOTYPE comment or an issue tracking the follow-up #Closed
@@ -565,7 +565,8 @@ internal override bool IsRecord | |||
} | |||
} | |||
|
|||
// PROTOTYPE(record-structs): update for record structs (is there a way to recognize a record struct from PE?) | |||
// Is there a way to recognize a record struct from PE? | |||
// Tracked by https://github.com/dotnet/roslyn/issues/52233 |
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.
// Tracked by #52233 [](start = 8, length = 59)
Have we resolved this design question? #Closed
@@ -515,6 +516,32 @@ record R(R X) | |||
Assert.Equal("R..ctor(R X)", initializer.ContainingSymbol.ToTestDisplayString()); | |||
} | |||
|
|||
[Fact] | |||
public void GetDeclaredSymbolOnFieldInitializer() |
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.
Field [](start = 39, length = 5)
Field or Property? #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.
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.
Changed test name to "Property".
This test is calling getting a declared symbol (out var) that's part of the intializer.
In reply to: 605127667 [](ancestors = 605127667,605126785)
@@ -5190,5 +5156,959 @@ public void ToString_UserDefinedPrintMembers_Static() | |||
Diagnostic(ErrorCode.ERR_StaticAPIInRecord, "PrintMembers").WithArguments("C1.PrintMembers(System.Text.StringBuilder)").WithLocation(4, 25) | |||
); | |||
} | |||
|
|||
[Fact] | |||
public void AmbigCtor_WithFieldInitializer() |
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.
AmbigCtor_WithFieldInitializer [](start = 20, length = 30)
It is not clear why the test is called this. It doesn't look like the scenario has any ambiguity. #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.
This is a ported test from record classes where it is an ambiguity. I kept the matching test names when moving them.
In reply to: 605132581 [](ancestors = 605132581)
} | ||
|
||
[Fact] | ||
public void GetDeclaredSymbolOnFieldInitializer() |
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.
GetDeclaredSymbolOnFieldInitializer [](start = 20, length = 35)
Similar comment as for the test in RecordTest.cs #Closed
{ | ||
context.RegisterSyntaxNodeAction(Handle1, SyntaxKind.NumericLiteralExpression); | ||
context.RegisterSyntaxNodeAction(Handle2, SyntaxKind.EqualsValueClause); | ||
context.RegisterSyntaxNodeAction(Fail, SyntaxKind.BaseConstructorInitializer); |
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.
SyntaxKind.BaseConstructorInitializer [](start = 55, length = 37)
Should we test ThisConstructorInitializer instead? #Closed
context.RegisterSyntaxNodeAction(Handle1, SyntaxKind.NumericLiteralExpression); | ||
context.RegisterSyntaxNodeAction(Handle2, SyntaxKind.EqualsValueClause); | ||
context.RegisterSyntaxNodeAction(Fail, SyntaxKind.BaseConstructorInitializer); | ||
context.RegisterSyntaxNodeAction(Fail, SyntaxKind.ConstructorDeclaration); |
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.
SyntaxKind.ConstructorDeclaration [](start = 55, length = 33)
Why not tets explicit constructor declaration as for record classes? #Closed
case SyntaxKind.RecordDeclaration: | ||
{ | ||
SynthesizedRecordConstructor symbol = TryGetSynthesizedRecordConstructor((RecordDeclarationSyntax)node); | ||
|
||
if (symbol is null) |
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.
if (symbol is null) [](start = 24, length = 19)
An empty line was unnecessarily removed above this line. #Closed
Done with review pass (commit 2) #Closed |
context.RegisterOperationAction(HandleLiteral, OperationKind.Literal); | ||
context.RegisterOperationAction(HandleParameterInitializer, OperationKind.ParameterInitializer); | ||
context.RegisterOperationAction(Fail, OperationKind.PropertyInitializer); | ||
context.RegisterOperationAction(Fail, OperationKind.FieldInitializer); |
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.
OperationKind.FieldInitializer) [](start = 54, length = 31)
Why not test the last two? Do we test them for record class? #Resolved
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.
They were not tested for record class (Handle5 there always fails).
In reply to: 605217978 [](ancestors = 605217978)
context.RegisterSyntaxNodeAction(Handle1, SyntaxKind.NumericLiteralExpression); | ||
context.RegisterSyntaxNodeAction(Handle2, SyntaxKind.EqualsValueClause); | ||
context.RegisterSyntaxNodeAction(Fail, SyntaxKind.BaseConstructorInitializer); | ||
context.RegisterSyntaxNodeAction(Fail, SyntaxKind.ConstructorDeclaration); |
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.
SyntaxKind.ConstructorDeclaration [](start = 55, length = 33)
Why not test these two? #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.
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.
LGTM (commit 6), except some possible reduction in test coverage around analyzer actions by comparison to record classes
Assert.False(point.IsRecord); | ||
Assert.Equal(TypeKind.Struct, point.TypeKind); | ||
Assert.Equal(SpecialType.System_ValueType, point.BaseTypeNoUseSiteDiagnostics.SpecialType); | ||
Assert.True(SyntaxFacts.IsTypeDeclaration(SyntaxKind.RecordStructDeclaration)); |
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 confused by this assert. It seems independent of any particular compilation, but is rather just testing the SyntaxKind constant. Was that intentional? #Resolved
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.
Yes, it's just testing that SyntaxFacts API :-)
Note that although it appears as a change in the diff, this is an existing check.
In reply to: 606013588 [](ancestors = 606013588)
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'll move it out of the validateModule
local function
In reply to: 606392703 [](ancestors = 606392703,606013588)
|
||
Assert.True(SyntaxFacts.IsTypeDeclaration(SyntaxKind.RecordStructDeclaration)); | ||
[Fact] | ||
public void IsRecord_AnonymousType() |
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 these and the following new tests belong in the RecordTests.cs, not in RecordStructTests.cs? #ByDesign
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.
var src = @" | ||
record struct R(R X) | ||
{ | ||
public R X { get; init; } = X; |
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.
Is there also a test for when no property is explicitly declared? #Resolved
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.
Test plan #51199
Fixes #52233 (public API)