-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Support generating records in syntax generator #67337
Support generating records in syntax generator #67337
Conversation
@@ -4,6 +4,7 @@ | |||
|
|||
using System.Diagnostics; | |||
using Microsoft.CodeAnalysis.CSharp.Syntax; | |||
using Roslyn.Utilities; |
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.
using Roslyn.Utilities; |
@@ -32,15 +33,19 @@ public partial class SyntaxFactory | |||
} | |||
|
|||
public static RecordDeclarationSyntax RecordDeclaration(SyntaxList<AttributeListSyntax> attributeLists, SyntaxTokenList modifiers, SyntaxToken keyword, SyntaxToken identifier, | |||
TypeParameterListSyntax typeParameterList, ParameterListSyntax parameterList, BaseListSyntax baseList, SyntaxList<TypeParameterConstraintClauseSyntax> constraintClauses, SyntaxList<MemberDeclarationSyntax> members) | |||
TypeParameterListSyntax? typeParameterList, ParameterListSyntax? parameterList, BaseListSyntax? baseList, SyntaxList<TypeParameterConstraintClauseSyntax> constraintClauses, SyntaxList<MemberDeclarationSyntax> members) | |||
{ |
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 was a bridge method for back compat. But actually trying to use it produces terrible results. For example, if you specify any members, you get: record C public string ToString();
(without braces).
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.
Consider adding a test to SyntaxFactoryTests.cs.
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.
will do. thanks :)
@@ -32,15 +33,19 @@ public partial class SyntaxFactory | |||
} | |||
|
|||
public static RecordDeclarationSyntax RecordDeclaration(SyntaxList<AttributeListSyntax> attributeLists, SyntaxTokenList modifiers, SyntaxToken keyword, SyntaxToken identifier, | |||
TypeParameterListSyntax typeParameterList, ParameterListSyntax parameterList, BaseListSyntax baseList, SyntaxList<TypeParameterConstraintClauseSyntax> constraintClauses, SyntaxList<MemberDeclarationSyntax> members) | |||
TypeParameterListSyntax? typeParameterList, ParameterListSyntax? parameterList, BaseListSyntax? baseList, SyntaxList<TypeParameterConstraintClauseSyntax> constraintClauses, SyntaxList<MemberDeclarationSyntax> members) |
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.
the api change is just making these three members optional. Obviously, and correctly, the type-params/param-list and base list of a record are all optional. This also matches all the other overloads in this file. This appears to just be an errant mistake that does not impact anyone to fix.
@333fred for confirmation.
@roslyn-compiler @dotnet/roslyn-ide ptal. |
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.
Compiler change LGTM (commit 3)
@jcouv ptal :) |
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.
Changes under Compilers (LGTM). Just a reminder that we ask to squash commits while merging.
Thanks for the reminder! :) |
Direct syntax factory tests added. |
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.
Compiler changes LGTM.
/azp run |
Azure Pipelines successfully started running 4 pipeline(s). |
Fixes #67335