Incorporate PR feedback

- Properly extract API proposal into it's own file - Remove data breakpoint info convenience method - Provide proposal for different data breakpoint sources (discussion) - Ensure that breakpoint mode is properly updated and passed through
microsoft · Sep 4, 2024 · 8f778fc · 8f778fc
1 parent ede5a2a
commit 8f778fc
Show file tree

Hide file tree

Showing 11 changed files with 238 additions and 181 deletions.
diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/debug.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/debug.test.ts
@@ -104,21 +104,48 @@ suite('vscode API - debug', function () {
 		assert.strictEqual(addressDbp.accessType, 'readWrite');
 	});
 
-	test('data breakpoint - dynamic variable', async function () {
-		debug.addBreakpoints([new DataBreakpoint({ type: 'dynamicVariable', name: 'i', variablesReference: 1000 }, 'readWrite', false, 'data', false, 'condition', 'hitCondition', 'logMessage')]);
+	test('data breakpoint - expression', async function () {
+		debug.addBreakpoints([new DataBreakpoint({ type: 'expression', expression: 'i' }, 'readWrite', false, 'data', false, 'condition', 'hitCondition', 'logMessage')]);
 		const dynamicVariableDbp = debug.breakpoints[debug.breakpoints.length - 1] as DataBreakpoint;
 		assert.strictEqual(dynamicVariableDbp.condition, 'condition');
 		assert.strictEqual(dynamicVariableDbp.hitCondition, 'hitCondition');
 		assert.strictEqual(dynamicVariableDbp.logMessage, 'logMessage');
 		assert.strictEqual(dynamicVariableDbp.enabled, false);
 		assert.strictEqual(dynamicVariableDbp.label, 'data');
-		assert.strictEqual(dynamicVariableDbp.source.type, 'dynamicVariable');
-		assert.strictEqual(dynamicVariableDbp.source.name, 'i');
-		assert.strictEqual(dynamicVariableDbp.source.variablesReference, 1000);
+		assert.strictEqual(dynamicVariableDbp.source.type, 'expression');
+		assert.strictEqual(dynamicVariableDbp.source.expression, 'i');
 		assert.strictEqual(dynamicVariableDbp.canPersist, false);
 		assert.strictEqual(dynamicVariableDbp.accessType, 'readWrite');
 	});
 
+	test('data breakpoint - scoped', async function () {
+		debug.addBreakpoints([new DataBreakpoint({ type: 'scoped', expression: 'exp()', frameId: 1 }, 'readWrite', false, 'data', false, 'condition', 'hitCondition', 'logMessage')]);
+		const scopedExpression = debug.breakpoints[debug.breakpoints.length - 1] as DataBreakpoint;
+		assert.strictEqual(scopedExpression.condition, 'condition');
+		assert.strictEqual(scopedExpression.hitCondition, 'hitCondition');
+		assert.strictEqual(scopedExpression.logMessage, 'logMessage');
+		assert.strictEqual(scopedExpression.enabled, false);
+		assert.strictEqual(scopedExpression.label, 'data');
+		assert.strictEqual(scopedExpression.source.type, 'scoped');
+		assert.strictEqual(scopedExpression.source.frameId, 1);
+		assert.strictEqual(scopedExpression.source.expression, 'exp()');
+		assert.strictEqual(scopedExpression.canPersist, false);
+		assert.strictEqual(scopedExpression.accessType, 'readWrite');
+
+		debug.addBreakpoints([new DataBreakpoint({ type: 'scoped', variable: 'var', variablesReference: 1 }, 'readWrite', false, 'data', false, 'condition', 'hitCondition', 'logMessage')]);
+		const scopedVariable = debug.breakpoints[debug.breakpoints.length - 1] as DataBreakpoint;
+		assert.strictEqual(scopedVariable.condition, 'condition');
+		assert.strictEqual(scopedVariable.hitCondition, 'hitCondition');
+		assert.strictEqual(scopedVariable.logMessage, 'logMessage');
+		assert.strictEqual(scopedVariable.enabled, false);
+		assert.strictEqual(scopedVariable.label, 'data');
+		assert.strictEqual(scopedVariable.source.type, 'scoped');
+		assert.strictEqual(scopedVariable.source.variablesReference, 1);
+		assert.strictEqual(scopedVariable.source.variable, 'var');
+		assert.strictEqual(scopedVariable.canPersist, false);
+		assert.strictEqual(scopedVariable.accessType, 'readWrite');
+	});
+
 	test('start debugging', async function () {
 		let stoppedEvents = 0;
 		let variablesReceived: () => void;

diff --git a/src/vs/platform/extensions/common/extensionsApiProposals.ts b/src/vs/platform/extensions/common/extensionsApiProposals.ts
@@ -154,6 +154,9 @@ const _allApiProposals = {
 	customEditorMove: {
 		proposal: 'https://raw.githubusercontent.com/microsoft/vscode/main/src/vscode-dts/vscode.proposed.customEditorMove.d.ts',
 	},
+	debugDataBreakpoints: {
+		proposal: 'https://raw.githubusercontent.com/microsoft/vscode/main/src/vscode-dts/vscode.proposed.debugDataBreakpoints.d.ts',
+	},
 	debugVisualization: {
 		proposal: 'https://raw.githubusercontent.com/microsoft/vscode/main/src/vscode-dts/vscode.proposed.debugVisualization.d.ts',
 	},

diff --git a/src/vs/workbench/api/browser/mainThreadDebugService.ts b/src/vs/workbench/api/browser/mainThreadDebugService.ts
@@ -8,8 +8,7 @@ import { URI as uri, UriComponents } from '../../../base/common/uri.js';
 import { IDebugService, IConfig, IDebugConfigurationProvider, IBreakpoint, IFunctionBreakpoint, IBreakpointData, IDebugAdapter, IDebugAdapterDescriptorFactory, IDebugSession, IDebugAdapterFactory, IDataBreakpoint, IDebugSessionOptions, IInstructionBreakpoint, DebugConfigurationProviderTriggerKind, IDebugVisualization, DataBreakpointSetType } from '../../contrib/debug/common/debug.js';
 import {
 	ExtHostContext, ExtHostDebugServiceShape, MainThreadDebugServiceShape, DebugSessionUUID, MainContext,
-	IBreakpointsDeltaDto, ISourceMultiBreakpointDto, ISourceBreakpointDto, IFunctionBreakpointDto, IDebugSessionDto, IDataBreakpointDto, IStartDebuggingOptions, IDebugConfiguration, IThreadFocusDto, IStackFrameFocusDto,
-	IDataBreakpointInfo
+	IBreakpointsDeltaDto, ISourceMultiBreakpointDto, ISourceBreakpointDto, IFunctionBreakpointDto, IDebugSessionDto, IDataBreakpointDto, IStartDebuggingOptions, IDebugConfiguration, IThreadFocusDto, IStackFrameFocusDto
 } from '../common/extHost.protocol.js';
 import { extHostNamedCustomer, IExtHostContext } from '../../services/extensions/common/extHostCustomers.js';
 import severity from '../../../base/common/severity.js';
@@ -236,8 +235,11 @@ export class MainThreadDebugService implements MainThreadDebugServiceShape, IDeb
 				this.debugService.addDataBreakpoint({
 					description: dto.label,
 					src: dto.source.type === 'variable' ? { type: DataBreakpointSetType.Variable, dataId: dto.source.dataId }
-						: dto.source.type === 'dynamicVariable' ? { type: DataBreakpointSetType.DynamicVariable, name: dto.source.name, variablesReference: dto.source.variablesReference }
-							: { type: DataBreakpointSetType.Address, address: dto.source.address, bytes: dto.source.bytes },
+						: dto.source.type === 'address' ? { type: DataBreakpointSetType.Address, address: dto.source.address, bytes: dto.source.bytes }
+							: dto.source.type === 'expression' ? { type: DataBreakpointSetType.Expression, expression: dto.source.expression }
+								: dto.source.frameId ? { type: DataBreakpointSetType.Scoped, expression: dto.source.expression, frameId: dto.source.frameId }
+									: dto.source.variablesReference ? { type: DataBreakpointSetType.Scoped, variable: dto.source.variable, variablesReference: dto.source.variablesReference }
+										: { type: DataBreakpointSetType.Variable, dataId: '' }, // should not happen
 					condition: dto.condition,
 					enabled: dto.enabled,
 					hitCondition: dto.hitCondition,
@@ -376,22 +378,6 @@ export class MainThreadDebugService implements MainThreadDebugServiceShape, IDeb
 		return Promise.reject(new ErrorNoTelemetry('debug session not found'));
 	}
 
-	public $getDataBreakpointInfo(sessionId: DebugSessionUUID, name: string, variablesReference?: number): Promise<IDataBreakpointInfo | undefined> {
-		const session = this.debugService.getModel().getSession(sessionId, true);
-		if (session) {
-			return Promise.resolve(session.dataBreakpointInfo(name, variablesReference));
-		}
-		return Promise.reject(new ErrorNoTelemetry('debug session not found'));
-	}
-
-	public $getDataBytesBreakpointInfo(sessionId: DebugSessionUUID, address: string, bytes?: number): Promise<IDataBreakpointInfo | undefined> {
-		const session = this.debugService.getModel().getSession(sessionId, true);
-		if (session) {
-			return Promise.resolve(session.dataBytesBreakpointInfo(address, bytes));
-		}
-		return Promise.reject(new ErrorNoTelemetry('debug session not found'));
-	}
-
 	public $stopDebugging(sessionId: DebugSessionUUID | undefined): Promise<void> {
 		if (sessionId) {
 			const session = this.debugService.getModel().getSession(sessionId, true);
@@ -473,23 +459,28 @@ export class MainThreadDebugService implements MainThreadDebugServiceShape, IDeb
 					condition: fbp.condition,
 					hitCondition: fbp.hitCondition,
 					logMessage: fbp.logMessage,
-					functionName: fbp.name
+					functionName: fbp.name,
+					mode: fbp.mode
 				} satisfies IFunctionBreakpointDto;
 			} else if ('src' in bp) {
 				const dbp: IDataBreakpoint = bp;
 				return {
 					type: 'data',
 					id: dbp.getId(),
 					source: dbp.src.type === DataBreakpointSetType.Variable ? { type: 'variable', dataId: dbp.src.dataId }
-						: dbp.src.type === DataBreakpointSetType.DynamicVariable ? { type: 'dynamicVariable', name: dbp.src.name, variablesReference: dbp.src.variablesReference }
-							: { type: 'address', address: dbp.src.address, bytes: dbp.src.bytes },
+						: dbp.src.type === DataBreakpointSetType.Address ? { type: 'address', address: dbp.src.address, bytes: dbp.src.bytes }
+							: dbp.src.type === DataBreakpointSetType.Expression ? { type: 'expression', expression: dbp.src.expression }
+								: dbp.src.frameId ? { type: 'scoped', expression: dbp.src.expression, frameId: dbp.src.frameId }
+									: dbp.src.variablesReference ? { type: 'scoped', variable: dbp.src.variable, variablesReference: dbp.src.variablesReference }
+										: { type: 'variable', dataId: '' }, // should not happen
 					enabled: dbp.enabled,
 					condition: dbp.condition,
 					hitCondition: dbp.hitCondition,
 					logMessage: dbp.logMessage,
 					accessType: dbp.accessType,
 					label: dbp.description,
-					canPersist: dbp.canPersist
+					canPersist: dbp.canPersist,
+					mode: dbp.mode
 				} satisfies IDataBreakpointDto;
 			} else if ('uri' in bp) {
 				const sbp: IBreakpoint = bp;
@@ -503,6 +494,7 @@ export class MainThreadDebugService implements MainThreadDebugServiceShape, IDeb
 					uri: sbp.uri,
 					line: sbp.lineNumber > 0 ? sbp.lineNumber - 1 : 0,
 					character: (typeof sbp.column === 'number' && sbp.column > 0) ? sbp.column - 1 : 0,
+					mode: sbp.mode
 				} satisfies ISourceBreakpointDto;
 			} else {
 				return undefined;

diff --git a/src/vs/workbench/api/common/extHost.protocol.ts b/src/vs/workbench/api/common/extHost.protocol.ts
@@ -1632,8 +1632,6 @@ export interface MainThreadDebugServiceShape extends IDisposable {
 	$setDebugSessionName(id: DebugSessionUUID, name: string): void;
 	$customDebugAdapterRequest(id: DebugSessionUUID, command: string, args: any): Promise<any>;
 	$getDebugProtocolBreakpoint(id: DebugSessionUUID, breakpoinId: string): Promise<DebugProtocol.Breakpoint | undefined>;
-	$getDataBreakpointInfo(id: DebugSessionUUID, name: string, variablesReference?: number): Promise<IDataBreakpointInfo | undefined>;
-	$getDataBytesBreakpointInfo(id: DebugSessionUUID, address: string, bytes?: number): Promise<IDataBreakpointInfo | undefined>;
 	$appendDebugConsole(value: string): void;
 	$registerBreakpoints(breakpoints: Array<ISourceMultiBreakpointDto | IFunctionBreakpointDto | IDataBreakpointDto>): Promise<void>;
 	$unregisterBreakpoints(breakpointIds: string[], functionBreakpointIds: string[], dataBreakpointIds: string[]): Promise<void>;
@@ -2394,8 +2392,6 @@ export interface IFunctionBreakpointDto extends IBreakpointDto {
 	mode?: string;
 }
 
-export type IDataBreakpointInfo = DebugProtocol.DataBreakpointInfoResponse['body'];
-
 export interface IDataBreakpointDto extends IBreakpointDto {
 	type: 'data';
 	source: vscode.DataBreakpointSource;

diff --git a/src/vs/workbench/api/common/extHostDebugService.ts b/src/vs/workbench/api/common/extHostDebugService.ts
@@ -31,6 +31,7 @@ import { IExtHostCommands } from './extHostCommands.js';
 import * as Convert from './extHostTypeConverters.js';
 import { coalesce } from '../../../base/common/arrays.js';
 import { IExtHostTesting } from './extHostTesting.js';
+import { Mutable } from '../../../base/common/types.js';
 
 export const IExtHostDebugService = createDecorator<IExtHostDebugService>('IExtHostDebugService');
 
@@ -446,6 +447,20 @@ export abstract class ExtHostDebugServiceBase extends DisposableCls implements I
 					functionName: bp.functionName,
 					mode: bp.mode,
 				});
+			} else if (bp instanceof DataBreakpoint) {
+				dtos.push({
+					type: 'data',
+					id: bp.id,
+					enabled: bp.enabled,
+					hitCondition: bp.hitCondition,
+					logMessage: bp.logMessage,
+					condition: bp.condition,
+					source: bp.source,
+					mode: bp.mode,
+					canPersist: bp.canPersist,
+					accessType: bp.accessType,
+					label: bp.label
+				});
 			}
 		}
 
@@ -756,28 +771,31 @@ export abstract class ExtHostDebugServiceBase extends DisposableCls implements I
 					const bp = this._breakpoints.get(bpd.id);
 					if (bp) {
 						if (bp instanceof FunctionBreakpoint && bpd.type === 'function') {
-							const fbp = <any>bp;
+							const fbp = <Mutable<FunctionBreakpoint>>bp;
 							fbp.enabled = bpd.enabled;
 							fbp.condition = bpd.condition;
 							fbp.hitCondition = bpd.hitCondition;
 							fbp.logMessage = bpd.logMessage;
 							fbp.functionName = bpd.functionName;
+							fbp.mode = bpd.mode;
 						} else if (bp instanceof SourceBreakpoint && bpd.type === 'source') {
-							const sbp = <any>bp;
+							const sbp = <Mutable<SourceBreakpoint>>bp;
 							sbp.enabled = bpd.enabled;
 							sbp.condition = bpd.condition;
 							sbp.hitCondition = bpd.hitCondition;
 							sbp.logMessage = bpd.logMessage;
 							sbp.location = new Location(URI.revive(bpd.uri), new Position(bpd.line, bpd.character));
+							sbp.mode = bpd.mode;
 						} else if (bp instanceof DataBreakpoint && bpd.type === 'data') {
-							const dbp = <any>bp;
+							const dbp = <Mutable<DataBreakpoint>>bp;
 							dbp.enabled = bpd.enabled;
 							dbp.condition = bpd.condition;
 							dbp.hitCondition = bpd.hitCondition;
 							dbp.logMessage = bpd.logMessage;
 							dbp.label = bpd.label;
 							dbp.source = bpd.source;
 							dbp.canPersist = bpd.canPersist;
+							dbp.mode = bpd.mode;
 							dbp.accessType = bpd.accessType;
 						}
 						c.push(bp);
@@ -1143,12 +1161,6 @@ export class ExtHostDebugSession {
 			},
 			getDebugProtocolBreakpoint(breakpoint: vscode.Breakpoint): Promise<vscode.DebugProtocolBreakpoint | undefined> {
 				return that._debugServiceProxy.$getDebugProtocolBreakpoint(that._id, breakpoint.id);
-			},
-			getDataBreakpointInfo(name: string, variablesReference?: number): Promise<vscode.DataBreakpointInfo | undefined> {
-				return that._debugServiceProxy.$getDataBreakpointInfo(that._id, name, variablesReference);
-			},
-			getDataBytesBreakpointInfo(address: string, bytes?: number): Promise<vscode.DataBreakpointInfo | undefined> {
-				return that._debugServiceProxy.$getDataBytesBreakpointInfo(that._id, address, bytes);
 			}
 		});
 	}

diff --git a/src/vs/workbench/api/common/extHostTypes.ts b/src/vs/workbench/api/common/extHostTypes.ts
@@ -3088,7 +3088,10 @@ export class DataBreakpoint extends Breakpoint {
 		this.label = label ? label
 			: this.source.type === 'variable' ? `DataId '${this.source.dataId}'`
 				: this.source.type === 'address' ? `Address '${this.source.address}${this.source.bytes ? `,${this.source.bytes}'` : ''}`
-					: `Variable '${this.source.name}${this.source.variablesReference ? `,${this.source.variablesReference}` : ''}'`;
+					: this.source.type === 'expression' ? `Expression '${this.source.expression}'`
+						: this.source.frameId ? `Scoped '${this.source.expression}@${this.source.frameId}'`
+							: this.source.variablesReference ? `Scoped '${this.source.variable}@${this.source.variablesReference}'`
+								: `Unknown data breakpoint`;
 	}
 }
 
@@ -4129,7 +4132,7 @@ export function validateTestCoverageCount(cc?: vscode.TestCoverageCount) {
 	}
 
 	if (cc.covered > cc.total) {
-		throw new Error(`The total number of covered items (${cc.covered}) cannot be greater than the total(${cc.total})`);
+		throw new Error(`The total number of covered items (${cc.covered}) cannot be greater than the total (${cc.total})`);
 	}
 
 	if (cc.total < 0) {

diff --git a/src/vs/workbench/contrib/debug/browser/debugSession.ts b/src/vs/workbench/contrib/debug/browser/debugSession.ts
@@ -553,8 +553,8 @@ export class DebugSession implements IDebugSession, IDisposable {
 		return this._dataBreakpointInfo({ name: address, bytes, asAddress: true });
 	}
 
-	dataBreakpointInfo(name: string, variablesReference?: number): Promise<IDataBreakpointInfoResponse | undefined> {
-		return this._dataBreakpointInfo({ name, variablesReference });
+	dataBreakpointInfo(name: string, variablesReference?: number, frameId?: number): Promise<IDataBreakpointInfoResponse | undefined> {
+		return this._dataBreakpointInfo({ name, variablesReference, frameId });
 	}
 
 	private async _dataBreakpointInfo(args: DebugProtocol.DataBreakpointInfoArguments): Promise<IDataBreakpointInfoResponse | undefined> {

diff --git a/src/vs/workbench/contrib/debug/common/debug.ts b/src/vs/workbench/contrib/debug/common/debug.ts
@@ -432,7 +432,7 @@ export interface IDebugSession extends ITreeElement {
 
 	sendBreakpoints(modelUri: uri, bpts: IBreakpoint[], sourceModified: boolean): Promise<void>;
 	sendFunctionBreakpoints(fbps: IFunctionBreakpoint[]): Promise<void>;
-	dataBreakpointInfo(name: string, variablesReference?: number): Promise<IDataBreakpointInfoResponse | undefined>;
+	dataBreakpointInfo(name: string, variablesReference?: number, frameId?: number): Promise<IDataBreakpointInfoResponse | undefined>;
 	dataBytesBreakpointInfo(address: string, bytes?: number): Promise<IDataBreakpointInfoResponse | undefined>;
 	sendDataBreakpoints(dbps: IDataBreakpoint[]): Promise<void>;
 	sendInstructionBreakpoints(dbps: IInstructionBreakpoint[]): Promise<void>;
@@ -646,7 +646,8 @@ export interface IExceptionBreakpoint extends IBaseBreakpoint {
 export const enum DataBreakpointSetType {
 	Variable,
 	Address,
-	DynamicVariable
+	Expression,
+	Scoped
 }
 
 /**
@@ -661,22 +662,39 @@ export type DataBreakpointSource =
 		/** An identifier for the data. If it was retrieved using a `variablesReference` it may only be valid in the current suspended state, otherwise it's valid indefinitely. */
 		dataId: string;
 	}
-	| {
-		/** The source type for address-based data breakpoints. This only works on sessions that have the `supportsDataBreakpointBytes` capability. */
-		type: DataBreakpointSetType.DynamicVariable;
-		/** The name of the variable's child to obtain data breakpoint information for. If `variablesReference` isn't specified, this can be an expression. */
-		name: string;
-		/** Reference to the variable container if the data breakpoint is requested for a child of the container. */
-		variablesReference?: number;
-	}
 	| {
 		/** The source type for address-based data breakpoints. This only works on sessions that have the `supportsDataBreakpointBytes` capability. */
 		type: DataBreakpointSetType.Address;
 		/** A memory address as a decimal value, or hex value if it is prefixed with `0x`. */
 		address: string;
 		/** If specified, returns information for the range of memory extending `bytes` number of bytes from the address. */
 		bytes?: number;
-	};
+	}
+	| {
+		/** The source type for address-based data breakpoints. This only works on sessions that have the `supportsDataBreakpointBytes` capability. */
+		type: DataBreakpointSetType.Expression;
+		/** A global expression that is first evaluated when the breakpoint is activated. */
+		expression: string;
+	}
+	| {
+		/** The source type for address-based data breakpoints. This only works on sessions that have the `supportsDataBreakpointBytes` capability. */
+		type: DataBreakpointSetType.Scoped;
+	} & (
+		| {
+			/** The name of the variable that is used for resolution. */
+			variable: string;
+			/** Reference to the variable container that has the variable named `variable`. */
+			variablesReference: number;
+			frameId?: never;
+		}
+		| {
+			/** The name of the expression that is used for resolution. */
+			expression: string;
+			/** Reference to the stack frame to which the expression is scoped. */
+			frameId: number;
+			variablesReference?: never;
+		}
+	);
 
 export interface IDataBreakpoint extends IBaseBreakpoint {
 	readonly description: string;