Tooling: Don't throw exceptions when generating code for file rooted outside of project #11864
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
Add ReadOnlySpan<T>.StartsWith(...) and EndWith methods that take a single T value. These were added for convenience in .NET 9. Note that these are polyfills that call into the .NET methods on .NET 9 or higher.
Add a couple of methods from System.IO.Path to PathUtilities: - IsPathFullyQualified - Doesn't exist on net472 or netstandard2.0 - IsPathRooted - The overload that takes a ReadOnlySpan<char> (used by IsPathFullyQualfied) doesn't exist on net472 or netstandard2.0
In a prior implementation, DefaultRazorProjectFIleSystem.NormalizeAndEnsureValidPath(...) called Path.Combine(...) which would return the second path if both were rooted. However, that behavior was not carried into the new implementation, resulting in cases where a path can be "double" rooted if it is an absolute path outside of the project root. This change uses PathUtilities.IsPathFullQualified(...) to restore the previous behavior.
If ITagHelperResolver.GetTagHelpersAsync(...) fails, it returns a default (not empty) ImmutableArray<TagHelperDescriptor>. If that happens, we would end up throwing an exception when trying to report success telemetry by accessing the "Length" property on the result. This change checks first to see if the result was actually a failure and reports failure instead, avoiding the exception.
Recently, a couple of calls to RazorProjectFileSystem.GetItem(...) were changed to pass a file path rather than a target path. This results in an exception being thrown in the DefaultRazorProjectFileSystem if the file path is rooted outside of the project root. Instead, it's important to pass the "target path", i.e. the "logical" path of a file within the project. This avoids breaking completely valid scenarios where a file is included in the Razor project from an outside path (such as a content file in a NuGet package).
chsienki
approved these changes
May 15, 2025
jaredpar
approved these changes
May 16, 2025
...Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/DefaultRazorProjectFileSystem.cs
Outdated
Show resolved
Hide resolved
src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/TestRazorProjectService.cs
Outdated
Show resolved
Hide resolved
|
/backport to release/dev17.14 |
|
Started backporting to release/dev17.14: https://github.com/dotnet/razor/actions/runs/15069900799 |
|
@DustinCampbell backporting to "release/dev17.14" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Add StartsWith and EndsWith extension methods
Applying: PathUtilities: Add IsPathFullyQualified and IsPathRooted methods
Applying: DefaultRazorProjectFileSystem: Don't join two rooted paths
Applying: ProjectStateUpdater: Don't throw when reporting success
Applying: Call RazorProjectFileSystem.GetItem(...) with correct path
Using index info to reconstruct a base tree...
M src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/IDocumentSnapshotExtensions.cs
M src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Legacy/ILegacyDocumentSnapshot.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Legacy/ILegacyDocumentSnapshot.cs
CONFLICT (content): Merge conflict in src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Legacy/ILegacyDocumentSnapshot.cs
Auto-merging src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/IDocumentSnapshotExtensions.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0005 Call RazorProjectFileSystem.GetItem(...) with correct path
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
Boo! |
2 tasks
phil-allen-msft
approved these changes
May 16, 2025
phil-allen-msft
added a commit
that referenced
this pull request
May 16, 2025
…de for file rooted outside of project (#11868) Backport of #11864 to release/dev17.14 ## Customer Impact ## Regression - [x] Yes - #11320 (This change inadvertently removed some exception swallowing that "hid" the problem from the user.) - [ ] No ## Testing - Manual testing of scenario with known repro project. The issue is present without the fix and resolved with the fix. - Tested mainline scenarios, such as OrchardCore and MudBlazor. These continue working with the fix. ## Risk Medium. This affects how every Razor file is compiled in Visual Studio at design-time. However, this risk is mitigated by testing large solutions such as OrchardCore and MudBlazor.
This was referenced May 20, 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.
Fixes AzDO 2477693
It is a perfectly valid scenario to use a file rooted outside of the project containing it. For example, a NuGet package might add Razor files as content along with "TargetPath" metadata to a project. However, recent changes broke this scenario.
The incorrect change was to start calling
RazorProjectFileSystem.GetItem(...)with the physical file path of a document. This is almost never correct becauseDefaultRazorProjectFileSystem.GetItem(...)will throw if the path passed in is rooted outside of the project root. Instead, it's more correct to pass in the document's target path, which is essentially a "logical path" to the file under the project root. When the target path is passed in,DefaultRazorProjectFileSystem.GetItem(...)will return a file path under the project root, which is what is needed by tooling to compute the applicable import file paths.I've fixed this issue comprehensively by correcting three other issues that I found along the way.
DefaultRazorProjecFileSystem.NormalizeAndEnsureValidPath(...)shouldn't join two rooted paths. Instead, if the path passed is rooted outside of the project root, it should just return the path. This was noticed because a rooted path was being passed in when it shouldn't have been.ProjectStateUpdateshouldn't throw when trying to send "success" telemetry on a failure.RazorProjectFileSystem.GetItem(...)with a document's target path rather than its physical file path.CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2710729&view=results
Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/637065