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

Remove special string node type #59008

Merged
merged 8 commits into from
Jan 24, 2022
Merged

Remove special string node type #59008

merged 8 commits into from
Jan 24, 2022

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Jan 22, 2022 •
edited by jcouv
Loading

It occurred to me in the meeting today that we don't actually need/want a special string string literal expression. Rather the singel string expression could have multiple kinds of tokens underneath it, all representing a constant string literal.

Relates to test plan #55306

@CyrusNajmabadi CyrusNajmabadi requested review from a team as code owners January 22, 2022 00:35
@CyrusNajmabadi CyrusNajmabadi changed the base branch from main to features/RawStringLiterals January 22, 2022 00:35
jcouv
jcouv reviewed Jan 22, 2022
View reviewed changes
src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt
@@ -33,9 +33,7 @@ Microsoft.CodeAnalysis.CSharp.SyntaxKind.InterpolatedMultiLineRawStringEndToken
Microsoft.CodeAnalysis.CSharp.SyntaxKind.InterpolatedMultiLineRawStringStartToken = 9082 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
Microsoft.CodeAnalysis.CSharp.SyntaxKind.InterpolatedSingleLineRawStringEndToken = 9081 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
Microsoft.CodeAnalysis.CSharp.SyntaxKind.InterpolatedSingleLineRawStringStartToken = 9080 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
Microsoft.CodeAnalysis.CSharp.SyntaxKind.MultiLineRawStringLiteralExpression = 9075 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
Copy link
Member

@jcouv jcouv Jan 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Consider resolving PROTOTYPE marker and updating compiler test plan while you're making a change #Closed

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, will probably need another PR anyways. I updated the test plan based on yesterday's feature review:
#55306

jcouv
jcouv approved these changes Jan 22, 2022
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 8)

@runfoapp runfoapp bot mentioned this pull request Jan 24, 2022
Closed
@jcouv
Copy link
Member

jcouv commented Jan 24, 2022

@chsienki @dotnet/roslyn-compiler for second review (relates to raw string literals). Thanks

@cston
Copy link
Member

cston commented Jan 24, 2022

Has the public API gone through API review?

@jcouv
Copy link
Member

jcouv commented Jan 24, 2022

We discussed API review during the feature review. Cyrus is going to start the process.

cston
cston reviewed Jan 24, 2022
View reviewed changes
src/Compilers/CSharp/Portable/Syntax/LiteralExpressionSyntax.cs
public static LiteralExpressionSyntax LiteralExpression(SyntaxKind kind)
=> SyntaxFactory.LiteralExpression(kind, SyntaxFactory.Token(GetLiteralExpressionTokenKind(kind)));

private static SyntaxKind GetLiteralExpressionTokenKind(SyntaxKind kind)
Copy link
Member

Choose a reason for hiding this comment

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

GetLiteralExpressionTokenKind

Consider asserting:

var result = kind switch { ... };
Debug.Assert(kind == SyntaxKindFacts.GetLiteralExpression(result));
return result;

RikkiGibson
RikkiGibson approved these changes Jan 24, 2022
View reviewed changes
Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM aside from one question

src/Compilers/CSharp/Portable/Syntax/LiteralExpressionSyntax.cs
public partial class SyntaxFactory
{
/// <summary>Creates a new LiteralExpressionSyntax instance.</summary>
public static LiteralExpressionSyntax LiteralExpression(SyntaxKind kind)
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't understand why these methods moved out of the generated file. Could you please clarify?

Copy link
Member Author

Choose a reason for hiding this comment

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

they're not getting generated autoamtically as there isn't a 1:1 correspondance with node-type and node-kind. note: we will discuss this at the API review. I personally think we don't need/want the 1:1 correspondence.

@CyrusNajmabadi CyrusNajmabadi merged commit dc1b6a7 into dotnet:features/RawStringLiterals Jan 24, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the removeSpecialStringNodeType branch January 24, 2022 22:14
@jcouv
Copy link
Member

jcouv commented Jan 24, 2022

@cston The issue tracking public API review is here: #59046

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cston cston cston left review comments

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@jcouv jcouv jcouv approved these changes

@allisonchou allisonchou Awaiting requested review from allisonchou

Assignees
No one assigned
Labels
Area-Compilers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@CyrusNajmabadi @jcouv @cston @RikkiGibson