Skip to content

Commit

Permalink
feat: support renamed sourcemap identifiers
Browse files Browse the repository at this point in the history
  • Loading branch information
connor4312 committed May 14, 2021
1 parent de896d2 commit fef3185
Show file tree
Hide file tree
Showing 29 changed files with 637 additions and 53 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ This changelog records changes to stable releases since 1.50.2. "TBA" changes he

## Nightly

- feat: support renamed sourcemap identifiers ([ref](https://github.com/microsoft/vscode/issues/12066))

This comment has been minimized.

Copy link
@aleclarson

aleclarson Mar 23, 2022

@connor4312 Is this feature tested with shadowed variables? I'm seeing an issue there. The variable in a higher scope seems to be preferred, when it should be the variable in the lowest scope. This leads to there being no way to inspect the (lower) variable in Watch view and the Debug Console.

This comment has been minimized.

Copy link
@aleclarson

aleclarson Mar 23, 2022

Wait, this might actually be caused by a sourcemap. Any idea if source mappings for shadowed variables should point to the same names index as the variable they're shadowing, or is that a mistake on the sourcemap generator's part?

This comment has been minimized.

Copy link
@connor4312

connor4312 Mar 23, 2022

Author Member

The closest rename before the current stack frame is used. js-debug does not ever (currently) parse ASTs for scripts, as this can be expensive--imagine giant minified code bundles. So, shadowing should usually work, but there may be cases where it doesn't such as if a variable is used in a closure which was declared before the variable was.

- feat: support DAP `hitBreakpointIds` ([#994](https://github.com/microsoft/vscode-js-debug/issues/994))
- refactor: include a mandator path in the CDP proxy ([#987](https://github.com/microsoft/vscode-js-debug/issues/987))
- fix: make sure servers are listening before returning
- fix: don't send infinite telemetry requests for React Native ([#981](https://github.com/microsoft/vscode-js-debug/issues/981))
Expand All @@ -12,7 +14,6 @@ This changelog records changes to stable releases since 1.50.2. "TBA" changes he
- chore: log errors activating auto attach
- fix: intermittent debug failures with browsers, especially Electron ([ref](https://github.com/microsoft/vscode/issues/123420)))
- fix: add additional languages for browser debugging ([ref](https://github.com/microsoft/vscode/issues/123484))
- feat: support DAP `hitBreakpointIds` ([#994](https://github.com/microsoft/vscode-js-debug/issues/994))
- feat: allow limited adjustment of launch config options during restart ([ref](https://github.com/microsoft/vscode/issues/118196))

## v1.56 (April 2021)
Expand Down
7 changes: 7 additions & 0 deletions demos/node/.vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,13 @@
"name": "[Node] Launch program",
"program": "${workspaceFolder}/main.js",
},
{
"type": "node",
"request": "launch",
"trace": true,
"name": "[Node] Launch Minified program",
"program": "${workspaceFolder}/main.min.js",
},
{
"name": "[Node] Launch File with Deno",
"type": "pwa-node",
Expand Down
2 changes: 2 additions & 0 deletions demos/node/main.min.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions demos/node/main.min.js.map

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

53 changes: 53 additions & 0 deletions demos/node/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion demos/node/package.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
{
"scripts": {
"start": "node main"
"start": "node main",
"minify": "terser main.js -m -o main.min.js --source-map \"url=main.min.js.map\" --toplevel"
},
"devDependencies": {
"terser": "^5.7.0"
}
}
10 changes: 5 additions & 5 deletions src/adapter/completions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ import { Identifier, MemberExpression, Node, Program } from 'estree';
import { inject, injectable } from 'inversify';
import Cdp from '../cdp/api';
import { ICdpApi } from '../cdp/connection';
import { IPosition } from '../common/positions';
import { getEnd, getStart, getText, parseProgram } from '../common/sourceCodeManipulations';
import { positionToOffset } from '../common/sourceUtils';
import { PositionToOffset } from '../common/stringUtils';
import Dap from '../dap/api';
import { IEvaluator, returnValueStr } from './evaluator';
import { StackFrame } from './stackTrace';
Expand All @@ -30,8 +31,7 @@ export interface ICompletionContext {
*/
export interface ICompletionExpression {
expression: string;
line: number;
column: number;
position: IPosition;
}

export interface ICompletionWithSort extends Dap.CompletionItem {
Expand Down Expand Up @@ -141,8 +141,7 @@ export class Completions {
options: ICompletionContext & ICompletionExpression,
): Promise<Dap.CompletionItem[]> {
const source = parseProgram(options.expression);

const offset = positionToOffset(options.expression, options.line, options.column);
const offset = new PositionToOffset(options.expression).convert(options.position);
let candidate: () => Promise<ICompletionWithSort[]> = () => Promise.resolve([]);

traverse(source, {
Expand Down Expand Up @@ -310,6 +309,7 @@ export class Completions {
const callFrameId = stackFrame && stackFrame.callFrameId();
const objRefResult = await this.evaluator.evaluate(
callFrameId ? { ...params, callFrameId } : { ...params, contextId: executionContextId },
{ stackFrame },
);

if (!objRefResult || objRefResult.exceptionDetails) {
Expand Down
2 changes: 2 additions & 0 deletions src/adapter/debugAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { Cdp } from '../cdp/api';
import { DisposableList, IDisposable } from '../common/disposable';
import { ILogger, LogTag } from '../common/logging';
import { getDeferred, IDeferred } from '../common/promiseUtil';
import { IRenameProvider } from '../common/sourceMaps/renameProvider';
import * as sourceUtils from '../common/sourceUtils';
import * as urlUtils from '../common/urlUtils';
import { AnyLaunchConfiguration } from '../configuration';
Expand Down Expand Up @@ -308,6 +309,7 @@ export class DebugAdapter implements IDisposable {
cdp,
this.dap,
delegate,
this._services.get(IRenameProvider),
this._services.get(ILogger),
this._services.get(IEvaluator),
this._services.get(ICompletions),
Expand Down
92 changes: 68 additions & 24 deletions src/adapter/evaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@ import { Node as AcornNode } from 'acorn';
import { generate } from 'astring';
import { randomBytes } from 'crypto';
import { replace } from 'estraverse';
import { ConditionalExpression } from 'estree';
import { ConditionalExpression, Expression } from 'estree';
import { inject, injectable } from 'inversify';
import Cdp from '../cdp/api';
import { ICdpApi } from '../cdp/connection';
import { IPosition } from '../common/positions';
import { parseProgram } from '../common/sourceCodeManipulations';
import { IRenameProvider, RenameMapping } from '../common/sourceMaps/renameProvider';
import { StackFrame } from './stackTrace';
import { getSourceSuffix } from './templates';

export const returnValueStr = '$returnValue';
Expand Down Expand Up @@ -97,14 +100,27 @@ export interface IPrepareOptions extends IEvaluatorBaseOptions {
* given remote objects.
*/
hoist?: ReadonlyArray<string>;

/**
* Optional information used to rename identifiers.
*/
renames?: RenamePrepareOptions;
}

export type RenamePrepareOptions = { position: IPosition; mapping: RenameMapping };

export interface IEvaluateOptions extends IEvaluatorBaseOptions {
/**
* Replaces the identifiers in the associated script with references to the
* given remote objects.
*/
hoist?: { [key: string]: Cdp.Runtime.RemoteObject };
hoist?: ReadonlyArray<string>;

/**
* Stack frame object on which the evaluation is being run. This is
* necessary to allow for renamed properties.
*/
stackFrame?: StackFrame;
}

/**
Expand All @@ -121,7 +137,10 @@ export class Evaluator implements IEvaluator {
return !!this.returnValue;
}

constructor(@inject(ICdpApi) private readonly cdp: Cdp.Api) {}
constructor(
@inject(ICdpApi) private readonly cdp: Cdp.Api,
@inject(IRenameProvider) private readonly renameProvider: IRenameProvider,
) {}

/**
* @inheritdoc
Expand All @@ -135,9 +154,9 @@ export class Evaluator implements IEvaluator {
*/
public prepare(
expression: string,
options: IPrepareOptions = {},
{ isInternalScript, hoist, renames }: IPrepareOptions = {},
): { canEvaluateDirectly: boolean; invoke: PreparedCallFrameExpr } {
if (options.isInternalScript !== false) {
if (isInternalScript !== false) {
expression += getSourceSuffix();
}

Expand All @@ -147,15 +166,16 @@ export class Evaluator implements IEvaluator {
// evalute the expression and unhoist it from the globals.
const toHoist = new Map<string, string>();
toHoist.set(returnValueStr, makeHoistedName());
for (const key of options.hoist ?? []) {
for (const key of hoist ?? []) {
toHoist.set(key, makeHoistedName());
}

const { transformed, hoisted } = this.replaceVariableInExpression(expression, toHoist);
const { transformed, hoisted } = this.replaceVariableInExpression(expression, toHoist, renames);
if (!hoisted.size) {
return {
canEvaluateDirectly: true,
invoke: params => this.cdp.Debugger.evaluateOnCallFrame({ ...params, expression }),
invoke: params =>
this.cdp.Debugger.evaluateOnCallFrame({ ...params, expression: transformed }),
};
}

Expand All @@ -178,18 +198,27 @@ export class Evaluator implements IEvaluator {
): Promise<Cdp.Debugger.EvaluateOnCallFrameResult>;
public evaluate(
params: Cdp.Runtime.EvaluateParams,
options?: IPrepareOptions,
options?: IEvaluateOptions,
): Promise<Cdp.Runtime.EvaluateResult>;
public async evaluate(
params: Cdp.Debugger.EvaluateOnCallFrameParams | Cdp.Runtime.EvaluateParams,
options?: IPrepareOptions,
options?: IEvaluateOptions,
) {
// no call frame means there will not be any relevant $returnValue to reference
if (!('callFrameId' in params)) {
return this.cdp.Runtime.evaluate(params);
}

return this.prepare(params.expression, options).invoke(params);
let prepareOptions: IPrepareOptions | undefined = options;
if (options?.stackFrame) {
const mapping = await this.renameProvider.provideOnStackframe(options.stackFrame);
prepareOptions = {
...prepareOptions,
renames: { mapping, position: options.stackFrame.rawPosition },
};
}

return this.prepare(params.expression, prepareOptions).invoke(params);
}

/**
Expand Down Expand Up @@ -221,9 +250,12 @@ export class Evaluator implements IEvaluator {
private replaceVariableInExpression(
expr: string,
hoistMap: Map<string /* identifier */, string /* hoised */>,
renames: RenamePrepareOptions | undefined,
): { hoisted: Set<string>; transformed: string } {
const hoisted = new Set<string>();
const replacement = (name: string): ConditionalExpression => ({
let mutated = false;

const replacement = (name: string, fallback: Expression): ConditionalExpression => ({
type: 'ConditionalExpression',
test: {
type: 'BinaryExpression',
Expand All @@ -237,30 +269,42 @@ export class Evaluator implements IEvaluator {
right: { type: 'Literal', value: 'undefined' },
},
consequent: { type: 'Identifier', name },
alternate: {
type: 'Identifier',
name: 'undefined',
},
alternate: fallback,
});

const parents: Node[] = [];
const transformed = replace(parseProgram(expr), {
enter: node => {
enter(node) {
const asAcorn = node as AcornNode;
if (
node.type === 'Identifier' &&
hoistMap.has(node.name) &&
expr[asAcorn.start - 1] !== '.'
) {
if (node.type !== 'Identifier' || expr[asAcorn.start - 1] === '.') {
return;
}

const hoistName = hoistMap.get(node.name);
if (hoistName) {
hoisted.add(node.name);
return replacement(hoistMap.get(node.name) as string);
mutated = true;
this.skip();
return replacement(hoistName, undefinedExpression);
}

const cname = renames?.mapping.getCompiledName(node.name, renames.position);
if (cname) {
mutated = true;
this.skip();
return replacement(cname, node);
}
},
leave: () => {
parents.pop();
},
});

return { hoisted, transformed: hoisted.size ? generate(transformed) : expr };
return { hoisted, transformed: mutated ? generate(transformed) : expr };
}
}

const undefinedExpression: Expression = {
type: 'Identifier',
name: 'undefined',
};
13 changes: 12 additions & 1 deletion src/adapter/stackTrace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import * as nls from 'vscode-nls';
import Cdp from '../cdp/api';
import { once } from '../common/objUtils';
import { Base0Position } from '../common/positions';
import Dap from '../dap/api';
import { asyncScopesNotAvailable } from '../dap/errors';
import { ProtocolError } from '../dap/protocolError';
Expand Down Expand Up @@ -181,6 +182,11 @@ export class StackFrame {
private _scope: IScope | undefined;
private _thread: Thread;

public get rawPosition() {
// todo: move RawLocation to use Positions, then just return that.
return new Base0Position(this._rawLocation.lineNumber, this._rawLocation.columnNumber);
}

static fromRuntime(
thread: Thread,
callFrame: Cdp.Runtime.CallFrame,
Expand Down Expand Up @@ -376,7 +382,12 @@ export class StackFrame {
return existing;
}

const scopeRef: IScopeRef = { callFrameId: scope.callFrameId, scopeNumber };
const scopeRef: IScopeRef = {
stackFrame: this,
callFrameId: scope.callFrameId,
scopeNumber,
};

const extraProperties: IExtraProperty[] = [];
if (scopeNumber === 0) {
extraProperties.push({ name: 'this', value: scope.thisObject });
Expand Down
Loading

0 comments on commit fef3185

Please sign in to comment.