-
Notifications
You must be signed in to change notification settings - Fork 683
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
Dedupe Build and Live Diagnostics #6543
Merged
Merged
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
076b499
try to connect via brokered service
beccamc 1ebb88d
Merge remote-tracking branch 'origin' into u/beccam/dedupeDiagnostics
8e58fa8
Merge remote-tracking branch 'origin' into u/beccam/dedupeDiagnostics
beccamc d012c04
receive diagnostics and display them
beccamc 0de66fb
some renaming
beccamc ece9f5c
update for FSA
beccamc aaf5545
update service name
beccamc d05e7e9
Merge branch 'main' into u/beccam/dedupeDiagnostics
beccamc 90a24bd
Merge remote-tracking branch 'origin' into u/beccam/dedupeDiagnostics
beccamc d50a024
project system errors
beccamc dde28b6
use fspath
beccamc fe6e705
pr comments
beccamc bb764bc
regex and options change
cb57a1e
change to dictionary
beccamc 5559303
Merge remote-tracking branch 'origin' into u/beccam/dedupeDiagnostics
8645247
fix integration tests
db5d8af
remove eslint ignore
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,149 @@ | ||
/*--------------------------------------------------------------------------------------------- | ||
* Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Licensed under the MIT License. See License.txt in the project root for license information. | ||
*--------------------------------------------------------------------------------------------*/ | ||
import * as vscode from 'vscode'; | ||
import { languageServerOptions } from '../shared/options'; | ||
|
||
export enum AnalysisSetting { | ||
FullSolution = 'fullSolution', | ||
OpenFiles = 'openFiles', | ||
None = 'none', | ||
} | ||
|
||
export class BuildDiagnosticsService { | ||
/** All the build results sent by the DevKit extension. */ | ||
private _allBuildDiagnostics: { [uri: string]: vscode.Diagnostic[] } = {}; | ||
|
||
/** The diagnostic results from build displayed by VS Code. When live diagnostics are available for a file, these are errors that only build knows about. | ||
* When live diagnostics aren't loaded for a file, then these are all of the diagnostics reported by the build.*/ | ||
private _diagnosticsReportedByBuild: vscode.DiagnosticCollection; | ||
|
||
constructor(buildDiagnostics: vscode.DiagnosticCollection) { | ||
this._diagnosticsReportedByBuild = buildDiagnostics; | ||
} | ||
|
||
public clearDiagnostics() { | ||
this._diagnosticsReportedByBuild.clear(); | ||
} | ||
|
||
public async setBuildDiagnostics(buildDiagnostics: { [uri: string]: vscode.Diagnostic[] }, buildOnlyIds: string[]) { | ||
this._allBuildDiagnostics = buildDiagnostics; | ||
const displayedBuildDiagnostics = new Array<[vscode.Uri, vscode.Diagnostic[]]>(); | ||
const allDocuments = vscode.workspace.textDocuments; | ||
|
||
for (const [uriPath, diagnosticList] of Object.entries(this._allBuildDiagnostics)) { | ||
// Check if the document is open | ||
const uri = vscode.Uri.file(uriPath); | ||
const document = allDocuments.find((d) => this.compareUri(d.uri, uri)); | ||
const isDocumentOpen = document !== undefined ? !document.isClosed : false; | ||
|
||
// Show the build-only diagnostics | ||
displayedBuildDiagnostics.push([ | ||
uri, | ||
BuildDiagnosticsService.filterDiagnosticsFromBuild(diagnosticList, buildOnlyIds, isDocumentOpen), | ||
]); | ||
} | ||
|
||
this._diagnosticsReportedByBuild.set(displayedBuildDiagnostics); | ||
} | ||
|
||
private compareUri(a: vscode.Uri, b: vscode.Uri): boolean { | ||
return a.fsPath.localeCompare(b.fsPath) === 0; | ||
} | ||
|
||
public async _onFileOpened(document: vscode.TextDocument, buildOnlyIds: string[]) { | ||
const uri = document.uri; | ||
const currentFileBuildDiagnostics = this._allBuildDiagnostics[uri.fsPath]; | ||
|
||
// The document is now open in the editor and live diagnostics are being shown. Filter diagnostics | ||
// reported by the build to show build-only problems. | ||
if (currentFileBuildDiagnostics) { | ||
const buildDiagnostics = BuildDiagnosticsService.filterDiagnosticsFromBuild( | ||
currentFileBuildDiagnostics, | ||
buildOnlyIds, | ||
true | ||
); | ||
this._diagnosticsReportedByBuild.set(uri, buildDiagnostics); | ||
} | ||
} | ||
|
||
public static filterDiagnosticsFromBuild( | ||
diagnosticList: vscode.Diagnostic[], | ||
buildOnlyIds: string[], | ||
isDocumentOpen: boolean | ||
): vscode.Diagnostic[] { | ||
const analyzerDiagnosticScope = languageServerOptions.analyzerDiagnosticScope as AnalysisSetting; | ||
const compilerDiagnosticScope = languageServerOptions.compilerDiagnosticScope as AnalysisSetting; | ||
|
||
// If compiler and analyzer diagnostics are set to "none", show everything reported by the build | ||
if (analyzerDiagnosticScope === AnalysisSetting.None && compilerDiagnosticScope === AnalysisSetting.None) { | ||
return diagnosticList; | ||
} | ||
|
||
// Filter the diagnostics reported by the build. Some may already be shown by live diagnostics. | ||
const buildDiagnosticsToDisplay: vscode.Diagnostic[] = []; | ||
|
||
// If it is a project system diagnostic (e.g. "Target framework out of support") | ||
// then always show it. It cannot be reported by live. | ||
const projectSystemDiagnostics = diagnosticList.filter((d) => | ||
BuildDiagnosticsService.isProjectSystemDiagnostic(d) | ||
); | ||
buildDiagnosticsToDisplay.push(...projectSystemDiagnostics); | ||
|
||
// If it is a "build-only"diagnostics (i.e. it can only be found by building) | ||
// then always show it. It cannot be reported by live. | ||
const buildOnlyDiagnostics = diagnosticList.filter((d) => | ||
BuildDiagnosticsService.isBuildOnlyDiagnostic(buildOnlyIds, d) | ||
); | ||
buildDiagnosticsToDisplay.push(...buildOnlyDiagnostics); | ||
|
||
// Check the analyzer diagnostic setting. If the setting is "none" or if the file is closed, | ||
// then no live analyzers are being shown and bulid analyzers should be added. | ||
// If FSA is on, then this is a no-op as FSA will report all analyzer diagnostics | ||
if ( | ||
analyzerDiagnosticScope === AnalysisSetting.None || | ||
(analyzerDiagnosticScope === AnalysisSetting.OpenFiles && !isDocumentOpen) | ||
) { | ||
const analyzerDiagnostics = diagnosticList.filter( | ||
// Needs to be analyzer diagnostics and not already reported as "build only" | ||
(d) => BuildDiagnosticsService.isAnalyzerDiagnostic(d) && !this.isBuildOnlyDiagnostic(buildOnlyIds, d) | ||
); | ||
buildDiagnosticsToDisplay.push(...analyzerDiagnostics); | ||
} | ||
|
||
// Check the compiler diagnostic setting. If the setting is "none" or if the file is closed, | ||
// then no live compiler diagnostics are being shown and bulid compiler diagnostics should be added. | ||
// If FSA is on, then this is a no-op as FSA will report all compiler diagnostics | ||
if ( | ||
compilerDiagnosticScope === AnalysisSetting.None || | ||
(compilerDiagnosticScope === AnalysisSetting.OpenFiles && !isDocumentOpen) | ||
) { | ||
const compilerDiagnostics = diagnosticList.filter( | ||
// Needs to be analyzer diagnostics and not already reported as "build only" | ||
(d) => BuildDiagnosticsService.isCompilerDiagnostic(d) && !this.isBuildOnlyDiagnostic(buildOnlyIds, d) | ||
); | ||
buildDiagnosticsToDisplay.push(...compilerDiagnostics); | ||
} | ||
|
||
return buildDiagnosticsToDisplay; | ||
beccamc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
private static isBuildOnlyDiagnostic(buildOnlyIds: string[], d: vscode.Diagnostic): boolean { | ||
return buildOnlyIds.find((b_id) => b_id === d.code) !== undefined; | ||
} | ||
|
||
private static isCompilerDiagnostic(d: vscode.Diagnostic): boolean { | ||
// eslint-disable-next-line prettier/prettier | ||
const regex = "[cC][sS][0-9]{4}"; | ||
beccamc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return d.code ? d.code.toString().match(regex) !== null : false; | ||
} | ||
|
||
private static isAnalyzerDiagnostic(d: vscode.Diagnostic): boolean { | ||
return d.code ? !this.isCompilerDiagnostic(d) : false; | ||
} | ||
|
||
private static isProjectSystemDiagnostic(d: vscode.Diagnostic): boolean { | ||
return d.code ? d.code.toString().startsWith('NETSDK') : false; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
/*--------------------------------------------------------------------------------------------- | ||
* Copyright (c) Microsoft Corporation. All rights reserved. | ||
* Licensed under the MIT License. See License.txt in the project root for license information. | ||
*--------------------------------------------------------------------------------------------*/ | ||
import { RoslynLanguageServer } from '../roslynLanguageServer'; | ||
import { CancellationToken } from 'vscode-jsonrpc'; | ||
import * as vscode from 'vscode'; | ||
|
||
interface IBuildResultDiagnostics { | ||
buildStarted(cancellationToken?: CancellationToken): Promise<void>; | ||
reportBuildResult( | ||
buildDiagnostics: { [uri: string]: vscode.Diagnostic[] }, | ||
cancellationToken?: CancellationToken | ||
): Promise<void>; | ||
} | ||
|
||
export class BuildResultDiagnostics implements IBuildResultDiagnostics { | ||
constructor(private _languageServerPromise: Promise<RoslynLanguageServer>) {} | ||
|
||
public async buildStarted(): Promise<void> { | ||
const langServer: RoslynLanguageServer = await this._languageServerPromise; | ||
langServer._buildDiagnosticService.clearDiagnostics(); | ||
} | ||
|
||
public async reportBuildResult(buildDiagnostics: { [uri: string]: vscode.Diagnostic[] }): Promise<void> { | ||
const langServer: RoslynLanguageServer = await this._languageServerPromise; | ||
const buildOnlyIds: string[] = await langServer.getBuildOnlyDiagnosticIds(CancellationToken.None); | ||
langServer._buildDiagnosticService.setBuildDiagnostics(buildDiagnostics, buildOnlyIds); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach sounds good to me as a starting point. I think we should dogfood the experience to confirm if the delay between build completion and background analysis reporting live diagnostics for open files (FSA off) or closed files (FSA on) is acceptable for real world solutions with large number of open files across the solution. Especially if there are compiler errors in the files which we are deferring to background analysis to report diagnostics. Otherwise, we may have to introduce some handshake between this diagnostic source and the existing live diagnostic source such that we report all build reported diagnostics for a file here until live background analysis has analyzed that file and reported live diagnostics for it.