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

Dedupe Build and Live Diagnostics #6543

Merged
merged 17 commits into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from 16 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
8 changes: 7 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@
"name": "Microsoft.CodeAnalysis.LanguageClient.SolutionSnapshotProvider",
"version": "0.1"
}
},
{
"moniker": {
"name": "Microsoft.VisualStudio.CSharpExtension.BuildResultService",
"version": "0.1"
}
}
],
"scripts": {
Expand Down Expand Up @@ -5665,4 +5671,4 @@
}
]
}
}
}
149 changes: 149 additions & 0 deletions src/lsptoolshost/buildDiagnosticsService.ts
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
Comment on lines +115 to +117
Copy link
Contributor

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.

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;
}
}
25 changes: 24 additions & 1 deletion src/lsptoolshost/roslynLanguageServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
SocketMessageReader,
MessageTransports,
RAL,
CancellationToken,
} from 'vscode-languageclient/node';
import { PlatformInformation } from '../shared/platform';
import { readConfigurations } from './configurationMiddleware';
Expand Down Expand Up @@ -57,6 +58,7 @@ import { commonOptions, languageServerOptions, omnisharpOptions } from '../share
import { NamedPipeInformation } from './roslynProtocol';
import { IDisposable } from '../disposable';
import { registerRestoreCommands } from './restore';
import { BuildDiagnosticsService } from './buildDiagnosticsService';

let _channel: vscode.OutputChannel;
let _traceChannel: vscode.OutputChannel;
Expand Down Expand Up @@ -95,6 +97,8 @@ export class RoslynLanguageServer {
/** The project files previously opened; we hold onto this for the same reason as _solutionFile. */
private _projectFiles: vscode.Uri[] = new Array<vscode.Uri>();

public _buildDiagnosticService: BuildDiagnosticsService;

constructor(
private _languageClient: RoslynLanguageClient,
private _platformInfo: PlatformInformation,
Expand All @@ -109,6 +113,10 @@ export class RoslynLanguageServer {
this.registerExtensionsChanged();
this.registerTelemetryChanged();

const diagnosticsReportedByBuild = vscode.languages.createDiagnosticCollection('csharp-build');
this._buildDiagnosticService = new BuildDiagnosticsService(diagnosticsReportedByBuild);
this.registerDocumentOpenForDiagnostics();

// Register Razor dynamic file info handling
this.registerDynamicFileInfo();

Expand Down Expand Up @@ -570,7 +578,7 @@ export class RoslynLanguageServer {
});
});

// The server process will create the named pipe used for communcation. Wait for it to be created,
// The server process will create the named pipe used for communication. Wait for it to be created,
// and listen for the server to pass back the connection information via stdout.
const namedPipeConnectionPromise = new Promise<NamedPipeInformation>((resolve) => {
_channel.appendLine('waiting for named pipe information from server...');
Expand Down Expand Up @@ -696,6 +704,16 @@ export class RoslynLanguageServer {
);
}

private registerDocumentOpenForDiagnostics() {
// When a file is opened process any build diagnostics that may be shown
this._languageClient.addDisposable(
vscode.workspace.onDidOpenTextDocument(async (event) => {
const buildIds = await this.getBuildOnlyDiagnosticIds(CancellationToken.None);
this._buildDiagnosticService._onFileOpened(event, buildIds);
})
);
}

private registerExtensionsChanged() {
// subscribe to extension change events so that we can get notified if C# Dev Kit is added/removed later.
this._languageClient.addDisposable(
Expand Down Expand Up @@ -817,6 +835,11 @@ export class RoslynLanguageServer {
}

public async getBuildOnlyDiagnosticIds(token: vscode.CancellationToken): Promise<string[]> {
// If the server isn't running, no build diagnostics to get
if (!this.isRunning()) {
return [];
}

const response = await this.sendRequest0(RoslynProtocol.BuildOnlyDiagnosticIdsRequest.type, token);
if (response) {
return response.ids;
Expand Down
30 changes: 30 additions & 0 deletions src/lsptoolshost/services/buildResultReporterService.ts
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);
}
}
9 changes: 9 additions & 0 deletions src/lsptoolshost/services/descriptors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,13 @@ export default class Descriptors {
protocolMajorVersion: 3,
}
);

static readonly csharpExtensionBuildResultService: ServiceRpcDescriptor = new ServiceJsonRpcDescriptor(
ServiceMoniker.create('Microsoft.VisualStudio.CSharpExtension.BuildResultService', '0.1'),
Formatters.MessagePack,
MessageDelimiters.BigEndianInt32LengthHeader,
{
protocolMajorVersion: 3,
}
);
}
5 changes: 5 additions & 0 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import { ServerStateChange } from './lsptoolshost/serverStateChange';
import { SolutionSnapshotProvider } from './lsptoolshost/services/solutionSnapshotProvider';
import { RazorTelemetryDownloader } from './razor/razorTelemetryDownloader';
import { commonOptions, omnisharpOptions, razorOptions } from './shared/options';
import { BuildResultDiagnostics } from './lsptoolshost/services/buildResultReporterService';
import { debugSessionTracker } from './coreclrDebug/provisionalDebugSessionTracker';

export async function activate(
Expand Down Expand Up @@ -431,6 +432,10 @@ function profferBrokeredServices(
serviceContainer.profferServiceFactory(
Descriptors.solutionSnapshotProviderRegistration,
(_mk, _op, _sb) => new SolutionSnapshotProvider(languageServerPromise)
),
serviceContainer.profferServiceFactory(
Descriptors.csharpExtensionBuildResultService,
(_mk, _op, _sb) => new BuildResultDiagnostics(languageServerPromise)
)
);
}
Expand Down
8 changes: 8 additions & 0 deletions src/shared/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ export interface LanguageServerOptions {
readonly preferCSharpExtension: boolean;
readonly startTimeout: number;
readonly crashDumpPath: string | undefined;
readonly analyzerDiagnosticScope: string;
readonly compilerDiagnosticScope: string;
}

export interface RazorOptions {
Expand Down Expand Up @@ -389,6 +391,12 @@ class LanguageServerOptionsImpl implements LanguageServerOptions {
public get crashDumpPath() {
return readOption<string | undefined>('dotnet.server.crashDumpPath', undefined);
}
public get analyzerDiagnosticScope() {
return readOption<string>('dotnet.backgroundAnalysis.analyzerDiagnosticsScope', 'openFiles');
}
public get compilerDiagnosticScope() {
return readOption<string>('dotnet.backgroundAnalysis.compilerDiagnosticsScope', 'openFiles');
}
}

class RazorOptionsImpl implements RazorOptions {
Expand Down
Loading