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 8 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
10 changes: 5 additions & 5 deletions src/Compilers/CSharp/Portable/Binder/BinderFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,25 +140,25 @@ internal Binder GetBinder(SyntaxNode node, int position, CSharpSyntaxNode member

internal InMethodBinder GetRecordConstructorInMethodBinder(SynthesizedRecordConstructor constructor)
{
TypeDeclarationSyntax typeDecl = constructor.GetSyntax();
Debug.Assert(typeDecl is RecordDeclarationSyntax);
var recordDecl = constructor.GetSyntax();
Debug.Assert(recordDecl.IsKind(SyntaxKind.RecordDeclaration));

var extraInfo = NodeUsage.ConstructorBodyOrInitializer;
var key = BinderFactoryVisitor.CreateBinderCacheKey(typeDecl, extraInfo);
var key = BinderFactoryVisitor.CreateBinderCacheKey(recordDecl, extraInfo);

if (!_binderCache.TryGetValue(key, out Binder resultBinder))
{
// Ctors cannot be generic
Debug.Assert(constructor.Arity == 0, "Generic Ctor, What to do?");
resultBinder = new InMethodBinder(constructor, GetInRecordBodyBinder(typeDecl));
resultBinder = new InMethodBinder(constructor, GetInRecordBodyBinder(recordDecl));

_binderCache.TryAdd(key, resultBinder);
}

return (InMethodBinder)resultBinder;
}

internal Binder GetInRecordBodyBinder(TypeDeclarationSyntax typeDecl)
internal Binder GetInRecordBodyBinder(RecordDeclarationSyntax typeDecl)
{
Debug.Assert(typeDecl.Kind() is SyntaxKind.RecordDeclaration);

Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3338,12 +3338,13 @@ private BoundNode BindSimpleProgramCompilationUnit(CompilationUnitSyntax compila
private BoundNode BindRecordConstructorBody(RecordDeclarationSyntax recordDecl, BindingDiagnosticBag diagnostics)
{
Debug.Assert(recordDecl.ParameterList is object);
Debug.Assert(recordDecl.IsKind(SyntaxKind.RecordDeclaration));

Binder bodyBinder = this.GetBinder(recordDecl);
Debug.Assert(bodyBinder != null);

BoundExpressionStatement initializer = null;
if (recordDecl.PrimaryConstructorBaseType is PrimaryConstructorBaseTypeSyntax baseWithArguments)
if (recordDecl.PrimaryConstructorBaseTypeIfClass is PrimaryConstructorBaseTypeSyntax baseWithArguments)
{
initializer = bodyBinder.BindConstructorInitializer(baseWithArguments, diagnostics);
}
Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,8 +398,9 @@ public override void VisitConstructorDeclaration(ConstructorDeclarationSyntax no
public override void VisitRecordDeclaration(RecordDeclarationSyntax node)
{
Debug.Assert(node.ParameterList is object);
Debug.Assert(node.IsKind(SyntaxKind.RecordDeclaration));

if (node.PrimaryConstructorBaseType is PrimaryConstructorBaseTypeSyntax baseWithArguments)
if (node.PrimaryConstructorBaseTypeIfClass is PrimaryConstructorBaseTypeSyntax baseWithArguments)
{
VisitNodeToBind(baseWithArguments);
}
Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Binder/LocalBinderFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,11 @@ public override void VisitConstructorDeclaration(ConstructorDeclarationSyntax no
public override void VisitRecordDeclaration(RecordDeclarationSyntax node)
{
Debug.Assert(node.ParameterList is object);
Debug.Assert(node.IsKind(SyntaxKind.RecordDeclaration));

Binder enclosing = new ExpressionVariableBinder(node, _enclosing);
AddToMap(node, enclosing);
Visit(node.PrimaryConstructorBaseType, enclosing);
Visit(node.PrimaryConstructorBaseTypeIfClass, enclosing);
}

public override void VisitPrimaryConstructorBaseType(PrimaryConstructorBaseTypeSyntax node)
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Compilation/MethodBodySemanticModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ internal override bool TryGetSpeculativeSemanticModelCore(SyntaxTreeSemanticMode
{
if (MemberSymbol is SynthesizedRecordConstructor primaryCtor &&
primaryCtor.GetSyntax() is RecordDeclarationSyntax recordDecl &&
Root.FindToken(position).Parent?.AncestorsAndSelf().OfType<PrimaryConstructorBaseTypeSyntax>().FirstOrDefault() == recordDecl.PrimaryConstructorBaseType)
Root.FindToken(position).Parent?.AncestorsAndSelf().OfType<PrimaryConstructorBaseTypeSyntax>().FirstOrDefault() == recordDecl.PrimaryConstructorBaseTypeIfClass)
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.

Root.FindToken(position).Parent?.AncestorsAndSelf().OfType().FirstOrDefault() == recordDecl.PrimaryConstructorBaseTypeIfClass

I am assuming this method is never called for a record struct, otherwise it should never return true, even in error scenarios. It might be worth adding an assert at the top of the method that MemberSymbol is not a primary constructor of a record struct. Also, I think we need to add a test that tries to get the speculative semantic model for a record struct at position of the first base when it has and doesn't have arguments. BTW, it looks like the is a bug, the following code compiles without any error:

public interface I1
{
   void M01();
}

record struct R(int X) : I1(X)
{
   void I1.M01() {}
}
``` #Closed

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.

There is also a bug for a regular record:

record R(int X) : I1()
{
    void I1.M01() {}
}

No error. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Added test BaseArguments_Speculation. That test will reach the body of this conditional when using record instead of record struct. With record struct we don't even get to the TryGetSpeculativeSemanticModelCore method.
I think it's because of the logic in SyntaxTreeSemanticModel.GetMemberModel which handles SyntaxKind.RecordDeclaration but not SyntaxKind.RecordStructDeclaration

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

Copy link
Contributor

Choose a reason for hiding this comment

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

That matches my expectations, that is why I am suggesting to add an assert to ensure the method (not the body of the if) is not reachable for a record struct

{
var binder = this.GetEnclosingBinder(position);
if (binder != null)
Expand Down
130 changes: 62 additions & 68 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 @@ -181,7 +181,7 @@ internal override IOperation GetOperationWorker(CSharpSyntaxNode node, Cancellat
case AccessorDeclarationSyntax accessor:
model = (accessor.Body != null || accessor.ExpressionBody != null) ? GetOrAddModel(node) : null;
break;
case RecordDeclarationSyntax { ParameterList: { }, PrimaryConstructorBaseType: { } } recordDeclaration when TryGetSynthesizedRecordConstructor(recordDeclaration) is SynthesizedRecordConstructor:
case RecordDeclarationSyntax { ParameterList: { }, PrimaryConstructorBaseTypeIfClass: { } } recordDeclaration when TryGetSynthesizedRecordConstructor(recordDeclaration) is SynthesizedRecordConstructor:
model = GetOrAddModel(recordDeclaration);
break;
default:
Expand Down Expand Up @@ -814,7 +814,7 @@ private MemberSemanticModel GetMemberModel(int position)
}
else
{
var argumentList = recordDecl.PrimaryConstructorBaseType?.ArgumentList;
var argumentList = recordDecl.PrimaryConstructorBaseTypeIfClass?.ArgumentList;
outsideMemberDecl = argumentList is null || !LookupPosition.IsBetweenTokens(position, argumentList.OpenParenToken, argumentList.CloseParenToken);
}
}
Expand Down Expand Up @@ -877,7 +877,7 @@ internal override MemberSemanticModel GetMemberModel(SyntaxNode node)
{
var recordDecl = (RecordDeclarationSyntax)memberDecl;
return recordDecl.ParameterList is object &&
recordDecl.PrimaryConstructorBaseType is PrimaryConstructorBaseTypeSyntax baseWithArguments &&
recordDecl.PrimaryConstructorBaseTypeIfClass is PrimaryConstructorBaseTypeSyntax baseWithArguments &&
(node == baseWithArguments || baseWithArguments.ArgumentList.FullSpan.Contains(span)) ? GetOrAddModel(memberDecl) : null;
}

Expand Down 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 @@ -2400,70 +2395,73 @@ internal override Func<SyntaxNode, bool> GetSyntaxNodesToAnalyzeFilter(SyntaxNod
break;

case RecordDeclarationSyntax recordDeclaration when TryGetSynthesizedRecordConstructor(recordDeclaration) is SynthesizedRecordConstructor ctor:
switch (declaredSymbol.Kind)
if (recordDeclaration.IsKind(SyntaxKind.RecordDeclaration))
{
case SymbolKind.Method:
Debug.Assert((object)declaredSymbol.GetSymbol() == (object)ctor);
return (node) =>
{
// Accept only nodes that either match, or above/below of a 'parameter list'/'base arguments list'.
if (node.Parent == recordDeclaration)
{
return node == recordDeclaration.ParameterList || node == recordDeclaration.BaseList;
}
else if (node.Parent is BaseListSyntax baseList)
{
return node == recordDeclaration.PrimaryConstructorBaseType;
}
else if (node.Parent is PrimaryConstructorBaseTypeSyntax baseType && baseType == recordDeclaration.PrimaryConstructorBaseType)
switch (declaredSymbol.Kind)
{
case SymbolKind.Method:
Debug.Assert((object)declaredSymbol.GetSymbol() == (object)ctor);
return (node) =>
{
return node == baseType.ArgumentList;
}

return true;
};

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) => node != recordDeclaration.ParameterList &&
!(node.Kind() == SyntaxKind.ArgumentList && node == recordDeclaration.PrimaryConstructorBaseType?.ArgumentList);

default:
ExceptionUtilities.UnexpectedValue(declaredSymbol.Kind);
break;
// Accept only nodes that either match, or above/below of a 'parameter list'/'base arguments list'.
if (node.Parent == recordDeclaration)
{
return node == recordDeclaration.ParameterList || node == recordDeclaration.BaseList;
}
else if (node.Parent is BaseListSyntax baseList)
{
return node == recordDeclaration.PrimaryConstructorBaseTypeIfClass;
}
else if (node.Parent is PrimaryConstructorBaseTypeSyntax baseType && baseType == recordDeclaration.PrimaryConstructorBaseTypeIfClass)
{
return node == baseType.ArgumentList;
}

return true;
};

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) => node != recordDeclaration.ParameterList &&
!(node.Kind() == SyntaxKind.ArgumentList && node == recordDeclaration.PrimaryConstructorBaseTypeIfClass?.ArgumentList);

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

case RecordStructDeclarationSyntax recordStructDeclaration when TryGetSynthesizedRecordConstructor(recordStructDeclaration) is SynthesizedRecordConstructor ctor:
switch (declaredSymbol.Kind)
else
{
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)
switch (declaredSymbol.Kind)
{
case SymbolKind.Method:
Debug.Assert((object)declaredSymbol.GetSymbol() == (object)ctor);
return (node) =>
{
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;
// Accept only nodes that either match, or above/below of a 'parameter list'.
if (node.Parent == recordDeclaration)
{
return node == recordDeclaration.ParameterList;
}

return true;
};

case SymbolKind.NamedType:
Debug.Assert((object)declaredSymbol.GetSymbol() == (object)ctor.ContainingSymbol);
// Accept nodes that do not match a 'parameter list'.
return (node) => node != recordDeclaration.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:
when recordDeclaration.PrimaryConstructorBaseTypeIfClass == declaredNode && TryGetSynthesizedRecordConstructor(recordDeclaration) is SynthesizedRecordConstructor ctor:
if ((object)declaredSymbol.GetSymbol() == (object)ctor)
{
// Only 'base arguments list' or nodes below it
Expand All @@ -2474,10 +2472,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
Loading