Replace "file kind" strings with an enum #11722

DustinCampbell · 2025-04-08T20:57:51Z

Throughout the compiler and tooling Razor files have a "kind" that is represented as a string. Prior to the Razor API being taken internal, using a string made it theoretically possible for the compiler to be extended to file types other than those that the Razor compiler already knows about. With extensibility no longer a priority, file kinds can be represented as an enum (RazorFileKind) that provides the values that the compiler supports, e.g.. Component, ComponentImport, Legacy. This makes checks for the file kind much cheaper, since they were all string comparisons before.

There are a lot of files changed, but much of the work is mechanical.

Important

This is a breaking change for the RazorSDK. When the Razor compiler flows to the .NET SDK, it will introduce build errors that can be fixed by applying this commit: dotnet/sdk@f33a5e7.

VS Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/626687

Add RazorFileKind enum that provides the allowable values from FileKinds. In addition, include a None value for scenarios where a file kind would be null.

Note: These helper methods will be removed once the code base is converted to RazorFIleKind.

Update RazorProjectItem.FileKind property to return a RazorFIleKind rather than a string. This requires changes to all sub types as well. Note: This will require changes to the RazorSdk, which declares custom RazorProjectItem descendants.

Update RazorIntegrationTestBase.FileKind to return a RazorFileKind and update inherited test classes.

Updates RemoteProjectItem to take a RazorFileKind rather than a string.

Updates NotFoundProjectItem to take a RazorFileKind rather than a string.

Updates SourceGeneratorProjectItem to take a RazorFileKind rather than a string.

Updates DefaultRazorProjectItem to take a RazorFileKind rather than a string.

Updates TestRazorProjectItem to take a RazorFileKind rather than a string.

Updates TestSnapshotProjectItem to take a RazorFileKind rather than a string.

Update RazorToolingIntegrationTestBase.FileKind to return a RazorFileKind and update inherited test classes.

Updates RazorProjectFileSystem.GetItem(...) to take a RazorFileKind rather than a string and update all overrides and callers.

This updates the various RazorProjectEngine Process and CreateCodeDocument methods to take a RazorFileKind rather than a string.

Also includes updates for DocumentSnapshotHandle, which affect serialization.

Note: This change deletes a lot of unused test infrastructure code from ToolingParserTestBase.

This change represents mechanical clean up: - Use collection expressions - Switch from IReadOnlyList<TagHelperDescriptor> to ImmutableArray<TagHelperDescriptor> - Make CreateProvider() helper private protected - Use primary constructor - Mark for nullability - Remove unused parameters - Update static test TagHelperDescriptors to be actual singletons and use fluent builder API.

- Add TryGetFileKindFromPath - Rename FilePathToRazorFileKind to GetFileKindFromPath and implement using TryGetFileKindFromPath. - Remove ComponentFilePathToRazorFileKind

davidwengier

LGTM

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

jjonescz · 2025-04-10T07:42:19Z

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

-            return FileKinds.ComponentImport;
-        }
-        else if (string.Equals(".razor", Path.GetExtension(filePath), StringComparison.OrdinalIgnoreCase))
+        if (string.Equals(LegacyFileExtension, extension, StringComparison.OrdinalIgnoreCase))


I guess this is based on pre-existing code, but it's weird we use OrdinalIgnoreCase in most comparisons here and Ordinal when checking for _Imports.razor.

Totally agree, but I took a look at the history and Ordinal has been used since that check was introduced.

jjonescz · 2025-04-10T07:42:59Z

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

+
+public enum RazorFileKind : byte
+{
+    None = 0,


Do we need both None file kind and nullable RazorFileKind? usages?

We do, unfortunately. None gets surfaced in certain cases, namely, RazorProjectItem instances representing legacy default imports. So, RazorFileKind? is needed in a few places to know whether a RazorFileKind has been provided or not.

I'm not sure I follow. How did it surface previously when the file kind was a string? I imagine (string?)null corresponds to RazorFileKind.None, so we shouldn't need another value (RazorFileKind?)null.

RazorFileKind.None allows us to represent valid "null" cases without forcing RazorFileKind? into our APIs. The same scenarios with string were marked as #nulalble disable, so calling code didn't really have to deal with it. A FileKind property could return null, but it was exceedingly rare, so most accesses never bothered checking for null. However, since RazorFIleKind is an enum, we would need to make FileKind properties return RazorFileKind? -- a nulalble struct -- and every caller would have to deal with it. IMO, this is a good happy medium for now.

This is the one case that's truly problematic:

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

Lines 48 to 51 in 00ef962

public virtual RazorFileKind FileKind

=> FilePath == null

? RazorFileKind.None

: FileKinds.GetFileKindFromPath(FilePath);

I will be following up with another PR to rework RazorProjectItem and make this scenario essentially impossible.

That doesn't work unfortunately. RazorFileKind? is used to indicate that a RazorFileKind has not been provided and it should be calculated from the file path. RazorFileKind.None is not the right signal for that -- at least not right now. RazorFileKind.None is used for the specific case where the RazorFileKind couldn't be computed from the file path.

So yeah, I'm confused how it could have worked before this PR when the only "special value" we had was null - and now we have two (None and null) and use them to indicate different things. Why didn't we need two values previously? I thought this PR just maps from string to an enum and back, i.e., it's a bi-directional mapping. Something like:

(enum <-> string) None <-> null Component <-> "component" Legacy <-> "mvc" Imports <-> "imports"

I definitely understand your confusion, and I tried to avoid this situation. However, the design of RaorProjectItem.FileKind and all of the test helpers that took string? fileKind = null made tis tricky. As I mentioned, RazorFileKind.None is a valid result for RazorProjectItem.FileKind in the case of legacy imports files. Replacing every RazorFileKind? with RazorFileKind.None causes RazorFileKind.None have two meanings:

The case where is RazorFileKind.None is a valid value.

The case where RazorFileKind.None would used to indicate that a RazorFileKind has not yet be specified and still needs to be computed.

I opted to keep RazorFileKind? in place for now to mean the latter case. I fully intend to clean this up with a different change to RazorProjectItem. However, I felt that change was outside the scope of this PR, which already touched a lot of code.

Okay, thanks for the explanation, sorry if I was slow to understand

Not at all! I'm very glad that you quickly spotted the obvious wart in this change. 😄

DustinCampbell added 26 commits April 7, 2025 15:38

Add RazorFileKind enum

24faf00

Add RazorFileKind enum that provides the allowable values from FileKinds. In addition, include a None value for scenarios where a file kind would be null.

FileKinds: Add a handful of helper methods for converting RazorFileKind

62a6751

Note: These helper methods will be removed once the code base is converted to RazorFIleKind.

RazorIntegrationTestBase.FileKind: Return RazorFileKind

cc8b62a

Update RazorIntegrationTestBase.FileKind to return a RazorFileKind and update inherited test classes.

RemoteProjectItem: Pass RazorFIleKind to constructor

216c453

Updates RemoteProjectItem to take a RazorFileKind rather than a string.

NotFoundProjectItem: Pass RazorFileKind to constructor

e33aec1

Updates NotFoundProjectItem to take a RazorFileKind rather than a string.

SourceGeneratorProjectItem: Pass RazorFileKind to constructor

99419bc

Updates SourceGeneratorProjectItem to take a RazorFileKind rather than a string.

DefaultRazorProjectItem: Pass RazorFileKind to constructor

da817af

Updates DefaultRazorProjectItem to take a RazorFileKind rather than a string.

TestRazorProjectItem: Pass RazorFileKind to constructor

332c289

Updates TestRazorProjectItem to take a RazorFileKind rather than a string.

TestSnapshotProjectItem: Pass RazorFileKind to constructor

06e66e8

Updates TestSnapshotProjectItem to take a RazorFileKind rather than a string.

RazorToolingIntegrationTestBase.FileKind: Return RazorFileKind

d5d33dd

Update RazorToolingIntegrationTestBase.FileKind to return a RazorFileKind and update inherited test classes.

RazorProjectFileSystem: Pass RazorFileKind to GetItem

2462253

Updates RazorProjectFileSystem.GetItem(...) to take a RazorFileKind rather than a string and update all overrides and callers.

RazorParserOptions.FileKind: Return RazorFileKind

053e3ca

RazorProjectEngine: Use RazorFileKind rather than string

6744afc

This updates the various RazorProjectEngine Process and CreateCodeDocument methods to take a RazorFileKind rather than a string.

RazorCodeDocument.FileKind: Return RazorFileKind

95bc98d

Remove a few FileKinds.GetFileKindFromFallPath callers

0f196f8

IntegrationTestBase.CreateProjectItemFromFile: Take RazorFIleKind

24f8929

ParserTestBase.ParseDocumentTest: Take RazorFileKind

45017be

RazorIntegrationTestBase.CreateProjectItem: Take RazorFIleKind

7c309de

HostDocument: Use RazorFileKind rather than string

77084ef

Also includes updates for DocumentSnapshotHandle, which affect serialization.

IDocumentSnapshot.FileKind: Use RazorFileKind rather than string

f22519c

Use RazorFileKind throughout tooling tests

ee6665e

Note: This change deletes a lot of unused test infrastructure code from ToolingParserTestBase.

FileKinds: Remove unused members

cc48729

Convert FileKinds Is<kind> methods to extension methods

610c5a8

Rationalize FileKinds methods

90fb494

- Add TryGetFileKindFromPath - Rename FilePathToRazorFileKind to GetFileKindFromPath and implement using TryGetFileKindFromPath. - Remove ComponentFilePathToRazorFileKind

DustinCampbell requested review from a team as code owners April 8, 2025 20:57

DustinCampbell force-pushed the razorfilekind branch from 583df77 to 90fb494 Compare April 9, 2025 23:40

davidwengier approved these changes Apr 10, 2025

View reviewed changes

jjonescz approved these changes Apr 10, 2025

View reviewed changes

333fred approved these changes Apr 10, 2025

View reviewed changes

DustinCampbell added 2 commits April 10, 2025 11:33

RazorFileKind: Add doc comments

0b043d8

Don't forget to update serialization formats!

2abd127

DustinCampbell force-pushed the razorfilekind branch from 1af82b1 to 2abd127 Compare April 10, 2025 21:02

DustinCampbell merged commit 9abf75c into dotnet:main Apr 10, 2025
11 checks passed

DustinCampbell deleted the razorfilekind branch April 10, 2025 21:38

dotnet-policy-service bot added this to the Next milestone Apr 10, 2025

This was referenced Apr 23, 2025

[Automated] PRs inserted in VS build main-10522.74 #11761

Closed

[Automated] PRs inserted in VS build feature.debugger.main-10524.278 #11778

Closed

[Automated] PRs inserted in VS build feature.debugger.main-10524.256 #11779

Closed

dotnet-bot mentioned this pull request May 31, 2025

[Automated] PRs inserted in VS build feature.telemetry.d18OpenTelemetry-10630.96 #11902

Closed

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

	public virtual RazorFileKind FileKind
	=> FilePath == null
	? RazorFileKind.None
	: FileKinds.GetFileKindFromPath(FilePath);

Replace "file kind" strings with an enum #11722

Replace "file kind" strings with an enum #11722

Uh oh!

Conversation

DustinCampbell commented Apr 8, 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!

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!

jjonescz Apr 11, 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!

DustinCampbell Apr 11, 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!

DustinCampbell Apr 14, 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!

jjonescz Apr 14, 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DustinCampbell commented Apr 8, 2025 •

edited

Loading

jjonescz Apr 11, 2025 •

edited

Loading

DustinCampbell Apr 11, 2025 •

edited

Loading

DustinCampbell Apr 14, 2025 •

edited

Loading

jjonescz Apr 14, 2025 •

edited

Loading