Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not load CodeStyle analyzers added by the SDK #75250

Merged
merged 2 commits into from
Sep 27, 2024
Merged

Do not load CodeStyle analyzers added by the SDK #75250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
53c4588
Do not load CodeStyle analyzers added by the SDK
JoeRobich Sep 26, 2024
71bec2e
Review feedback
JoeRobich Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 53 additions & 1 deletion ...sualStudio/Core/Test/ProjectSystemShim/VisualStudioProjectTests/AnalyzerReferenceTests.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.ProjectSystemShim
Dim project = Await environment.ProjectFactory.CreateAndAddToWorkspaceAsync(
"Project", LanguageNames.CSharp, CancellationToken.None)

' add Razor source generator and a couple more other analyzer filess:
' add Razor source generator and a couple more other analyzer files:
Dim path1 = Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk.Razor", "source-generators", "Microsoft.NET.Sdk.Razor.SourceGenerators.dll")
Dim path2 = Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk.Razor", "source-generators", "SdkDependency1.dll")
project.AddAnalyzerReference(path1)
Expand All @@ -204,5 +204,57 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.ProjectSystemShim
AssertEx.Equal({path1, path2}, environment.Workspace.CurrentSolution.Projects.Single().AnalyzerReferences.Select(Function(r) r.FullPath))
End Using
End Function

<WpfFact>
Public Async Function CodeStyleAnalyzers_CSharp_FromSdk_AreIgnored() As Task
Using environment = New TestEnvironment()
Dim project = Await environment.ProjectFactory.CreateAndAddToWorkspaceAsync(
"Project", LanguageNames.CSharp, CancellationToken.None)

' These are the in-box C# codestyle analyzers that ship with the SDK
project.AddAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "cs", "Microsoft.CodeAnalysis.CodeStyle.dll"))
project.AddAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "cs", "Microsoft.CodeAnalysis.CodeStyle.Fixes.dll"))
project.AddAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "cs", "Microsoft.CodeAnalysis.CSharp.CodeStyle.dll"))
project.AddAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "cs", "Microsoft.CodeAnalysis.CSharp.CodeStyle.Fixes.dll"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all of these DLLs already in box that we're mapping to?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably needs VB too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the in box code style analyzers. We are ensuring that they do not get returned as AnalyzerReferences from the Project. Which means they will not be used for analysis.

Comment on lines +215 to +218
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do resource binaries get added too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe resource binaries are added as AnalyzerReferences to the project. I would expect they are loaded by the AnalyzerAssemblyLoader.


' Ensure they are not returned when getting AnalyzerReferences
Assert.Empty(environment.Workspace.CurrentSolution.Projects.Single().AnalyzerReferences)

' Add a non-codestyle analyzer to the project
project.AddAnalyzerReference(Path.Combine(TempRoot.Root, "Dir", "File.dll"))

' Ensure it is returned as expected
AssertEx.Equal(
{
Path.Combine(TempRoot.Root, "Dir", "File.dll")
}, environment.Workspace.CurrentSolution.Projects.Single().AnalyzerReferences.Select(Function(r) r.FullPath))
End Using
End Function

<WpfFact>
Public Async Function CodeStyleAnalyzers_VisualBasic_FromSdk_AreIgnored() As Task
Using environment = New TestEnvironment()
Dim project = Await environment.ProjectFactory.CreateAndAddToWorkspaceAsync(
"Project", LanguageNames.VisualBasic, CancellationToken.None)

' These are the in-box VB codestyle analyzers that ship with the SDK
project.AddAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "vb", "Microsoft.CodeAnalysis.CodeStyle.dll"))
project.AddAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "vb", "Microsoft.CodeAnalysis.CodeStyle.Fixes.dll"))
project.AddAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "vb", "Microsoft.CodeAnalysis.VisualBasic.CodeStyle.dll"))
project.AddAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "vb", "Microsoft.CodeAnalysis.VisualBasic.CodeStyle.Fixes.dll"))

' Ensure they are not returned when getting AnalyzerReferences
Assert.Empty(environment.Workspace.CurrentSolution.Projects.Single().AnalyzerReferences)

' Add a non-codestyle analyzer to the project
project.AddAnalyzerReference(Path.Combine(TempRoot.Root, "Dir", "File.dll"))

' Ensure it is returned as expected
AssertEx.Equal(
{
Path.Combine(TempRoot.Root, "Dir", "File.dll")
}, environment.Workspace.CurrentSolution.Projects.Single().AnalyzerReferences.Select(Function(r) r.FullPath))
End Using
End Function
End Class
End Namespace
69 changes: 50 additions & 19 deletions src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,8 +1030,39 @@ public void RemoveAnalyzerReference(string fullPath)
}
}

private OneOrMany<string> GetMappedAnalyzerPaths(string fullPath)
{
fullPath = Path.GetFullPath(fullPath);

if (IsSdkCodeStyleAnalyzer(fullPath))
{
// We discard the CodeStyle analyzers added by the SDK when the EnforceCodeStyleInBuild property is set.
// The same analyzers ship in-box as part of the Features layer and are version matched to the compiler.
return OneOrMany<string>.Empty;
JoeRobich marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to call it out explicitly, doing this here will:

  1. Also apply to VS Code (which might be reasonable since we'll have similar issues there I imagine)
  2. Will not apply to MSBuildWorkspace, which might be bad, or might not be.

The latter we can of course defer for this change (since this has huge dogfooding benefit), but wanted to call out the former.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't love that we're expanding this mechanism, given the work we're doing to allow the SDK to redirect analyzers via #74820

That isn't a reason to block on this, given its not done yet, but could we open an issue to investigate if we could replace this with the new API when it's introduced?

}

if (IsSdkRazorSourceGenerator(fullPath))
{
// Map all files in the SDK directory that contains the Razor source generator to source generator files loaded from VSIX.
// Include the generator and all its dependencies shipped in VSIX, discard the generator and all dependencies in the SDK
return GetMappedRazorSourceGenerator(fullPath);
}

return OneOrMany.Create(fullPath);
}

private static readonly string s_csharpCodeStyleAnalyzerSdkDirectory = CreateDirectoryPathFragment("Sdks", "Microsoft.NET.Sdk", "codestyle", "cs");
private static readonly string s_visualBasicCodeStyleAnalyzerSdkDirectory = CreateDirectoryPathFragment("Sdks", "Microsoft.NET.Sdk", "codestyle", "vb");

private bool IsSdkCodeStyleAnalyzer(string fullPath) => Language switch
{
LanguageNames.CSharp => DirectoryNameEndsWith(fullPath, s_csharpCodeStyleAnalyzerSdkDirectory),
LanguageNames.VisualBasic => DirectoryNameEndsWith(fullPath, s_visualBasicCodeStyleAnalyzerSdkDirectory),
_ => false,
};

internal const string RazorVsixExtensionId = "Microsoft.VisualStudio.RazorExtension";
private static readonly string s_razorSourceGeneratorSdkDirectory = Path.Combine("Sdks", "Microsoft.NET.Sdk.Razor", "source-generators") + PathUtilities.DirectorySeparatorStr;
private static readonly string s_razorSourceGeneratorSdkDirectory = CreateDirectoryPathFragment("Sdks", "Microsoft.NET.Sdk.Razor", "source-generators");
private static readonly ImmutableArray<string> s_razorSourceGeneratorAssemblyNames =
[
"Microsoft.NET.Sdk.Razor.SourceGenerators",
Expand All @@ -1041,33 +1072,33 @@ public void RemoveAnalyzerReference(string fullPath)
private static readonly ImmutableArray<string> s_razorSourceGeneratorAssemblyRootedFileNames = s_razorSourceGeneratorAssemblyNames.SelectAsArray(
assemblyName => PathUtilities.DirectorySeparatorStr + assemblyName + ".dll");

private OneOrMany<string> GetMappedAnalyzerPaths(string fullPath)
private static bool IsSdkRazorSourceGenerator(string fullPath) => DirectoryNameEndsWith(fullPath, s_razorSourceGeneratorSdkDirectory);

private OneOrMany<string> GetMappedRazorSourceGenerator(string fullPath)
{
fullPath = Path.GetFullPath(fullPath);
// Map all files in the SDK directory that contains the Razor source generator to source generator files loaded from VSIX.
// Include the generator and all its dependencies shipped in VSIX, discard the generator and all dependencies in the SDK
if (fullPath.LastIndexOf(s_razorSourceGeneratorSdkDirectory, StringComparison.OrdinalIgnoreCase) + s_razorSourceGeneratorSdkDirectory.Length - 1 ==
fullPath.LastIndexOf(Path.DirectorySeparatorChar))
{
var vsixRazorAnalyzers = _hostInfo.HostDiagnosticAnalyzerProvider.GetAnalyzerReferencesInExtensions().SelectAsArray(
predicate: item => item.extensionId == RazorVsixExtensionId,
selector: item => item.reference.FullPath);
var vsixRazorAnalyzers = _hostInfo.HostDiagnosticAnalyzerProvider.GetAnalyzerReferencesInExtensions().SelectAsArray(
predicate: item => item.extensionId == RazorVsixExtensionId,
selector: item => item.reference.FullPath);

if (!vsixRazorAnalyzers.IsEmpty)
if (!vsixRazorAnalyzers.IsEmpty)
{
if (s_razorSourceGeneratorAssemblyRootedFileNames.Any(
static (fileName, fullPath) => fullPath.EndsWith(fileName, StringComparison.OrdinalIgnoreCase), fullPath))
{
if (s_razorSourceGeneratorAssemblyRootedFileNames.Any(
static (fileName, fullPath) => fullPath.EndsWith(fileName, StringComparison.OrdinalIgnoreCase), fullPath))
{
return OneOrMany.Create(vsixRazorAnalyzers);
}

return OneOrMany.Create(ImmutableArray<string>.Empty);
return OneOrMany.Create(vsixRazorAnalyzers);
}

return OneOrMany<string>.Empty;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My one fear is that this is hard to test in a unit test. Do we have an item on the queue to do manual verification of Razor / Code style once this is all merged in?

Have we thought about adding an integration test that verifies we redirected these loads?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be easy enough to write an integration test for this. Will do as a follow up after the snap.

}

return OneOrMany.Create(fullPath);
}

private static string CreateDirectoryPathFragment(params string[] paths) => Path.Combine([" ", .. paths, " "]).Trim();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

" "

Why did this switch from the directory separator char to a space?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So Path.Combine will add the separator then we trim it off the spaces. Was just trying to put the work onto Path.Combine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is dumb. Happy to revert it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely not dumb, just a little too tricky for me to spot without the explanation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to do a follow up that adds an integration test. Will add a comment here as well.


private static bool DirectoryNameEndsWith(string fullPath, string ending) => fullPath.LastIndexOf(ending, StringComparison.OrdinalIgnoreCase) + ending.Length - 1 ==
fullPath.LastIndexOf(Path.DirectorySeparatorChar);

#endregion

private void DocumentFileChangeContext_FileChanged(object? sender, string fullFilePath)
Expand Down
Loading