When replacing CodeStyle analyzers do not provide Features analyzers fallback options #75534

JoeRobich · 2024-10-16T20:14:30Z

This PR adds a ProjectAttribute to track whether the Project uses the SDK CodeStyle analyzers. This attribute is populated by the ProjectSystemProject which has access to the unmapped set of AnalyzerReferences. This attribute is then used when creating the HostAnalyzerStateSets to determine whether the Features analyzers should be treated as Project analyzers or not. It is also used by the DiagnosticComputer when building the Host and Project CompilationWithAnalyzers to ensure they mirror the statesets.

Suggest to review a commit at a time.

Also plumbs through updating the attribute throughout the various Solution and *State classes.

Use the unmapped references to update the HasSdkCodeStyleAnalyzer ProjectAttribute.

When Features analyzers are being used to replace SDK CodeStyle analyzer we need to treat them as project analyzers and not provide them with the Host's fallback options.

…analyzers. In particular we will treat Features analyzers as project analyzers when a project is using SDK CodeStyle analyzers. This mirrors the change that redirects Features analyzers in the HostStateSets. This is necessary to keep the CompilationsWithAnalyzers in sync with the statesets.

tmat · 2024-10-23T17:54:08Z

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

@@ -1061,6 +1093,14 @@ private OneOrMany<string> GetMappedAnalyzerPaths(string fullPath)
        _ => false,
    };

+    private void UpdateHasSdkCodeStyleAnalyzers()


Is this used?

tmat · 2024-10-23T17:55:39Z

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

+            _unmappedProjectAnalyzerPaths.Add(fullPath);
+
+            // Determine if we started using SDK CodeStyle analyzers while access to _unmappedProjectAnalyzerPaths is gated.
+            containsSdkCodeStyleAnalyzers = _unmappedProjectAnalyzerPaths.Any(IsSdkCodeStyleAnalyzer);


Can we do containsSdkCodeStyleAnalyzers = _hasSdkCodeStyleAnalyzers | IsSdkCodeStyleAnalyzer(fullPath) instead of enumerating all the paths each time?

agree on tweaking this.

Changing this to only track the codestyle analyzer paths instead of tracking all analyzer paths. Only one call to IsSdkCodeStyleAnalyzer when adding or removing.

tmat · 2024-10-23T17:56:46Z

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

@@ -58,6 +58,11 @@ internal sealed partial class ProjectSystemProject
    /// </summary>
    private readonly HashSet<string> _projectAnalyzerPaths = [];

+    /// <summary>


Add explanation of what mapping of analyzer references means.

Changing this field name to _sdkCodeStyleAnalyzerPaths which I think is self explanatory.

tmat · 2024-10-23T18:02:08Z

...tocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.StateManager.HostStates.cs

+                    var hostAnalyzerCollection = new List<ImmutableArray<DiagnosticAnalyzer>>();
+                    var projectAnalyzerCollection = new List<ImmutableArray<DiagnosticAnalyzer>>();
+
+                    foreach (var kvp in analyzersPerReference)


Deconstruct: foreach (var (referenceId, analyzers) in analyzersPerReference)

tmat · 2024-10-23T18:08:24Z

src/Workspaces/Core/Portable/Diagnostics/Extensions.cs

+        "Microsoft.CodeAnalysis.VisualBasic.Features.dll",
+    }.ToImmutableHashSet(StringComparer.OrdinalIgnoreCase);
+
+    private static ImmutableSegmentedDictionary<string, bool> s_analyzerFullPathToIsFeatureAnalyzer = ImmutableSegmentedDictionary<string, bool>.Empty;


Do we need to cache this? It's just simple comparison of 3 strings.

You can simplify to:

bool IsFeaturesAnalyzer(this AnalyzerReference reference) { var fileNameSpan = reference.FullPath.AsSpan(FileNameUtilities.IndexOfFileName(reference.FullPath)); return fileNameSpan.Equals("Microsoft.CodeAnalysis.Features.dll", StringComparison.OrdinalIgnoreCase) || fileNameSpan.Equals("Microsoft.CodeAnalysis.CSharp.Features.dll", StringComparison.OrdinalIgnoreCase) || fileNameSpan.Equals("Microsoft.CodeAnalysis.VisualBasic.Features.dll", StringComparison.OrdinalIgnoreCase); }

agreed. dictionaries fo rthis seem unnecessary.

tmat · 2024-10-23T18:19:42Z

Reviewed up to commit 5.

sharwell · 2024-10-21T18:53:36Z

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

+            _unmappedProjectAnalyzerPaths.Add(fullPath);
+
+            // Determine if we started using SDK CodeStyle analyzers while access to _unmappedProjectAnalyzerPaths is gated.
+            containsSdkCodeStyleAnalyzers = _unmappedProjectAnalyzerPaths.Any(IsSdkCodeStyleAnalyzer);


❓ What about

containsSdkCdoeStyleAnalyzers = HasSdkCodeStyleAnalyzers || IsSdkCodeStyleAnalyzer(fullPath);

sharwell · 2024-10-21T18:54:38Z

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

+            _unmappedProjectAnalyzerPaths.Remove(fullPath);
+
+            // Determine if we are still using SDK CodeStyle analyzers while access to _unmappedProjectAnalyzerPaths is gated.
+            containsSdkCodeStyleAnalyzers = _unmappedProjectAnalyzerPaths.Any(IsSdkCodeStyleAnalyzer);


💡 Can also simplify this one:

containsSdkCodeStyleAnalyzers = IsSdkCodeStyleAnalyzer(fullPath) ? _unmappedProjectAnalyzerPaths.Any(IsSdkCodeStyleAnalyzer) : HasSdkCodeStyleAnalyzers;

CyrusNajmabadi · 2024-10-23T19:17:27Z

...tocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.StateManager.HostStates.cs


                    return new HostAnalyzerStateSets(analyzerMap);
                }
+
+                static (IEnumerable<ImmutableArray<DiagnosticAnalyzer>> HostAnalyzerCollection, IEnumerable<ImmutableArray<DiagnosticAnalyzer>> ProjectAnalyzerCollection) GetAnalyzerCollections(


def don't like returning these as lazy enumerbles. can you make both an ImmutableArray<ImmutableArray<...

I could but then we have to do something about the common case where we aren't redirecting and are returning analyzersPerReference.Values which is an IEnumerable<ImmutableArray<DiagnosticAnalyzer>>. The use of these IEnumerable's is also piped into StateManager.CreateStateSetMap. This might be cleanup for a follow up PR.

which is an IEnumerable<ImmutableArray>

Ick :) Can that one change?

at the very least, don't return mutable lists. return an immutable array here. that way we don't have to worry about it changing.

Updated to use ArrayBuilder instead of List.

JoeRobich added 3 commits October 16, 2024 13:09

Add a HasSdkCodeStyleAnalyzers property to ProjectAttributes

0a838b9

Also plumbs through updating the attribute throughout the various Solution and *State classes.

Track unmapped AnalyzerReferences added by the project system.

180455c

Use the unmapped references to update the HasSdkCodeStyleAnalyzer ProjectAttribute.

Selectively treat some Host analyzers as Project analyzers.

03eccdd

When Features analyzers are being used to replace SDK CodeStyle analyzer we need to treat them as project analyzers and not provide them with the Host's fallback options.

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

dotnet-policy-service bot added the VSCode label Oct 16, 2024

JoeRobich marked this pull request as ready for review October 22, 2024 06:49

JoeRobich requested a review from a team as a code owner October 22, 2024 06:49

JoeRobich requested review from tmat and sharwell October 22, 2024 06:49

Added integration tests

1198405

tmat reviewed Oct 23, 2024

View reviewed changes

sharwell reviewed Oct 23, 2024

View reviewed changes

sharwell approved these changes Oct 23, 2024

View reviewed changes

CyrusNajmabadi reviewed Oct 23, 2024

View reviewed changes

PR feedback

7344695

tmat approved these changes Oct 24, 2024

View reviewed changes

arunchndr approved these changes Oct 24, 2024

View reviewed changes

Backport test skip from #75612

dff04fe

JoeRobich merged commit 16f58fa into release/dev17.12 Oct 24, 2024
21 of 25 checks passed

This was referenced Nov 7, 2024

[Automated] PRs inserted in VS build main-35507.17 #75799

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35507.203 #75826

Closed

dotnet-bot mentioned this pull request Nov 19, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35518.109 #75965

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When replacing CodeStyle analyzers do not provide Features analyzers fallback options #75534

When replacing CodeStyle analyzers do not provide Features analyzers fallback options #75534

JoeRobich commented Oct 16, 2024 •

edited

Loading

tmat Oct 23, 2024

tmat Oct 23, 2024

CyrusNajmabadi Oct 23, 2024

JoeRobich Oct 23, 2024

tmat Oct 23, 2024

JoeRobich Oct 23, 2024

tmat Oct 23, 2024 •

edited

Loading

tmat Oct 23, 2024

tmat Oct 23, 2024 •

edited

Loading

CyrusNajmabadi Oct 23, 2024

tmat commented Oct 23, 2024

sharwell Oct 21, 2024

sharwell Oct 21, 2024

CyrusNajmabadi Oct 23, 2024

JoeRobich Oct 23, 2024

CyrusNajmabadi Oct 23, 2024

CyrusNajmabadi Oct 23, 2024

JoeRobich Oct 23, 2024

When replacing CodeStyle analyzers do not provide Features analyzers fallback options #75534

When replacing CodeStyle analyzers do not provide Features analyzers fallback options #75534

Conversation

JoeRobich commented Oct 16, 2024 • 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

tmat Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat commented Oct 23, 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

JoeRobich commented Oct 16, 2024 •

edited

Loading

tmat Oct 23, 2024 •

edited

Loading

tmat Oct 23, 2024 •

edited

Loading