Record-structs: Address some PROTOTYPE markers by jcouv · Pull Request #52256 · dotnet/roslyn

jcouv · 2021-03-30T17:20:34Z

Test plan #51199

Fixes #52233 (public API)

jcouv · 2021-03-30T17:22:29Z

src/Compilers/CSharp/CSharpAnalyzerDriver/CSharpDeclarationComputer.cs

+                case SyntaxKind.RecordStructDeclaration:
+                    {
+                        if (associatedSymbol is IMethodSymbol ctor)
+                        {


📝 I didn't manage to cover this if and couldn't find coverage for above if either. #Resolved

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)

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)

jcouv · 2021-03-30T17:23:27Z

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

            return GetITypeSymbol(DefaultNullableAnnotation);
        }

-        // PROTOTYPE(record-structs): rename to IsRecordClass?


📝 not planning to rename the internal API #Resolved

jcouv · 2021-03-30T17:25:34Z

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

                case SyntaxKind.InterfaceKeyword:
                    return SyntaxKind.InterfaceDeclaration;
                case SyntaxKind.RecordKeyword:
-                    // PROTOTYPE(record-structs): anything we can do? 


📝 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

jcouv · 2021-03-30T17:26:09Z

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

                    return SyntaxKind.InterfaceKeyword;
                case SyntaxKind.RecordDeclaration:
                    return SyntaxKind.RecordKeyword;
-                // PROTOTYPE(record-structs): update for record structs


📝 We'll see if something is needed here during IDE work #Resolved

jcouv · 2021-03-30T17:27:49Z

src/Compilers/CSharp/Test/Syntax/Parsing/RecordParsing.cs

        [Fact, CompilerTrait(CompilerFeature.RecordStructs)]
        public void RecordInterfaceParsing()
        {
-            // PROTOTYPE(record-structs): should we treat this as a binding error instead, for better error recovery?


📝 Added a bullet to test plan #Resolved

jcouv · 2021-03-30T17:28:32Z

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

        [Fact]
        public void RecordStructLanguageVersion()
        {
-            // PROTOTYPE(record-structs): can we improve the error recovery, maybe treating this as a record struct with missing `record`?


📝 Added a bullet to test plan #Resolved

AlekseyTs · 2021-03-31T17:54:44Z

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

-                        }
-
-                        return createMethodBodySemanticModel(node, symbol);
+                        return symbol is null ? null : createMethodBodySemanticModel(node, symbol);


return symbol is null ? null : createMethodBodySemanticModel(node, symbol); [](start = 24, length = 75)

Please revert style change. #Closed

AlekseyTs · 2021-03-31T18:09:44Z

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

                        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; 


//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

AlekseyTs · 2021-03-31T18:21:37Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs


-        // 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


// Tracked by #52233 [](start = 8, length = 59)

Have we resolved this design question? #Closed

AlekseyTs · 2021-03-31T18:27:19Z

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

        }

+        [Fact]
+        public void GetDeclaredSymbolOnFieldInitializer()


Field [](start = 39, length = 5)

Field or Property? #Closed

And the test doesn't do either.

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

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)

AlekseyTs · 2021-03-31T18:36:50Z

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

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


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

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)

AlekseyTs · 2021-03-31T18:37:26Z

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

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


GetDeclaredSymbolOnFieldInitializer [](start = 20, length = 35)

Similar comment as for the test in RecordTest.cs #Closed

AlekseyTs · 2021-03-31T18:43:30Z

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

+            {
+                context.RegisterSyntaxNodeAction(Handle1, SyntaxKind.NumericLiteralExpression);
+                context.RegisterSyntaxNodeAction(Handle2, SyntaxKind.EqualsValueClause);
+                context.RegisterSyntaxNodeAction(Fail, SyntaxKind.BaseConstructorInitializer);


SyntaxKind.BaseConstructorInitializer [](start = 55, length = 37)

Should we test ThisConstructorInitializer instead? #Closed

AlekseyTs · 2021-03-31T18:44:08Z

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

+                context.RegisterSyntaxNodeAction(Handle1, SyntaxKind.NumericLiteralExpression);
+                context.RegisterSyntaxNodeAction(Handle2, SyntaxKind.EqualsValueClause);
+                context.RegisterSyntaxNodeAction(Fail, SyntaxKind.BaseConstructorInitializer);
+                context.RegisterSyntaxNodeAction(Fail, SyntaxKind.ConstructorDeclaration);


SyntaxKind.ConstructorDeclaration [](start = 55, length = 33)

Why not tets explicit constructor declaration as for record classes? #Closed

AlekseyTs · 2021-03-31T18:47:21Z

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

                    {
                        SynthesizedRecordConstructor symbol = TryGetSynthesizedRecordConstructor((RecordDeclarationSyntax)node);
-
                        if (symbol is null)


if (symbol is null) [](start = 24, length = 19)

An empty line was unnecessarily removed above this line. #Closed

AlekseyTs · 2021-03-31T18:51:46Z

Done with review pass (commit 2) #Closed

AlekseyTs · 2021-03-31T21:01:31Z

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

+                context.RegisterOperationAction(HandleLiteral, OperationKind.Literal);
+                context.RegisterOperationAction(HandleParameterInitializer, OperationKind.ParameterInitializer);
+                context.RegisterOperationAction(Fail, OperationKind.PropertyInitializer);
+                context.RegisterOperationAction(Fail, OperationKind.FieldInitializer);


OperationKind.FieldInitializer) [](start = 54, length = 31)

Why not test the last two? Do we test them for record class? #Resolved

They were not tested for record class (Handle5 there always fails).

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

AlekseyTs · 2021-03-31T21:02:28Z

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

+                context.RegisterSyntaxNodeAction(Handle1, SyntaxKind.NumericLiteralExpression);
+                context.RegisterSyntaxNodeAction(Handle2, SyntaxKind.EqualsValueClause);
+                context.RegisterSyntaxNodeAction(Fail, SyntaxKind.BaseConstructorInitializer);
+                context.RegisterSyntaxNodeAction(Fail, SyntaxKind.ConstructorDeclaration);


SyntaxKind.ConstructorDeclaration [](start = 55, length = 33)

Why not test these two? #Closed

Added. Thanks

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

AlekseyTs

LGTM (commit 6), except some possible reduction in test coverage around analyzer actions by comparison to record classes

RikkiGibson · 2021-04-02T00:27:05Z

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

+                Assert.False(point.IsRecord);
+                Assert.Equal(TypeKind.Struct, point.TypeKind);
+                Assert.Equal(SpecialType.System_ValueType, point.BaseTypeNoUseSiteDiagnostics.SpecialType);
+                Assert.True(SyntaxFacts.IsTypeDeclaration(SyntaxKind.RecordStructDeclaration));


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

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)

I'll move it out of the validateModule local function

In reply to: 606392703 [](ancestors = 606392703,606013588)

RikkiGibson · 2021-04-02T00:28:19Z

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


-            Assert.True(SyntaxFacts.IsTypeDeclaration(SyntaxKind.RecordStructDeclaration));
+        [Fact]
+        public void IsRecord_AnonymousType()


Do these and the following new tests belong in the RecordTests.cs, not in RecordStructTests.cs? #ByDesign

Could do either way.

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

RikkiGibson · 2021-04-02T00:30:30Z

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

+            var src = @"
+record struct R(R X)
+{
+    public R X { get; init; } = X;


Is there also a test for when no property is explicitly declared? #Resolved

Added. Thanks

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

Record-structs: Address some PROTOTYPE markers

595d312

jcouv added Area-Compilers Feature - Records Records labels Mar 30, 2021

jcouv added this to the C# 10 milestone Mar 30, 2021

jcouv self-assigned this Mar 30, 2021

jcouv commented Mar 30, 2021

View reviewed changes

This was referenced Mar 30, 2021

Agents canceling during dotnet builds jaredpar/runfo#47

Closed

Azure Agent timeout #48030

Closed

jcouv marked this pull request as ready for review March 30, 2021 19:34

jcouv requested a review from a team as a code owner March 30, 2021 19:34

AlekseyTs reviewed Mar 31, 2021

View reviewed changes

Address feedback

6ef4144

AlekseyTs reviewed Mar 31, 2021

View reviewed changes

Julien Couvreur added 2 commits March 31, 2021 12:19

Add public API

92a6a9c

Address feedback

8700ac6

jcouv requested a review from AlekseyTs March 31, 2021 20:09

nits

1299dba

nits

67e324a

AlekseyTs reviewed Mar 31, 2021

View reviewed changes

AlekseyTs approved these changes Mar 31, 2021

View reviewed changes

Fix test

aa89338

RikkiGibson self-assigned this Mar 31, 2021

jcouv mentioned this pull request Mar 31, 2021

Test plan for "record structs" #51199

Closed

92 tasks

RikkiGibson reviewed Apr 2, 2021

View reviewed changes

RikkiGibson approved these changes Apr 2, 2021

View reviewed changes

Address feedback

ac54980

jcouv enabled auto-merge (squash) April 2, 2021 19:57

jcouv merged commit 74728e4 into dotnet:features/record-structs Apr 2, 2021

jcouv deleted the rs-symbol7 branch April 2, 2021 21:50

Conversation

jcouv commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Mar 31, 2021 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

jcouv commented Mar 30, 2021 •

edited

Loading

jcouv Mar 30, 2021 •

edited

Loading

jcouv Mar 30, 2021 •

edited

Loading

jcouv Mar 30, 2021 •

edited

Loading

jcouv Mar 30, 2021 •

edited

Loading

jcouv Mar 30, 2021 •

edited

Loading

jcouv Mar 30, 2021 •

edited

Loading

AlekseyTs Mar 31, 2021 •

edited

Loading

AlekseyTs Mar 31, 2021 •

edited

Loading

AlekseyTs Mar 31, 2021 •

edited

Loading

AlekseyTs Mar 31, 2021 •

edited

Loading

AlekseyTs Mar 31, 2021 •

edited

Loading

AlekseyTs Mar 31, 2021 •

edited

Loading

AlekseyTs Mar 31, 2021 •

edited

Loading

AlekseyTs Mar 31, 2021 •

edited

Loading

AlekseyTs Mar 31, 2021 •

edited

Loading

AlekseyTs commented Mar 31, 2021 •

edited

Loading

AlekseyTs Mar 31, 2021 •

edited by jcouv

Loading

AlekseyTs Mar 31, 2021 •

edited

Loading

RikkiGibson Apr 2, 2021 •

edited by jcouv

Loading

RikkiGibson Apr 2, 2021 •

edited by jcouv

Loading

RikkiGibson Apr 2, 2021 •

edited by jcouv

Loading