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.

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

Include project diagnostic suppressors in host analyzer execution #75684

Merged
merged 4 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,9 @@ private async Task<DiagnosticAnalysisResultMap<DiagnosticAnalyzer, DiagnosticAna

if (hostAnalysisResult is not null)
{
telemetry = telemetry.AddRange(hostAnalysisResult.AnalyzerTelemetryInfo);
// Use SetItems instead of AddRange so exceptions will not occur if diagnostic suppressors are
// counted in both project and host analysis results.
telemetry = telemetry.SetItems(hostAnalysisResult.AnalyzerTelemetryInfo);
sharwell marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,15 @@ private static (ImmutableArray<DiagnosticAnalyzer> projectAnalyzers, ImmutableAr
}
}

return (projectBuilder.ToImmutableAndClear(), hostBuilder.ToImmutableAndClear());
var projectAnalyzers = projectBuilder.ToImmutableAndClear();

if (hostAnalyzerIds.Any())
{
// If any host analyzers are active, make sure to also include any project diagnostic suppressors
hostBuilder.AddRange(projectAnalyzers.WhereAsArray(static a => a is DiagnosticSuppressor));
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Don't need "WhereAsArray" here, "Where" should do.

}

return (projectAnalyzers, hostBuilder.ToImmutableAndClear());
}

private async Task<(CompilationWithAnalyzersPair? compilationWithAnalyzers, BidirectionalMap<string, DiagnosticAnalyzer> analyzerToIdMap)> GetOrCreateCompilationWithAnalyzersAsync(CancellationToken cancellationToken)
Expand Down Expand Up @@ -609,6 +617,7 @@ private async Task<CompilationWithAnalyzersCacheEntry> CreateCompilationWithAnal

var analyzers = reference.GetAnalyzers(_project.Language);
projectAnalyzerBuilder.AddRange(analyzers);
hostAnalyzerBuilder.AddRange(analyzers.WhereAsArray(static a => a is DiagnosticSuppressor));
analyzerMapBuilder.AppendAnalyzerMap(analyzers);
}

Expand Down
Loading