Compiler: Rationalize nodes, tokens, visitors, walkers, and rewriters, oh my! #11853
+835
−828
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SyntaxToken should inherit from SyntaxNode rather than RazorSyntaxNode.
- Add nullability annotations - Make Visit method target RazorSyntaxNode - Handle SyntaxTokens separately
The SyntaxWalker is the only visitor that takes a TextSpan to limit what nodes and tokens are visited. However, that feature was only used by the SemanticTokensVisitor in the tooling layer. This change moves the range checking feature out of SyntaxWalker and into the visitor that needs it.
Update existing SyntaxWalker and SyntaxRewriter descendants for nullability and other API changes.
There are two syntax serializers with slightly different output: one used to implement SyntaxNode.SerializedValue and FindToken unit tests, and one used by legacy syntax tests. This change unifies both syntax serializers by inheriting from the same base class: SyntaxSerializer. The implementation has been greatly simplified to a single SyntaxWalker. In the end there are two simple serializers: 1. SyntaxNode.Serializer - This is protected and nested inside of SyntaxNode. It's used to implement the SerializedValue property for RazorSyntaxNode and SyntaxToken. 2. TestSyntaxSerializer - This is used by all tests and customizes the output of the serializer by overriding a couple of methods.
The SyntaxReplacer needs to override VisitToken to enable ReplaceToken. To do this, I've brought over more of the Roslyn implementation. Also, I've pushed the implementation of methods that call into SyntaxReplacer into RazorSyntaxNode (where they're meaningful) and made them abstract in SyntaxNode. The implementations in SyntaxList and SyntaxToken throw.
The DiagnostSyntaxWalker needs to override and handle VisitToken.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Razor's syntax model doesn't provide as much separation between syntax nodes and tokens as Roslyn's. This is due in part to the fact that Roslyn's model is designed to support two languages, not just one. When Razor's syntax model was adapted from Roslyn's, some mistakes were made when modifying it fit a single language. Currently, the base hierarchy of Razor's syntax model like so:
--- config: class: hideEmptyMembersBox: true --- classDiagram SyntaxNode <|-- SyntaxList SyntaxNode <|-- RazorSyntaxNode RazorSyntaxNode <|-- SyntaxToken RazorSyntaxNode <|-- RazorBlockSyntax RazorSyntaxNode <|-- `...Add Other RazorSyntaxNodes`A glaring mistake in the class hierarchy above is that
SyntaxTokeninherits fromRazorSyntaxNode, which is reserved for Razor non-terminal syntax nodes. This PR changesSyntaxTokento inherit fromSyntaxNodeand moves several methods intended for non-terminals to `RazorSyntaxNode. This creates a much more sensible hierarchy:--- config: class: hideEmptyMembersBox: true --- classDiagram SyntaxNode <|-- SyntaxList SyntaxNode <|-- SyntaxToken SyntaxNode <|-- RazorSyntaxNode RazorSyntaxNode <|-- RazorBlockSyntax RazorSyntaxNode <|-- `...And Other RazorSyntaxNodes`This PR adds much cleaner separation between
SyntaxNode,SyntaxListandSyntaxToken, and updates theSyntaxVisitor,SyntaxWalker, andSyntaxRewriterAPIs to better match Roslyn's. This removes a lot of baggage fromSyntaxToken, making it easier to convert to a struct in the near future.CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2706052&view=results
Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/635607
Toolset Test Run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2706053&view=results