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

Jump to bottom

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

Merged
merged 7 commits into from
Oct 24, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0a838b9
Add a HasSdkCodeStyleAnalyzers property to ProjectAttributes
JoeRobich Oct 16, 2024
180455c
Track unmapped AnalyzerReferences added by the project system.
JoeRobich Oct 16, 2024
03eccdd
Selectively treat some Host analyzers as Project analyzers.
JoeRobich Oct 16, 2024
e9f7c04
Allow the DiagnosticComputer to treat some Host analyzers as Project …
JoeRobich Oct 22, 2024
1198405
Added integration tests
JoeRobich Oct 23, 2024
7344695
PR feedback
JoeRobich Oct 24, 2024
dff04fe
Backport test skip from #75612
JoeRobich Oct 24, 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
12 changes: 1 addition & 11 deletions ...ol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.StateManager.HostStates.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.IO;
using System.Linq;
using Roslyn.Utilities;

Expand Down Expand Up @@ -78,12 +77,6 @@ static HostAnalyzerStateSets CreateLanguageSpecificAnalyzerMap(HostAnalyzerState
}
}

private static readonly ImmutableHashSet<string> FeaturesAnalyzerFileNames = [
"Microsoft.CodeAnalysis.Features.dll",
"Microsoft.CodeAnalysis.CSharp.Features.dll",
"Microsoft.CodeAnalysis.VisualBasic.Features.dll",
];

private static ImmutableHashSet<object> GetReferenceIdsToRedirectAsProjectAnalyzers(Project project)
{
if (project.State.HasSdkCodeStyleAnalyzers)
Expand All @@ -102,11 +95,8 @@ static ImmutableHashSet<object> GetFeaturesAnalyzerReferenceIds(HostDiagnosticAn

foreach (var analyzerReference in hostAnalyzers.HostAnalyzerReferences)
{
var fileName = Path.GetFileName(analyzerReference.FullPath)!;
if (FeaturesAnalyzerFileNames.Contains(fileName))
{
if (analyzerReference.IsFeaturesAnalyzer())
builder.Add(analyzerReference.Id);
}
}

return builder.ToImmutable();
Expand Down
29 changes: 29 additions & 0 deletions ...sualStudio/Core/Test/ProjectSystemShim/VisualStudioProjectTests/AnalyzerReferenceTests.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,12 +211,18 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.ProjectSystemShim
Dim project = Await environment.ProjectFactory.CreateAndAddToWorkspaceAsync(
"Project", LanguageNames.CSharp, CancellationToken.None)

' Ensure HasSdkCodeStyleAnalyzers is proper.
Assert.False(project.HasSdkCodeStyleAnalyzers)

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

' Ensure HasSdkCodeStyleAnalyzers is being properly updated.
Assert.True(project.HasSdkCodeStyleAnalyzers)

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

Expand All @@ -228,6 +234,15 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.ProjectSystemShim
{
Path.Combine(TempRoot.Root, "Dir", "File.dll")
}, environment.Workspace.CurrentSolution.Projects.Single().AnalyzerReferences.Select(Function(r) r.FullPath))

' Remove codestyle analyzers
project.RemoveAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "cs", "Microsoft.CodeAnalysis.CodeStyle.dll"))
project.RemoveAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "cs", "Microsoft.CodeAnalysis.CodeStyle.Fixes.dll"))
project.RemoveAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "cs", "Microsoft.CodeAnalysis.CSharp.CodeStyle.dll"))
project.RemoveAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "cs", "Microsoft.CodeAnalysis.CSharp.CodeStyle.Fixes.dll"))

' Ensure HasSdkCodeStyleAnalyzers is being properly updated.
Assert.False(project.HasSdkCodeStyleAnalyzers)
End Using
End Function

Expand All @@ -237,12 +252,18 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.ProjectSystemShim
Dim project = Await environment.ProjectFactory.CreateAndAddToWorkspaceAsync(
"Project", LanguageNames.VisualBasic, CancellationToken.None)

' Ensure HasSdkCodeStyleAnalyzers is proper.
Assert.False(project.HasSdkCodeStyleAnalyzers)

' 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 HasSdkCodeStyleAnalyzers is being properly updated.
Assert.True(project.HasSdkCodeStyleAnalyzers)

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

Expand All @@ -254,6 +275,14 @@ Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.ProjectSystemShim
{
Path.Combine(TempRoot.Root, "Dir", "File.dll")
}, environment.Workspace.CurrentSolution.Projects.Single().AnalyzerReferences.Select(Function(r) r.FullPath))

project.RemoveAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "vb", "Microsoft.CodeAnalysis.CodeStyle.dll"))
project.RemoveAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "vb", "Microsoft.CodeAnalysis.CodeStyle.Fixes.dll"))
project.RemoveAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "vb", "Microsoft.CodeAnalysis.VisualBasic.CodeStyle.dll"))
project.RemoveAnalyzerReference(Path.Combine(TempRoot.Root, "Sdks", "Microsoft.NET.Sdk", "codestyle", "vb", "Microsoft.CodeAnalysis.VisualBasic.CodeStyle.Fixes.dll"))

' Ensure HasSdkCodeStyleAnalyzers is being properly updated.
Assert.False(project.HasSdkCodeStyleAnalyzers)
End Using
End Function
End Class
Expand Down
22 changes: 21 additions & 1 deletion src/Workspaces/Core/Portable/Diagnostics/Extensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -81,14 +82,33 @@ public static string GetAnalyzerId(this DiagnosticAnalyzer analyzer)

private static string GetAssemblyQualifiedName(Type type)
{
// AnalyzerFileReference now includes things like versions, public key as part of its identity.
// AnalyzerFileReference now includes things like versions, public key as part of its identity.
// so we need to consider them.
return RoslynImmutableInterlocked.GetOrAdd(
ref s_typeToAssemblyQualifiedName,
type,
static type => type.AssemblyQualifiedName ?? throw ExceptionUtilities.UnexpectedValue(type));
}

private static readonly ImmutableHashSet<string> s_featuresAnalyzerFileNames = new[] {
"Microsoft.CodeAnalysis.Features.dll",
"Microsoft.CodeAnalysis.CSharp.Features.dll",
"Microsoft.CodeAnalysis.VisualBasic.Features.dll",
}.ToImmutableHashSet(StringComparer.OrdinalIgnoreCase);

private static ImmutableSegmentedDictionary<string, bool> s_analyzerFullPathToIsFeatureAnalyzer = ImmutableSegmentedDictionary<string, bool>.Empty;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@tmat tmat Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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);
}

Copy link
Member

Choose a reason for hiding this comment

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

agreed. dictionaries fo rthis seem unnecessary.


public static bool IsFeaturesAnalyzer(this AnalyzerReference reference)
{
if (reference.FullPath is null)
return false;

return RoslynImmutableInterlocked.GetOrAdd(
ref s_analyzerFullPathToIsFeatureAnalyzer,
reference.FullPath,
static fullPath => s_featuresAnalyzerFileNames.Contains(Path.GetFileName(fullPath)));
}

public static async Task<ImmutableDictionary<DiagnosticAnalyzer, DiagnosticAnalysisResultBuilder>> ToResultBuilderMapAsync(
this AnalysisResult analysisResult,
ImmutableArray<Diagnostic> additionalPragmaSuppressionDiagnostics,
Expand Down
26 changes: 22 additions & 4 deletions src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ internal class DiagnosticComputer
/// The <see cref="CompilationWithAnalyzers"/> instance is shared between all the following document analyses modes for the project:
/// 1. Span-based analysis for active document (lightbulb)
/// 2. Background analysis for active and open documents.
///
///
/// NOTE: We do not re-use this cache for project analysis as it leads to significant memory increase in the OOP process.
/// Additionally, we only store the cache entry for the last project to be analyzed instead of maintaining a CWT keyed off
/// each project in the solution, as the CWT does not seem to drop entries until ForceGC happens, leading to significant memory
Expand Down Expand Up @@ -171,7 +171,7 @@ private async Task<SerializableDiagnosticAnalysisResults> GetHighPriorityDiagnos
// the executing high priority tasks before starting its execution.
// - Note that it is critical to do this step prior to Step 3 below to ensure that
// any canceled normal priority tasks in Step 3 do not resume execution prior to
// completion of this high priority computeTask.
// completion of this high priority computeTask.
lock (s_gate)
{
Debug.Assert(!s_highPriorityComputeTasks.Contains(computeTask));
Expand Down Expand Up @@ -583,7 +583,18 @@ private async Task<CompilationWithAnalyzersCacheEntry> CreateCompilationWithAnal
}

var analyzers = reference.GetAnalyzers(_project.Language);
hostAnalyzerBuilder.AddRange(analyzers);

// At times some Host analyzers should be treated as project analyzers and
// not be given access to the Host fallback options. In particular when we
// replace SDK CodeStyle analyzers with the Features analyzers.
if (ShouldRedirectAnalyzers(_project, reference))
{
projectAnalyzerBuilder.AddRange(analyzers);
}
else
{
hostAnalyzerBuilder.AddRange(analyzers);
}
analyzerMapBuilder.AppendAnalyzerMap(analyzers);
}

Expand All @@ -607,6 +618,13 @@ private async Task<CompilationWithAnalyzersCacheEntry> CreateCompilationWithAnal
var analyzerToIdMap = new BidirectionalMap<string, DiagnosticAnalyzer>(analyzerMapBuilder);

return new CompilationWithAnalyzersCacheEntry(_solutionChecksum, _project, compilationWithAnalyzers, analyzerToIdMap);

static bool ShouldRedirectAnalyzers(Project project, AnalyzerReference reference)
{
// When replacing SDK CodeStyle analyzers we should redirect Features analyzers
// so they are treated as project analyzers.
return project.State.HasSdkCodeStyleAnalyzers && reference.IsFeaturesAnalyzer();
}
}

private async Task<CompilationWithAnalyzersPair> CreateCompilationWithAnalyzerAsync(ImmutableArray<DiagnosticAnalyzer> projectAnalyzers, ImmutableArray<DiagnosticAnalyzer> hostAnalyzers, CancellationToken cancellationToken)
Expand All @@ -626,7 +644,7 @@ private async Task<CompilationWithAnalyzersPair> CreateCompilationWithAnalyzerAs

// Run analyzers concurrently, with performance logging and reporting suppressed diagnostics.
// This allows all client requests with or without performance data and/or suppressed diagnostics to be satisfied.
// TODO: can we support analyzerExceptionFilter in remote host?
// TODO: can we support analyzerExceptionFilter in remote host?
// right now, host doesn't support watson, we might try to use new NonFatal watson API?
var projectAnalyzerOptions = new CompilationWithAnalyzersOptions(
options: _project.AnalyzerOptions,
Expand Down
Loading