Update source generator to pass cancellation tokens to compiler phases and passes by DustinCampbell · Pull Request #12269 · dotnet/razor

DustinCampbell · 2025-09-25T17:01:10Z

Key changes:

Pass cancellation tokens in the source generator when executing compiler phases.
Compiler phases made up of multiple passes propagate cancellation tokens to the passes.
Pass a cancellation token to DocumentWriter.WriteDocument(...) and call ThrowIfCancellationRequested in its visitor.
Pass a cancellation token to the TagHelperParseTreeRewriter and call ThrowIfCancellationRequested in its visitor.
Pass a cancellation token to the DirectiveVisitor in the tag helper context discovery phase to call ThrowIfCancellationRequested.
Pass a cancellation token into CSharpCodeParser and HtmlMarkupParser and call ThrowIfCancellationRequested at a few places during parsing.
Update both IRazorSyntaxTreePass implementations (DefaultDirectiveSyntaxTreePass and HtmlNodeOpitmizationPass) to pass cancellation tokens into their visitors and call ThrowIfCancellationRequested.

Other changes:

Change some methods that take a RazorCodeDocument and DocumentIntermediateNode to just take the code document and retrieve the node from the code document. This avoids a potential pitfall where a the RazorCodeDocument and DocumentIntermediateNode passed in aren't related. (This happens a fair amount in tests.) I didn't enforce this everywhere -- it's an ongoing effort.
Clean up the DirectiveVisitor and its two sub-types for legacy tag helpers and components to clarify terminology and share more code.
Rationalize and simplify the CodeTarget system a bit.
Merge DocumentWriter and DefaultDocumentWriter.
Don't use ImmutableHashSet and ImmutableDictionary in the CSharpCodeParser for pure look-up scenarios.
Avoid boxing ImmutableArray<DirectiveDescriptor> to IEnumerable<DirectiveDescriptor> in CSharpCodeParser's constructor.
Avoid LINQ in the TokenizerBackParser.
Additional clean up and small tweaks.

Tip

There are lots of mechanical test updates in this pull request that can largely be skimmed or skipped over.

CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2802919&view=results
Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/674371
Toolset Run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2802921&view=results

Rationalize and clean up CodeTarget, DefaultCodeTarget, ComponentCodeTarget, CodeTargetBuilder, DefaultCodeTargetBuilder, and related classes and tests. - Enable nullability - Pull RazorCodeGenerationOptions from RazorCodeDocument.Options - Push functionality into CodeTarget and share between DefaultCodeTarget and ComponentCodeTarget - Use ImmutableArray<ICodeTargetExtension>.Builder rather than ICollection<ICodeTargetExtension> - Clean up DocumentClassifierPassBase a bit - Update tests

- Merge DocumentWriter and DefaultDocumentWriter - Make DocumentWriter a static class with a single static WriteDocument method. - Always retrieve RazorCodeGenerationOptions from the RazorCodeDocument - Change DocumentWriter to only take just a RazorCodeDocument and acquire the DocumentIntermediateNode from it. This avoids the potential pitfall of a RazorCodeDocument and a DocumentIntermediateNode are passed that don't correspond with one another. - Update tests

- Add ThrowIfCancellationRequested() in each Visit* method - Use BuildMethodDeclaration extension method in visitor, and update that method to take MethodParameter instead of a tuple. - Pass CancellationToken from DefaultRazorCSharpLoweringPhase

…yPhase Pass the CancellationToken into the DirectiveVisitor and call ThrowIfCancellationRequested at various points. Note: This change indulges in some long-overdue refactoring of DirectiveVisitor, TagHelperDirectiveVisitor, and ComponentDirectiveVisitor. More data has been push into DirectiveVisitor, and some naming and terminology has been updated to clarify between components and legacy tag helpers.

- Enable nullability throughout - Update IRazorSyntaxTreePass, DefaultDirectiveSyntaxTreePass, and HtmlNodeOptimizationPass to take CancellationTokens

CSharpCodeParser takes an IEnumerable<DirectiveDescriptor>, but it should take an ImmutableArray<DirectiveDescriptor> to avoid boxing RazorParserOptions.Directives. This requires mechanical changes in compiler many tests.

These two collection types have O(log N) look-up instead of O(1) and are best used in scenarios that take advantage of them as persistent data structures. However, that's not really the case here, since they're built up in the constructor and never modified during the lifetime of CSharpCodeParser. This change removes the immutable data structions in favor of Dictionary<TKey, TValue> and introduces a simple KeywordSet abstraction that wraps a FrozenSet<string> or HashSet<string>.

- Remove LINQ code - Use static lambdas to remove closures - Use PooledArrayBuilder<SyntaxToken> rather than a pooled List<SyntaxToken>

- Add CancellationToken to ParserContext - Add CancellationToken to RazorSyntaxTree.Parse - Call CancellationToken.ThrowIfCancellationRequested in HtmlMarkupParser and CSharpCodeParser.

- Pass CancellationToken to RazorProjectEngine.Process when generating decl files - Pass CancellationToken when executing compiler phases. - Don't box ImmutableArray<RazorSourceDocument> unnecessarily when calling GetDeclarationProjectEngine on GetGenerationProjectEngine

ToddGrun · 2025-09-25T21:28:01Z

src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/DocumentClassifierPassBase.cs


-    protected IReadOnlyList<ICodeTargetExtension> TargetExtensions { get; private set; }
+    protected ImmutableArray<ICodeTargetExtension> TargetExtensions
+        => _targetExtensions.NullToEmpty();


=> _targetExtensions.NullToEmpty();

nit: could just initialize to empty and then not worry about null

I suppose. It's really just for sanity. This property shouldn't be accessed before OnInitialized is called.

ToddGrun · 2025-09-25T21:29:50Z

...r/Microsoft.AspNetCore.Razor.Language/test/Components/ComponentDocumentClassifierPassTest.cs


        // Act
-        processor.ExecutePass<ComponentDocumentClassifierPass>(() => new(Version));
+        processor.ExecutePass<ComponentDocumentClassifierPass>();


processor.ExecutePass();

Is the Version property necessary anymore?

Yes, it's overridden from the base RazorProjectEngineTestBase class where it's used to construct the RazorConfiguration used to construct RazorProjectEngine instances in this test infrastructure.

ToddGrun · 2025-09-25T22:43:31Z

...iler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/Legacy/TagHelperParseTreeRewriter.cs

+        private readonly Stack<TagTracker> _trackerStack = new();
+        private readonly ErrorSink _errorSink = errorSink;
+        private readonly RazorParserOptions _options = options;
+        private readonly ISet<TagHelperDescriptor> _usedDescriptors = usedDescriptors ?? new HashSet<TagHelperDescriptor>();


usedDescriptors ??

Out of curiosity, why allocate and populate if it's not going to be used by the caller?

I didn't bother with extra null checks because production code always passes an instance. Tests always ignored the result when it was an out parameter, so I refactored it to not require an instance but didn't change how the code functions.

chsienki · 2025-09-25T22:42:31Z

...Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/DocumentWriter.cs

+internal static class DocumentWriter
 {
-    public static DocumentWriter CreateDefault(CodeTarget codeTarget, RazorCodeGenerationOptions options)
+    public static RazorCSharpDocument WriteDocument(RazorCodeDocument codeDocument)


Nit: having a static class with a single static method is a smell to me. Could this just be a method on RazorCodeDocument instead?

I agree that the static class with a single static method is small, but I think making it an instance method on RazorCodeDocument sort of breaks the contract between the compiler phases.

Typically, the compiler phases produce some data and add it to the RazorCodeDocument that's being processed. If this became an instance member on RazorCodeDocument, it'd be a bit odd because the contract of is that RazorCodeDocument.GetCSharpDocument() returns null until all of the necessary phases have already run. In order to maintain that contract, the DefaultRazorCSharpLoweringPhase would now do something like this:

var csharpDocument = codeDocument.WriteDocument(cancellationToken); codeDocument.SetCSharpDocument(csharpDocument);

It seems fundamentally strange that there would be a method to produce a new RazorCSharpDocument every time it's called on RazorCodeDocument. Instead, I would expect the contract to be that RazorCodeDocument.GetCSharpDocument() would check to see if there's already a cached RazorCSharpDocument and then write the document if there isn't. However, that could be called before the phases and passes have that are necessary for producing a correct RazorCSharpDocument.

I've rambled a bit (😄 ), but I think the right place to put this method is on DefaultRazoCSharpLoweringPhase itself. That brings in more in line with other phases.

chsienki · 2025-09-25T22:44:52Z

...Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/DocumentWriter.cs

@@ -70,11 +76,9 @@ public override void VisitDocument(DocumentIntermediateNode node)
                else
                {
                    // CodeQL [SM02196] This is supported by the underlying Roslyn APIs and as consumers we must also support it.


This looks like a CodeQL suppression message, is moving it going to break the suppression?

Not that I'm aware of. Is there something about CodeQL that links this comment to the file?

...osoft.CodeAnalysis.Razor.Compiler/src/Language/DefaultRazorTagHelperContextDiscoveryPhase.cs

chsienki · 2025-09-25T22:57:13Z

src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/Legacy/KeywordSet.cs

+
+namespace Microsoft.AspNetCore.Razor.Language.Legacy;
+
+internal abstract class KeywordSet


Nit: I'm fine with this if you think there is value in it. But HashSet<T> and FrozenSet<T> both implement ISet<T> so you could just use that instead?

You're right. I went a little overboard with this 😄

FWIW, I recall that I had wanted to keep the contract a little stronger since ISet<T> has methods for mutation that will mutate in the HashSet<T> case but throw in the FrozenSet<T> case. Since we only need to expose the Contains method, I felt like weakening the contract to ISet<T> was a little too permissive. I wish we could use IReadOnlySet<T>, but that's .NET-only and we still have netstandard2.0. Sigh...

I think I can get the best of both worlds by wrapping ISet<T> or ICollection<T> and making KeywordSet a struct. Is that a happy medium?

...osoft.CodeAnalysis.Razor.Compiler/src/Language/DefaultRazorTagHelperContextDiscoveryPhase.cs

ToddGrun · 2025-09-26T16:12:21Z

...Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/DocumentWriter.cs

+
+        public override void VisitFieldDeclaration(FieldDeclarationIntermediateNode node)
+        {
+            _cancellationToken.ThrowIfCancellationRequested();


_cancellationToken.ThrowIfCancellationRequested();

All these feel a bit too much too, can we just stop checking these at the method level?

You mean in VisitMethodDeclaration? Doesn't most Razor code generation occur within a couple of large methods? Wouldn't we want to cancel within those?

Yep, I was hoping we could just trim the checks at VisitMethodDeclaration.

I guess I just feel that although checking CTs is cheap, doing so this often adds a lot of code, and might end up costing more than it saves if the tokens aren't frequently cancelled.

My expectation is that checking CTs is far cheaper than what each NodeWriter.Write* call actually does. I do like putting the CT checks here because it means that they don't have to be littered all over the IntermediateNodeWriters.

ToddGrun

- Change KeywordSet to a struct - Set capacities of various hash sets and dictionaries in CSharpCodeParser.

This method doesn't really hold its own weight to be on a separate static class and its only used by DefaultRazorCharpLoweringPhase. So, move it there. In addition, move all tests from DocumentWriterTest to DefaultRazorCSharpLoweringPhaseTest.

DustinCampbell · 2025-09-26T20:59:59Z

@ToddGrun, @chsienki: I pushed a few commits to address your feedback and made some additional changes to CSharpCodeParser. Let me know if you have any concerns with the new changes.

DustinCampbell · 2025-09-26T21:34:56Z

I updated the CI Build, Test Insertion, and Toolset Run links above with the latest changes.

DustinCampbell added 13 commits September 25, 2025 08:03

Pass CancellationToken to DocumentWriter

147a418

- Add ThrowIfCancellationRequested() in each Visit* method - Use BuildMethodDeclaration extension method in visitor, and update that method to take MethodParameter instead of a tuple. - Pass CancellationToken from DefaultRazorCSharpLoweringPhase

Thread CancellationToken through DefaultRazorTagHelperRewritePhase

49d4950

Thread CancellationToken through DefaultRazorSyntaxTreePhase

1d52b9f

- Enable nullability throughout - Update IRazorSyntaxTreePass, DefaultDirectiveSyntaxTreePass, and HtmlNodeOptimizationPass to take CancellationTokens

Pass ImmutableArray to CSharpCodeParser rather than IEnumerable

d653712

CSharpCodeParser takes an IEnumerable<DirectiveDescriptor>, but it should take an ImmutableArray<DirectiveDescriptor> to avoid boxing RazorParserOptions.Directives. This requires mechanical changes in compiler many tests.

Clean up and improve TokenizerBackedParser and HtmlMarkupParser a bit

621258e

- Remove LINQ code - Use static lambdas to remove closures - Use PooledArrayBuilder<SyntaxToken> rather than a pooled List<SyntaxToken>

Thread CancellationToken through DefaultRazorParsingPhase

190770f

- Add CancellationToken to ParserContext - Add CancellationToken to RazorSyntaxTree.Parse - Call CancellationToken.ThrowIfCancellationRequested in HtmlMarkupParser and CSharpCodeParser.

Pass CancellationToken into all passes

1ca30a1

Compiler phases call ThrowIfCancellationRequested in-between passes

d28200e

DustinCampbell requested a review from a team as a code owner September 25, 2025 17:01

DustinCampbell requested review from ToddGrun and chsienki September 25, 2025 17:03

ToddGrun reviewed Sep 25, 2025

View reviewed changes

chsienki approved these changes Sep 25, 2025

View reviewed changes

ToddGrun reviewed Sep 26, 2025

View reviewed changes

...osoft.CodeAnalysis.Razor.Compiler/src/Language/DefaultRazorTagHelperContextDiscoveryPhase.cs Outdated Show resolved Hide resolved

ToddGrun reviewed Sep 26, 2025

View reviewed changes

ToddGrun approved these changes Sep 26, 2025

View reviewed changes

DustinCampbell added 3 commits September 26, 2025 10:10

CR feedback and setting optimal capacities in CSharpCodeParser

d90acc5

- Change KeywordSet to a struct - Set capacities of various hash sets and dictionaries in CSharpCodeParser.

CR Feedback: Move CancellationToken checks outside of fast loops

0a4e035

chsienki approved these changes Sep 26, 2025

View reviewed changes

ToddGrun approved these changes Sep 26, 2025

View reviewed changes

DustinCampbell merged commit 8f21442 into dotnet:main Sep 30, 2025
11 checks passed

DustinCampbell deleted the phase-cancellation branch September 30, 2025 15:31

dotnet-policy-service bot added this to the Next milestone Sep 30, 2025

davidwengier mentioned this pull request Oct 6, 2025

Bump Razor to 10.0.0-preview.25503.1 dotnet/vscode-csharp#8679

Merged

dotnet-maestro bot mentioned this pull request Oct 21, 2025

[release/10.0.2xx] Source code updates from dotnet/razor dotnet/dotnet#3012

Merged

davidwengier modified the milestones: Next, 18.3 Jan 6, 2026


		namespace Microsoft.AspNetCore.Razor.Language.Legacy;

		internal abstract class KeywordSet

Conversation

DustinCampbell commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DustinCampbell Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DustinCampbell Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DustinCampbell Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ToddGrun left a comment

Choose a reason for hiding this comment

Uh oh!

DustinCampbell commented Sep 26, 2025

Uh oh!

DustinCampbell commented Sep 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DustinCampbell commented Sep 25, 2025 •

edited

Loading

DustinCampbell Sep 25, 2025 •

edited

Loading

DustinCampbell Sep 26, 2025 •

edited

Loading

DustinCampbell Sep 26, 2025 •

edited

Loading