Compiler: (Finally!) Remove ItemCollection by DustinCampbell · Pull Request #11939 · dotnet/razor

DustinCampbell · 2025-06-11T20:09:16Z

ItemCollection is essentially a property bag that results in dictionaries being used instead of plain ol' fields, and it's been overused in Razor. I've been trying to get rid of it for nearly a year. #10720, #10764, #11360, and #11931 each represent changes that remove a use of ItemCollection. This pull request finally gets rid of ItemCollection completely.

The last use of ItemCollection is the RazorCodeDocument.Items, which is used by the compiler to store various objects during compilation, and tooling uses it to cache some expensive-to-create data. This change stores all of the compiler objects on RazorCodeDocument directly. The data cached by tooling is stored on the side in a ConditionalWeakTable.

As part of this, I've refactoring the RazorCodeDocument.TryComputeNamespace extension method to be more understandable and maintainable.

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

DustinCampbell · 2025-06-11T21:27:12Z

~~There's an AspNetCoreMvc test failing with this change: Microsoft.AspNetCore.Mvc.FunctionalTests.RazorPagesNamespaceTest.Page_ImportedNamespace_UsedFromViewImports(). Will fix soon!~~

Fixed now.

davidwengier

Tooling looks good. PR title brings nothing but joy :D

Store the RazorSyntaxTree produced before the tag helper rewrite as a field on RazorCodeDocument instead of the ItemCollection.

Store the IReadOnlyList<TagHelperDescriptors> as a field on RazorCodeDocument instead of the ItemCollection.

Store the ISet<TagHelperDescriptors> as a field on RazorCodeDocument instead of the ItemCollection.

Store the RazorSyntaxTree as a field on RazorCodeDocument instead of the ItemCollection.

Store the import RazorSyntaxTrees as a field on RazorCodeDocument instead of the ItemCollection.

Store the DocumentIntermediateNode as a field on RazorCodeDocument instead of the ItemCollection.

Store the RazorCSharpDocument as a field on RazorCodeDocument instead of the ItemCollection.

Store CssScope on RazonCodeGenerationOptions instead of the RazorCodeDocument's ItemCollection.

This is just a bit of clean up. The code generation options do not need to be stored on RazorCSharpDocument or RazorHtmlDocument, since they're already stored on RazorCodeDocument.

Avoid creating a new NamespaceDirectiveVisitor for each RazorSyntaxTree

Store the computed namespace name and span on RazorCodeDocument instead of the ItemCollection.

Store the RazorHtmlDocument as a field on RazorCodeDocument instead of the ItemCollection.

Tooling stashes a couple of items in RazorCodeDocument.Items. Employ a ConditionalWeakTable to avoid this.

At long last, ItemCollection is now deleted.

Introduce PropertyTable, which wraps an array of values and exposes each slot as a property. Properties can be reference types or value types. For value types, a StrongBox<T> is used in the array to hold the value.

@namespace

When searching imports for @namespace directives, we need to iterate them in reverse order to ensure that we prefer imports closer to the source document.

ToddGrun · 2025-06-12T16:12:30Z

-            return syntaxTree.Root is RazorSyntaxNode root
-                ? root.FindInnermostNode(absoluteIndex)
-                : null;
+            return codeDocument.GetRequiredSyntaxRoot().FindInnermostNode(absoluteIndex);


GetRequiredSyntaxRoot

nit: used to not throw if tree.Root was null.

Yes, but this was wrong and unnecessarily paranoid, so I changed it here. RazorSyntaxTree.Root is annotated as non-null and RazorSyntaxTree's constructor throws if root is null:

razor/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/RazorSyntaxTree.cs

Lines 20 to 34 in a81a6da

internal RazorSyntaxTree(

SyntaxNode root,

RazorSourceDocument source,

ImmutableArray<RazorDiagnostic> diagnostics,

RazorParserOptions options)

{

ArgHelper.ThrowIfNull(root);

ArgHelper.ThrowIfNull(source);

ArgHelper.ThrowIfNull(options);

Root = root;

Source = source;

_diagnostics = diagnostics.NullToEmpty();

Options = options;

}

ToddGrun

- Remove the index consts because they don't really add much value, since they're just used once and its clear what they mean. - Make Property<T> reference the _values slot directly on modern .NET with a ref field. - Make BoxedProperty<T>.StrongBox a readonly field. - Add comments

chsienki

Awesome.

DustinCampbell · 2025-06-13T20:49:11Z

Thanks everyone! ♥️

DustinCampbell requested review from a team as code owners June 11, 2025 20:09

davidwengier approved these changes Jun 11, 2025

View reviewed changes

Comment thread ...Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/RazorCodeDocumentExtensions.CachedData.cs Outdated

DustinCampbell changed the base branch from dev/dustinca/intermediate-nodes to main June 12, 2025 13:48

DustinCampbell changed the base branch from main to dev/dustinca/intermediate-nodes June 12, 2025 15:21

DustinCampbell changed the base branch from dev/dustinca/intermediate-nodes to main June 12, 2025 15:21

DustinCampbell added 23 commits June 12, 2025 08:30

Store PreTagHelpersSyntaxTree on RazorCodeDocument

c4da41c

Store the RazorSyntaxTree produced before the tag helper rewrite as a field on RazorCodeDocument instead of the ItemCollection.

Store TagHelpers on RazorCodeDocument

f6b2810

Store the IReadOnlyList<TagHelperDescriptors> as a field on RazorCodeDocument instead of the ItemCollection.

Store ReferencedTagHelpers on RazorCodeDocument

5af0dab

Store the ISet<TagHelperDescriptors> as a field on RazorCodeDocument instead of the ItemCollection.

Store SyntaxTree on RazorCodeDocument

ef1aa54

Store the RazorSyntaxTree as a field on RazorCodeDocument instead of the ItemCollection.

Store ImportSyntaxTrees on RazorCodeDocument

2523551

Store the import RazorSyntaxTrees as a field on RazorCodeDocument instead of the ItemCollection.

Store DocumentIntermediateNode on RazorCodeDocument

4b8bf37

Store the DocumentIntermediateNode as a field on RazorCodeDocument instead of the ItemCollection.

Store RazorCSharpDocument on RazorCodeDocument

aac9d56

Store the RazorCSharpDocument as a field on RazorCodeDocument instead of the ItemCollection.

Store CssScope on RazorCodeGenerationOptions

810e6a1

Store CssScope on RazonCodeGenerationOptions instead of the RazorCodeDocument's ItemCollection.

Remove RazorCodeGenerationOptions from RazorHtml/CSharpDocument

568c59b

This is just a bit of clean up. The code generation options do not need to be stored on RazorCSharpDocument or RazorHtmlDocument, since they're already stored on RazorCodeDocument.

Extract TryComputeNamespace into NamespaceComputer

a0cad22

StringTokenizer: Use ReadOnlySpan<char> for separators

bce5263

NamespaceComputer: Refactor to make logic a bit clearer

be59da2

NamespaceComputer: Introduce NamspaceBuilder helper

766983b

NamespaceComputer: Extract logic to find namespace directive

736c2b0

NamespaceComputer: Add nullability annotations

c517b23

NamespaceComputer: Refactor NamespaceDirectiveVisitor

c477d25

Avoid creating a new NamespaceDirectiveVisitor for each RazorSyntaxTree

Store computed namespace name and span on RazorCodeDocument

0b15454

Store the computed namespace name and span on RazorCodeDocument instead of the ItemCollection.

Store RazorHtmlDocument on RazorCodeDocument

09614cb

Store the RazorHtmlDocument as a field on RazorCodeDocument instead of the ItemCollection.

Use ConditionalWeakTable in tooling to cache RazorCodeDocument data

dd56851

Tooling stashes a couple of items in RazorCodeDocument.Items. Employ a ConditionalWeakTable to avoid this.

Remove RazorCodeDocument.Items and ItemCollection

9dbaaca

At long last, ItemCollection is now deleted.

RazorCodeDocument: Use array rather than fields for properties

ca807e6

Introduce PropertyTable, which wraps an array of values and exposes each slot as a property. Properties can be reference types or value types. For value types, a StrongBox<T> is used in the array to hold the value.

RazorCodeDocument: Various renames for consistency

f2afe17

Add RazorCodeDocument.Clone() method to support the legacy editor

0c9b51a

DustinCampbell added 3 commits June 12, 2025 08:33

Add helper to clone cached tooling data for legacy editor

38f10c2

NamespaceComputer: Process imports in correct order

dc29f2b

When searching imports for @namespace directives, we need to iterate them in reverse order to ensure that we prefer imports closer to the source document.

CR Feedback: Rename method for clarity

5efed97

DustinCampbell force-pushed the remove-itemcollection branch from aa89304 to 5efed97 Compare June 12, 2025 15:34

ToddGrun reviewed Jun 12, 2025

View reviewed changes

Comment thread src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/NamespaceComputer.cs Outdated

CR Feedback: Don't acquire builder if it won't be used.

5f10309

ToddGrun approved these changes Jun 12, 2025

View reviewed changes

chsienki reviewed Jun 12, 2025

View reviewed changes

Comment thread ...mpiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/RazorCodeDocument.PropertyTable.cs Outdated

DustinCampbell added 3 commits June 13, 2025 09:09

Merge branch 'main' into remove-itemcollection

0fa92b8

Make PropertyTable create and own the array it mutates

e9e3e78

chsienki approved these changes Jun 13, 2025

View reviewed changes

333fred approved these changes Jun 13, 2025

View reviewed changes

DustinCampbell merged commit 72bf181 into dotnet:main Jun 13, 2025
11 checks passed

DustinCampbell deleted the remove-itemcollection branch June 13, 2025 20:49

dotnet-policy-service Bot added this to the Next milestone Jun 13, 2025

This was referenced Jun 19, 2025

[Automated] PRs inserted in VS build main-10718.203 #11963

Closed

[Automated] PRs inserted in VS build feature.debugger.main-10720.140 #11967

Closed

[Automated] PRs inserted in VS build feature.vc.slnExplorerSymbolsProviderRewrite-10725.120 #11978

Closed

dotnet-bot mentioned this pull request Jun 29, 2025

[Automated] PRs inserted in VS build feature.Wix5-10728.31 #11990

Closed

RikkiGibson modified the milestones: Next, 18.0 P1 Aug 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Compiler: (Finally!) Remove ItemCollection#11939

Compiler: (Finally!) Remove ItemCollection#11939
DustinCampbell merged 30 commits into
dotnet:mainfrom
DustinCampbell:remove-itemcollection

DustinCampbell commented Jun 11, 2025 •

edited

Loading

Uh oh!

DustinCampbell commented Jun 11, 2025 •

edited

Loading

Uh oh!

davidwengier left a comment

Uh oh!

Uh oh!

ToddGrun Jun 12, 2025

Uh oh!

DustinCampbell Jun 12, 2025

Uh oh!

Uh oh!

ToddGrun left a comment

Uh oh!

Uh oh!

chsienki left a comment

Uh oh!

DustinCampbell commented Jun 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

	internal RazorSyntaxTree(
	SyntaxNode root,
	RazorSourceDocument source,
	ImmutableArray<RazorDiagnostic> diagnostics,
	RazorParserOptions options)
	{
	ArgHelper.ThrowIfNull(root);
	ArgHelper.ThrowIfNull(source);
	ArgHelper.ThrowIfNull(options);

	Root = root;
	Source = source;
	_diagnostics = diagnostics.NullToEmpty();
	Options = options;
	}

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

davidwengier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ToddGrun Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

DustinCampbell Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ToddGrun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chsienki left a comment

Choose a reason for hiding this comment

Uh oh!

DustinCampbell commented Jun 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DustinCampbell commented Jun 11, 2025 •

edited

Loading

DustinCampbell commented Jun 11, 2025 •

edited

Loading