Add new public APIs to CompilationWithAnalyzers to fetch syntax and s…

[mavasani]

…emantic diagnostics for a specific tree, which are categorized by each analyzer that reported the diagnostic.

Current APIs and IDE usage

We have existing APIs on CompilationWithAnalyzers that return the analyzers diagnostics for a given tree:

roslyn/src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs

Lines 465 to 474 in 5ae370f

	/// <summary>
	/// Returns syntax diagnostics produced by given <paramref name="analyzers"/> from analyzing the given <paramref name="tree"/>.
	/// Depending on analyzers' behavior, returned diagnostics can have locations outside the tree,
	/// and some diagnostics that would be reported for the tree by an analysis of the complete compilation
	/// can be absent.
	/// </summary>
	/// <param name="tree">Syntax tree to analyze.</param>
	/// <param name="analyzers">Analyzers whose diagnostics are required. All the given analyzers must be from the analyzers passed into the constructor of <see cref="CompilationWithAnalyzers"/>.</param>
	/// <param name="cancellationToken">Cancellation token.</param>
	public async Task<ImmutableArray<Diagnostic>> GetAnalyzerSyntaxDiagnosticsAsync(SyntaxTree tree, ImmutableArray<DiagnosticAnalyzer> analyzers, CancellationToken cancellationToken)

roslyn/src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs

Lines 538 to 548 in 5ae370f

	/// <summary>
	/// Returns semantic diagnostics produced by all <see cref="Analyzers"/> from analyzing the given <paramref name="model"/>, optionally scoped to a <paramref name="filterSpan"/>.
	/// Depending on analyzers' behavior, returned diagnostics can have locations outside the tree,
	/// and some diagnostics that would be reported for the tree by an analysis of the complete compilation
	/// can be absent.
	/// </summary>
	/// <param name="model">Semantic model representing the syntax tree to analyze.</param>
	/// <param name="filterSpan">An optional span within the tree to scope analysis.</param>
	/// <param name="analyzers">Analyzers whose diagnostics are required. All the given analyzers must be from the analyzers passed into the constructor of <see cref="CompilationWithAnalyzers"/>.</param>
	/// <param name="cancellationToken">Cancellation token.</param>
	public async Task<ImmutableArray<Diagnostic>> GetAnalyzerSemanticDiagnosticsAsync(SemanticModel model, TextSpan? filterSpan, ImmutableArray<DiagnosticAnalyzer> analyzers, CancellationToken cancellationToken)

This is sufficient for the current IDE analyzer execution model for open files - run each analyzer sequentially on each open file by calling the above APIs with a single analyzer at a time.

New proposed APIs and IDE usage

As part of #44408, we are moving open file analyzer execution to OOP (outside devenv.exe). This allows us to execute all open file analysis concurrently in that process. However, the above APIs are not sufficient for this analysis model as we need to categorize diagnostics by each analyzer. The returned diagnostics for the above existing APIs are a single array ImmutableArray<Diagnostic> instead of a dictionary keyed by each analyzer. Additionally, the AD0001 diagnostics reported for analyzer exceptions cannot be mapped to specific analyzer. This PR proposes adding overloads for GetAnalysisResultAsync, which are equivalent APIs that return AnalysisResult instead of ImmutableArray<Diagnostic>:

        /// <summary>
        /// Returns an <see cref="AnalysisResult"/> populated with <see cref="AnalysisResult.SyntaxDiagnostics"/> produced by all <see cref="Analyzers"/> from analyzing the given <paramref name="tree"/>.
        /// Depending on analyzers' behavior, some diagnostics that would be reported for the tree by an analysis of the complete compilation can be absent.
        /// </summary>
        /// <param name="tree">Syntax tree to analyze.</param>
        /// <param name="cancellationToken">Cancellation token.</param>
        public Task<AnalysisResult> GetAnalysisResultAsync(SyntaxTree tree, CancellationToken cancellationToken);

        /// <summary>
        /// Returns an <see cref="AnalysisResult"/> populated with <see cref="AnalysisResult.SyntaxDiagnostics"/> produced by given <paramref name="analyzers"/> from analyzing the given <paramref name="tree"/>.
        /// Depending on analyzers' behavior, some diagnostics that would be reported for the tree by an analysis of the complete compilation can be absent.
        /// </summary>
        /// <param name="tree">Syntax tree to analyze.</param>
        /// <param name="analyzers">Analyzers whose diagnostics are required. All the given analyzers must be from the analyzers passed into the constructor of <see cref="CompilationWithAnalyzers"/>.</param>
        /// <param name="cancellationToken">Cancellation token.</param>
        public Task<AnalysisResult> GetAnalysisResultAsync(SyntaxTree tree, ImmutableArray<DiagnosticAnalyzer> analyzers, CancellationToken cancellationToken);

        /// <summary>
        /// Returns an <see cref="AnalysisResult"/> populated with <see cref="AnalysisResult.SemanticDiagnostics"/> produced by all <see cref="Analyzers"/> from analyzing the given <paramref name="model"/>, optionally scoped to a <paramref name="filterSpan"/>.
        /// Depending on analyzers' behavior, some diagnostics that would be reported for the tree by an analysis of the complete compilation can be absent.
        /// </summary>
        /// <param name="model">Semantic model representing the syntax tree to analyze.</param>
        /// <param name="filterSpan">An optional span within the tree to scope analysis.</param>
        /// <param name="cancellationToken">Cancellation token.</param>
        public Task<AnalysisResult> GetAnalysisResultAsync(SemanticModel model, TextSpan? filterSpan, CancellationToken cancellationToken);

        /// <summary>
        /// Returns an <see cref="AnalysisResult"/> populated with <see cref="AnalysisResult.SemanticDiagnostics"/> produced by the given <paramref name="analyzers"/> from analyzing the given <paramref name="model"/>, optionally scoped to a <paramref name="filterSpan"/>.
        /// Depending on analyzers' behavior, some diagnostics that would be reported for the tree by an analysis of the complete compilation can be absent.
        /// </summary>
        /// <param name="model">Semantic model representing the syntax tree to analyze.</param>
        /// <param name="filterSpan">An optional span within the tree to scope analysis.</param>
        /// <param name="analyzers">Analyzers whose diagnostics are required. All the given analyzers must be from the analyzers passed into the constructor of <see cref="CompilationWithAnalyzers"/>.</param>
        /// <param name="cancellationToken">Cancellation token.</param>
        public Task<AnalysisResult> GetAnalysisResultAsync(SemanticModel model, TextSpan? filterSpan, ImmutableArray<DiagnosticAnalyzer> analyzers, CancellationToken cancellationToken);

See #45322 (comment) for additional details.

NOTE: I will send out a separate email to Roslyn API review committee to get an API approval and post an update here.

[sharwell]

How often do we have a project with two distinct diagnostic analyzers enabled that support the same diagnostic ID?

Additionally, the AD0001 diagnostics reported for analyzer exceptions cannot be mapped to specific analyzer.

If this is the only remaining problem, it could be addressed by adding a property to the diagnostic's property bag.

[mavasani]

How often do we have a project with two distinct diagnostic analyzers enabled that support the same diagnostic ID?

I don't think that is common, but there is nothing preventing separate analyzer packages to use the same ID. In such a scenario, the IDE behavior would become non-deterministic.

If this is the only remaining problem, it could be addressed by adding a property to the diagnostic's property bag.

What would be the key for the property? If it is the fully qualified analyzer type name, then we again run into the same potential problem about clashes. There is nothing that prevents a custom analyzer host to pass in multiple analyzer instances of the same analyzer, which would lead to the same problem. Additionally, this seems a little bit hacky mode of handshake between the analyzer driver and the analyzer host.

Another potential problem is performance. The core analyzer driver already tracks which diagnostics were reported by this analyzer, and requiring the IDE to again figure out this mapping from diagnostic IDs to analyzers seems unnecessary performance overhead.

I think your suggestion can work if this API addition or approval was non-trivial, but having this API seems a much more cleaner solution to the problem. Note that CompilationWithAnalyzers API only affects a handful of clients which are writing custom analyzer hosts, so a new API should have very limited impact, if any.

[sharwell]

I guess at this point my questions (or "ponderings") are:

1. Do we need 4 methods, or can we get away with just two? Suppose we didn't pass analyzers explicitly and relied on the ones the CompilationWithAnalyzers instance was created with. Are there any known scenarios that would not work with this? We could always add the other API in the future if an unknown scenario demanded it. (Note that this is just a question about the public API; I would not be surprised to see the other two methods implemented as internal in code for some helper purpose.)

2. If an analyzer does not report diagnostics, is the key present or omitted from the result? The documentation for the method should clarify one of the following:

   1. The analyzer will not be included in the result (bars future implementations from including it for convenience)
   2. The analyzer will be included in the result, and will map to an empty immutable array (bars future implementations from omitting it as an optimization)
   3. An analyzer that does not map to any diagnostics may either be omitted from the result, or map to an empty array.

   I prefer either the third option here because it is usable (particularly so via the GetValueOrDefault method) but also doesn't constrain our implementation decisions, or the first option as a more restrictive variant.

[mavasani]

Do we need 4 methods, or can we get away with just two? Suppose we didn't pass analyzers explicitly and relied on the ones the CompilationWithAnalyzers instance was created with. Are there any known scenarios that would not work with this? We could always add the other API in the future if an unknown scenario demanded it. (Note that this is just a question about the public API; I would not be surprised to see the other two methods implemented as internal in code for some helper purpose.)

Yes, for lightbulb case we explicitly execute a subset of analyzers. Also in open file analysis case, we skip executing analyzers whose results are cached and not invalidated. We can possibly remove the overload that doesn’t take explicit analyzer argument. However, that will make it inconsistent with other existing APIs on the type, all of which have two overloads, one that takes explicit analyzers argument and one that doesn’t and defaults to all analyzers.

If an analyzer does not report diagnostics, is the key present or omitted from the result? The documentation for the method should clarify one of the following:

I am fine with any option here. Current implementation and doc comments indicate third option. Clients should always check if key exists or not.

[mavasani]

@sharwell Actually, we will need to a similar GetCategorizedXXXDiagnosticsAsync API as part of #45076, so IDE can get additional file diagnostics per-analyzer per-additional file.

One alternative to GetCategorizedXXXDiagnosticsAsync APIs would be to expose a new stateful API public AnalysisResult GetCurrentAnalysisResult(); on CompilationWithAnalyzers, which exposes the categorized diagnostics from AnalysisResult from all the analysis APIs invoked on the CompilationWithAnalyzers. So the client has 2 potential options:

1. Don't care about categorized diagnostics: Invoke the current GetXXXDiagnosticsAsync and get the diagnostics generated from analysis
2. Care about categorized diagnostics: Invoke the current GetXXXDiagnosticsAsync, ignore the result, and invoke GetCurrentAnalysisResult() and access the required categorized diagnostics from the returned AnalysisResult

With this proposal, we avoid requiring new GetCategorizedXXXDiagnosticsAsync for each new partial analysis API added to CompilationWithAnalyzers. However, this comes at the expense of now exposing the stateful API public AnalysisResult GetCurrentAnalysisResult();. Current APIs on CompilationWithAnalyzers have a nice property of all of them being non-stateful from the perspective of the clients.

Thoughts?

[mavasani]

Had an offline discussion with @AlekseyTs, who had a better proposal of adding overloads to existing GetAnalysisResultAsync public API with more fine grained input scopes. This ensures that the change doesn’t pollute the CompilationWithAnalyzers API surface as these are just overloads, and also exposes all the required data for clients.

[mavasani]

Thanks @cston. Ping @dotnet/roslyn-compiler for additional review.

[AlekseyTs]

LGTM (iteration 5)

[mavasani]

Thank you @AlekseyTs!