Clean up and rationalize imports in the compiler #11409
Open
Conversation
This file contains 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
This provides better debugger strings for the RazorProjectItems that represent default imports.
Some were sealed, some weren't.
This change makes the IImportProjectFeature interface internal. In addition, the SetImportFeature extension method on RazorProjectEngineBuilder has been moved to the common test library, since it's really intended to be used by tests. This is a public API breaking change, but there are no users of these APIs in dotnet/sdk.
Consolidate the three "TestImportProjectFeature" classes and place it in Microsoft.AspNetCore.Razor.Test.Common.
This change introduces a DefaultImportProjectItem that is shared across features that add default imports.
Add a couple of methods to PooledArrayBuilder to produce a reversed ImmutableArray.
This change rewrites RazorProjectFileSystem.FindHierarchicalItems(...) to avoid allocations. It now returns results in reverse order, since every caller in Razor immediately called `Reverse()`. In addition, the access has been reduced to internal since there are no external callers of this method.
333fred
reviewed
Jan 23, 2025
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/DefaultImportProjectItem.cs
Show resolved
Hide resolved
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/RazorProjectFileSystem.cs
Outdated
Show resolved
Hide resolved
333fred
approved these changes
Jan 23, 2025
chsienki
reviewed
Jan 23, 2025
...Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Mvc.Version1_X/MvcImportProjectFeature.cs
Show resolved
Hide resolved
chsienki
approved these changes
Jan 23, 2025
FindHierarchicalItem has a "max depth" check to ensure that it returns a maximum of 255 file paths. However, all this method does is perform string manipulation by looking for '/' characters. It does not actually touch the file system. Technically, this could result in a breaking change because this method is used to find applicable Imports files. So, if the user had a Razor project with 256+ nested subdirectories, the compiler would now consider Imports that it might not have considered in the past. However, that seems like a highly unlikely situation and it's even more unlikely that a user would depend on that behavior.
333fred
approved these changes
Jan 24, 2025
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.
This change represents a lot of clean up and consolidate a lot of code around how imports are provided and queried for in the compiler.
There are a couple of API breaking changes here that shouldn't have external impact. In particular, I've made
IImportProjectFeatureinternal and changed its API significantly. In addition, I've rewrittenRazorProjectFileSystem.FindHierarchicalItems(...)to reduce allocations, make it more efficient, and return its results in reversed order (since every caller immediately calledReverse()).CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2626377&view=results
Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/605137