Make Razor redirect code more robust #78039

jaredpar · 2025-04-08T00:47:33Z

In an attempt to normalize the Razor experience the Workspaces layer will redirect Razor analyzer references to the ones checked into Visual Studio. This detection had a couple of corner cases that it didn't handle:

When customers used Razor from anywhere other than the .NET SDK. This would impact customers that check in artifacts as part of their build.
In a future where Razor changes the set of DLLs that are registered with /analyzer and customers use an older SDK, this logic would attempt to remap DLLs that did not exist. That is because it just assumes that DLLs coming from the SDK exist in VS.

This attempts to make the process more robust by identifying an analyzer as being part of the Razor closure if it exists in the same directory as the Razor compiler. Then for all such DLLs it will just return the set of registered Razor analyzers.

This does have one remaining corner case where a customer registers in an analyzer in a directory with a file named MS.CA.Razor.Compiler.dll but that file is not registered as an analyzer. That seems a bit out of bounds as they're shipping MS.CA.Razor.Compiler in a state that is not supported. Happy to hear if anyone has ideas on how to solve this though.

In an attempt to normalize the Razor experience the Workspaces layer will redirect Razor analyzer references to the ones checked into Visual Studio. This detection had a couple of corner cases that it didn't handle: 1. When customers used Razor from anywhere other than the .NET SDK. This would impact customers that check in artifacts as part of their build. 2. In a future where Razor changes the set of DLLs that are registered with `/analyzer` and customers use an older SDK, this logic would attempt to remap DLLs that did not exist. That is because it just assumes that DLLs coming from the SDK exist in VS. This attempts to make the process more robust by identifying an analyzer as being part of the Razor closure if it exists in the same directory as the Razor compiler. Then for all such DLLs it will just return the set of registered Razor analyzers. This does have one remaining corner case where a customer registers in an analyzer in a directory with a file named MS.CA.Razor.Compiler.dll but that file is not registered as an analyzer. That seems a bit out of bounds as they're shipping MS.CA.Razor.Compiler in a state that is not supported. Happy to hear if anyone has ideas on how to solve this though.

jaredpar · 2025-04-08T16:21:56Z

@jasonmalinowski, @davidwengier, @chsienki, @jjonescz PTAL

jaredpar · 2025-04-08T16:23:39Z

src/VisualStudio/Core/Test/ProjectSystemShim/VisualStudioProjectTests/AnalyzerReferenceTests.vb

+                ' This does not throw because within the razor generator only the actual generator dll triggers real 
+                ' add / removes. At a glance this may seem problematic but it's an unavoidable outcome once the 
+                ' project system begins mapping / mangling analyzer paths. There will be cases where existing names 
+                ' do not map to analyzers and it ends up with this behavior. 


This could be fixed by putting a bit more logic into ProjectSystemProject where by we keep track of the original paths that were mapped to empty. Then we could have the Add / Remove calls manipulate that table and get back throwing behavior for double remove. Ended up deciding against that because it's a half measure. Yes we get back the Add / Remove semantics but it doesn't fix the fact that AnalyzerReferences in the project don't change.

I could be convinced to go the other direction.

I'm fine with this, since at some point I hope this gets properly refactored. 😄

jjonescz · 2025-04-09T15:56:15Z

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs

+    /// </remarks>
+    internal static bool IsAnyRazorAnalyzerFilePath(string fullPath)
+    {
+        var dir = Path.GetDirectoryName(fullPath)!;


The suppression shouldn't be necessary thanks to the assert below

Suggested change

var dir = Path.GetDirectoryName(fullPath)!;

var dir = Path.GetDirectoryName(fullPath);

jjonescz · 2025-04-10T08:16:14Z

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs

+        return (razorGeneratorFullPaths.IsEmpty, IsFileName(fullPath, RazorSourceGenaratorDllName)) switch
        {
-            if (s_razorSourceGeneratorAssemblyRootedFileNames.Any(
-                static (fileName, fullPath) => fullPath.EndsWith(fileName, StringComparison.OrdinalIgnoreCase), fullPath))
-            {
-                return OneOrMany.Create(vsixRazorAnalyzers);
-            }
+            (true, _) => OneOrMany.Create(fullPath),
+            (false, true) => OneOrMany.Create(razorGeneratorFullPaths),
+            (false, false) => OneOrMany<string>.Empty,
+        };


If we rewrote this as

Suggested change

return (razorGeneratorFullPaths.IsEmpty, IsFileName(fullPath, RazorSourceGenaratorDllName)) switch

{

if (s_razorSourceGeneratorAssemblyRootedFileNames.Any(

static (fileName, fullPath) => fullPath.EndsWith(fileName, StringComparison.OrdinalIgnoreCase), fullPath))

{

return OneOrMany.Create(vsixRazorAnalyzers);

}

(true, _) => OneOrMany.Create(fullPath),

(false, true) => OneOrMany.Create(razorGeneratorFullPaths),

(false, false) => OneOrMany<string>.Empty,

};

if (razorGeneratorFullPaths.IsEmpty)

{

return OneOrMany.Create(fullPath);

}

return IsFileName(fullPath, RazorSourceGenaratorDllName)

? OneOrMany.Create(razorGeneratorFullPaths)

: OneOrMany<string>.Empty;

I think the code might be more readable and wouldn't need to call IsFileName unnecessarily.

Oh this is much more readable. It also makes it possible to add comments in the logic too. 😉

jjonescz · 2025-04-10T08:17:17Z

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs


-            return OneOrMany<string>.Empty;
+        static bool IsFileName(string filePath, string fileName)


I think the convention is using first letter lowercase for local function names? Might be just compiler layer convention though, not sure.

jjonescz · 2025-04-10T08:19:35Z

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs


-            return OneOrMany<string>.Empty;
+        static bool IsFileName(string filePath, string fileName)


Perhaps "hasFileName" because it checks whether the path has the given file name.

jjonescz · 2025-04-10T08:24:41Z

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs

+    /// </summary>
+    /// <remarks>
+    /// For <see cref="RazorSourceGenaratorDllName"/> this will return all registered analyzers in the 
+    /// Razor extension. For everything else it will return an empty set.


For everything else it will return an empty set.

It looks like the method also returns just the fullPath itself in case the Razor extension does not exist or have any analyzers registered (but the doc comment suggest it would return an empty set)

Consider also testing this case.

jjonescz · 2025-04-10T08:26:47Z

src/VisualStudio/Core/Test/ProjectSystemShim/VisualStudioProjectTests/AnalyzerReferenceTests.vb


-                Assert.Empty(environment.Workspace.CurrentSolution.Projects.Single().AnalyzerReferences)
+                ' Deliberately left Util.dll as we want to verify the behavior when the sdk and vsix disagree on total


Suggested change

' Deliberately left Util.dll as we want to verify the behavior when the sdk and vsix disagree on total

' Deliberately left out Util.dll as we want to verify the behavior when the sdk and vsix disagree on total

jjonescz · 2025-04-10T08:28:22Z

src/VisualStudio/Core/Test/ProjectSystemShim/VisualStudioProjectTests/AnalyzerReferenceTests.vb

            Using environment = New TestEnvironment()
+
+                Dim vsixDir = CreateAnalyzerDir(ProjectSystemProject.RazorSourceGenaratorDllName, "Analyzer.dll")


Shouldn't Util.dll be listed here?

This should really get refactored so the test setup is one helper for all tests to avoid this oops.

jjonescz · 2025-04-10T08:32:26Z

src/VisualStudio/Core/Test/ProjectSystemShim/VisualStudioProjectTests/AnalyzerReferenceTests.vb


-                AssertEx.Equal(
+                Dim vsixDir = CreateAnalyzerDir(ProjectSystemProject.RazorSourceGenaratorDllName, "Analyzer.dll")


Should this be Util.dll to match the vsix setup below?

Applies to the tests below as well.

jjonescz · 2025-04-10T08:35:12Z

src/VisualStudio/Core/Test/ProjectSystemShim/VisualStudioProjectTests/AnalyzerReferenceTests.vb

+                Dim filePath = DirectCast(a, AnalyzerFileReference).FullPath
+                list.Add(filePath)
+            Next
+            Return list


Consider moving the OrderBy that's used by all callers here

jasonmalinowski

I approve in the sense I don't have much to add further, but @jjonescz has a lot of great feedback and you should fix all of his feedback. 😄

jasonmalinowski · 2025-04-16T05:41:12Z

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs

-    private static bool IsSdkRazorSourceGenerator(string fullPath) => DirectoryNameEndsWith(fullPath, s_razorSourceGeneratorSdkDirectory);
-
-    private OneOrMany<string> GetMappedRazorSourceGenerator(string fullPath)
+    internal const string RazorSourceGenaratorDllName = "Microsoft.CodeAnalysis.Razor.Compiler.dll";


Suggested change

internal const string RazorSourceGenaratorDllName = "Microsoft.CodeAnalysis.Razor.Compiler.dll";

internal const string RazorCompilerDllName = "Microsoft.CodeAnalysis.Razor.Compiler.dll";

Minimally fix the spelling of 'generator', but the field name doesn't seem to match the content anymore.

jasonmalinowski · 2025-04-16T05:50:11Z

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs

+        return (razorGeneratorFullPaths.IsEmpty, IsFileName(fullPath, RazorSourceGenaratorDllName)) switch
        {
-            if (s_razorSourceGeneratorAssemblyRootedFileNames.Any(
-                static (fileName, fullPath) => fullPath.EndsWith(fileName, StringComparison.OrdinalIgnoreCase), fullPath))
-            {
-                return OneOrMany.Create(vsixRazorAnalyzers);
-            }
+            (true, _) => OneOrMany.Create(fullPath),
+            (false, true) => OneOrMany.Create(razorGeneratorFullPaths),
+            (false, false) => OneOrMany<string>.Empty,
+        };


Oh this is much more readable. It also makes it possible to add comments in the logic too. 😉

jasonmalinowski · 2025-04-16T05:56:31Z

src/VisualStudio/Core/Test/ProjectSystemShim/VisualStudioProjectTests/AnalyzerReferenceTests.vb

+                Assert.Equal(2, getAnalyzerRefs().Count)
+                Assert.Equal(
+                    {
+                        Path.Combine(vsixDir, ProjectSystemProject.RazorSourceGenaratorDllName),
+                        Path.Combine(vsixDir, "Util.dll")
+                    }, GetAnalyzerFilePaths(getAnalyzerRefs()).OrderBy(Function(x) Path.GetFileName(x), StringComparer.Ordinal))


Consider adding just a "assert the file names are" helper; then you could just use that one helper everywhere, and also replace all the count assertions with that which makes it a bit clearer you expect things didn't change.

(I'm also not a fan of the count assertion before the set assertion: if it's got not-2, it'll fail, but the failure assertion won't make it obvious what happened.)

jasonmalinowski · 2025-04-16T05:58:02Z

src/VisualStudio/Core/Test/ProjectSystemShim/VisualStudioProjectTests/AnalyzerReferenceTests.vb

+                Dim providerFactory = DirectCast(environment.ExportProvider.GetExportedValue(Of IVisualStudioDiagnosticAnalyzerProviderFactory), MockVisualStudioDiagnosticAnalyzerProviderFactory)
+                providerFactory.Extensions =
                {
-                    Path.Combine(TempRoot.Root, "RazorVsix", "Microsoft.CodeAnalysis.Razor.Compiler.SourceGenerators.dll"),
-                    Path.Combine(TempRoot.Root, "RazorVsix", "VsixDependency1.dll"),
-                    Path.Combine(TempRoot.Root, "RazorVsix", "VsixDependency2.dll"),
-                    Path.Combine(TempRoot.Root, "Some other directory", "Microsoft.CodeAnalysis.Razor.Compiler.SourceGenerators.dll"),
-                    Path.Combine(TempRoot.Root, "Dir", "File.dll")
-                }, environment.Workspace.CurrentSolution.Projects.Single().AnalyzerReferences.Select(Function(r) r.FullPath))
+                    ({
+                        Path.Combine(vsixDir, ProjectSystemProject.RazorSourceGenaratorDllName),
+                        Path.Combine(vsixDir, "Util.dll")
+                     },
+                     ProjectSystemProject.RazorVsixExtensionId)
+                }


Consider extracting this to a helper.

jasonmalinowski · 2025-04-16T05:59:26Z

src/VisualStudio/Core/Test/ProjectSystemShim/VisualStudioProjectTests/AnalyzerReferenceTests.vb

+                Dim getAnalyzerRefs =
+                    Function() As IReadOnlyList(Of AnalyzerReference)
+                        Return environment.Workspace.CurrentSolution.Projects.Single().AnalyzerReferences
+                    End Function


Just inline this logic into GetAnalyzerFilePaths since it's being repeated everywhere.

jasonmalinowski · 2025-04-16T06:01:08Z

src/VisualStudio/Core/Test/ProjectSystemShim/VisualStudioProjectTests/AnalyzerReferenceTests.vb

            Using environment = New TestEnvironment()
+
+                Dim vsixDir = CreateAnalyzerDir(ProjectSystemProject.RazorSourceGenaratorDllName, "Analyzer.dll")


This should really get refactored so the test setup is one helper for all tests to avoid this oops.

jasonmalinowski · 2025-04-16T06:03:14Z

src/VisualStudio/Core/Test/ProjectSystemShim/VisualStudioProjectTests/AnalyzerReferenceTests.vb

+                ' This does not throw because within the razor generator only the actual generator dll triggers real 
+                ' add / removes. At a glance this may seem problematic but it's an unavoidable outcome once the 
+                ' project system begins mapping / mangling analyzer paths. There will be cases where existing names 
+                ' do not map to analyzers and it ends up with this behavior. 


I'm fine with this, since at some point I hope this gets properly refactored. 😄

jaredpar · 2025-04-17T17:14:41Z

@jasonmalinowski, @jjonescz thanks for the reviews. @chsienki saw this and thinks he has a better way of approaching the problem. He's going to explore it once we get past our 17.14 QB issues. But for now I'm holding off on this until he has time to explore his idea.

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 8, 2025

dotnet-policy-service bot added the VSCode label Apr 8, 2025

more

3cd85a3

jaredpar marked this pull request as ready for review April 8, 2025 16:21

jaredpar requested a review from a team as a code owner April 8, 2025 16:21

jaredpar commented Apr 8, 2025

View reviewed changes

jjonescz approved these changes Apr 10, 2025

View reviewed changes

jasonmalinowski approved these changes Apr 16, 2025

View reviewed changes

jaredpar marked this pull request as draft April 17, 2025 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Razor redirect code more robust #78039

Make Razor redirect code more robust #78039

jaredpar commented Apr 8, 2025

jaredpar commented Apr 8, 2025

jaredpar Apr 8, 2025

jasonmalinowski Apr 16, 2025

jjonescz Apr 9, 2025

jjonescz Apr 10, 2025

jasonmalinowski Apr 16, 2025

jjonescz Apr 10, 2025

jjonescz Apr 10, 2025

jjonescz Apr 10, 2025 •

edited

Loading

jjonescz Apr 10, 2025

jjonescz Apr 10, 2025

jasonmalinowski Apr 16, 2025

jjonescz Apr 10, 2025

jjonescz Apr 10, 2025

jasonmalinowski left a comment

jasonmalinowski Apr 16, 2025

jasonmalinowski Apr 16, 2025

jasonmalinowski Apr 16, 2025

jasonmalinowski Apr 16, 2025

jasonmalinowski Apr 16, 2025

jasonmalinowski Apr 16, 2025

jasonmalinowski Apr 16, 2025

jasonmalinowski Apr 16, 2025

jaredpar commented Apr 17, 2025

	var dir = Path.GetDirectoryName(fullPath)!;
	var dir = Path.GetDirectoryName(fullPath);


		return OneOrMany<string>.Empty;
		static bool IsFileName(string filePath, string fileName)


		Assert.Empty(environment.Workspace.CurrentSolution.Projects.Single().AnalyzerReferences)
		' Deliberately left Util.dll as we want to verify the behavior when the sdk and vsix disagree on total

	' Deliberately left Util.dll as we want to verify the behavior when the sdk and vsix disagree on total
	' Deliberately left out Util.dll as we want to verify the behavior when the sdk and vsix disagree on total

		Using environment = New TestEnvironment()

		Dim vsixDir = CreateAnalyzerDir(ProjectSystemProject.RazorSourceGenaratorDllName, "Analyzer.dll")


		AssertEx.Equal(
		Dim vsixDir = CreateAnalyzerDir(ProjectSystemProject.RazorSourceGenaratorDllName, "Analyzer.dll")

	internal const string RazorSourceGenaratorDllName = "Microsoft.CodeAnalysis.Razor.Compiler.dll";
	internal const string RazorCompilerDllName = "Microsoft.CodeAnalysis.Razor.Compiler.dll";

Make Razor redirect code more robust #78039

Are you sure you want to change the base?

Make Razor redirect code more robust #78039

Conversation

jaredpar commented Apr 8, 2025

jaredpar commented Apr 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Apr 17, 2025

jjonescz Apr 10, 2025 •

edited

Loading