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.

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

SyntaxTokenParser API Additions #73002

Closed
333fred opened this issue Apr 12, 2024 · 4 comments
Closed

SyntaxTokenParser API Additions #73002

333fred opened this issue Apr 12, 2024 · 4 comments
Labels
api-approved API was approved in API review, it can be implemented Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request untriaged Issues and PRs which have not yet been triaged by a lead

Comments

@333fred
Copy link
Member

333fred commented Apr 12, 2024

Background and Motivation

Followup to #72518 with a couple of more APIs for some edge cases in Razor. The motivations here are the same, we just need a bit more flexibility around trivia and error recovery than is currently exposed. There are two main changes:

  • We'd like the ability to explicitly parse just trivia, rather than the entire token. There are some edge cases that require us to call Reset to get before a token, but we will want all the trivia from before that token handled correctly (particularly any directives). There's no way of doing this with the current API; adding the ability to call ParseLeadingTrivia will give us the necessary granularity. Technically, Razor doesn't need the ParseTrailingTrivia API, and it could be skipped if we don't want to commit to it, but I added it for completeness.
  • Currently, we added some error recovery for @: to treat it like a single-line comment. Unfortunately, there's some edge cases that make this unworkable, mainly when we need to parse C# from somewhere else later on that same line. To handle this, we add a dedicated token type for bad razor content, BadRazorContentToken, which will cover this case. We don't use the existing BadToken type here because BadToken is a bit special; it can cause parsing loops to decide that no further progress can be made and that they should bail upwards to let a containing scope handle the bad tokens. We don't want this to happen in regular C# parsing, so we introduce a specific token kind that will mainly just get converted into a SkippedTokenTrivia by most parsing loops without causing them to exit.
  • Before we ship this API, I would like to propose making it experimental, just in case we need to adjust in 17.11. The goal would be to mark it shipped shortly after we ship the razor parsing improvements and know the API is solid.

Proposed API

namespace Microsoft.CodeAnalysis.CSharp;
+ [Experimental("RSEXPERIMENTAL003")]
public sealed class SyntaxTokenParser : IDisposable
{
+     /// <summary>
+    /// Parse the leading trivia of the next token from the input at the current position. This will advance the internal position of the
+    /// token parser to the end of the leading trivia of the next token. The returned result will have a token with <see cref="CSharpExtensions.Kind(SyntaxToken)"/>
+    /// of <see cref="SyntaxKind.None"/>, <see cref="SyntaxToken.IsMissing"/> set to <see langword="true"/>, and a parent of <see langword="null"/>. The
+    /// parsed trivia will be set as the <see cref="SyntaxToken.LeadingTrivia"/> of the token.
+    /// </summary>
+    public Result ParseLeadingTrivia();

+    /// <summary>
+    /// Parse syntax trivia from the current position, according the rules of trailing syntax trivia. This will advance the internal position of the
+    /// token parser to the end of the trailing trivia from the current location. The returned result will have a token with <see cref="CSharpExtensions.Kind(SyntaxToken)"/>
+    /// of <see cref="SyntaxKind.None"/>, <see cref="SyntaxToken.IsMissing"/> set to <see langword="true"/>, and a parent of <see langword="null"/>. The
+    /// parsed trivia will be set as the <see cref="SyntaxToken.TrailingTrivia"/> of the token.
+    /// </summary>
+    public Result ParseTrailingTrivia();
}

public enum SyntaxKind : ushort
{
+    BadRazorContentToken = 8523,
}

public static class SyntaxFactory
{
+       [Experimental(RoslynExperiments.SyntaxTokenParser, UrlFormat = RoslynExperiments.SyntaxTokenParser_Url)]
        public static SyntaxTokenParser CreateTokenParser(SourceText sourceText, CSharpParseOptions? options = null)
}

Usage Examples

var result = parser.ParseNextToken();

if (result.Token.IsKind(SyntaxKind.BadRazorContentToken)
{
    // Transition to HTML
    parser.Reset(result);
    // Ensure that any directives before the `@:` are handled correctly
    result = parser.ParseLeadingTrivia();
    this.HandleTrivia(result.Token);
    OtherParser.OpenExpression();
}

Alternative Designs

None

Risks

We do have to add a dedicated token type here, which isn't the cleanest design, but we don't have another token type to use here that won't have potential negative effects on the actual C# parser.

@333fred 333fred added Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request labels Apr 12, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 12, 2024
@333fred 333fred added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Apr 13, 2024
@333fred 333fred added the blocking API needs to reviewed with priority to unblock work label May 1, 2024
@333fred
Copy link
Member Author

333fred commented May 9, 2024

API Review

  • Do we have a concern that SyntaxKind.None and IsMissing would cause code to misbehave?
    • This is a fairly specialized API, unlikely to be used in existing codepaths unintentionally.

Conclusion: Approved

@333fred 333fred closed this as completed May 9, 2024
@333fred 333fred added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation blocking API needs to reviewed with priority to unblock work labels May 9, 2024
@huoyaoyuan
Copy link
Member

This one is listed as "Rejected" in caption of apireview, but "Approved" in conclusion: https://github.com/dotnet/apireviews/tree/main/2024/05-09-roslyn-reviews#syntaxtokenparser-api-additions

Can you please double check and correct it?

@333fred
Copy link
Member Author

333fred commented May 11, 2024

@terrajobst any idea why that would have happened?

@terrajobst
Copy link
Member

terrajobst commented Jun 26, 2024

The website was authored before GitHub had "closed as not planned" vs "closed as completed". Also, I don't believe Octokit.NET exposes this today.

The assumption was that if an item is closed during the review timeframe it's considered a rejection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

No branches or pull requests

3 participants