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.

Sign up for GitHub

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

fix(breakpoints): Breakpoints could get overwritten when multiple TS files map to a single JS file. #285

Merged
merged 3 commits into from
Jan 28, 2025
Merged

fix(breakpoints): Breakpoints could get overwritten when multiple TS files map to a single JS file. #285

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6f7db07
fix(breakpoints): Breakpoints could get overwritten when multiple TS …
chmeyer-ms Jan 28, 2025
341305d
pr feedback
chmeyer-ms Jan 28, 2025
707aeab
bp map cleanup
chmeyer-ms Jan 28, 2025
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
125 changes: 75 additions & 50 deletions src/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,19 +47,19 @@

interface PluginDetails {
name: string;
module_uuid: string;

Check warning on line 50 in src/session.ts

View workflow job for this annotation

GitHub Actions / Build and Test 

Type Property name `module_uuid` must match one of the following formats: camelCase
}

interface ProtocolCapabilities {
type: string;
version: number;
plugins: PluginDetails[];
require_passcode?: boolean;

Check warning on line 57 in src/session.ts

View workflow job for this annotation

GitHub Actions / Build and Test 

Type Property name `require_passcode` must match one of the following formats: camelCase
}

interface ProfilerCapture {
type: string;
capture_base_path: string;

Check warning on line 62 in src/session.ts

View workflow job for this annotation

GitHub Actions / Build and Test 

Type Property name `capture_base_path` must match one of the following formats: camelCase
capture_data: string;
}

Expand Down Expand Up @@ -131,6 +131,7 @@
private _sourceFileWatcher?: FileSystemWatcher;
private _activeThreadId: number = 0; // the one being debugged
private _localRoot: string = '';
private _sourceBreakpointsMap: Map<string, DebugProtocol.SourceBreakpoint[] | undefined> = new Map();
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be simpler in usage if the value is just DebugProtocol.SourceBreakpoint[] without the undefined. Just each time you set it, like on line 353, do this._sourceBreakpointsMap.set(args.source.path, args.breakpoints ?? []); to handle the undefined input case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, good call.

private _sourceMapRoot?: string;
private _generatedSourceRoot?: string;
private _inlineSourceMap: boolean = false;
Expand Down Expand Up @@ -246,7 +247,7 @@
// VSCode extension has been activated due to the 'onDebug' activation request defined in packages.json
protected initializeRequest(
response: DebugProtocol.InitializeResponse,
args: DebugProtocol.InitializeRequestArguments
_args: DebugProtocol.InitializeRequestArguments
): void {
const capabilities: DebugProtocol.Capabilities = {
// indicates VSCode should send the configurationDoneRequest
Expand All @@ -269,9 +270,9 @@

// VSCode starts MC exe, then waits for MC to boot and connect back to a listening VSCode
protected async launchRequest(
response: DebugProtocol.LaunchResponse,
args: DebugProtocol.LaunchRequestArguments,
request?: DebugProtocol.Request
_response: DebugProtocol.LaunchResponse,
_args: DebugProtocol.LaunchRequestArguments,
_request?: DebugProtocol.Request
) {
// not implemented
}
Expand All @@ -280,7 +281,7 @@
protected async attachRequest(
response: DebugProtocol.AttachResponse,
args: IAttachRequestArguments,
request?: DebugProtocol.Request
_request?: DebugProtocol.Request
) {
this.closeSession();

Expand Down Expand Up @@ -337,7 +338,7 @@
protected async setBreakPointsRequest(
response: DebugProtocol.SetBreakpointsResponse,
args: DebugProtocol.SetBreakpointsArguments,
request?: DebugProtocol.Request
_request?: DebugProtocol.Request
) {
response.body = {
breakpoints: [],
Expand All @@ -348,52 +349,76 @@
return;
}

let originalLocalAbsolutePath = path.normalize(args.source.path);
// store source breakpoints per file
this._sourceBreakpointsMap.set(args.source.path, args.breakpoints);

const originalBreakpoints = args.breakpoints || [];
const generatedBreakpoints: DebugProtocol.SourceBreakpoint[] = [];
let generatedRemoteLocalPath = undefined;
// rebuild the generated breakpoints map each time a breakpoint is changed in any file
let generatedBreakpointsMap: Map<string, DebugProtocol.SourceBreakpoint[]> = new Map();

try {
// first get generated remote file path, will throw if fails
generatedRemoteLocalPath = await this._sourceMaps.getGeneratedRemoteRelativePath(originalLocalAbsolutePath);

// for all breakpoint positions set on the source file, get generated/mapped positions
if (originalBreakpoints.length) {
for (let originalBreakpoint of originalBreakpoints) {
const generatedPosition = await this._sourceMaps.getGeneratedPositionFor({
source: originalLocalAbsolutePath,
column: originalBreakpoint.column || 0,
line: originalBreakpoint.line,
});
generatedBreakpoints.push({
line: generatedPosition.line || 0,
column: 0,
});
// get generated breakpoints from all sources
for (let [sourcePath, sourceBreakpoints] of this._sourceBreakpointsMap) {
let originalLocalAbsolutePath = path.normalize(sourcePath);

const originalBreakpoints = sourceBreakpoints || [];
let generatedRemoteLocalPath = undefined;

try {
// first get generated remote file path, will throw if fails
generatedRemoteLocalPath = await this._sourceMaps.getGeneratedRemoteRelativePath(
originalLocalAbsolutePath
);

// append to any existing breakpoints for this generated file
if (!generatedBreakpointsMap.has(generatedRemoteLocalPath)) {
generatedBreakpointsMap.set(generatedRemoteLocalPath, []);
}
const generatedBreakpoints = generatedBreakpointsMap.get(generatedRemoteLocalPath)!;

// for all breakpoint positions set on the source file, get generated/mapped positions
if (originalBreakpoints.length) {
for (let originalBreakpoint of originalBreakpoints) {
const generatedPosition = await this._sourceMaps.getGeneratedPositionFor({
source: originalLocalAbsolutePath,
column: originalBreakpoint.column || 0,
Copy link

Choose a reason for hiding this comment

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

Using null coalescing operator should be better:

Suggested change
column: originalBreakpoint.column || 0,
column: originalBreakpoint.column ?? 0,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip. Looks like I have this in a LOT of places.

line: originalBreakpoint.line,
});
generatedBreakpoints.push({
line: generatedPosition.line || 0,
Copy link

Choose a reason for hiding this comment

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

Same here

Suggested change
line: generatedPosition.line || 0,
line: generatedPosition.line ?? 0,

column: 0,
});
}
}
} catch (e) {
this.log((e as Error).message, LogLevel.Error);
this.sendErrorResponse(
response,
1002,
`Failed to resolve breakpoint for ${originalLocalAbsolutePath}.`
);
continue;
}
} catch (e) {
this.log((e as Error).message, LogLevel.Error);
this.sendErrorResponse(response, 1002, `Failed to resolve breakpoint for ${originalLocalAbsolutePath}.`);
return;
}

const envelope = {
type: 'breakpoints',
breakpoints: {
path: generatedRemoteLocalPath,
breakpoints: generatedBreakpoints.length ? generatedBreakpoints : undefined,
},
};
// send full set of breakpoints for each generated file, a message per file
for (let [generatedRemoteLocalPath, generatedBreakpoints] of generatedBreakpointsMap) {
const envelope = {
type: 'breakpoints',
breakpoints: {
path: generatedRemoteLocalPath,
breakpoints: generatedBreakpoints.length ? generatedBreakpoints : undefined,
},
};
this.sendDebuggeeMessage(envelope);
}

this.sendDebuggeeMessage(envelope);
// notify vscode breakpoints have been set
this.sendResponse(response);
}

protected setExceptionBreakPointsRequest(
response: DebugProtocol.SetExceptionBreakpointsResponse,
args: DebugProtocol.SetExceptionBreakpointsArguments,
request?: DebugProtocol.Request
_request?: DebugProtocol.Request
): void {
this.sendDebuggeeMessage({
type: 'stopOnException',
Expand All @@ -405,8 +430,8 @@

protected configurationDoneRequest(
response: DebugProtocol.ConfigurationDoneResponse,
args: DebugProtocol.ConfigurationDoneArguments,
request?: DebugProtocol.Request
_args: DebugProtocol.ConfigurationDoneArguments,
_request?: DebugProtocol.Request
): void {
this.sendDebuggeeMessage({
type: 'resume',
Expand All @@ -416,7 +441,7 @@
}

// VSCode wants current threads (substitute JS contexts)
protected threadsRequest(response: DebugProtocol.ThreadsResponse, request?: DebugProtocol.Request): void {
protected threadsRequest(response: DebugProtocol.ThreadsResponse, _request?: DebugProtocol.Request): void {
response.body = {
threads: Array.from(this._threads.keys()).map(
thread => new Thread(thread, `thread 0x${thread.toString(16)}`)
Expand Down Expand Up @@ -478,7 +503,7 @@
protected variablesRequest(
response: DebugProtocol.VariablesResponse,
args: DebugProtocol.VariablesArguments,
request?: DebugProtocol.Request
_request?: DebugProtocol.Request
) {
// get variables at this reference (all vars in scope or vars in object/array)
this.sendDebugeeRequest(this._activeThreadId, response, args, (body: any) => {
Expand Down Expand Up @@ -513,7 +538,7 @@
protected pauseRequest(
response: DebugProtocol.PauseResponse,
args: DebugProtocol.PauseArguments,
request?: DebugProtocol.Request
_request?: DebugProtocol.Request
) {
this.sendDebugeeRequest(args.threadId, response, args, (body: any) => {
response.body = body;
Expand All @@ -531,7 +556,7 @@
protected stepInRequest(
response: DebugProtocol.StepInResponse,
args: DebugProtocol.StepInArguments,
request?: DebugProtocol.Request
_request?: DebugProtocol.Request
) {
this.sendDebugeeRequest(args.threadId, response, args, (body: any) => {
response.body = body;
Expand All @@ -542,7 +567,7 @@
protected stepOutRequest(
response: DebugProtocol.StepOutResponse,
args: DebugProtocol.StepOutArguments,
request?: DebugProtocol.Request
_request?: DebugProtocol.Request
) {
this.sendDebugeeRequest(args.threadId, response, args, (body: any) => {
response.body = body;
Expand All @@ -552,8 +577,8 @@

protected disconnectRequest(
response: DebugProtocol.DisconnectResponse,
args: DebugProtocol.DisconnectArguments,
request?: DebugProtocol.Request
_args: DebugProtocol.DisconnectArguments,
_request?: DebugProtocol.Request
): void {
// closeSession triggers the 'close' event on the socket which will call terminateSession
this.closeServer();
Expand Down Expand Up @@ -719,7 +744,7 @@
// ------------------------------------------------------------------------

// async send message of type 'request' with promise and await results.
private sendDebugeeRequestAsync(thread: number, response: DebugProtocol.Response, args: any): Promise<any> {
private sendDebugeeRequestAsync(_thread: number, response: DebugProtocol.Response, args: any): Promise<any> {
let promise = new Promise((resolve, reject) => {
let requestSeq = response.request_seq;
this._requests.set(requestSeq, {
Expand All @@ -733,7 +758,7 @@
}

// send message of type 'request' and callback with results.
private sendDebugeeRequest(thread: number, response: DebugProtocol.Response, args: any, callback: Function) {
private sendDebugeeRequest(_thread: number, response: DebugProtocol.Response, args: any, callback: Function) {
let requestSeq = response.request_seq;
this._requests.set(requestSeq, {
onSuccess: callback,
Expand Down
Loading