Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions src/EditorFeatures/Test/CodeFixes/CodeFixServiceTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Editor.Implementation.Suggestions;
using Microsoft.CodeAnalysis.Editor.UnitTests.Extensions;
Copy link
Member Author

Choose a reason for hiding this comment

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

bridging tests to new api by use of an extension method.

using Microsoft.CodeAnalysis.ErrorLogger;
using Microsoft.CodeAnalysis.ErrorReporting;
using Microsoft.CodeAnalysis.Extensions;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using Microsoft.CodeAnalysis.CSharp.RemoveUnnecessarySuppressions;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Diagnostics.CSharp;
using Microsoft.CodeAnalysis.Editor.UnitTests.Extensions;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Remote.Diagnostics;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Imports Microsoft.CodeAnalysis.CSharp
Imports Microsoft.CodeAnalysis.Diagnostics
Imports Microsoft.CodeAnalysis.Diagnostics.CSharp
Imports Microsoft.CodeAnalysis.Editor.UnitTests
Imports Microsoft.CodeAnalysis.Editor.UnitTests.Extensions
Imports Microsoft.CodeAnalysis.Host.Mef
Imports Microsoft.CodeAnalysis.Serialization
Imports Microsoft.CodeAnalysis.SolutionCrawler
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// 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 System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Diagnostics;

namespace Microsoft.CodeAnalysis.Editor.UnitTests.Extensions;

internal static class IDiagnosticServiceExtensions
{
public static Task<ImmutableArray<DiagnosticData>> ForceAnalyzeProjectAsync(
Copy link
Member Author

Choose a reason for hiding this comment

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

unit test only extension for tests that want to validate the diags that CodeAnalysisDiagnosticAnalyzerService gets back. I will do a future pr that has these features literally just test that service instead of diving into one of its helper methosd.

this IDiagnosticAnalyzerService service, Project project, CancellationToken cancellationToken)
{
return CodeAnalysisDiagnosticAnalyzerServiceHelpers.ForceCodeAnalysisDiagnosticsAsync(
service, project, new(), cancellationToken);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Text;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CodeFixes;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,113 @@
using System.Collections.Concurrent;
using System.Collections.Immutable;
using System.Composition;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.Diagnostics;

internal static class CodeAnalysisDiagnosticAnalyzerServiceHelpers
{
private static Func<DiagnosticAnalyzer, bool> GetDiagnosticAnalyzerFilter(
Project project, DiagnosticAnalyzerInfoCache infoCache)
{
return analyzer =>
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 logic is a move. but it allows CodeAnalysisDiagnosticAnalyzerService to be entire explicit about which analyzers it runs as that is specific to its scenario. the DiagAnalService doesn't need to care or think about it.

{
if (analyzer == FileContentLoadAnalyzer.Instance ||
analyzer == GeneratorDiagnosticsPlaceholderAnalyzer.Instance ||
analyzer.IsCompilerAnalyzer())
{
return true;
}

if (analyzer.IsBuiltInAnalyzer())
{
// always return true for builtin analyzer. we can't use
// descriptor check since many builtin analyzer always return
// hidden descriptor regardless what descriptor it actually
// return on runtime. they do this so that they can control
// severity through option page rather than rule set editor.
// this is special behavior only ide analyzer can do. we hope
// once we support editorconfig fully, third party can use this
// ability as well and we can remove this kind special treatment on builtin
// analyzer.
return true;
}

if (analyzer is DiagnosticSuppressor)
{
// Always execute diagnostic suppressors.
return true;
}

if (project.CompilationOptions is null)
{
// Skip compilation options based checks for non-C#/VB projects.
return true;
}

// For most of analyzers, the number of diagnostic descriptors is small, so this should be cheap.
var descriptors = infoCache.GetDiagnosticDescriptors(analyzer);
var analyzerConfigOptions = project.GetAnalyzerConfigOptions();
return descriptors.Any(static (d, arg) =>
Copy link
Member

Choose a reason for hiding this comment

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

This bit is actually readable now, thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh good. I'm glad!

{
var severity = d.GetEffectiveSeverity(
arg.CompilationOptions,
arg.analyzerConfigOptions?.ConfigOptionsWithFallback,
arg.analyzerConfigOptions?.TreeOptions);
return severity != ReportDiagnostic.Hidden;
},
(project.CompilationOptions, analyzerConfigOptions));
};
}

public static async Task<ImmutableArray<DiagnosticData>> ForceCodeAnalysisDiagnosticsAsync(
IDiagnosticAnalyzerService diagnosticAnalyzerService, Project project, DiagnosticAnalyzerInfoCache infoCache, CancellationToken cancellationToken)
{
// We are being asked to explicitly analyze this project. As such we do *not* want to use the
// default rules determining which analyzers to run. For example, even if compiler diagnostics
// are set to 'none' for live diagnostics, we still want to run them here.
//
// As such, we are very intentionally not calling into _diagnosticAnalyzerService.GetDefaultAnalyzerFilter
// here. We want to control the rules entirely when this is called.
var filter = GetDiagnosticAnalyzerFilter(project, infoCache);

// Compute all the diagnostics for all the documents in the project.
//
// Note: in this case we want diagnostics for source generated documents as well. So ensure those are
// generated and included in the results.
var sourceGeneratorDocuments = await project.GetSourceGeneratedDocumentsAsync(cancellationToken).ConfigureAwait(false);

var documentDiagnostics = await diagnosticAnalyzerService.GetDiagnosticsForIdsAsync(
project, [.. project.DocumentIds, .. project.AdditionalDocumentIds, .. sourceGeneratorDocuments.Select(d => d.Id)],
diagnosticIds: null, filter, includeLocalDocumentDiagnostics: true, cancellationToken).ConfigureAwait(false);

// Then all the non-document diagnostics for that project as well.
var projectDiagnostics = await diagnosticAnalyzerService.GetProjectDiagnosticsForIdsAsync(
project, diagnosticIds: null, filter, cancellationToken).ConfigureAwait(false);

return [.. documentDiagnostics, .. projectDiagnostics];
}
}

[ExportWorkspaceServiceFactory(typeof(ICodeAnalysisDiagnosticAnalyzerService)), Shared]
[method: ImportingConstructor]
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal sealed class CodeAnalysisDiagnosticAnalyzerServiceFactory() : IWorkspaceServiceFactory
internal sealed class CodeAnalysisDiagnosticAnalyzerServiceFactory(
DiagnosticAnalyzerInfoCache.SharedGlobalCache infoCache) : IWorkspaceServiceFactory
{
public IWorkspaceService CreateService(HostWorkspaceServices workspaceServices)
=> new CodeAnalysisDiagnosticAnalyzerService(workspaceServices.Workspace);
=> new CodeAnalysisDiagnosticAnalyzerService(infoCache.AnalyzerInfoCache, workspaceServices.Workspace);

private sealed class CodeAnalysisDiagnosticAnalyzerService : ICodeAnalysisDiagnosticAnalyzerService
{
private readonly IDiagnosticAnalyzerService _diagnosticAnalyzerService;
private readonly DiagnosticAnalyzerInfoCache _infoCache;
private readonly Workspace _workspace;

/// <summary>
Expand All @@ -44,9 +132,11 @@ private sealed class CodeAnalysisDiagnosticAnalyzerService : ICodeAnalysisDiagno
private readonly ConcurrentSet<ProjectId> _clearedProjectIds = [];

public CodeAnalysisDiagnosticAnalyzerService(
DiagnosticAnalyzerInfoCache infoCache,
Workspace workspace)
{
_workspace = workspace;
_infoCache = infoCache;
_diagnosticAnalyzerService = _workspace.Services.GetRequiredService<IDiagnosticAnalyzerService>();

_ = workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChanged);
Expand Down Expand Up @@ -88,8 +178,8 @@ public async ValueTask RunAnalysisAsync(Project project, CancellationToken cance

Contract.ThrowIfFalse(project.Solution.Workspace == _workspace);

// Execute force analysis for the project.
var diagnostics = await _diagnosticAnalyzerService.ForceAnalyzeProjectAsync(project, cancellationToken).ConfigureAwait(false);
var diagnostics = await CodeAnalysisDiagnosticAnalyzerServiceHelpers.ForceCodeAnalysisDiagnosticsAsync(
_diagnosticAnalyzerService, project, _infoCache, cancellationToken).ConfigureAwait(false);

// Add the given project to the analyzed projects list **after** analysis has completed.
// We need this ordering to ensure that 'HasProjectBeenAnalyzed' call above functions correctly.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.SolutionCrawler;
Copy link
Member Author

Choose a reason for hiding this comment

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

only used for doc comments crefs.

using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.Diagnostics;
Expand All @@ -22,8 +23,22 @@ internal interface IDiagnosticAnalyzerService : IWorkspaceService
/// </remarks>
void RequestDiagnosticRefresh();

/// <inheritdoc cref="IRemoteDiagnosticAnalyzerService.ForceAnalyzeProjectAsync"/>
Task<ImmutableArray<DiagnosticData>> ForceAnalyzeProjectAsync(Project project, CancellationToken cancellationToken);
Copy link
Member Author

Choose a reason for hiding this comment

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

removing this was the primary goal of this PR.

/// <summary>
/// The default analyzer filter that will be used in functions like <see cref="GetDiagnosticsForIdsAsync"/> if
/// no filter is provided. The default filter has the following rules:
/// <list type="number">
/// <item>The standard compiler analyzer will not be run if the compiler diagnostic scope is <see cref="CompilerDiagnosticsScope.None"/>.</item>
/// <item>A regular analyzer will not be run if <see cref="ProjectState.RunAnalyzers"/> is false.</item>
/// <item>A regular analyzer will not be run if if the background analysis scope is <see cref="BackgroundAnalysisScope.None"/>.</item>
/// <item>If a set of diagnostic ids are provided, the analyzer will not be run unless it declares at least one
/// descriptor in that set.</item>
/// <item>Otherwise, the analyzer will be run</item>
/// </list>
/// </summary>
/// <param name="additionalFilter">An additional filter that can accept or reject analyzers that the default
/// rules have accepted.</param>
Func<DiagnosticAnalyzer, bool> GetDefaultAnalyzerFilter(
Copy link
Member Author

Choose a reason for hiding this comment

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

codifying what this is, and making it something a caller can override if they do not want it.

Project project, ImmutableHashSet<string>? diagnosticIds, Func<DiagnosticAnalyzer, bool>? additionalFilter = null);
Copy link
Member

Choose a reason for hiding this comment

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

It does feel a little weird that we pass another filter to retrieve the default filter. Wondering if places that use the additionalFilter should instead be creating their own single filter (which may wrap a call to the default filter), instead of it being passed in?

It might make usages more clear that even though they are getting the default filter, they may not be using the default filter behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Agreeing here -- my assumption would have been that the additional filter was only used for ones this didn't answer on, but I guess the comment says it's the other way around.

Or maybe just have a pattern that this filter method returns a nullable boolean, and then have a Compose method somewhere that takes a bunch of filters, calls them in order, and has some default true/false to fallback to if nobody answers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrmm... i'll see if there's a better way to do this. Originally, i just returned the filtetr, and then the callers had to combine it with theirs.

In practice, the pattern was always teh same. So having this pattern just helped encode both the idea of:

  1. get the default rules
  2. get the default rules, augmented with some other set.

So with this, and the ability for someone to pass a callback to GetDiagnostics meant we could handle all 3 cases:

  1. get the default rules
  2. get the default rules, augmented with some other set.
  3. use an entirely custom set of rules

But yeah, i agree it's not necessarily stunning here...


/// <inheritdoc cref="IRemoteDiagnosticAnalyzerService.GetDeprioritizationCandidatesAsync"/>
Task<ImmutableArray<DiagnosticAnalyzer>> GetDeprioritizationCandidatesAsync(
Expand All @@ -40,7 +55,10 @@ Task<ImmutableArray<DiagnosticAnalyzer>> GetDeprioritizationCandidatesAsync(
/// <param name="documentIds">Optional documents to scope the returned diagnostics. If <see langword="default"/>,
/// then diagnostics will be returned for <see cref="Project.DocumentIds"/> and <see cref="Project.AdditionalDocumentIds"/>.</param>
/// <param name="diagnosticIds">Optional set of diagnostic IDs to scope the returned diagnostics.</param>
/// <param name="shouldIncludeAnalyzer">Option callback to filter out analyzers to execute for computing diagnostics.</param>
/// <param name="shouldIncludeAnalyzer">Optional callback to filter out analyzers to execute for computing diagnostics.
/// If not present, <see cref="GetDefaultAnalyzerFilter"/> will be used. If present, no default behavior
/// is used, and the callback is defered to entirely. To augment the existing default rules call
/// <see cref="GetDefaultAnalyzerFilter"/> explicitly, and pass the result of that into this method.</param>
/// <param name="includeLocalDocumentDiagnostics">
/// Indicates if local document diagnostics must be returned.
/// Local diagnostics are the ones that are reported by analyzers on the same file for which the callback was received
Expand All @@ -62,7 +80,10 @@ Task<ImmutableArray<DiagnosticData>> GetDiagnosticsForIdsAsync(
/// </summary>
/// <param name="project">Project to fetch the diagnostics for.</param>
/// <param name="diagnosticIds">Optional set of diagnostic IDs to scope the returned diagnostics.</param>
/// <param name="shouldIncludeAnalyzer">Option callback to filter out analyzers to execute for computing diagnostics.</param>
/// <param name="shouldIncludeAnalyzer">Optional callback to filter out analyzers to execute for computing diagnostics.
/// If not present, <see cref="GetDefaultAnalyzerFilter"/> will be used. If present, no default behavior
/// is used, and the callback is defered to entirely. To augment the existing default rules call
/// <see cref="GetDefaultAnalyzerFilter"/> explicitly, and pass the result of that into this method.</param>
Task<ImmutableArray<DiagnosticData>> GetProjectDiagnosticsForIdsAsync(
Project project, ImmutableHashSet<string>? diagnosticIds, Func<DiagnosticAnalyzer, bool>? shouldIncludeAnalyzer, CancellationToken cancellationToken);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,25 +124,27 @@ private ImmutableArray<DiagnosticAnalyzer> GetDiagnosticAnalyzers(
ImmutableHashSet<string>? diagnosticIds,
Func<DiagnosticAnalyzer, bool>? shouldIncludeAnalyzer)
{
var analyzersForProject = GetProjectAnalyzers(project);
var analyzers = analyzersForProject.WhereAsArray(a => ShouldIncludeAnalyzer(project, a));
shouldIncludeAnalyzer ??= GetDefaultAnalyzerFilter(project, diagnosticIds, additionalFilter: null);

return analyzers;
var analyzersForProject = GetProjectAnalyzers(project);
return analyzersForProject.WhereAsArray(shouldIncludeAnalyzer);
}

bool ShouldIncludeAnalyzer(Project project, DiagnosticAnalyzer analyzer)
public Func<DiagnosticAnalyzer, bool> GetDefaultAnalyzerFilter(
Project project, ImmutableHashSet<string>? diagnosticIds, Func<DiagnosticAnalyzer, bool>? additionalFilter)
=> analyzer =>
{
if (!DocumentAnalysisExecutor.IsAnalyzerEnabledForProject(analyzer, project, this._globalOptions))
return false;

if (shouldIncludeAnalyzer != null && !shouldIncludeAnalyzer(analyzer))
if (additionalFilter != null && !additionalFilter(analyzer))
return false;

if (diagnosticIds != null && _analyzerInfoCache.GetDiagnosticDescriptors(analyzer).All(d => !diagnosticIds.Contains(d.Id)))
return false;

return true;
}
}
};

public Task<ImmutableArray<DiagnosticData>> GetDiagnosticsForIdsAsync(
Project project, ImmutableArray<DocumentId> documentIds, ImmutableHashSet<string>? diagnosticIds, Func<DiagnosticAnalyzer, bool>? shouldIncludeAnalyzer, bool includeLocalDocumentDiagnostics, CancellationToken cancellationToken)
Expand Down
Loading
Loading