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

[Lightbulb Perf] Expose FilterSpan/FilterTree public API on remaining analysis contexts #68033

Closed
mavasani opened this issue May 1, 2023 · 3 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented Area-Analyzers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request Performance-Scenario-Diagnostics This issue affects diagnostics computation performance for lightbulb, background analysis, tagger.
Milestone

Comments

@mavasani
Copy link
Contributor

mavasani commented May 1, 2023

#67257 added FilterSpan public API to SyntaxTreeAnalysisContext and SemanticModelAnalysisContext. These APIs were implemented with #67818 and our IDE code style analyzers moved over to the new API with #68014.
The performance measurements from both these PRs (added to the PR descriptions) showed quite good performance improvements in lightbulb code path from these changes. Hence, we would like to add similar FilterSpan API to other context types, and also a FilterTree API to context types where it is relevant.

Proposal

  1. Add FilterSpan API to the following contexts:

         /// <summary>
         /// Optional filter span for which to compute diagnostics.
         /// </summary>
         public TextSpan? FilterSpan { get; }
    1. public readonly struct SymbolAnalysisContext
    2. public abstract class SymbolStartAnalysisContext
    3. public abstract class CodeBlockStartAnalysisContext<TLanguageKindEnum> where TLanguageKindEnum : struct
    4. public readonly struct CodeBlockAnalysisContext
    5. public abstract class OperationBlockStartAnalysisContext
    6. public readonly struct OperationBlockAnalysisContext
    7. public readonly struct SyntaxNodeAnalysisContext
    8. public readonly struct OperationAnalysisContext
  2. Add FilterTree API to the following contexts:

         /// <summary>
         /// Optional filter tree for which to compute diagnostics.
         /// </summary>
         public SyntaxTree? FilterTree { get; }
    1. public readonly struct SymbolAnalysisContext
    2. public abstract class SymbolStartAnalysisContext

      Note that rest of the contexts are only scoped to a specific syntax tree and there is already some property chain to get to this tree from the context object. For example, OperationTreeAnalysisContext.Operation.Syntax.SyntaxTree, SyntaxNodeAnalysisContext.Node.SyntaxTree, etc. Exposing a FilterTree property on them would seem to unnecessarily pollute the API surface and make it confusing for the analyzer authors that they ought to be doing something with it. I'd vote for not exposing FilterTree property on rest of the context types, but I am fine if the API review decides otherwise for consistency purposes.
  3. Open question: Should we add FilterSpan API to AdditionalFileAnalysisContext?

    public readonly struct AdditionalFileAnalysisContext

    If yes, then this would require us to add public API overloads to below methods that take a TextSpan? filterSpan parameter (similar to existing APIs to fetch single file syntax and semantic diagnostics):
    /// <summary>
    /// Returns an <see cref="AnalysisResult"/> populated with <see cref="AnalysisResult.AdditionalFileDiagnostics"/> produced by all <see cref="Analyzers"/> from analyzing the given additional <paramref name="file"/>.
    /// The given <paramref name="file"/> must be part of <see cref="AnalyzerOptions.AdditionalFiles"/> for the <see cref="AnalysisOptions"/> for this CompilationWithAnalyzers instance.
    /// Depending on analyzers' behavior, some diagnostics that would be reported for the file by an analysis of the complete compilation can be absent.
    /// </summary>
    /// <param name="file">Additional file to analyze.</param>
    /// <param name="cancellationToken">Cancellation token.</param>
    public async Task<AnalysisResult> GetAnalysisResultAsync(AdditionalText file, CancellationToken cancellationToken)
    {
    VerifyAdditionalFile(file);
    return await GetAnalysisResultCoreAsync(new SourceOrAdditionalFile(file), Analyzers, cancellationToken).ConfigureAwait(false);
    }
    /// <summary>
    /// Returns an <see cref="AnalysisResult"/> populated with <see cref="AnalysisResult.AdditionalFileDiagnostics"/> produced by given <paramref name="analyzers"/> from analyzing the given additional <paramref name="file"/>.
    /// The given <paramref name="file"/> must be part of <see cref="AnalyzerOptions.AdditionalFiles"/> for the <see cref="AnalysisOptions"/> for this CompilationWithAnalyzers instance.
    /// Depending on analyzers' behavior, some diagnostics that would be reported for the file by an analysis of the complete compilation can be absent.
    /// </summary>
    /// <param name="file">Additional file 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<AnalysisResult> GetAnalysisResultAsync(AdditionalText file, ImmutableArray<DiagnosticAnalyzer> analyzers, CancellationToken cancellationToken)
    {
    VerifyAdditionalFile(file);
    VerifyExistingAnalyzersArgument(analyzers);
    return await GetAnalysisResultCoreAsync(new SourceOrAdditionalFile(file), analyzers, cancellationToken).ConfigureAwait(false);
    }

    Proposed API overloads to add

     public async Task<AnalysisResult> GetAnalysisResultAsync(AdditionalText file, TextSpan? filterSpan, CancellationToken cancellationToken);
     public async Task<AnalysisResult> GetAnalysisResultAsync(AdditionalText file, TextSpan? filterSpan, ImmutableArray<DiagnosticAnalyzer> analyzers, CancellationToken cancellationToken)

    Note that we have already added Roslyn support for code fixes and refactorings in additional files with Add support for code refactorings and code fixes in non-source files #64364, so we can consume this new API on the Roslyn side and thread in filter span for lightbulb code path.

Performance measurements

@mavasani mavasani added Area-Analyzers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request labels May 1, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label May 1, 2023
@mavasani mavasani added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged Issues and PRs which have not yet been triaged by a lead labels May 1, 2023
@mavasani mavasani added this to the 17.7 milestone May 1, 2023
@mavasani mavasani self-assigned this May 1, 2023
mavasani added a commit to mavasani/roslyn that referenced this issue May 2, 2023
…lAnalysisContext

Work towards dotnet#68033

- Add FilterTree/FilterSpan API on SymbolStartAnalysisContext and SymbolAnalysisContext
- Move IDE code style SymbolStart analyzers to respect the FilterTree/FilterSpan API
@mavasani mavasani added the Performance-Scenario-Diagnostics This issue affects diagnostics computation performance for lightbulb, background analysis, tagger. label May 4, 2023
@mavasani
Copy link
Contributor Author

mavasani commented May 9, 2023

Tagging @333fred @CyrusNajmabadi @sharwell - this is ready to be brought up at the next API review meeting.

mavasani added a commit to mavasani/roslyn that referenced this issue May 9, 2023
@333fred
Copy link
Member

333fred commented May 9, 2023

@mavasani can you please add a diff view of the apis to the first post?

@333fred
Copy link
Member

333fred commented May 11, 2023

API Review

  • What if we did SyntaxTree everywhere?
    • Prefer FilterTree naming
  • We don't have enough info to make a decision on AdditionalTexts here, so that will need to be a followup.

Conclusion: APIs 1 and 2 are approved (adding FilterSpan and FilterTree). We will add FilterTree even where it might be duplicative, for ease of API usage.

@333fred 333fred added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 11, 2023
mavasani added a commit to mavasani/roslyn that referenced this issue May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented Area-Analyzers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request Performance-Scenario-Diagnostics This issue affects diagnostics computation performance for lightbulb, background analysis, tagger.
Projects
None yet
Development

No branches or pull requests

2 participants