Do not load CodeStyle analyzers added by the SDK #75250

JoeRobich · 2024-09-26T08:15:31Z

To avoid a version mismatch between the version of Roslyn in the IDE and the version of Roslyn CodeStyle analyzers added by the SDK (#72811), we will not load the CodeStyle analyzers added by the SDK. The IDE ships with the same analyzers which are version matched and used during analysis.

The analyzers are still listed in the Solution Explorer but without any associated analyzers.

ToddGrun · 2024-09-26T13:58:21Z

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

+        return OneOrMany.Create(fullPath);
+    }
+
+    private static readonly string s_csharpCodeStyleAnalyzerSdkDirectory = Path.Combine("Sdks", "Microsoft.NET.Sdk", "codestyle", "cs") + PathUtilities.DirectorySeparatorStr;


PathUtilities.DirectorySeparatorStr

Should this be at the beginning of the string too?

ToddGrun · 2024-09-26T14:09:03Z

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

+
+        if (Language == LanguageNames.VisualBasic &&
+            fullPath.LastIndexOf(s_visualBasicCodeStyleAnalyzerSdkDirectory, StringComparison.OrdinalIgnoreCase) + s_visualBasicCodeStyleAnalyzerSdkDirectory.Length - 1 ==
+            fullPath.LastIndexOf(Path.DirectorySeparatorChar))


nit: this would be a bit easier to read as a separate fn since it's done in 3 places

sharwell · 2024-09-26T16:23:59Z

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

+        return OneOrMany.Create(fullPath);
+    }
+
+    private static readonly string s_csharpCodeStyleAnalyzerSdkDirectory = Path.Combine("Sdks", "Microsoft.NET.Sdk", "codestyle", "cs") + PathUtilities.DirectorySeparatorStr;


💭 Should we just look at the assembly name? With the current approach, if users explicitly reference a version of the code style package instead of using the one in the SDK, they will end up in the same bad IDE state we have today. The other option is to just stop shipping the code style layer as a separate package.

Thus far we've tried to limit this to when you load the analyzer / generator from the .NET SDK. The idea being that we want to isolate the .NET SDK from being able to negatively influence Visual Studio behavior. But our sample size is one, Razor, and there is no package option for that.

The guidance we're trying to give to 3rd party package is they need to be aware of the VS versions they can be loaded in and multi-target analyze for that.

My recommendation is start with the change as is, let's see if we get negative feedback and we can adjust based on that.

sharwell · 2024-09-26T16:26:18Z

Note that this will load the analyzers which ship in the IDE, but it does not correct the error where the wrong options are provided to those analyzers (analyzers in the IDE use values from Tools > Options, but those same analyzers in a build do not).

jaredpar · 2024-09-26T17:28:02Z

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

+        return OneOrMany.Create(fullPath);
+    }
+
+    private static readonly string s_csharpCodeStyleAnalyzerSdkDirectory = Path.Combine("Sdks", "Microsoft.NET.Sdk", "codestyle", "cs") + PathUtilities.DirectorySeparatorStr;


Thus far we've tried to limit this to when you load the analyzer / generator from the .NET SDK. The idea being that we want to isolate the .NET SDK from being able to negatively influence Visual Studio behavior. But our sample size is one, Razor, and there is no package option for that.

The guidance we're trying to give to 3rd party package is they need to be aware of the VS versions they can be loaded in and multi-target analyze for that.

My recommendation is start with the change as is, let's see if we get negative feedback and we can adjust based on that.

jaredpar · 2024-09-26T17:41:17Z

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

+                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"))


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

Probably needs VB too.

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.

jaredpar · 2024-09-26T17:42:27Z

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

            }
+
+            return OneOrMany<string>.Empty;


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?

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

jasonmalinowski · 2024-09-26T20:09:41Z

The analyzers are still listed in the Solution Explorer but without any associated analyzers.

Do we have a design-time target somewhere we can add a hack in that removes these at the MSBuild level instead? This is a perfectly fine tactical fix of course, but that'd avoid side effects like this.

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

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

jasonmalinowski · 2024-09-26T20:04:19Z

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

+                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"))


Do resource binaries get added too?

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

jasonmalinowski · 2024-09-26T20:04:40Z

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

+                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"))


Probably needs VB too.

jasonmalinowski · 2024-09-26T20:05:30Z

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

+
+    private bool IsSdkCodeStyleAnalyzer(string fullPath)
+    {
+        if (Language == LanguageNames.CSharp &&


Any reason not to just match on Sdks\Microsoft.NET.Sdk\codestyle and work for both languages at once?

Happy to make that change. Didn't have a good feel with how specific we wanted to get.

Actually will keep it as is. The code currently checks for the analyzer path to end with the folder path fragment which is a stronger guarantee than just containing the fragment.

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

jasonmalinowski · 2024-09-26T20:51:03Z

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

+        {
+            // 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;


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

Also apply to VS Code (which might be reasonable since we'll have similar issues there I imagine)

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.

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?

ToddGrun · 2024-09-26T23:18:39Z

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

        }

        return OneOrMany.Create(fullPath);
    }

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


" "

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

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

Maybe it is dumb. Happy to revert it.

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

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

JoeRobich · 2024-09-27T05:17:24Z

/azp run

azure-pipelines · 2024-09-27T05:17:53Z

Azure Pipelines successfully started running 3 pipeline(s).

KirillOsenkov · 2024-10-15T05:08:19Z

@JoeRobich quick question: will it work if the paths to analyzers contain ..?

I haven't tried in VS, but some other hosts receive paths like this:
C:\Program Files\dotnet\sdk\9.0.100-rc.1.24452.12\Sdks\Microsoft.NET.Sdk\targets\..\codestyle\cs\Microsoft.CodeAnalysis.CodeStyle.dll, so need to first call Path.GetFullPath() on the reference before calling IsSdk...

I haven't checked whether the VS project system normalizes paths before this codepath is hit.

JoeRobich · 2024-10-17T20:29:37Z

so need to first call Path.GetFullPath() on the reference before calling IsSdk...

Thanks for checking! We do call Path.GetFullPath() just before.

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

Lines 1033 to 1037 in 3bff362

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

JoeRobich requested review from tmat and sharwell September 26, 2024 08:15

JoeRobich requested a review from a team as a code owner September 26, 2024 08:15

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 26, 2024

dotnet-policy-service bot added the VSCode label Sep 26, 2024

build-analysis bot mentioned this pull request Sep 26, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

ToddGrun reviewed Sep 26, 2024

View reviewed changes

CyrusNajmabadi mentioned this pull request Sep 26, 2024

Load roslyn version of the codestyle analyzers to fix torn load state (causes ReflectionTypeLoadException) #72811

Open

sharwell reviewed Sep 26, 2024

View reviewed changes

arunchndr approved these changes Sep 26, 2024

View reviewed changes

jaredpar reviewed Sep 26, 2024

View reviewed changes

dibarbet reviewed Sep 26, 2024

View reviewed changes

src/Workspaces/Core/Portable/Workspace/ProjectSystem/ProjectSystemProject.cs Show resolved Hide resolved

jasonmalinowski approved these changes Sep 26, 2024

View reviewed changes

JoeRobich enabled auto-merge (squash) September 26, 2024 22:28

ToddGrun reviewed Sep 26, 2024

View reviewed changes

JoeRobich disabled auto-merge September 27, 2024 00:02

build-analysis bot mentioned this pull request Sep 27, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

JoeRobich changed the base branch from main to release/dev17.12 September 27, 2024 02:15

JoeRobich requested review from a team as code owners September 27, 2024 02:15

Do not load CodeStyle analyzers added by the SDK

53c4588

Review feedback

71bec2e

JoeRobich force-pushed the dev/jorobich/discard-codestyle-analyzers branch from 0ca4cb7 to 71bec2e Compare September 27, 2024 06:38

JoeRobich merged commit 23f900a into release/dev17.12 Sep 27, 2024
25 checks passed

333fred deleted the dev/jorobich/discard-codestyle-analyzers branch September 27, 2024 16:45

This was referenced Oct 1, 2024

[Automated] PRs inserted in VS build main-35330.119 #75310

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35401.07 #75313

Closed

sharwell mentioned this pull request Oct 2, 2024

Only provide fallback options for host analyzers #75317

Merged

This was referenced Oct 2, 2024

NullRef in GenerateVariable.CSharpGenerateVariableCodeFixProvider.GetCodeActionsAsync #75204

Closed

Bail out gracefully when we can't find service #75457

Merged

dotnet-bot mentioned this pull request Oct 11, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35410.215 #75481

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not load CodeStyle analyzers added by the SDK #75250

Do not load CodeStyle analyzers added by the SDK #75250

JoeRobich commented Sep 26, 2024 •

edited

Loading

ToddGrun Sep 26, 2024

ToddGrun Sep 26, 2024

sharwell Sep 26, 2024 •

edited

Loading

jaredpar Sep 26, 2024

sharwell commented Sep 26, 2024

jaredpar Sep 26, 2024

jaredpar Sep 26, 2024

jasonmalinowski Sep 26, 2024

JoeRobich Sep 26, 2024

jaredpar Sep 26, 2024

JoeRobich Sep 27, 2024

jasonmalinowski commented Sep 26, 2024

jasonmalinowski Sep 26, 2024

JoeRobich Sep 26, 2024

jasonmalinowski Sep 26, 2024

jasonmalinowski Sep 26, 2024

JoeRobich Sep 26, 2024

JoeRobich Sep 26, 2024

jasonmalinowski Sep 26, 2024

chsienki Sep 27, 2024

ToddGrun Sep 26, 2024

JoeRobich Sep 27, 2024

JoeRobich Sep 27, 2024

ToddGrun Sep 27, 2024

JoeRobich Sep 27, 2024

JoeRobich commented Sep 27, 2024

azure-pipelines bot commented Sep 27, 2024

KirillOsenkov commented Oct 15, 2024

JoeRobich commented Oct 17, 2024

Do not load CodeStyle analyzers added by the SDK #75250

Do not load CodeStyle analyzers added by the SDK #75250

Conversation

JoeRobich commented Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell commented Sep 26, 2024

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 commented Sep 26, 2024

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

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

JoeRobich commented Sep 27, 2024

azure-pipelines bot commented Sep 27, 2024

KirillOsenkov commented Oct 15, 2024

JoeRobich commented Oct 17, 2024

JoeRobich commented Sep 26, 2024 •

edited

Loading

sharwell Sep 26, 2024 •

edited

Loading