Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Record-structs: remove RecordStructDeclarationSyntax #52702

Merged
merged 13 commits into from
Apr 22, 2021
Merged

Record-structs: remove RecordStructDeclarationSyntax #52702

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
545a25e
Record-structs: remove RecordStructDeclarationSyntax
jcouv Apr 17, 2021
d659903
Address feedback
jcouv Apr 18, 2021
9c48dd6
Use StructKeyword
jcouv Apr 19, 2021
1c89f88
Address feedback
jcouv Apr 20, 2021
3ef4083
Further simplifications
jcouv Apr 20, 2021
8618ce5
tweak
jcouv Apr 20, 2021
f897772
Fix bug with parameter lists on implemented interface
jcouv Apr 20, 2021
e03625c
Merge remote-tracking branch 'dotnet/features/record-structs' into re…
jcouv Apr 21, 2021
02326be
test cleanup
jcouv Apr 21, 2021
778b843
Add extra check for first item
jcouv Apr 21, 2021
57bb245
nit
jcouv Apr 21, 2021
f4c4d0c
More tests
jcouv Apr 21, 2021
5c551d2
Add assertion and speculation on SimpleBaseType
jcouv Apr 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 1 addition & 14 deletions src/Compilers/CSharp/CSharpAnalyzerDriver/CSharpDeclarationComputer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ private static void ComputeDeclarations(
return;
}
case SyntaxKind.RecordDeclaration:
case SyntaxKind.RecordStructDeclaration:
{
if (associatedSymbol is IMethodSymbol ctor)
{
Expand All @@ -115,20 +116,6 @@ private static void ComputeDeclarations(
return;
}

goto case SyntaxKind.ClassDeclaration;
}
case SyntaxKind.RecordStructDeclaration:
{
if (associatedSymbol is IMethodSymbol ctor)
{
var recordDeclaration = (RecordStructDeclarationSyntax)node;
Debug.Assert(ctor.MethodKind == MethodKind.Constructor && recordDeclaration.ParameterList is object);

var codeBlocks = GetParameterListInitializersAndAttributes(recordDeclaration.ParameterList);
builder.Add(GetDeclarationInfo(node, associatedSymbol, codeBlocks));
return;
}

goto case SyntaxKind.ClassDeclaration;
}
case SyntaxKind.ClassDeclaration:
Expand Down
6 changes: 1 addition & 5 deletions src/Compilers/CSharp/Portable/Binder/BinderFactory.BinderFactoryVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,9 +758,6 @@ public override Binder VisitInterfaceDeclaration(InterfaceDeclarationSyntax node
public override Binder VisitRecordDeclaration(RecordDeclarationSyntax node)
=> VisitTypeDeclarationCore(node);

public override Binder VisitRecordStructDeclaration(RecordStructDeclarationSyntax node)
=> VisitTypeDeclarationCore(node);

public override Binder VisitNamespaceDeclaration(NamespaceDeclarationSyntax parent)
{
if (!LookupPosition.IsInNamespaceDeclaration(_position, parent))
Expand Down Expand Up @@ -1142,8 +1139,7 @@ private Binder GetParameterNameAttributeValueBinder(MemberDeclarationSyntax memb
return new WithParametersBinder(method.Parameters, nextBinder);
}

if (memberSyntax is RecordDeclarationSyntax { ParameterList: { ParameterCount: > 0 } }
or RecordStructDeclarationSyntax { ParameterList: { ParameterCount: > 0 } })
if (memberSyntax is RecordDeclarationSyntax { ParameterList: { ParameterCount: > 0 } })
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RecordDeclarationSyntax

Have you confirmed that all remaining type checks for RecordDeclarationSyntax are applicable to record structs or appropriately adjusted? Same for Visit methods. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I have a list. The places that previously only handled RecordDeclarationSyntax are guarded (kind check or base type check). Some are guarded locally and are checked elsewhere (unreachable).
For the latter cases, I didn't see anything wrong with them. I could add assertions if you think that's useful.
An example is ExpressionVariableFinder.VisitRecordDeclaration.

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

{
Binder outerBinder = VisitCore(memberSyntax);
SourceNamedTypeSymbol recordType = ((NamespaceOrTypeSymbol)outerBinder.ContainingMemberOrLambda).GetSourceTypeMember((TypeDeclarationSyntax)memberSyntax);
Expand Down
38 changes: 1 addition & 37 deletions src/Compilers/CSharp/Portable/Compilation/SyntaxTreeSemanticModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1255,9 +1255,8 @@ MemberSemanticModel createMethodBodySemanticModel(CSharpSyntaxNode memberDecl, S
}
}

private SynthesizedRecordConstructor TryGetSynthesizedRecordConstructor(TypeDeclarationSyntax node)
private SynthesizedRecordConstructor TryGetSynthesizedRecordConstructor(RecordDeclarationSyntax node)
{
Debug.Assert(node is RecordDeclarationSyntax or RecordStructDeclarationSyntax);
NamedTypeSymbol recordType = GetDeclaredType(node);
var symbol = recordType.GetMembersUnordered().OfType<SynthesizedRecordConstructor>().SingleOrDefault();

Expand Down Expand Up @@ -2027,10 +2026,6 @@ private ParameterSymbol GetMethodParameterSymbol(
{
method = TryGetSynthesizedRecordConstructor(recordDecl);
}
else if (memberDecl is RecordStructDeclarationSyntax recordStructDecl && recordStructDecl.ParameterList == paramList)
{
method = TryGetSynthesizedRecordConstructor(recordStructDecl);
}
else
{
method = (GetDeclaredSymbol(memberDecl, cancellationToken) as IMethodSymbol).GetSymbol();
Expand Down Expand Up @@ -2435,33 +2430,6 @@ internal override Func<SyntaxNode, bool> GetSyntaxNodesToAnalyzeFilter(SyntaxNod
}
break;

case RecordStructDeclarationSyntax recordStructDeclaration when TryGetSynthesizedRecordConstructor(recordStructDeclaration) is SynthesizedRecordConstructor ctor:
switch (declaredSymbol.Kind)
{
case SymbolKind.Method:
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case SymbolKind.Method:

I think we should preserve the difference for this case. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing why (here or below). We get the same behavior. Do you mean just as an optimization?

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. This would be necessary if we add assertion in PrimaryConstructorBaseType as you suggested... Let me take a look

In reply to: 616930880 [](ancestors = 616930880,616734766)

Copy link
Contributor

@AlekseyTs AlekseyTs Apr 20, 2021 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing why (here or below). We get the same behavior. Do you mean just as an optimization?

I do not think we should look at bases for record structs even when they are present in an error scenario #Resolved

Debug.Assert((object)declaredSymbol.GetSymbol() == (object)ctor);
return (node) =>
{
// Accept only nodes that either match, or above/below of a 'parameter list'.
if (node.Parent == recordStructDeclaration)
{
return node == recordStructDeclaration.ParameterList;
}

return true;
};

case SymbolKind.NamedType:
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case SymbolKind.NamedType:

I think we should preserve the difference for this case. #Closed

Debug.Assert((object)declaredSymbol.GetSymbol() == (object)ctor.ContainingSymbol);
// Accept nodes that do not match a 'parameter list'.
return (node) => node != recordStructDeclaration.ParameterList;

default:
ExceptionUtilities.UnexpectedValue(declaredSymbol.Kind);
break;
}
break;

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Apr 18, 2021 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. this change alone makes me want this PR :) #Resolved

case PrimaryConstructorBaseTypeSyntax { Parent: BaseListSyntax { Parent: RecordDeclarationSyntax recordDeclaration } } baseType
when recordDeclaration.PrimaryConstructorBaseType == declaredNode && TryGetSynthesizedRecordConstructor(recordDeclaration) is SynthesizedRecordConstructor ctor:
if ((object)declaredSymbol.GetSymbol() == (object)ctor)
Expand All @@ -2474,10 +2442,6 @@ internal override Func<SyntaxNode, bool> GetSyntaxNodesToAnalyzeFilter(SyntaxNod
case ParameterSyntax param when declaredSymbol.Kind == SymbolKind.Property && param.Parent?.Parent is RecordDeclarationSyntax recordDeclaration && recordDeclaration.ParameterList == param.Parent:
Debug.Assert(declaredSymbol.GetSymbol() is SynthesizedRecordPropertySymbol);
return (node) => false;

case ParameterSyntax param when declaredSymbol.Kind == SymbolKind.Property && param.Parent?.Parent is RecordStructDeclarationSyntax recordStructDeclaration && recordStructDeclaration.ParameterList == param.Parent:
Debug.Assert(declaredSymbol.GetSymbol() is SynthesizedRecordPropertySymbol);
return (node) => false;
}

return null;
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1918,7 +1918,7 @@ internal static BoundExpression BindImplicitConstructorInitializer(
NamedTypeSymbol baseType = containingType.BaseTypeNoUseSiteDiagnostics;

SourceMemberMethodSymbol sourceConstructor = constructor as SourceMemberMethodSymbol;
Debug.Assert(sourceConstructor?.SyntaxNode is RecordDeclarationSyntax or RecordStructDeclarationSyntax
Debug.Assert(sourceConstructor?.SyntaxNode is RecordDeclarationSyntax
|| ((ConstructorDeclarationSyntax)sourceConstructor?.SyntaxNode)?.Initializer == null);

// The common case is that the type inherits directly from object.
Expand Down
14 changes: 10 additions & 4 deletions src/Compilers/CSharp/Portable/Declarations/DeclarationTreeBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,10 +380,16 @@ public override SingleNamespaceOrTypeDeclaration VisitInterfaceDeclaration(Inter
}

public override SingleNamespaceOrTypeDeclaration VisitRecordDeclaration(RecordDeclarationSyntax node)
=> VisitTypeDeclaration(node, DeclarationKind.Record);
{
var declarationKind = node.Kind() switch
{
SyntaxKind.RecordDeclaration => DeclarationKind.Record,
SyntaxKind.RecordStructDeclaration => DeclarationKind.RecordStruct,
_ => throw ExceptionUtilities.UnexpectedValue(node.Kind())
};

public override SingleNamespaceOrTypeDeclaration VisitRecordStructDeclaration(RecordStructDeclarationSyntax node)
=> VisitTypeDeclaration(node, DeclarationKind.RecordStruct);
return VisitTypeDeclaration(node, declarationKind);
}

private SingleNamespaceOrTypeDeclaration VisitTypeDeclaration(TypeDeclarationSyntax node, DeclarationKind kind)
{
Expand All @@ -407,7 +413,7 @@ private SingleNamespaceOrTypeDeclaration VisitTypeDeclaration(TypeDeclarationSyn

// A record with parameters at least has a primary constructor
if (((declFlags & SingleTypeDeclaration.TypeDeclarationFlags.HasAnyNontypeMembers) == 0) &&
node is RecordDeclarationSyntax { ParameterList: { } } or RecordStructDeclarationSyntax { ParameterList: { } })
node is RecordDeclarationSyntax { ParameterList: { } })
{
declFlags |= SingleTypeDeclaration.TypeDeclarationFlags.HasAnyNontypeMembers;
}
Expand Down
7 changes: 1 addition & 6 deletions src/Compilers/CSharp/Portable/Generated/CSharp.Generated.g4
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading