Skip to content

Commit db55fa1

Browse files
DonJayamanneDonJayamanne
authored
Ensure all diagnostic services do not handle the same messages more than once (microsoft#4038)
* Fix issues * Ensure all diagnostic checks accept resource args * Clear cache when testing * Linter issues * Address code review comments * Address code review * Fixed tests
1 parent 4e9f738 commit db55fa1

14 files changed

Lines changed: 423 additions & 160 deletions

‎src/client/application/diagnostics/base.ts‎

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
import { injectable, unmanaged } from 'inversify';
77
import { DiagnosticSeverity } from 'vscode';
8+
import { IWorkspaceService } from '../../common/application/types';
89
import { Resource } from '../../common/types';
910
import { IServiceContainer } from '../../ioc/types';
1011
import { sendTelemetryEvent } from '../../telemetry';
@@ -21,15 +22,49 @@ export abstract class BaseDiagnostic implements IDiagnostic {
2122

2223
@injectable()
2324
export abstract class BaseDiagnosticsService implements IDiagnosticsService {
25+
protected static handledDiagnosticCodeKeys: string[] = [];
2426
protected readonly filterService: IDiagnosticFilterService;
25-
constructor(@unmanaged() private readonly supportedDiagnosticCodes: string[],
26-
@unmanaged() protected serviceContainer: IServiceContainer) {
27+
constructor(
28+
@unmanaged() private readonly supportedDiagnosticCodes: string[],
29+
@unmanaged() protected serviceContainer: IServiceContainer
30+
) {
2731
this.filterService = serviceContainer.get<IDiagnosticFilterService>(IDiagnosticFilterService);
2832
}
2933
public abstract diagnose(resource: Resource): Promise<IDiagnostic[]>;
30-
public abstract handle(diagnostics: IDiagnostic[]): Promise<void>;
34+
public async handle(diagnostics: IDiagnostic[]): Promise<void> {
35+
if (diagnostics.length === 0) {
36+
return;
37+
}
38+
const diagnosticsToHandle = diagnostics.filter(item => {
39+
const key = this.getDiagnosticsKey(item);
40+
if (BaseDiagnosticsService.handledDiagnosticCodeKeys.indexOf(key) !== -1) {
41+
return false;
42+
}
43+
BaseDiagnosticsService.handledDiagnosticCodeKeys.push(key);
44+
return true;
45+
});
46+
await this.onHandle(diagnosticsToHandle);
47+
}
3148
public async canHandle(diagnostic: IDiagnostic): Promise<boolean> {
3249
sendTelemetryEvent(EventName.DIAGNOSTICS_MESSAGE, undefined, { code: diagnostic.code });
3350
return this.supportedDiagnosticCodes.filter(item => item === diagnostic.code).length > 0;
3451
}
52+
protected abstract onHandle(diagnostics: IDiagnostic[]): Promise<void>;
53+
/**
54+
* Returns a key used to keep track of whether a diagnostic was handled or not.
55+
* So as to prevent handling/displaying messages multiple times for the same diagnostic.
56+
*
57+
* @protected
58+
* @param {IDiagnostic} diagnostic
59+
* @returns {string}
60+
* @memberof BaseDiagnosticsService
61+
*/
62+
protected getDiagnosticsKey(diagnostic: IDiagnostic): string {
63+
if (diagnostic.scope === DiagnosticScope.Global) {
64+
return diagnostic.code;
65+
}
66+
const workspace = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
67+
const workspaceFolder = diagnostic.resource ? workspace.getWorkspaceFolder(diagnostic.resource) : undefined;
68+
return `${diagnostic.code}dbe75733-0407-4124-a1b2-ca769dc30523${workspaceFolder ? workspaceFolder.uri.fsPath : ''}`;
69+
}
3570
}

‎src/client/application/diagnostics/checks/envPathVariable.ts‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,19 @@ import { DiagnosticCodes } from '../constants';
1616
import { DiagnosticCommandPromptHandlerServiceId, MessageCommandPrompt } from '../promptHandler';
1717
import { DiagnosticScope, IDiagnostic, IDiagnosticHandlerService } from '../types';
1818

19-
const InvalidEnvPathVariableMessage = 'The environment variable \'{0}\' seems to have some paths containing the \'"\' character.' +
19+
const InvalidEnvPathVariableMessage =
20+
'The environment variable \'{0}\' seems to have some paths containing the \'"\' character.' +
2021
' The existence of such a character is known to have caused the {1} extension to not load. If the extension fails to load please modify your paths to remove this \'"\' character.';
2122

2223
export class InvalidEnvironmentPathVariableDiagnostic extends BaseDiagnostic {
2324
constructor(message: string, resource: Resource) {
24-
super(DiagnosticCodes.InvalidEnvironmentPathVariableDiagnostic,
25-
message, DiagnosticSeverity.Warning, DiagnosticScope.Global, resource);
25+
super(
26+
DiagnosticCodes.InvalidEnvironmentPathVariableDiagnostic,
27+
message,
28+
DiagnosticSeverity.Warning,
29+
DiagnosticScope.Global,
30+
resource
31+
);
2632
}
2733
}
2834

@@ -35,20 +41,21 @@ export class EnvironmentPathVariableDiagnosticsService extends BaseDiagnosticsSe
3541
constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) {
3642
super([DiagnosticCodes.InvalidEnvironmentPathVariableDiagnostic], serviceContainer);
3743
this.platform = this.serviceContainer.get<IPlatformService>(IPlatformService);
38-
this.messageService = serviceContainer.get<IDiagnosticHandlerService<MessageCommandPrompt>>(IDiagnosticHandlerService, DiagnosticCommandPromptHandlerServiceId);
44+
this.messageService = serviceContainer.get<IDiagnosticHandlerService<MessageCommandPrompt>>(
45+
IDiagnosticHandlerService,
46+
DiagnosticCommandPromptHandlerServiceId
47+
);
3948
}
4049
public async diagnose(resource: Resource): Promise<IDiagnostic[]> {
41-
if (this.platform.isWindows &&
42-
this.doesPathVariableHaveInvalidEntries()) {
50+
if (this.platform.isWindows && this.doesPathVariableHaveInvalidEntries()) {
4351
const env = this.serviceContainer.get<IApplicationEnvironment>(IApplicationEnvironment);
44-
const message = InvalidEnvPathVariableMessage
45-
.format(this.platform.pathVariableName, env.extensionName);
52+
const message = InvalidEnvPathVariableMessage.format(this.platform.pathVariableName, env.extensionName);
4653
return [new InvalidEnvironmentPathVariableDiagnostic(message, resource)];
4754
} else {
4855
return [];
4956
}
5057
}
51-
public async handle(diagnostics: IDiagnostic[]): Promise<void> {
58+
protected async onHandle(diagnostics: IDiagnostic[]): Promise<void> {
5259
// This class can only handle one type of diagnostic, hence just use first item in list.
5360
if (diagnostics.length === 0 || !this.canHandle(diagnostics[0])) {
5461
return;

‎src/client/application/diagnostics/checks/invalidDebuggerType.ts‎

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,20 @@ import { DiagnosticCodes } from '../constants';
1717
import { DiagnosticCommandPromptHandlerServiceId, MessageCommandPrompt } from '../promptHandler';
1818
import { DiagnosticScope, IDiagnostic, IDiagnosticHandlerService } from '../types';
1919

20-
const InvalidDebuggerTypeMessage = 'Your launch.json file needs to be updated to change the "pythonExperimental" debug ' +
20+
const InvalidDebuggerTypeMessage =
21+
'Your launch.json file needs to be updated to change the "pythonExperimental" debug ' +
2122
'configurations to use the "python" debugger type, otherwise Python debugging may ' +
2223
'not work. Would you like to automatically update your launch.json file now?';
2324

2425
export class InvalidDebuggerTypeDiagnostic extends BaseDiagnostic {
2526
constructor(message: string, resource: Resource) {
26-
super(DiagnosticCodes.InvalidDebuggerTypeDiagnostic,
27-
message, DiagnosticSeverity.Error, DiagnosticScope.WorkspaceFolder,
28-
resource);
27+
super(
28+
DiagnosticCodes.InvalidDebuggerTypeDiagnostic,
29+
message,
30+
DiagnosticSeverity.Error,
31+
DiagnosticScope.WorkspaceFolder,
32+
resource
33+
);
2934
}
3035
}
3136

@@ -39,7 +44,10 @@ export class InvalidDebuggerTypeDiagnosticsService extends BaseDiagnosticsServic
3944
protected readonly fs: IFileSystem;
4045
constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) {
4146
super([DiagnosticCodes.InvalidEnvironmentPathVariableDiagnostic], serviceContainer);
42-
this.messageService = serviceContainer.get<IDiagnosticHandlerService<MessageCommandPrompt>>(IDiagnosticHandlerService, DiagnosticCommandPromptHandlerServiceId);
47+
this.messageService = serviceContainer.get<IDiagnosticHandlerService<MessageCommandPrompt>>(
48+
IDiagnosticHandlerService,
49+
DiagnosticCommandPromptHandlerServiceId
50+
);
4351
const cmdManager = serviceContainer.get<ICommandManager>(ICommandManager);
4452
this.fs = this.serviceContainer.get<IFileSystem>(IFileSystem);
4553
cmdManager.registerCommand(CommandName, this.fixLaunchJson, this);
@@ -51,7 +59,7 @@ export class InvalidDebuggerTypeDiagnosticsService extends BaseDiagnosticsServic
5159
return [];
5260
}
5361
}
54-
public async handle(diagnostics: IDiagnostic[]): Promise<void> {
62+
protected async onHandle(diagnostics: IDiagnostic[]): Promise<void> {
5563
// This class can only handle one type of diagnostic, hence just use first item in list.
5664
if (diagnostics.length === 0 || !this.canHandle(diagnostics[0])) {
5765
return;
@@ -61,7 +69,10 @@ export class InvalidDebuggerTypeDiagnosticsService extends BaseDiagnosticsServic
6169
const options = [
6270
{
6371
prompt: 'Yes, update launch.json',
64-
command: commandFactory.createCommand(diagnostic, { type: 'executeVSCCommand', options: 'python.debugger.replaceExperimental' })
72+
command: commandFactory.createCommand(diagnostic, {
73+
type: 'executeVSCCommand',
74+
options: 'python.debugger.replaceExperimental'
75+
})
6576
},
6677
{
6778
prompt: 'No, I will do it later'
@@ -76,15 +87,19 @@ export class InvalidDebuggerTypeDiagnosticsService extends BaseDiagnosticsServic
7687
return false;
7788
}
7889

79-
const results = await Promise.all(workspaceService.workspaceFolders!.map(workspaceFolder => this.isExperimentalDebuggerUsedInWorkspace(workspaceFolder)));
90+
const results = await Promise.all(
91+
workspaceService.workspaceFolders!.map(workspaceFolder =>
92+
this.isExperimentalDebuggerUsedInWorkspace(workspaceFolder)
93+
)
94+
);
8095
return results.filter(used => used === true).length > 0;
8196
}
8297
private getLaunchJsonFile(workspaceFolder: WorkspaceFolder) {
8398
return path.join(workspaceFolder.uri.fsPath, '.vscode', 'launch.json');
8499
}
85100
private async isExperimentalDebuggerUsedInWorkspace(workspaceFolder: WorkspaceFolder) {
86101
const launchJson = this.getLaunchJsonFile(workspaceFolder);
87-
if (!await this.fs.fileExists(launchJson)) {
102+
if (!(await this.fs.fileExists(launchJson))) {
88103
return false;
89104
}
90105

@@ -97,10 +112,12 @@ export class InvalidDebuggerTypeDiagnosticsService extends BaseDiagnosticsServic
97112
return false;
98113
}
99114

100-
await Promise.all(workspaceService.workspaceFolders!.map(workspaceFolder => this.fixLaunchJsonInWorkspace(workspaceFolder)));
115+
await Promise.all(
116+
workspaceService.workspaceFolders!.map(workspaceFolder => this.fixLaunchJsonInWorkspace(workspaceFolder))
117+
);
101118
}
102119
private async fixLaunchJsonInWorkspace(workspaceFolder: WorkspaceFolder) {
103-
if (!await this.isExperimentalDebuggerUsedInWorkspace(workspaceFolder)) {
120+
if (!(await this.isExperimentalDebuggerUsedInWorkspace(workspaceFolder))) {
104121
return;
105122
}
106123

‎src/client/application/diagnostics/checks/invalidPythonPathInDebugger.ts‎

Lines changed: 66 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -19,49 +19,64 @@ import { BaseDiagnostic, BaseDiagnosticsService } from '../base';
1919
import { IDiagnosticsCommandFactory } from '../commands/types';
2020
import { DiagnosticCodes } from '../constants';
2121
import { DiagnosticCommandPromptHandlerServiceId, MessageCommandPrompt } from '../promptHandler';
22-
import { DiagnosticScope, IDiagnostic, IDiagnosticCommand, IDiagnosticHandlerService, IInvalidPythonPathInDebuggerService } from '../types';
22+
import {
23+
DiagnosticScope,
24+
IDiagnostic,
25+
IDiagnosticCommand,
26+
IDiagnosticHandlerService,
27+
IInvalidPythonPathInDebuggerService
28+
} from '../types';
2329

2430
export class InvalidPythonPathInDebuggerSettingsDiagnostic extends BaseDiagnostic {
2531
constructor(resource: Resource) {
26-
super(DiagnosticCodes.InvalidPythonPathInDebuggerSettingsDiagnostic,
27-
Diagnostics.invalidPythonPathInDebuggerSettings(), DiagnosticSeverity.Error, DiagnosticScope.WorkspaceFolder,
28-
resource);
32+
super(
33+
DiagnosticCodes.InvalidPythonPathInDebuggerSettingsDiagnostic,
34+
Diagnostics.invalidPythonPathInDebuggerSettings(),
35+
DiagnosticSeverity.Error,
36+
DiagnosticScope.WorkspaceFolder,
37+
resource
38+
);
2939
}
3040
}
3141

3242
export class InvalidPythonPathInDebuggerLaunchDiagnostic extends BaseDiagnostic {
3343
constructor(resource: Resource) {
34-
super(DiagnosticCodes.InvalidPythonPathInDebuggerLaunchDiagnostic,
35-
Diagnostics.invalidPythonPathInDebuggerLaunch(), DiagnosticSeverity.Error, DiagnosticScope.WorkspaceFolder,
36-
resource);
44+
super(
45+
DiagnosticCodes.InvalidPythonPathInDebuggerLaunchDiagnostic,
46+
Diagnostics.invalidPythonPathInDebuggerLaunch(),
47+
DiagnosticSeverity.Error,
48+
DiagnosticScope.WorkspaceFolder,
49+
resource
50+
);
3751
}
3852
}
3953

4054
export const InvalidPythonPathInDebuggerServiceId = 'InvalidPythonPathInDebuggerServiceId';
4155

4256
@injectable()
43-
export class InvalidPythonPathInDebuggerService extends BaseDiagnosticsService implements IInvalidPythonPathInDebuggerService {
44-
constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer,
57+
export class InvalidPythonPathInDebuggerService extends BaseDiagnosticsService
58+
implements IInvalidPythonPathInDebuggerService {
59+
constructor(
60+
@inject(IServiceContainer) serviceContainer: IServiceContainer,
4561
@inject(IWorkspaceService) private readonly workspace: IWorkspaceService,
4662
@inject(IDiagnosticsCommandFactory) private readonly commandFactory: IDiagnosticsCommandFactory,
4763
@inject(IInterpreterHelper) private readonly interpreterHelper: IInterpreterHelper,
4864
@inject(IConfigurationService) private readonly configService: IConfigurationService,
49-
@inject(IDiagnosticHandlerService) @named(DiagnosticCommandPromptHandlerServiceId) protected readonly messageService: IDiagnosticHandlerService<MessageCommandPrompt>) {
50-
super([DiagnosticCodes.InvalidPythonPathInDebuggerSettingsDiagnostic, DiagnosticCodes.InvalidPythonPathInDebuggerLaunchDiagnostic], serviceContainer);
65+
@inject(IDiagnosticHandlerService)
66+
@named(DiagnosticCommandPromptHandlerServiceId)
67+
protected readonly messageService: IDiagnosticHandlerService<MessageCommandPrompt>
68+
) {
69+
super(
70+
[
71+
DiagnosticCodes.InvalidPythonPathInDebuggerSettingsDiagnostic,
72+
DiagnosticCodes.InvalidPythonPathInDebuggerLaunchDiagnostic
73+
],
74+
serviceContainer
75+
);
5176
}
5277
public async diagnose(_resource: Resource): Promise<IDiagnostic[]> {
5378
return [];
5479
}
55-
public async handle(diagnostics: IDiagnostic[]): Promise<void> {
56-
// This class can only handle one type of diagnostic, hence just use first item in list.
57-
if (diagnostics.length === 0 || !this.canHandle(diagnostics[0])) {
58-
return;
59-
}
60-
const diagnostic = diagnostics[0];
61-
const commandPrompts = this.getCommandPrompts(diagnostic);
62-
63-
await this.messageService.handle(diagnostic, { commandPrompts });
64-
}
6580
public async validatePythonPath(pythonPath?: string, resource?: Uri) {
6681
pythonPath = pythonPath ? this.resolveVariables(pythonPath, resource) : undefined;
6782
let pathInLaunchJson = true;
@@ -85,6 +100,16 @@ export class InvalidPythonPathInDebuggerService extends BaseDiagnosticsService i
85100
}
86101
return false;
87102
}
103+
protected async onHandle(diagnostics: IDiagnostic[]): Promise<void> {
104+
// This class can only handle one type of diagnostic, hence just use first item in list.
105+
if (diagnostics.length === 0 || !this.canHandle(diagnostics[0])) {
106+
return;
107+
}
108+
const diagnostic = diagnostics[0];
109+
const commandPrompts = this.getCommandPrompts(diagnostic);
110+
111+
await this.messageService.handle(diagnostic, { commandPrompts });
112+
}
88113
protected resolveVariables(pythonPath: string, resource: Uri | undefined): string {
89114
const workspaceFolder = resource ? this.workspace.getWorkspaceFolder(resource) : undefined;
90115
const systemVariables = new SystemVariables(workspaceFolder ? workspaceFolder.uri.fsPath : undefined);
@@ -93,22 +118,30 @@ export class InvalidPythonPathInDebuggerService extends BaseDiagnosticsService i
93118
private getCommandPrompts(diagnostic: IDiagnostic): { prompt: string; command?: IDiagnosticCommand }[] {
94119
switch (diagnostic.code) {
95120
case DiagnosticCodes.InvalidPythonPathInDebuggerSettingsDiagnostic: {
96-
return [{
97-
prompt: 'Select Python Interpreter',
98-
command: this.commandFactory.createCommand(diagnostic, { type: 'executeVSCCommand', options: 'python.setInterpreter' })
99-
}];
121+
return [
122+
{
123+
prompt: 'Select Python Interpreter',
124+
command: this.commandFactory.createCommand(diagnostic, {
125+
type: 'executeVSCCommand',
126+
options: 'python.setInterpreter'
127+
})
128+
}
129+
];
100130
}
101131
case DiagnosticCodes.InvalidPythonPathInDebuggerLaunchDiagnostic: {
102-
return [{
103-
prompt: 'Open launch.json',
104-
// tslint:disable-next-line:no-object-literal-type-assertion
105-
command: {
106-
diagnostic, invoke: async (): Promise<void> => {
107-
const launchJson = this.getLaunchJsonFile(workspc.workspaceFolders![0]);
108-
await openFile(launchJson);
132+
return [
133+
{
134+
prompt: 'Open launch.json',
135+
// tslint:disable-next-line:no-object-literal-type-assertion
136+
command: {
137+
diagnostic,
138+
invoke: async (): Promise<void> => {
139+
const launchJson = this.getLaunchJsonFile(workspc.workspaceFolders![0]);
140+
await openFile(launchJson);
141+
}
109142
}
110143
}
111-
}];
144+
];
112145
}
113146
default: {
114147
throw new Error('Invalid diagnostic for \'InvalidPythonPathInDebuggerService\'');

0 commit comments

Comments
 (0)