Skip to content

Add support for incremental analysis for method body editing scenario…#63130

Merged
mavasani merged 9 commits intodotnet:mainfrom
mavasani:MethodBodyAnalysisSpanApis
Aug 11, 2022
Merged

Add support for incremental analysis for method body editing scenario…#63130
mavasani merged 9 commits intodotnet:mainfrom
mavasani:MethodBodyAnalysisSpanApis

Conversation

@mavasani
Copy link
Contributor

@mavasani mavasani commented Aug 2, 2022

…s for LSP document pull diagnostics computation path, i.e. below APIs:

/// <summary>
/// Try to return up to date diagnostics for the given span for the document.
///
/// It will return true if it was able to return all up-to-date diagnostics.
/// otherwise, false indicating there are some missing diagnostics in the diagnostic list
///
/// This API will only force complete analyzers that support span based analysis, i.e. compiler analyzer and
/// <see cref="IBuiltInAnalyzer"/>s that support <see cref="DiagnosticAnalyzerCategory.SemanticSpanAnalysis"/>.
/// For the rest of the analyzers, it will only return diagnostics if the analyzer has already been executed.
/// Use <see cref="GetDiagnosticsForSpanAsync(Document, TextSpan?, Func{string, bool}?, bool, bool, CodeActionRequestPriority, Func{string, IDisposable?}?, CancellationToken)"/>
/// if you want to force complete all analyzers and get up-to-date diagnostics for all analyzers for the given span.
/// </summary>
Task<(ImmutableArray<DiagnosticData> diagnostics, bool upToDate)> TryGetDiagnosticsForSpanAsync(Document document, TextSpan range, Func<string, bool>? shouldIncludeDiagnostic, bool includeSuppressedDiagnostics = false, CodeActionRequestPriority priority = CodeActionRequestPriority.None, CancellationToken cancellationToken = default);
/// <summary>
/// Return up to date diagnostics for the given span for the document
/// <para>
/// This can be expensive since it is force analyzing diagnostics if it doesn't have up-to-date one yet.
/// Predicate <paramref name="shouldIncludeDiagnostic"/> filters out analyzers from execution if
/// none of its reported diagnostics should be included in the result.
/// </para>
/// </summary>
Task<ImmutableArray<DiagnosticData>> GetDiagnosticsForSpanAsync(Document document, TextSpan? range, Func<string, bool>? shouldIncludeDiagnostic, bool includeCompilerDiagnostics, bool includeSuppressedDiagnostics = false, CodeActionRequestPriority priority = CodeActionRequestPriority.None, Func<string, IDisposable?>? addOperationScope = null, CancellationToken cancellationToken = default);

The incremental analysis is currently guarded by a feature flag.

@ghost ghost added the Area-Analyzers label Aug 2, 2022
{
internal partial class DiagnosticIncrementalAnalyzer
{
private sealed class MemberRangeMap
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Almost all the code in this file is ported from the prior implementation of incremental method body analysis in DiagnosticIncrementalAnalyzer v1 engine.

}
}

public MemberRanges GetOrCreateMemberRanges(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only new method I have added on top of the previous implementation.

/// If we are unable to perform this incremental diagnostics update, we fallback to computing
/// the diagnostics for the entire document.
/// </summary>
private sealed class IncrementalMemberEditAnalyzer
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This type has the meat of the incremental analysis support.

#endif
}

private static bool TryGetUpdatedDocumentDiagnostics(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the core logic that computes latest diagnostics using latest changed member diagnostics and cached diagnostics from document snapshot prior to member edit. Almost all the code in the method is borrowed from prior implementation. I primarily added additional location handling here as we were either dropping them or not updating the spans for those, both of them were leading to asserts.

@mavasani mavasani marked this pull request as ready for review August 4, 2022 09:46
@mavasani mavasani requested a review from a team as a code owner August 4, 2022 09:46
@mavasani mavasani enabled auto-merge August 10, 2022 10:44
@mavasani mavasani merged commit d7be460 into dotnet:main Aug 11, 2022
@ghost ghost added this to the Next milestone Aug 11, 2022
@mavasani mavasani deleted the MethodBodyAnalysisSpanApis branch August 11, 2022 03:07
@dibarbet dibarbet modified the milestones: Next, 17.4 P2 Sep 1, 2022
mavasani added a commit to mavasani/roslyn that referenced this pull request Sep 21, 2022
Fixes [AB#1618061](https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1618061)

This was introduced by me with dotnet#63130. Earlier we used to cache full document diagnostics computed only from background analysis, which also handles updating the error list simulataneously. With the above PR, we also do this caching from other code paths if we are computing full document diagnostics (diagnostic tagger, LSP pull diags, etc.), but we don't raise events to update the error list from these code paths - basically the code added here: https://github.com/mavasani/roslyn/blob/cd5e6895d3dc8dfad270505b37d28be70077a645/src/Features/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_GetDiagnosticsForSpan.cs#L323-L329. So when background analysis gets to refreshing the document diagnostics, it founds cached items and ends up not updating the error list.

With this PR, we now cache the computed full document diagnostics in the new code paths only in LSP pull diagnostics mode. Verified that the repro in the above bug is fixed after this change.
mavasani added a commit to mavasani/roslyn that referenced this pull request Sep 21, 2022
Fixes [AB#1618061](https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1618061)

This was introduced by me with dotnet#63130. Earlier we used to cache full document diagnostics computed only from background analysis, which also handles updating the error list simulataneously. With the above PR, we also do this caching from other code paths if we are computing full document diagnostics (diagnostic tagger, LSP pull diags, etc.), but we don't raise events to update the error list from these code paths - basically the code added here: https://github.com/mavasani/roslyn/blob/cd5e6895d3dc8dfad270505b37d28be70077a645/src/Features/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_GetDiagnosticsForSpan.cs#L323-L329. So when background analysis gets to refreshing the document diagnostics, it founds cached items and ends up not updating the error list.

With this PR, we now cache the computed full document diagnostics in the new code paths only in LSP pull diagnostics mode. Verified that the repro in the above bug is fixed after this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants