Finalize the design for the shape of the BaseTypeSyntax for records and implement SemanticModel APIs around it. #45351

AlekseyTs · 2020-06-21T22:34:04Z

Closes #44795.

…nd implement SemanticModel APIs around it. Closes dotnet#44795.

AlekseyTs · 2020-06-22T15:49:49Z

@jcouv, @agocke, @RikkiGibson, @dotnet/roslyn-compiler Please review.

AlekseyTs · 2020-06-23T01:59:22Z

@jcouv, @agocke, @RikkiGibson, @dotnet/roslyn-compiler Please review.

gafter · 2020-06-23T02:26:42Z

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

@@ -8,16 +8,11 @@ namespace Microsoft.CodeAnalysis.CSharp.Syntax
 {
    public partial class RecordDeclarationSyntax
    {
-        internal SimpleBaseTypeSyntax? BaseWithArguments
+        internal PrimaryConstructorBaseTypeSyntax? PrimaryConstructorBaseTypeSyntax


PrimaryConstructorBaseTypeSyntax [](start = 51, length = 32)

To be consistent with the way properties on syntax nodes are named, you should drop the Syntax suffix. #Closed

To be consistent with the way properties on syntax nodes are named, you should drop the Syntax suffix.

Renamed

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

gafter · 2020-06-23T02:32:20Z

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

@@ -1197,6 +1197,8 @@ private static BoundExpression MakeLiteral(SyntaxNode syntax, ConstantValue cons
                case SyntaxKind.BaseConstructorInitializer:
                case SyntaxKind.ThisConstructorInitializer:
                    return new SourceLocation(((ConstructorInitializerSyntax)syntax).ArgumentList.OpenParenToken);
+                case SyntaxKind.PrimaryConstructorBaseType:


SyntaxKind [](start = 21, length = 10)

Is this tested? #Closed

Is this tested?

The need to test this code path is recorder in the test plan issue.

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

Can you please link to the test plan issue?

In reply to: 443928479 [](ancestors = 443928479,443927180)

Can you please link to the test plan issue?

#40726, several items, right above "Productivity" section.

In reply to: 443929342 [](ancestors = 443929342,443928479,443927180)

gafter · 2020-06-23T02:34:37Z

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

-                    nodeToBind.Kind() == SyntaxKind.SimpleBaseType || // initializer for a record constructor
-                    nodeToBind.Kind() == SyntaxKind.ArgumentList && nodeToBind.Parent is ConstructorInitializerSyntax ||
+                    nodeToBind.Kind() == SyntaxKind.PrimaryConstructorBaseType || // initializer for a record constructor
+                    nodeToBind.Kind() == SyntaxKind.ArgumentList && (nodeToBind.Parent is ConstructorInitializerSyntax || nodeToBind.Parent is PrimaryConstructorBaseTypeSyntax) ||


PrimaryConstructorBaseTypeSyntax [](start = 143, length = 32)

Is this tested? #Closed

Is this tested?

Yes, this scenario is exercised by speculative operations.

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

Can you please identify a test which exercises this?

In reply to: 443929018 [](ancestors = 443929018,443927699)

Can you please identify a test which exercises this?

BaseArguments_19, GetSpeculativeSymbolInfo for an initializer with a an out var in an argument.

In reply to: 443929431 [](ancestors = 443929431,443929018,443927699)

This line as well Assert.Equal("Base..ctor(System.Int32 X)", speculativeModel!.GetSymbolInfo((SyntaxNode)speculativePrimaryInitializer).Symbol.ToTestDisplayString());

In reply to: 443930921 [](ancestors = 443930921,443929431,443929018,443927699)

gafter · 2020-06-23T02:39:36Z

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

+    public Base() {}
+}
+
+record C(int X, int Y) : Base(GetInt(X, out var xx) + xx, Y), I


Base [](start = 25, length = 4)

This should also be tested with a class (not record) as the base, as permitted by https://github.com/dotnet/csharplang/blob/master/proposals/records.md #Closed

This should also be tested with a class (not record) as the base, as permitted by https://github.com/dotnet/csharplang/blob/master/proposals/records.md

Any particular aspect you have in mind? There were some existing tests with classes with the old representation.

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

BaseArguments_12 for example.

In reply to: 443929594 [](ancestors = 443929594,443928983)

That addresses my concern.

In reply to: 443930063 [](ancestors = 443930063,443929594,443928983)

gafter · 2020-06-23T02:39:55Z

record C(int X, int Y) : Base(X, Y)

~~This should also be tested with a class (not record) as the base, as permitted by https://github.com/dotnet/csharplang/blob/master/proposals/records.md~~ #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:7029 in b65a77f. [](commit_id = b65a77f, deletion_comment = False)

gafter · 2020-06-23T02:41:35Z

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

+                                _nodeBinder.BindConstructorInitializer(ctorInitializer, diagnostics);
+                                break;
+                            case PrimaryConstructorBaseTypeSyntax ctorInitializer:
+                                _nodeBinder.BindConstructorInitializer(ctorInitializer, diagnostics);


BindConstructorInitializer [](start = 44, length = 26)

Can you please identify a test for this code path? #Closed

Can you please identify a test for this code path?

For example, sBaseArguments_19, OutVarTests.VerifyModelForOutVar(speculativeModel, xxDecl, xxRef);

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

AlekseyTs · 2020-06-23T14:32:08Z

@jcouv, @agocke, @RikkiGibson, @dotnet/roslyn-compiler Please review.

gafter

AlekseyTs · 2020-06-23T16:36:49Z

@jcouv, @agocke, @RikkiGibson, @dotnet/roslyn-compiler Please review, need a second sign-off..

333fred · 2020-06-23T21:50:06Z

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

+                var diagnostics = DiagnosticBag.GetInstance();
+                binder = new ExecutableCodeBinder(constructorInitializer, binder.ContainingMemberOrLambda, binder);
+
+                BoundExpressionStatement bnode = binder.BindConstructorInitializer(constructorInitializer, diagnostics);


I believe we need to do a nullable rewrite of these nodes. #Closed

I believe we need to do a nullable rewrite of these nodes.

It doesn't look like we do this for any GetSpeculativeXXX APIs. Please open an issue if you think they should do the rewrite.

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

Added a note to #35038.

In reply to: 444537180 [](ancestors = 444537180,444528944)

333fred · 2020-06-23T21:56:56Z

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

                nodeToBind.Kind() == SyntaxKind.SwitchExpressionArm ||
-                nodeToBind.Kind() == SyntaxKind.ArgumentList && nodeToBind.Parent is ConstructorInitializerSyntax ||
+                nodeToBind.Kind() == SyntaxKind.ArgumentList && (nodeToBind.Parent is ConstructorInitializerSyntax || nodeToBind.Parent is PrimaryConstructorBaseTypeSyntax) ||


nodeToBind.Parent is ConstructorInitializerSyntax || nodeToBind.Parent is PrimaryConstructorBaseTypeSyntax [](start = 65, length = 106)

This is used a couple of times. Consider making a helper. #WontFix

333fred · 2020-06-23T22:04:47Z

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

+                symbolInfo = model.GetSymbolInfo((SyntaxNode)baseWithargs);
+                Assert.Equal(SymbolInfo.None, symbolInfo);
+                symbolInfo = model.GetSymbolInfo(baseWithargs);
+                Assert.Equal(SymbolInfo.None, symbolInfo);


Why don't we get a list of failed candidates here? #Closed

Why don't we get a list of failed candidates here?

Because there are no candidates, the PrimaryConstructorBaseTypeSyntax is never bound for anything other than a record.

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

333fred · 2020-06-23T22:06:42Z

Done review pass (commit 2). A couple of questions. I don't see any tests involving nullability for these, is there an item in the test plan tracking nullable public API testing for record constructors? #Closed

AlekseyTs · 2020-06-23T22:21:33Z

A couple of questions

Answered.

I don't see any tests involving nullability for these, is there an item in the test plan tracking nullable public API testing for record constructors?

This isn't really related to the shape of the syntax. Added a note to the Test Plan #40726.

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

333fred · 2020-06-23T22:32:13Z

This isn't really related to the shape of the syntax. Added a note to the Test Plan #40726.

Thanks.

In reply to: 648458952 [](ancestors = 648458952,648452941)

333fred

LGTM (commit 2)

Finalize the design for the shape of the BaseTypeSyntax for records a…

b65a77f

…nd implement SemanticModel APIs around it. Closes dotnet#44795.

AlekseyTs added Area-Compilers New Language Feature - Records Records labels Jun 21, 2020

AlekseyTs requested review from agocke, RikkiGibson, jcouv and a team June 21, 2020 22:34

gafter reviewed Jun 23, 2020

View reviewed changes

Rename + add some tests

d3d4e21

gafter approved these changes Jun 23, 2020

View reviewed changes

333fred reviewed Jun 23, 2020

View reviewed changes

333fred approved these changes Jun 23, 2020

View reviewed changes

AlekseyTs merged commit 09db48d into dotnet:master Jun 23, 2020

ghost added this to the Next milestone Jun 23, 2020

dibarbet modified the milestones: Next, 16.7.P4 Jun 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finalize the design for the shape of the BaseTypeSyntax for records and implement SemanticModel APIs around it. #45351

Finalize the design for the shape of the BaseTypeSyntax for records and implement SemanticModel APIs around it. #45351

AlekseyTs commented Jun 21, 2020

AlekseyTs commented Jun 22, 2020

AlekseyTs commented Jun 23, 2020

gafter Jun 23, 2020 •

edited

Loading

AlekseyTs Jun 23, 2020

gafter Jun 23, 2020 •

edited

Loading

AlekseyTs Jun 23, 2020

gafter Jun 23, 2020

AlekseyTs Jun 23, 2020

gafter Jun 23, 2020 •

edited

Loading

AlekseyTs Jun 23, 2020

gafter Jun 23, 2020

AlekseyTs Jun 23, 2020

AlekseyTs Jun 23, 2020

gafter Jun 23, 2020 •

edited

Loading

AlekseyTs Jun 23, 2020 •

edited

Loading

AlekseyTs Jun 23, 2020

gafter Jun 23, 2020

gafter commented Jun 23, 2020 •

edited

Loading

gafter Jun 23, 2020 •

edited

Loading

AlekseyTs Jun 23, 2020

AlekseyTs commented Jun 23, 2020

gafter left a comment

AlekseyTs commented Jun 23, 2020

333fred Jun 23, 2020 •

edited

Loading

AlekseyTs Jun 23, 2020

333fred Jun 23, 2020

333fred Jun 23, 2020 •

edited by AlekseyTs

Loading

333fred Jun 23, 2020 •

edited

Loading

AlekseyTs Jun 23, 2020

333fred commented Jun 23, 2020 •

edited

Loading

AlekseyTs commented Jun 23, 2020

333fred commented Jun 23, 2020

333fred left a comment

Finalize the design for the shape of the BaseTypeSyntax for records and implement SemanticModel APIs around it. #45351

Finalize the design for the shape of the BaseTypeSyntax for records and implement SemanticModel APIs around it. #45351

Conversation

AlekseyTs commented Jun 21, 2020

AlekseyTs commented Jun 22, 2020

AlekseyTs commented Jun 23, 2020

gafter Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Jun 23, 2020 • edited Loading

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

gafter Jun 23, 2020 • edited Loading

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

gafter Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Jun 23, 2020 • edited Loading

gafter Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jun 23, 2020

gafter left a comment

Choose a reason for hiding this comment

AlekseyTs commented Jun 23, 2020

333fred Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred Jun 23, 2020 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

333fred Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented Jun 23, 2020 • edited Loading

AlekseyTs commented Jun 23, 2020

333fred commented Jun 23, 2020

333fred left a comment

Choose a reason for hiding this comment

gafter Jun 23, 2020 •

edited

Loading

gafter Jun 23, 2020 •

edited

Loading

gafter Jun 23, 2020 •

edited

Loading

gafter Jun 23, 2020 •

edited

Loading

AlekseyTs Jun 23, 2020 •

edited

Loading

gafter commented Jun 23, 2020 •

edited

Loading

gafter Jun 23, 2020 •

edited

Loading

333fred Jun 23, 2020 •

edited

Loading

333fred Jun 23, 2020 •

edited by AlekseyTs

Loading

333fred Jun 23, 2020 •

edited

Loading

333fred commented Jun 23, 2020 •

edited

Loading