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

Include project diagnostic suppressors in host analyzer execution #75684

Merged
merged 4 commits into from
Nov 22, 2024
Merged

Include project diagnostic suppressors in host analyzer execution #75684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8d2f1e
Include project diagnostic suppressors in host analyzer execution
sharwell Oct 31, 2024
dff447c
Wrap analysis results in AnalysisResultPair
sharwell Nov 21, 2024
b15ca1f
Remove unused parameters
sharwell Nov 21, 2024
aa3df16
Avoid possible key collection in analyzer telemetry
sharwell Nov 21, 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
29 changes: 11 additions & 18 deletions ...Features/CSharpTest/Diagnostics/DiagnosticAnalyzerDriver/DiagnosticAnalyzerDriverTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,28 +656,21 @@ await TestNuGetAndVsixAnalyzerCoreAsync(
// 1) No duplicate diagnostics
// 2) Both NuGet and Vsix analyzers execute
// 3) Appropriate diagnostic filtering is done - Nuget suppressor suppresses VSIX analyzer.
//
// 🐛 After splitting fallback options into separate CompilationWithAnalyzers for project and host analyzers,
// NuGet-installed suppressors no longer act on VSIX-installed analyzer diagnostics. Fixing this requires us to
// add NuGet-installed analyzer references to the host CompilationWithAnalyzers, with an additional flag
// indicating that only suppressors should run for these references.
// https://github.com/dotnet/roslyn/issues/75399
const bool FalseButShouldBeTrue = false;
await TestNuGetAndVsixAnalyzerCoreAsync(
nugetAnalyzers: ImmutableArray.Create(firstNugetAnalyzer),
expectedNugetAnalyzersExecuted: true,
vsixAnalyzers: ImmutableArray.Create(vsixAnalyzer),
expectedVsixAnalyzersExecuted: true,
nugetSuppressors: ImmutableArray.Create(nugetSuppressor),
expectedNugetSuppressorsExecuted: FalseButShouldBeTrue,
expectedNugetSuppressorsExecuted: true,
vsixSuppressors: ImmutableArray<VsixSuppressor>.Empty,
expectedVsixSuppressorsExecuted: false,
new[]
{
(Diagnostic("A", "Class").WithLocation(1, 7), nameof(NuGetAnalyzer)),
(Diagnostic("X", "Class", isSuppressed: FalseButShouldBeTrue).WithLocation(1, 7), nameof(VsixAnalyzer)),
(Diagnostic("Y", "Class", isSuppressed: FalseButShouldBeTrue).WithLocation(1, 7), nameof(VsixAnalyzer)),
(Diagnostic("Z", "Class", isSuppressed: FalseButShouldBeTrue).WithLocation(1, 7), nameof(VsixAnalyzer))
(Diagnostic("X", "Class", isSuppressed: true).WithLocation(1, 7), nameof(VsixAnalyzer)),
(Diagnostic("Y", "Class", isSuppressed: true).WithLocation(1, 7), nameof(VsixAnalyzer)),
(Diagnostic("Z", "Class", isSuppressed: true).WithLocation(1, 7), nameof(VsixAnalyzer))
});

// Suppressors with duplicate support for VsixAnalyzer, but not 100% overlap. Verify the following:
Expand All @@ -691,15 +684,15 @@ await TestNuGetAndVsixAnalyzerCoreAsync(
vsixAnalyzers: ImmutableArray.Create(vsixAnalyzer),
expectedVsixAnalyzersExecuted: true,
nugetSuppressors: ImmutableArray.Create(partialNugetSuppressor),
expectedNugetSuppressorsExecuted: FalseButShouldBeTrue,
expectedNugetSuppressorsExecuted: true,
vsixSuppressors: ImmutableArray.Create(vsixSuppressor),
expectedVsixSuppressorsExecuted: false,
new[]
{
(Diagnostic("A", "Class").WithLocation(1, 7), nameof(NuGetAnalyzer)),
(Diagnostic("X", "Class", isSuppressed: false).WithLocation(1, 7), nameof(VsixAnalyzer)),
(Diagnostic("Y", "Class", isSuppressed: FalseButShouldBeTrue).WithLocation(1, 7), nameof(VsixAnalyzer)),
(Diagnostic("Z", "Class", isSuppressed: FalseButShouldBeTrue).WithLocation(1, 7), nameof(VsixAnalyzer))
(Diagnostic("Y", "Class", isSuppressed: true).WithLocation(1, 7), nameof(VsixAnalyzer)),
(Diagnostic("Z", "Class", isSuppressed: true).WithLocation(1, 7), nameof(VsixAnalyzer))
});

// Suppressors with duplicate support for VsixAnalyzer, with 100% overlap. Verify the following:
Expand All @@ -713,15 +706,15 @@ await TestNuGetAndVsixAnalyzerCoreAsync(
vsixAnalyzers: ImmutableArray.Create(vsixAnalyzer),
expectedVsixAnalyzersExecuted: true,
nugetSuppressors: ImmutableArray.Create(nugetSuppressor),
expectedNugetSuppressorsExecuted: FalseButShouldBeTrue,
expectedNugetSuppressorsExecuted: true,
vsixSuppressors: ImmutableArray.Create(vsixSuppressor),
expectedVsixSuppressorsExecuted: false,
new[]
{
(Diagnostic("A", "Class").WithLocation(1, 7), nameof(NuGetAnalyzer)),
(Diagnostic("X", "Class", isSuppressed: FalseButShouldBeTrue).WithLocation(1, 7), nameof(VsixAnalyzer)),
(Diagnostic("Y", "Class", isSuppressed: FalseButShouldBeTrue).WithLocation(1, 7), nameof(VsixAnalyzer)),
(Diagnostic("Z", "Class", isSuppressed: FalseButShouldBeTrue).WithLocation(1, 7), nameof(VsixAnalyzer))
(Diagnostic("X", "Class", isSuppressed: true).WithLocation(1, 7), nameof(VsixAnalyzer)),
(Diagnostic("Y", "Class", isSuppressed: true).WithLocation(1, 7), nameof(VsixAnalyzer)),
(Diagnostic("Z", "Class", isSuppressed: true).WithLocation(1, 7), nameof(VsixAnalyzer))
});
}

Expand Down
2 changes: 2 additions & 0 deletions src/LanguageServer/Protocol/Features/Diagnostics/DocumentAnalysisExecutor_Helpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ static string GetLanguageSpecificId(string? language, string noLanguageId, strin
// Create driver that holds onto compilation and associated analyzers
var filteredProjectAnalyzers = projectAnalyzers.WhereAsArray(static a => !a.IsWorkspaceDiagnosticAnalyzer());
var filteredHostAnalyzers = hostAnalyzers.WhereAsArray(static a => !a.IsWorkspaceDiagnosticAnalyzer());
var filteredProjectSuppressors = filteredProjectAnalyzers.WhereAsArray(static a => a is DiagnosticSuppressor);
filteredHostAnalyzers = filteredHostAnalyzers.AddRange(filteredProjectSuppressors);
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 This path is hit by the test that was failing, and revealed a case of AddRange causing a problem. It's not clear how well this or other paths are exercised, so I'm certainly concerned with bugs slipping through.


// PERF: there is no analyzers for this compilation.
// compilationWithAnalyzer will throw if it is created with no analyzers which is perf optimization.
Expand Down
43 changes: 10 additions & 33 deletions ...es/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InProcOrRemoteHostAnalyzerRunner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,14 +122,14 @@ private async Task<DiagnosticAnalysisResultMap<DiagnosticAnalyzer, DiagnosticAna
{
var version = await DiagnosticIncrementalAnalyzer.GetDiagnosticVersionAsync(project, cancellationToken).ConfigureAwait(false);

var (projectAnalysisResult, hostAnalysisResult, additionalPragmaSuppressionDiagnostics) = await compilationWithAnalyzers.GetAnalysisResultAsync(
var (analysisResult, additionalPragmaSuppressionDiagnostics) = await compilationWithAnalyzers.GetAnalysisResultAsync(
documentAnalysisScope, project, AnalyzerInfoCache, cancellationToken).ConfigureAwait(false);

if (logPerformanceInfo)
{
// if remote host is there, report performance data
var asyncToken = _asyncOperationListener.BeginAsyncOperation(nameof(AnalyzeInProcAsync));
var _ = FireAndForgetReportAnalyzerPerformanceAsync(documentAnalysisScope, project, client, projectAnalysisResult, hostAnalysisResult, cancellationToken).CompletesAsyncOperation(asyncToken);
var _ = FireAndForgetReportAnalyzerPerformanceAsync(documentAnalysisScope, project, client, analysisResult, cancellationToken).CompletesAsyncOperation(asyncToken);
}

var projectAnalyzers = documentAnalysisScope?.ProjectAnalyzers ?? compilationWithAnalyzers.ProjectAnalyzers;
Expand All @@ -138,37 +138,20 @@ private async Task<DiagnosticAnalysisResultMap<DiagnosticAnalyzer, DiagnosticAna

// get compiler result builder map
var builderMap = ImmutableDictionary<DiagnosticAnalyzer, DiagnosticAnalysisResultBuilder>.Empty;
if (projectAnalysisResult is not null)
if (analysisResult is not null)
{
var map = await projectAnalysisResult.ToResultBuilderMapAsync(
var map = await analysisResult.ToResultBuilderMapAsync(
additionalPragmaSuppressionDiagnostics, documentAnalysisScope, project, version,
compilationWithAnalyzers.ProjectCompilation!, projectAnalyzers, skippedAnalyzersInfo,
compilationWithAnalyzers.ReportSuppressedDiagnostics, cancellationToken).ConfigureAwait(false);
builderMap = builderMap.AddRange(map);
}

if (hostAnalysisResult is not null)
{
var map = await hostAnalysisResult.ToResultBuilderMapAsync(
additionalPragmaSuppressionDiagnostics, documentAnalysisScope, project, version,
compilationWithAnalyzers.HostCompilation!, hostAnalyzers, skippedAnalyzersInfo,
projectAnalyzers, hostAnalyzers, skippedAnalyzersInfo,
compilationWithAnalyzers.ReportSuppressedDiagnostics, cancellationToken).ConfigureAwait(false);
builderMap = builderMap.AddRange(map);
}

var result = builderMap.ToImmutableDictionary(kv => kv.Key, kv => DiagnosticAnalysisResult.CreateFromBuilder(kv.Value));
var telemetry = ImmutableDictionary<DiagnosticAnalyzer, AnalyzerTelemetryInfo>.Empty;
if (getTelemetryInfo)
if (getTelemetryInfo && analysisResult is not null)
{
if (projectAnalysisResult is not null)
{
telemetry = telemetry.AddRange(projectAnalysisResult.AnalyzerTelemetryInfo);
}

if (hostAnalysisResult is not null)
{
telemetry = telemetry.AddRange(hostAnalysisResult.AnalyzerTelemetryInfo);
}
telemetry = analysisResult.MergedAnalyzerTelemetryInfo;
}

return DiagnosticAnalysisResultMap.Create(result, telemetry);
Expand All @@ -178,8 +161,7 @@ private async Task FireAndForgetReportAnalyzerPerformanceAsync(
DocumentAnalysisScope? documentAnalysisScope,
Project project,
RemoteHostClient? client,
AnalysisResult? projectAnalysisResult,
AnalysisResult? hostAnalysisResult,
AnalysisResultPair? analysisResult,
CancellationToken cancellationToken)
{
if (client == null)
Expand All @@ -194,14 +176,9 @@ private async Task FireAndForgetReportAnalyzerPerformanceAsync(
var forSpanAnalysis = documentAnalysisScope?.Span.HasValue ?? false;

ImmutableArray<AnalyzerPerformanceInfo> performanceInfo = [];
if (projectAnalysisResult is not null)
{
performanceInfo = performanceInfo.AddRange(projectAnalysisResult.AnalyzerTelemetryInfo.ToAnalyzerPerformanceInfo(AnalyzerInfoCache));
}

if (hostAnalysisResult is not null)
if (analysisResult is not null)
{
performanceInfo = performanceInfo.AddRange(hostAnalysisResult.AnalyzerTelemetryInfo.ToAnalyzerPerformanceInfo(AnalyzerInfoCache));
performanceInfo = performanceInfo.AddRange(analysisResult.MergedAnalyzerTelemetryInfo.ToAnalyzerPerformanceInfo(AnalyzerInfoCache));
}

_ = await client.TryInvokeAsync<IRemoteDiagnosticAnalyzerService>(
Expand Down
Loading
Loading