From c8d2f1ef84ec23879fed259ff580ecd4144e1bed Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 31 Oct 2024 16:07:47 -0500 Subject: [PATCH 1/4] Include project diagnostic suppressors in host analyzer execution Fixes #75399 --- .../DiagnosticAnalyzerDriverTests.cs | 29 +++++++------------ .../DocumentAnalysisExecutor_Helpers.cs | 2 ++ ...alyzer.InProcOrRemoteHostAnalyzerRunner.cs | 4 ++- .../DiagnosticAnalyzer/DiagnosticComputer.cs | 11 ++++++- 4 files changed, 26 insertions(+), 20 deletions(-) diff --git a/src/EditorFeatures/CSharpTest/Diagnostics/DiagnosticAnalyzerDriver/DiagnosticAnalyzerDriverTests.cs b/src/EditorFeatures/CSharpTest/Diagnostics/DiagnosticAnalyzerDriver/DiagnosticAnalyzerDriverTests.cs index cc31be1188cb3..c4f33d130bbb8 100644 --- a/src/EditorFeatures/CSharpTest/Diagnostics/DiagnosticAnalyzerDriver/DiagnosticAnalyzerDriverTests.cs +++ b/src/EditorFeatures/CSharpTest/Diagnostics/DiagnosticAnalyzerDriver/DiagnosticAnalyzerDriverTests.cs @@ -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.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: @@ -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: @@ -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)) }); } diff --git a/src/LanguageServer/Protocol/Features/Diagnostics/DocumentAnalysisExecutor_Helpers.cs b/src/LanguageServer/Protocol/Features/Diagnostics/DocumentAnalysisExecutor_Helpers.cs index 12b8d9ceac862..41dade51c947f 100644 --- a/src/LanguageServer/Protocol/Features/Diagnostics/DocumentAnalysisExecutor_Helpers.cs +++ b/src/LanguageServer/Protocol/Features/Diagnostics/DocumentAnalysisExecutor_Helpers.cs @@ -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); // PERF: there is no analyzers for this compilation. // compilationWithAnalyzer will throw if it is created with no analyzers which is perf optimization. diff --git a/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InProcOrRemoteHostAnalyzerRunner.cs b/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InProcOrRemoteHostAnalyzerRunner.cs index 9523e3dd6c3d8..efc68d8751622 100644 --- a/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InProcOrRemoteHostAnalyzerRunner.cs +++ b/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InProcOrRemoteHostAnalyzerRunner.cs @@ -167,7 +167,9 @@ private async Task 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)); + } + + return (projectAnalyzers, hostBuilder.ToImmutableAndClear()); } private async Task<(CompilationWithAnalyzersPair? compilationWithAnalyzers, BidirectionalMap analyzerToIdMap)> GetOrCreateCompilationWithAnalyzersAsync(CancellationToken cancellationToken) @@ -609,6 +617,7 @@ private async Task CreateCompilationWithAnal var analyzers = reference.GetAnalyzers(_project.Language); projectAnalyzerBuilder.AddRange(analyzers); + hostAnalyzerBuilder.AddRange(analyzers.WhereAsArray(static a => a is DiagnosticSuppressor)); analyzerMapBuilder.AppendAnalyzerMap(analyzers); } From dff447c63bea432b695a07c7da32abb7e0a219bd Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 21 Nov 2024 15:53:30 -0600 Subject: [PATCH 2/4] Wrap analysis results in AnalysisResultPair --- ...alyzer.InProcOrRemoteHostAnalyzerRunner.cs | 45 +--- .../Diagnostics/AnalysisResultPair.cs | 232 ++++++++++++++++++ .../CompilationWithAnalyzersPair.cs | 17 +- .../Core/Portable/Diagnostics/Extensions.cs | 58 +++-- .../DiagnosticAnalyzer/DiagnosticComputer.cs | 41 ++-- 5 files changed, 299 insertions(+), 94 deletions(-) create mode 100644 src/Workspaces/Core/Portable/Diagnostics/AnalysisResultPair.cs diff --git a/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InProcOrRemoteHostAnalyzerRunner.cs b/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InProcOrRemoteHostAnalyzerRunner.cs index efc68d8751622..712f4aaafc25d 100644 --- a/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InProcOrRemoteHostAnalyzerRunner.cs +++ b/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InProcOrRemoteHostAnalyzerRunner.cs @@ -122,14 +122,14 @@ private async Task.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, + compilationWithAnalyzers.ProjectCompilation, compilationWithAnalyzers.HostCompilation, 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.Empty; - if (getTelemetryInfo) + if (getTelemetryInfo && analysisResult is not null) { - if (projectAnalysisResult is not null) - { - telemetry = telemetry.AddRange(projectAnalysisResult.AnalyzerTelemetryInfo); - } - - if (hostAnalysisResult is not null) - { - // 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); - } + telemetry = analysisResult.MergedAnalyzerTelemetryInfo; } return DiagnosticAnalysisResultMap.Create(result, telemetry); @@ -180,8 +161,7 @@ private async Task FireAndForgetReportAnalyzerPerformanceAsync( DocumentAnalysisScope? documentAnalysisScope, Project project, RemoteHostClient? client, - AnalysisResult? projectAnalysisResult, - AnalysisResult? hostAnalysisResult, + AnalysisResultPair? analysisResult, CancellationToken cancellationToken) { if (client == null) @@ -196,14 +176,9 @@ private async Task FireAndForgetReportAnalyzerPerformanceAsync( var forSpanAnalysis = documentAnalysisScope?.Span.HasValue ?? false; ImmutableArray 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( diff --git a/src/Workspaces/Core/Portable/Diagnostics/AnalysisResultPair.cs b/src/Workspaces/Core/Portable/Diagnostics/AnalysisResultPair.cs new file mode 100644 index 0000000000000..6a3b25d488546 --- /dev/null +++ b/src/Workspaces/Core/Portable/Diagnostics/AnalysisResultPair.cs @@ -0,0 +1,232 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Immutable; +using Microsoft.CodeAnalysis.Diagnostics.Telemetry; +using Roslyn.Utilities; + +namespace Microsoft.CodeAnalysis.Diagnostics; + +internal sealed class AnalysisResultPair +{ + private readonly AnalysisResult? _projectAnalysisResult; + private readonly AnalysisResult? _hostAnalysisResult; + + private ImmutableDictionary>>? _lazyMergedSyntaxDiagnostics; + private ImmutableDictionary>>? _lazyMergedSemanticDiagnostics; + private ImmutableDictionary>>? _lazyMergedAdditionalFileDiagnostics; + private ImmutableDictionary>? _lazyMergedCompilationDiagnostics; + private ImmutableDictionary? _lazyMergedAnalyzerTelemetryInfo; + + public AnalysisResultPair(AnalysisResult? projectAnalysisResult, AnalysisResult? hostAnalysisResult) + { + if (projectAnalysisResult is not null && hostAnalysisResult is not null) + { + } + else + { + Contract.ThrowIfTrue(projectAnalysisResult is null && hostAnalysisResult is null); + } + + _projectAnalysisResult = projectAnalysisResult; + _hostAnalysisResult = hostAnalysisResult; + } + + public AnalysisResult? ProjectAnalysisResult => _projectAnalysisResult; + + public AnalysisResult? HostAnalysisResult => _hostAnalysisResult; + + public ImmutableDictionary>> MergedSyntaxDiagnostics + { + get + { + return InterlockedOperations.Initialize( + ref _lazyMergedSyntaxDiagnostics, + static arg => + { + if (arg.projectDiagnostics is null) + { + // project and host diagnostics cannot both be null + Contract.ThrowIfNull(arg.hostDiagnostics); + return arg.hostDiagnostics; + } + else if (arg.hostDiagnostics is null) + { + return arg.projectDiagnostics; + } + + return MergeDiagnostics(arg.projectDiagnostics, arg.hostDiagnostics); + }, + (projectDiagnostics: ProjectAnalysisResult?.SyntaxDiagnostics, hostDiagnostics: HostAnalysisResult?.SyntaxDiagnostics)); + } + } + + public ImmutableDictionary>> MergedSemanticDiagnostics + { + get + { + return InterlockedOperations.Initialize( + ref _lazyMergedSemanticDiagnostics, + static arg => + { + if (arg.projectDiagnostics is null) + { + // project and host diagnostics cannot both be null + Contract.ThrowIfNull(arg.hostDiagnostics); + return arg.hostDiagnostics; + } + else if (arg.hostDiagnostics is null) + { + return arg.projectDiagnostics; + } + + return MergeDiagnostics(arg.projectDiagnostics, arg.hostDiagnostics); + }, + (projectDiagnostics: ProjectAnalysisResult?.SemanticDiagnostics, hostDiagnostics: HostAnalysisResult?.SemanticDiagnostics)); + } + } + + public ImmutableDictionary>> MergedAdditionalFileDiagnostics + { + get + { + return InterlockedOperations.Initialize( + ref _lazyMergedAdditionalFileDiagnostics, + static arg => + { + if (arg.projectDiagnostics is null) + { + // project and host diagnostics cannot both be null + Contract.ThrowIfNull(arg.hostDiagnostics); + return arg.hostDiagnostics; + } + else if (arg.hostDiagnostics is null) + { + return arg.projectDiagnostics; + } + + return MergeDiagnostics(arg.projectDiagnostics, arg.hostDiagnostics); + }, + (projectDiagnostics: ProjectAnalysisResult?.AdditionalFileDiagnostics, hostDiagnostics: HostAnalysisResult?.AdditionalFileDiagnostics)); + } + } + + public ImmutableDictionary> MergedCompilationDiagnostics + { + get + { + return InterlockedOperations.Initialize( + ref _lazyMergedCompilationDiagnostics, + static arg => + { + if (arg.projectDiagnostics is null) + { + // project and host diagnostics cannot both be null + Contract.ThrowIfNull(arg.hostDiagnostics); + return arg.hostDiagnostics; + } + else if (arg.hostDiagnostics is null) + { + return arg.projectDiagnostics; + } + + return MergeDiagnostics(arg.projectDiagnostics, arg.hostDiagnostics); + }, + (projectDiagnostics: ProjectAnalysisResult?.CompilationDiagnostics, hostDiagnostics: HostAnalysisResult?.CompilationDiagnostics)); + } + } + + public ImmutableDictionary MergedAnalyzerTelemetryInfo + { + get + { + return InterlockedOperations.Initialize( + ref _lazyMergedAnalyzerTelemetryInfo, + static arg => + { + if (arg.projectTelemetryInfo is null) + { + // project and host telemetry cannot both be null + Contract.ThrowIfNull(arg.hostTelemetryInfo); + return arg.hostTelemetryInfo; + } + else if (arg.hostTelemetryInfo is null) + { + return arg.projectTelemetryInfo; + } + + return MergeTelemetry(arg.projectTelemetryInfo, arg.hostTelemetryInfo); + }, + (projectTelemetryInfo: ProjectAnalysisResult?.AnalyzerTelemetryInfo, hostTelemetryInfo: HostAnalysisResult?.AnalyzerTelemetryInfo)); + } + } + + public static AnalysisResultPair? FromResult(AnalysisResult? projectAnalysisResult, AnalysisResult? hostAnalysisResult) + { + if (projectAnalysisResult is null && hostAnalysisResult is null) + return null; + + return new AnalysisResultPair(projectAnalysisResult, hostAnalysisResult); + } + + private static ImmutableDictionary>> MergeDiagnostics( + ImmutableDictionary>> first, + ImmutableDictionary>> second) + where TKey : class + { + var localSyntaxDiagnostics = first.ToBuilder(); + foreach (var (tree, treeDiagnostics) in second) + { + if (!localSyntaxDiagnostics.TryGetValue(tree, out var projectSyntaxDiagnostics)) + { + localSyntaxDiagnostics.Add(tree, treeDiagnostics); + continue; + } + + localSyntaxDiagnostics[tree] = MergeDiagnostics(projectSyntaxDiagnostics, treeDiagnostics); + } + + return localSyntaxDiagnostics.ToImmutable(); + } + + private static ImmutableDictionary> MergeDiagnostics( + ImmutableDictionary> first, + ImmutableDictionary> second) + { + var analyzerToDiagnostics = first.ToBuilder(); + foreach (var (analyzer, diagnostics) in second) + { + if (!analyzerToDiagnostics.TryGetValue(analyzer, out var firstDiagnostics)) + { + analyzerToDiagnostics.Add(analyzer, diagnostics); + continue; + } + + analyzerToDiagnostics[analyzer] = firstDiagnostics.AddRange(diagnostics); + } + + return analyzerToDiagnostics.ToImmutable(); + } + + private static ImmutableDictionary MergeTelemetry( + ImmutableDictionary first, + ImmutableDictionary second) + { + var analyzerToDiagnostics = first.ToBuilder(); + foreach (var (analyzer, telemetry) in second) + { + if (!analyzerToDiagnostics.TryGetValue(analyzer, out var firstTelemetry)) + { + analyzerToDiagnostics.Add(analyzer, telemetry); + continue; + } + + // For telemetry info, keep whichever instance had the longest time + if (telemetry.ExecutionTime > firstTelemetry.ExecutionTime) + analyzerToDiagnostics[analyzer] = telemetry; + } + + return analyzerToDiagnostics.ToImmutable(); + } +} diff --git a/src/Workspaces/Core/Portable/Diagnostics/CompilationWithAnalyzersPair.cs b/src/Workspaces/Core/Portable/Diagnostics/CompilationWithAnalyzersPair.cs index 3c34a9b9da17a..7388477be4129 100644 --- a/src/Workspaces/Core/Portable/Diagnostics/CompilationWithAnalyzersPair.cs +++ b/src/Workspaces/Core/Portable/Diagnostics/CompilationWithAnalyzersPair.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Immutable; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -66,7 +67,7 @@ public Task GetAnalyzerTelemetryInfoAsync(DiagnosticAnaly } } - public async Task<(AnalysisResult? projectAnalysisResult, AnalysisResult? hostAnalysisResult)> GetAnalysisResultAsync(CancellationToken cancellationToken) + public async Task GetAnalysisResultAsync(CancellationToken cancellationToken) { var projectAnalysisResult = ProjectCompilationWithAnalyzers is not null ? await ProjectCompilationWithAnalyzers.GetAnalysisResultAsync(cancellationToken).ConfigureAwait(false) @@ -75,10 +76,10 @@ public Task GetAnalyzerTelemetryInfoAsync(DiagnosticAnaly ? await HostCompilationWithAnalyzers.GetAnalysisResultAsync(cancellationToken).ConfigureAwait(false) : null; - return (projectAnalysisResult, hostAnalysisResult); + return AnalysisResultPair.FromResult(projectAnalysisResult, hostAnalysisResult); } - public async Task<(AnalysisResult? projectAnalysisResult, AnalysisResult? hostAnalysisResult)> GetAnalysisResultAsync(SyntaxTree tree, TextSpan? filterSpan, ImmutableArray projectAnalyzers, ImmutableArray hostAnalyzers, CancellationToken cancellationToken) + public async Task GetAnalysisResultAsync(SyntaxTree tree, TextSpan? filterSpan, ImmutableArray projectAnalyzers, ImmutableArray hostAnalyzers, CancellationToken cancellationToken) { var projectAnalysisResult = projectAnalyzers.Any() ? await ProjectCompilationWithAnalyzers!.GetAnalysisResultAsync(tree, filterSpan, projectAnalyzers, cancellationToken).ConfigureAwait(false) @@ -87,10 +88,10 @@ public Task GetAnalyzerTelemetryInfoAsync(DiagnosticAnaly ? await HostCompilationWithAnalyzers!.GetAnalysisResultAsync(tree, filterSpan, hostAnalyzers, cancellationToken).ConfigureAwait(false) : null; - return (projectAnalysisResult, hostAnalysisResult); + return AnalysisResultPair.FromResult(projectAnalysisResult, hostAnalysisResult); } - public async Task<(AnalysisResult? projectAnalysisResult, AnalysisResult? hostAnalysisResult)> GetAnalysisResultAsync(AdditionalText file, TextSpan? filterSpan, ImmutableArray projectAnalyzers, ImmutableArray hostAnalyzers, CancellationToken cancellationToken) + public async Task GetAnalysisResultAsync(AdditionalText file, TextSpan? filterSpan, ImmutableArray projectAnalyzers, ImmutableArray hostAnalyzers, CancellationToken cancellationToken) { var projectAnalysisResult = projectAnalyzers.Any() ? await ProjectCompilationWithAnalyzers!.GetAnalysisResultAsync(file, filterSpan, projectAnalyzers, cancellationToken).ConfigureAwait(false) @@ -99,10 +100,10 @@ public Task GetAnalyzerTelemetryInfoAsync(DiagnosticAnaly ? await HostCompilationWithAnalyzers!.GetAnalysisResultAsync(file, filterSpan, hostAnalyzers, cancellationToken).ConfigureAwait(false) : null; - return (projectAnalysisResult, hostAnalysisResult); + return AnalysisResultPair.FromResult(projectAnalysisResult, hostAnalysisResult); } - public async Task<(AnalysisResult? projectAnalysisResult, AnalysisResult? hostAnalysisResult)> GetAnalysisResultAsync(SemanticModel model, TextSpan? filterSpan, ImmutableArray projectAnalyzers, ImmutableArray hostAnalyzers, CancellationToken cancellationToken) + public async Task GetAnalysisResultAsync(SemanticModel model, TextSpan? filterSpan, ImmutableArray projectAnalyzers, ImmutableArray hostAnalyzers, CancellationToken cancellationToken) { var projectAnalysisResult = projectAnalyzers.Any() ? await ProjectCompilationWithAnalyzers!.GetAnalysisResultAsync(model, filterSpan, projectAnalyzers, cancellationToken).ConfigureAwait(false) @@ -111,6 +112,6 @@ public Task GetAnalyzerTelemetryInfoAsync(DiagnosticAnaly ? await HostCompilationWithAnalyzers!.GetAnalysisResultAsync(model, filterSpan, hostAnalyzers, cancellationToken).ConfigureAwait(false) : null; - return (projectAnalysisResult, hostAnalysisResult); + return AnalysisResultPair.FromResult(projectAnalysisResult, hostAnalysisResult); } } diff --git a/src/Workspaces/Core/Portable/Diagnostics/Extensions.cs b/src/Workspaces/Core/Portable/Diagnostics/Extensions.cs index 0226ee3e59f92..7d812dbc9abc2 100644 --- a/src/Workspaces/Core/Portable/Diagnostics/Extensions.cs +++ b/src/Workspaces/Core/Portable/Diagnostics/Extensions.cs @@ -99,13 +99,15 @@ public static bool IsFeaturesAnalyzer(this AnalyzerReference reference) } public static async Task> ToResultBuilderMapAsync( - this AnalysisResult analysisResult, + this AnalysisResultPair analysisResult, ImmutableArray additionalPragmaSuppressionDiagnostics, DocumentAnalysisScope? documentAnalysisScope, Project project, VersionStamp version, - Compilation compilation, - ImmutableArray analyzers, + Compilation? projectCompilation, + Compilation? hostCompilation, + ImmutableArray projectAnalyzers, + ImmutableArray hostAnalyzers, SkippedHostAnalyzersInfo skippedAnalyzersInfo, bool includeSuppressedDiagnostics, CancellationToken cancellationToken) @@ -125,7 +127,7 @@ public static async Task(); - foreach (var analyzer in analyzers) + foreach (var analyzer in projectAnalyzers.ConcatFast(hostAnalyzers)) { cancellationToken.ThrowIfCancellationRequested(); @@ -151,24 +153,24 @@ public static async Task d.Location.SourceTree == treeToAnalyze); - AddDiagnosticsToResult(diagnostics, ref result, compilation, treeToAnalyze, additionalDocumentId: null, + AddDiagnosticsToResult(diagnostics, ref result, projectCompilation, hostCompilation, treeToAnalyze, additionalDocumentId: null, documentAnalysisScope.Span, AnalysisKind.Semantic, diagnosticIdsToFilter, includeSuppressedDiagnostics); } } @@ -221,7 +223,7 @@ public static async Task d.Location.SourceTree!)) { - AddDiagnosticsToResult(group.AsImmutable(), ref result, compilation, group.Key, additionalDocumentId: null, + AddDiagnosticsToResult(group.AsImmutable(), ref result, projectCompilation, hostCompilation, group.Key, additionalDocumentId: null, span: null, AnalysisKind.Semantic, diagnosticIdsToFilter, includeSuppressedDiagnostics); } } @@ -238,7 +240,8 @@ static void AddAnalyzerDiagnosticsToResult( DiagnosticAnalyzer analyzer, ImmutableDictionary> diagnosticsByAnalyzer, ref DiagnosticAnalysisResultBuilder result, - Compilation compilation, + Compilation? projectCompilation, + Compilation? hostCompilation, SyntaxTree? tree, DocumentId? additionalDocumentId, TextSpan? span, @@ -248,7 +251,7 @@ static void AddAnalyzerDiagnosticsToResult( { if (diagnosticsByAnalyzer.TryGetValue(analyzer, out var diagnostics)) { - AddDiagnosticsToResult(diagnostics, ref result, compilation, + AddDiagnosticsToResult(diagnostics, ref result, projectCompilation, hostCompilation, tree, additionalDocumentId, span, kind, diagnosticIdsToFilter, includeSuppressedDiagnostics); } } @@ -256,7 +259,8 @@ static void AddAnalyzerDiagnosticsToResult( static void AddDiagnosticsToResult( ImmutableArray diagnostics, ref DiagnosticAnalysisResultBuilder result, - Compilation compilation, + Compilation? projectCompilation, + Compilation? hostCompilation, SyntaxTree? tree, DocumentId? additionalDocumentId, TextSpan? span, @@ -270,7 +274,9 @@ static void AddDiagnosticsToResult( } diagnostics = diagnostics.Filter(diagnosticIdsToFilter, includeSuppressedDiagnostics, span); - Debug.Assert(diagnostics.Length == CompilationWithAnalyzers.GetEffectiveDiagnostics(diagnostics, compilation).Count()); + + // ⚠️ No obvious way to keep this assertion + //Debug.Assert(diagnostics.Length == CompilationWithAnalyzers.GetEffectiveDiagnostics(diagnostics, compilation).Count()); switch (kind) { @@ -322,7 +328,7 @@ public static ImmutableArray Filter( filterSpan.HasValue && !filterSpan.Value.IntersectsWith(diagnostic.Location.SourceSpan)); } - public static async Task<(AnalysisResult? projectAnalysisResult, AnalysisResult? hostAnalysisResult, ImmutableArray additionalDiagnostics)> GetAnalysisResultAsync( + public static async Task<(AnalysisResultPair? analysisResult, ImmutableArray additionalDiagnostics)> GetAnalysisResultAsync( this CompilationWithAnalyzersPair compilationWithAnalyzers, DocumentAnalysisScope? documentAnalysisScope, Project project, @@ -332,10 +338,10 @@ public static ImmutableArray Filter( var result = await GetAnalysisResultAsync(compilationWithAnalyzers, documentAnalysisScope, cancellationToken).ConfigureAwait(false); var additionalDiagnostics = await compilationWithAnalyzers.GetPragmaSuppressionAnalyzerDiagnosticsAsync( documentAnalysisScope, project, analyzerInfoCache, cancellationToken).ConfigureAwait(false); - return (result.projectAnalysisResult, result.hostAnalysisResult, additionalDiagnostics); + return (result, additionalDiagnostics); } - private static async Task<(AnalysisResult? projectAnalysisResult, AnalysisResult? hostAnalysisResult)> GetAnalysisResultAsync( + private static async Task GetAnalysisResultAsync( CompilationWithAnalyzersPair compilationWithAnalyzers, DocumentAnalysisScope? documentAnalysisScope, CancellationToken cancellationToken) diff --git a/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs b/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs index fe58571af4202..afe8535c9ef3b 100644 --- a/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs +++ b/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs @@ -381,7 +381,7 @@ private async Task AnalyzeAsync( ? new DocumentAnalysisScope(_document, _span, projectAnalyzers, hostAnalyzers, _analysisKind!.Value) : null; - var (projectAnalysisResult, hostAnalysisResult, additionalPragmaSuppressionDiagnostics) = await compilationWithAnalyzers.GetAnalysisResultAsync( + var (analysisResult, additionalPragmaSuppressionDiagnostics) = await compilationWithAnalyzers.GetAnalysisResultAsync( documentAnalysisScope, _project, _analyzerInfoCache, cancellationToken).ConfigureAwait(false); if (logPerformanceInfo && _performanceTracker != null) @@ -396,31 +396,23 @@ private async Task AnalyzeAsync( if (documentAnalysisScope == null) unitCount += _project.DocumentIds.Count; - var projectPerformanceInfo = projectAnalysisResult?.AnalyzerTelemetryInfo.ToAnalyzerPerformanceInfo(_analyzerInfoCache) ?? []; - var hostPerformanceInfo = hostAnalysisResult?.AnalyzerTelemetryInfo.ToAnalyzerPerformanceInfo(_analyzerInfoCache) ?? []; - _performanceTracker.AddSnapshot(projectPerformanceInfo.Concat(hostPerformanceInfo), unitCount, forSpanAnalysis: _span.HasValue); + var performanceInfo = analysisResult?.MergedAnalyzerTelemetryInfo.ToAnalyzerPerformanceInfo(_analyzerInfoCache) ?? []; + _performanceTracker.AddSnapshot(performanceInfo, unitCount, forSpanAnalysis: _span.HasValue); } } var builderMap = ImmutableDictionary.Empty; - if (projectAnalysisResult is not null) + if (analysisResult is not null) { - builderMap = builderMap.AddRange(await projectAnalysisResult.ToResultBuilderMapAsync( + builderMap = builderMap.AddRange(await analysisResult.ToResultBuilderMapAsync( additionalPragmaSuppressionDiagnostics, documentAnalysisScope, - _project, VersionStamp.Default, compilationWithAnalyzers.ProjectCompilation!, - projectAnalyzers, skippedAnalyzersInfo, reportSuppressedDiagnostics, cancellationToken).ConfigureAwait(false)); - } - - if (hostAnalysisResult is not null) - { - builderMap = builderMap.AddRange(await hostAnalysisResult.ToResultBuilderMapAsync( - additionalPragmaSuppressionDiagnostics, documentAnalysisScope, - _project, VersionStamp.Default, compilationWithAnalyzers.HostCompilation!, - hostAnalyzers, skippedAnalyzersInfo, reportSuppressedDiagnostics, cancellationToken).ConfigureAwait(false)); + _project, VersionStamp.Default, compilationWithAnalyzers.ProjectCompilation, compilationWithAnalyzers.HostCompilation, + projectAnalyzers, hostAnalyzers, skippedAnalyzersInfo, reportSuppressedDiagnostics, cancellationToken).ConfigureAwait(false)); } + // TODO Does this need adjustment? var telemetry = getTelemetryInfo - ? GetTelemetryInfo(projectAnalysisResult, hostAnalysisResult, projectAnalyzers, hostAnalyzers, analyzerToIdMap) + ? GetTelemetryInfo(analysisResult, projectAnalyzers, hostAnalyzers, analyzerToIdMap) : []; return new SerializableDiagnosticAnalysisResults(Dehydrate(builderMap, analyzerToIdMap), telemetry); @@ -448,15 +440,14 @@ private async Task AnalyzeAsync( } private static ImmutableArray<(string analyzerId, AnalyzerTelemetryInfo)> GetTelemetryInfo( - AnalysisResult? projectAnalysisResult, - AnalysisResult? hostAnalysisResult, + AnalysisResultPair? analysisResult, ImmutableArray projectAnalyzers, ImmutableArray hostAnalyzers, BidirectionalMap analyzerToIdMap) { Func shouldInclude; - if (projectAnalyzers.Length < (projectAnalysisResult?.AnalyzerTelemetryInfo.Count ?? 0) - || hostAnalyzers.Length < (hostAnalysisResult?.AnalyzerTelemetryInfo.Count ?? 0)) + if (projectAnalyzers.Length < (analysisResult?.ProjectAnalysisResult?.AnalyzerTelemetryInfo.Count ?? 0) + || hostAnalyzers.Length < (analysisResult?.HostAnalysisResult?.AnalyzerTelemetryInfo.Count ?? 0)) { // Filter the telemetry info to the executed analyzers. using var _1 = PooledHashSet.GetInstance(out var analyzersSet); @@ -471,9 +462,9 @@ private async Task AnalyzeAsync( } using var _2 = ArrayBuilder<(string analyzerId, AnalyzerTelemetryInfo)>.GetInstance(out var telemetryBuilder); - if (projectAnalysisResult is not null) + if (analysisResult?.ProjectAnalysisResult is not null) { - foreach (var (analyzer, analyzerTelemetry) in projectAnalysisResult.AnalyzerTelemetryInfo) + foreach (var (analyzer, analyzerTelemetry) in analysisResult.ProjectAnalysisResult.AnalyzerTelemetryInfo) { if (shouldInclude(analyzer)) { @@ -483,9 +474,9 @@ private async Task AnalyzeAsync( } } - if (hostAnalysisResult is not null) + if (analysisResult?.HostAnalysisResult is not null) { - foreach (var (analyzer, analyzerTelemetry) in hostAnalysisResult.AnalyzerTelemetryInfo) + foreach (var (analyzer, analyzerTelemetry) in analysisResult.HostAnalysisResult.AnalyzerTelemetryInfo) { if (shouldInclude(analyzer)) { From b15ca1fe968eea643fd18b20654de01ba649273a Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 21 Nov 2024 16:07:22 -0600 Subject: [PATCH 3/4] Remove unused parameters --- ...alyzer.InProcOrRemoteHostAnalyzerRunner.cs | 2 +- .../Core/Portable/Diagnostics/Extensions.cs | 29 +++++++------------ .../DiagnosticAnalyzer/DiagnosticComputer.cs | 2 +- 3 files changed, 12 insertions(+), 21 deletions(-) diff --git a/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InProcOrRemoteHostAnalyzerRunner.cs b/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InProcOrRemoteHostAnalyzerRunner.cs index 712f4aaafc25d..125475d2ccc85 100644 --- a/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InProcOrRemoteHostAnalyzerRunner.cs +++ b/src/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.InProcOrRemoteHostAnalyzerRunner.cs @@ -142,7 +142,7 @@ private async Task projectAnalyzers, ImmutableArray hostAnalyzers, SkippedHostAnalyzersInfo skippedAnalyzersInfo, @@ -155,13 +153,13 @@ public static async Task d.Location.SourceTree == treeToAnalyze); - AddDiagnosticsToResult(diagnostics, ref result, projectCompilation, hostCompilation, treeToAnalyze, additionalDocumentId: null, + AddDiagnosticsToResult(diagnostics, ref result, treeToAnalyze, additionalDocumentId: null, documentAnalysisScope.Span, AnalysisKind.Semantic, diagnosticIdsToFilter, includeSuppressedDiagnostics); } } @@ -223,7 +221,7 @@ public static async Task d.Location.SourceTree!)) { - AddDiagnosticsToResult(group.AsImmutable(), ref result, projectCompilation, hostCompilation, group.Key, additionalDocumentId: null, + AddDiagnosticsToResult(group.AsImmutable(), ref result, group.Key, additionalDocumentId: null, span: null, AnalysisKind.Semantic, diagnosticIdsToFilter, includeSuppressedDiagnostics); } } @@ -240,8 +238,6 @@ static void AddAnalyzerDiagnosticsToResult( DiagnosticAnalyzer analyzer, ImmutableDictionary> diagnosticsByAnalyzer, ref DiagnosticAnalysisResultBuilder result, - Compilation? projectCompilation, - Compilation? hostCompilation, SyntaxTree? tree, DocumentId? additionalDocumentId, TextSpan? span, @@ -251,7 +247,7 @@ static void AddAnalyzerDiagnosticsToResult( { if (diagnosticsByAnalyzer.TryGetValue(analyzer, out var diagnostics)) { - AddDiagnosticsToResult(diagnostics, ref result, projectCompilation, hostCompilation, + AddDiagnosticsToResult(diagnostics, ref result, tree, additionalDocumentId, span, kind, diagnosticIdsToFilter, includeSuppressedDiagnostics); } } @@ -259,8 +255,6 @@ static void AddAnalyzerDiagnosticsToResult( static void AddDiagnosticsToResult( ImmutableArray diagnostics, ref DiagnosticAnalysisResultBuilder result, - Compilation? projectCompilation, - Compilation? hostCompilation, SyntaxTree? tree, DocumentId? additionalDocumentId, TextSpan? span, @@ -275,9 +269,6 @@ static void AddDiagnosticsToResult( diagnostics = diagnostics.Filter(diagnosticIdsToFilter, includeSuppressedDiagnostics, span); - // ⚠️ No obvious way to keep this assertion - //Debug.Assert(diagnostics.Length == CompilationWithAnalyzers.GetEffectiveDiagnostics(diagnostics, compilation).Count()); - switch (kind) { case AnalysisKind.Syntax: diff --git a/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs b/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs index afe8535c9ef3b..80883ae2bb622 100644 --- a/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs +++ b/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs @@ -406,7 +406,7 @@ private async Task AnalyzeAsync( { builderMap = builderMap.AddRange(await analysisResult.ToResultBuilderMapAsync( additionalPragmaSuppressionDiagnostics, documentAnalysisScope, - _project, VersionStamp.Default, compilationWithAnalyzers.ProjectCompilation, compilationWithAnalyzers.HostCompilation, + _project, VersionStamp.Default, projectAnalyzers, hostAnalyzers, skippedAnalyzersInfo, reportSuppressedDiagnostics, cancellationToken).ConfigureAwait(false)); } From aa3df1679a93431e4fc9007f3300ed8d73d80ae9 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Thu, 21 Nov 2024 16:11:05 -0600 Subject: [PATCH 4/4] Avoid possible key collection in analyzer telemetry --- .../DiagnosticAnalyzer/DiagnosticComputer.cs | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs b/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs index 80883ae2bb622..36c3701037783 100644 --- a/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs +++ b/src/Workspaces/Remote/ServiceHub/Services/DiagnosticAnalyzer/DiagnosticComputer.cs @@ -410,7 +410,6 @@ private async Task AnalyzeAsync( projectAnalyzers, hostAnalyzers, skippedAnalyzersInfo, reportSuppressedDiagnostics, cancellationToken).ConfigureAwait(false)); } - // TODO Does this need adjustment? var telemetry = getTelemetryInfo ? GetTelemetryInfo(analysisResult, projectAnalyzers, hostAnalyzers, analyzerToIdMap) : []; @@ -462,21 +461,9 @@ private async Task AnalyzeAsync( } using var _2 = ArrayBuilder<(string analyzerId, AnalyzerTelemetryInfo)>.GetInstance(out var telemetryBuilder); - if (analysisResult?.ProjectAnalysisResult is not null) - { - foreach (var (analyzer, analyzerTelemetry) in analysisResult.ProjectAnalysisResult.AnalyzerTelemetryInfo) - { - if (shouldInclude(analyzer)) - { - var analyzerId = GetAnalyzerId(analyzerToIdMap, analyzer); - telemetryBuilder.Add((analyzerId, analyzerTelemetry)); - } - } - } - - if (analysisResult?.HostAnalysisResult is not null) + if (analysisResult is not null) { - foreach (var (analyzer, analyzerTelemetry) in analysisResult.HostAnalysisResult.AnalyzerTelemetryInfo) + foreach (var (analyzer, analyzerTelemetry) in analysisResult.MergedAnalyzerTelemetryInfo) { if (shouldInclude(analyzer)) {