From ac4de453c8cd42d7eb2d3a6939c5b7abae88fea8 Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Fri, 21 Jun 2024 08:46:52 -0700 Subject: [PATCH 1/2] wip --- src/adapter/evaluator.ts | 15 ++++++++++++++- src/adapter/threads.ts | 12 ++++++++++-- src/dap/api.d.ts | 17 +++++++++++++++++ 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/src/adapter/evaluator.ts b/src/adapter/evaluator.ts index a24be5c5e..59393b131 100644 --- a/src/adapter/evaluator.ts +++ b/src/adapter/evaluator.ts @@ -13,6 +13,7 @@ import { IPosition } from '../common/positions'; import { parseProgram, replace } from '../common/sourceCodeManipulations'; import { IRenameProvider, RenameMapping } from '../common/sourceMaps/renameProvider'; import { isInPatternSlot } from '../common/sourceUtils'; +import { Source } from './source'; import { StackFrame } from './stackTrace'; import { getSourceSuffix } from './templates'; @@ -121,6 +122,12 @@ export interface IEvaluateOptions extends IEvaluatorBaseOptions { * necessary to allow for renamed properties. */ stackFrame?: StackFrame; + + /** + * A manually-provided source location for the evaluation, as an alternative + * to {@link stackFrame} + */ + location?: { source: Source; position: IPosition }; } /** @@ -210,7 +217,13 @@ export class Evaluator implements IEvaluator { } let prepareOptions: IPrepareOptions | undefined = options; - if (options?.stackFrame) { + if (options?.location) { + const mapping = await this.renameProvider.provideForSource(options.location.source); + prepareOptions = { + ...prepareOptions, + renames: { mapping, position: options.location.position }, + }; + } else if (options?.stackFrame) { const mapping = await this.renameProvider.provideOnStackframe(options.stackFrame); prepareOptions = { ...prepareOptions, diff --git a/src/adapter/threads.ts b/src/adapter/threads.ts index 3cada8c8b..a6645db05 100644 --- a/src/adapter/threads.ts +++ b/src/adapter/threads.ts @@ -30,7 +30,7 @@ import { UserDefinedBreakpoint } from './breakpoints/userDefinedBreakpoint'; import { ICompletions } from './completions'; import { ExceptionMessage, IConsole, QueryObjectsMessage } from './console'; import { customBreakpoints } from './customBreakpoints'; -import { IEvaluator } from './evaluator'; +import { IEvaluateOptions, IEvaluator } from './evaluator'; import { IExceptionPauseService } from './exceptionPauseService'; import * as objectPreview from './objectPreview'; import { PreviewContextType, getContextForType } from './objectPreview/contexts'; @@ -560,6 +560,14 @@ export class Thread implements IVariableStoreLocationProvider { type: 'evaluation', }); + let location: IEvaluateOptions['location']; + if (args.source && args.line && args.column) { + const source = this._sourceContainer.source(args.source); + if (source) { + location = { source, position: new Base1Position(args.line, args.column) }; + } + } + const responsePromise = this.evaluator.evaluate( callFrameId ? { ...params, callFrameId } @@ -567,7 +575,7 @@ export class Thread implements IVariableStoreLocationProvider { ...params, contextId: this._selectedContext ? this._selectedContext.description.id : undefined, }, - { isInternalScript: false, stackFrame: stackFrame?.root }, + { isInternalScript: false, stackFrame: stackFrame?.root, location }, ); // Report result for repl immediately so that the user could see the expression they entered. diff --git a/src/dap/api.d.ts b/src/dap/api.d.ts index 020e01f98..88d5ad58e 100644 --- a/src/dap/api.d.ts +++ b/src/dap/api.d.ts @@ -2210,6 +2210,23 @@ export namespace Dap { */ frameId?: integer; + /** + * The contextual line where the expression should be evaluated. In the 'hover' context, this should be set to the start of the expression being hovered. + */ + line?: integer; + + /** + * The contextual column where the expression should be evaluated. This may be provided if `line` is also provided. + * + * It is measured in UTF-16 code units and the client capability `columnsStartAt1` determines whether it is 0- or 1-based. + */ + column?: integer; + + /** + * The contextual source in which the `line` is found. This must be provided if `line` is provided. + */ + source?: Source; + /** * The context in which the evaluate request is used. */ From 0e393ee701c1fcc47d24691b770e2064d150b887 Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Fri, 21 Jun 2024 11:13:24 -0700 Subject: [PATCH 2/2] feat: show correct values of shadowed variables in hovers Fixes a pain point I've had for years! ![](https://memes.peet.io/img/24-06-e64930d8-f2be-4723-8b61-0027216621bb.png) Closes #2022 --- CHANGELOG.md | 1 + package-lock.json | 14 +- package.json | 2 +- src/adapter/evaluator.ts | 132 ++++++++++++++---- src/adapter/exceptionPauseService.ts | 4 +- src/adapter/source.ts | 7 + src/adapter/stackTrace.ts | 7 +- src/adapter/threads.ts | 11 +- src/adapter/variableStore.ts | 8 +- src/common/promiseUtil.ts | 13 ++ .../evaluate/evaluate-shadowed-variables.txt | 15 ++ src/test/evaluate/evaluate.ts | 34 +++++ 12 files changed, 203 insertions(+), 45 deletions(-) create mode 100644 src/test/evaluate/evaluate-shadowed-variables.txt diff --git a/CHANGELOG.md b/CHANGELOG.md index de3efe7c1..e77c0dc36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ This changelog records changes to stable releases since 1.50.2. "TBA" changes he ## Nightly (only) +- feat: show correct values of shadowed variables in hovers ([#2022](https://github.com/microsoft/vscode-js-debug/issues/2022)) - fix: hanging on certain Linux environments ([vscode#214872](https://github.com/microsoft/vscode/issues/214872)) ## v1.90 (May 2024) diff --git a/package-lock.json b/package-lock.json index dad3295bc..8a8601f74 100644 --- a/package-lock.json +++ b/package-lock.json @@ -110,7 +110,7 @@ "stream-buffers": "^3.0.2", "ts-node": "^10.9.2", "tsx": "^4.7.0", - "typescript": "^5.3.3", + "typescript": "^5.5.2", "vsce": "^2.7.0" }, "engines": { @@ -14816,9 +14816,9 @@ } }, "node_modules/typescript": { - "version": "5.3.3", - "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.3.3.tgz", - "integrity": "sha512-pXWcraxM0uxAS+tN0AG/BF2TyqmHO014Z070UsJ+pFvYuRSq8KH8DmWpnbXe0pEPDHXZV3FcAbJkijJ5oNEnWw==", + "version": "5.5.2", + "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.5.2.tgz", + "integrity": "sha512-NcRtPEOsPFFWjobJEtfihkLCZCXZt/os3zf8nTxjVH3RvTSxjrCamJpbExGvYOF+tFHc3pA65qpdwPbzjohhew==", "dev": true, "bin": { "tsc": "bin/tsc", @@ -27022,9 +27022,9 @@ } }, "typescript": { - "version": "5.3.3", - "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.3.3.tgz", - "integrity": "sha512-pXWcraxM0uxAS+tN0AG/BF2TyqmHO014Z070UsJ+pFvYuRSq8KH8DmWpnbXe0pEPDHXZV3FcAbJkijJ5oNEnWw==", + "version": "5.5.2", + "resolved": "https://registry.npmjs.org/typescript/-/typescript-5.5.2.tgz", + "integrity": "sha512-NcRtPEOsPFFWjobJEtfihkLCZCXZt/os3zf8nTxjVH3RvTSxjrCamJpbExGvYOF+tFHc3pA65qpdwPbzjohhew==", "dev": true }, "uc.micro": { diff --git a/package.json b/package.json index a3369f7af..7646d41e3 100644 --- a/package.json +++ b/package.json @@ -157,7 +157,7 @@ "stream-buffers": "^3.0.2", "ts-node": "^10.9.2", "tsx": "^4.7.0", - "typescript": "^5.3.3", + "typescript": "^5.5.2", "vsce": "^2.7.0" }, "main": "./src/extension.js", diff --git a/src/adapter/evaluator.ts b/src/adapter/evaluator.ts index 59393b131..5598f8c3f 100644 --- a/src/adapter/evaluator.ts +++ b/src/adapter/evaluator.ts @@ -9,13 +9,15 @@ 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 { Base1Position, IPosition, Range } from '../common/positions'; +import { findIndexAsync } from '../common/promiseUtil'; import { parseProgram, replace } from '../common/sourceCodeManipulations'; import { IRenameProvider, RenameMapping } from '../common/sourceMaps/renameProvider'; import { isInPatternSlot } from '../common/sourceUtils'; import { Source } from './source'; import { StackFrame } from './stackTrace'; import { getSourceSuffix } from './templates'; +import { VariableStore } from './variableStore'; export const returnValueStr = '$returnValue'; @@ -25,12 +27,16 @@ const makeHoistedName = () => hoistedPrefix + randomBytes(8).toString('hex'); export const IEvaluator = Symbol('IEvaluator'); +export type PreparedHoistFn = ( + variable: string, +) => Promise | Cdp.Runtime.RemoteObject | undefined; + /** * Prepared call that can be invoked later on a callframe.. */ export type PreparedCallFrameExpr = ( params: Omit, - hoisted?: { [key: string]: Cdp.Runtime.RemoteObject }, + hoist?: PreparedHoistFn, ) => Promise; /** @@ -110,13 +116,13 @@ export interface IPrepareOptions extends IEvaluatorBaseOptions { 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?: ReadonlyArray; +export type LocationEvaluateOptions = { + source: Source; + position: IPosition; + variables: VariableStore; +}; +export interface IEvaluateOptions extends IEvaluatorBaseOptions { /** * Stack frame object on which the evaluation is being run. This is * necessary to allow for renamed properties. @@ -127,7 +133,7 @@ export interface IEvaluateOptions extends IEvaluatorBaseOptions { * A manually-provided source location for the evaluation, as an alternative * to {@link stackFrame} */ - location?: { source: Source; position: IPosition }; + location?: LocationEvaluateOptions; } /** @@ -188,10 +194,13 @@ export class Evaluator implements IEvaluator { return { canEvaluateDirectly: false, - invoke: (params, hoistMap = {}) => + invoke: (params, doHoist) => Promise.all( - [...toHoist].map(([ident, hoisted]) => - this.hoistValue(ident === returnValueStr ? this.returnValue : hoistMap[ident], hoisted), + [...toHoist].map(async ([ident, hoisted]) => + this.hoistValue( + ident === returnValueStr ? this.returnValue : await doHoist?.(ident), + hoisted, + ), ), ).then(() => this.cdp.Debugger.evaluateOnCallFrame({ ...params, expression: transformed })), }; @@ -209,35 +218,49 @@ export class Evaluator implements IEvaluator { ): Promise; public async evaluate( params: Cdp.Debugger.EvaluateOnCallFrameParams | Cdp.Runtime.EvaluateParams, - options?: IEvaluateOptions, + 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); } - let prepareOptions: IPrepareOptions | undefined = options; - if (options?.location) { - const mapping = await this.renameProvider.provideForSource(options.location.source); - prepareOptions = { - ...prepareOptions, - renames: { mapping, position: options.location.position }, - }; - } else if (options?.stackFrame) { - const mapping = await this.renameProvider.provideOnStackframe(options.stackFrame); - prepareOptions = { - ...prepareOptions, - renames: { mapping, position: options.stackFrame.rawPosition }, - }; + const prepareOptions: IPrepareOptions | undefined = { ...options }; + const { location, stackFrame } = options; + let hoist: PreparedHoistFn | undefined; + if (location) { + await Promise.all([ + // 1. Get the rename mapping at the desired position + Promise.resolve(this.renameProvider.provideForSource(location.source)).then(mapping => { + prepareOptions.renames = { mapping, position: location.position }; + }), + // 2. Hoist variables that may be shadowed. Identify the scope containing + // the location and mark any variables that appear in a higher scope + // (and therefore could be shadowed) as hoistable. + stackFrame && + this.setupShadowedVariableHoisting(location, stackFrame).then(r => { + if (r) { + hoist = r.doHoist; + prepareOptions.hoist = [...r.hoistable]; + } + }), + ]); + } else if (stackFrame) { + const mapping = await this.renameProvider.provideOnStackframe(stackFrame); + prepareOptions.renames = { mapping, position: stackFrame.rawPosition }; } - return this.prepare(params.expression, prepareOptions).invoke(params); + return this.prepare(params.expression, prepareOptions).invoke(params, hoist); } /** * Hoists the return value of the expression to the `globalThis`. */ public async hoistValue(object: Cdp.Runtime.RemoteObject | undefined, hoistedVar: string) { + if (object === undefined) { + return; + } + const objectId = object?.objectId; const dehoist = `setTimeout(() => { delete globalThis.${hoistedVar} }, 0)`; @@ -256,6 +279,61 @@ export class Evaluator implements IEvaluator { } } + /** + * Returns shadowed variables at the given location in the stack's scopes + * and a function that can be used to hoist the variables. + * + * It does this by identifying the scope the evaluation is being run in, + * marking all variables found in scopes above it as hoistable, and + * creating a function that will return the RemoteObject of a given + * shadowed variable. + */ + private async setupShadowedVariableHoisting( + { position, source, variables }: LocationEvaluateOptions, + stackFrame: StackFrame, + ) { + const { scopes } = await stackFrame.scopes(); + + const scopeIndex = await findIndexAsync( + scopes, + async s => + s.source && + s.line && + s.endLine && + new Range( + new Base1Position(s.line, s.column || 1), + new Base1Position(s.endLine, s.endColumn || Infinity), + ).contains(position) && + (await source.equalsDap(s.source)), + ); + if (scopeIndex === -1) { + return; + } + + const hoistable = new Set(); + await Promise.all( + scopes.slice(0, scopeIndex).map(async s => { + const vars = await variables.getVariableNames({ + variablesReference: s.variablesReference, + }); + for (const { name } of vars) { + hoistable.add(name); + } + }), + ); + + const doHoist: PreparedHoistFn = async variable => { + for (let i = scopeIndex; i < scopes.length; i++) { + const vars = await variables.getVariableNames({ + variablesReference: scopes[i].variablesReference, + }); + return vars.find(v => v.name === variable)?.remoteObject; + } + }; + + return { hoistable, doHoist }; + } + /** * Replaces a variable in the given expression with the `hoisted` variable, * returning the identifiers which were hoisted. diff --git a/src/adapter/exceptionPauseService.ts b/src/adapter/exceptionPauseService.ts index 3762b8857..937388068 100644 --- a/src/adapter/exceptionPauseService.ts +++ b/src/adapter/exceptionPauseService.ts @@ -168,7 +168,9 @@ export class ExceptionPauseService implements IExceptionPauseService { } private async evalCondition(evt: Cdp.Debugger.PausedEvent, method: PreparedCallFrameExpr) { - const r = await method({ callFrameId: evt.callFrames[0].callFrameId }, { error: evt.data }); + const r = await method({ callFrameId: evt.callFrames[0].callFrameId }, v => + v === 'error' ? evt.data : undefined, + ); return !!r?.result.value; } diff --git a/src/adapter/source.ts b/src/adapter/source.ts index cf1653d27..868e116c3 100644 --- a/src/adapter/source.ts +++ b/src/adapter/source.ts @@ -143,6 +143,13 @@ export class Source { return obj; } + public async equalsDap(s: Dap.Source) { + const existingAbsolutePath = await this._existingAbsolutePath; + return existingAbsolutePath + ? !s.sourceReference && existingAbsolutePath === s.path + : s.sourceReference === this.sourceReference; + } + private setSourceMapUrl(sourceMapMetadata?: ISourceMapMetadata) { if (!sourceMapMetadata) { this.sourceMap = undefined; diff --git a/src/adapter/stackTrace.ts b/src/adapter/stackTrace.ts index f5fce2e7f..26980354c 100644 --- a/src/adapter/stackTrace.ts +++ b/src/adapter/stackTrace.ts @@ -377,6 +377,11 @@ export class StackFrame implements IStackFrameElement { return new Base1Position(this._rawLocation.lineNumber, this._rawLocation.columnNumber); } + /** Raw chain from the runtime, applicable only to debug-triggered traces */ + public get rawScopeChain() { + return this._scope?.chain || []; + } + static fromRuntime( thread: Thread, callFrame: Cdp.Runtime.CallFrame, @@ -674,7 +679,7 @@ export class StackFrame implements IStackFrameElement { .getVariableNames({ variablesReference: scopeVariable.id, }) - .then(variables => variables.map(label => ({ label, type: 'property' }))), + .then(variables => variables.map(({ name }) => ({ label: name, type: 'property' }))), ); } const completions = await Promise.all(promises); diff --git a/src/adapter/threads.ts b/src/adapter/threads.ts index a6645db05..b6ba97367 100644 --- a/src/adapter/threads.ts +++ b/src/adapter/threads.ts @@ -30,7 +30,7 @@ import { UserDefinedBreakpoint } from './breakpoints/userDefinedBreakpoint'; import { ICompletions } from './completions'; import { ExceptionMessage, IConsole, QueryObjectsMessage } from './console'; import { customBreakpoints } from './customBreakpoints'; -import { IEvaluateOptions, IEvaluator } from './evaluator'; +import { IEvaluator, LocationEvaluateOptions } from './evaluator'; import { IExceptionPauseService } from './exceptionPauseService'; import * as objectPreview from './objectPreview'; import { PreviewContextType, getContextForType } from './objectPreview/contexts'; @@ -524,7 +524,6 @@ export class Thread implements IVariableStoreLocationProvider { args: Dap.EvaluateParamsExtended, variables: VariableStore, ): Promise { - const callFrameId = stackFrame?.root.callFrameId(); // For clipboard evaluations, return a safe JSON-stringified string. const params: Cdp.Runtime.EvaluateParams = args.context === 'clipboard' @@ -560,14 +559,16 @@ export class Thread implements IVariableStoreLocationProvider { type: 'evaluation', }); - let location: IEvaluateOptions['location']; + let location: LocationEvaluateOptions | undefined; if (args.source && args.line && args.column) { + const variables = this._pausedVariables; const source = this._sourceContainer.source(args.source); - if (source) { - location = { source, position: new Base1Position(args.line, args.column) }; + if (source && variables) { + location = { source, position: new Base1Position(args.line, args.column), variables }; } } + const callFrameId = stackFrame?.root.callFrameId(); const responsePromise = this.evaluator.evaluate( callFrameId ? { ...params, callFrameId } diff --git a/src/adapter/variableStore.ts b/src/adapter/variableStore.ts index 9fd4c096e..a98e398bc 100644 --- a/src/adapter/variableStore.ts +++ b/src/adapter/variableStore.ts @@ -545,7 +545,7 @@ class Variable implements IVariable { constructor( protected readonly context: VariableContext, - protected readonly remoteObject: Cdp.Runtime.RemoteObject, + public readonly remoteObject: Cdp.Runtime.RemoteObject, ) {} /** @@ -1392,14 +1392,16 @@ export class VariableStore { * Gets variable names from a known {@link IVariableContainer}. An optimized * version of `getVariables` that saves work generating previews. */ - public async getVariableNames(params: Dap.VariablesParams): Promise { + public async getVariableNames( + params: Dap.VariablesParams, + ): Promise<{ name: string; remoteObject: Cdp.Runtime.RemoteObject }[]> { const container = this.vars.get(params.variablesReference); if (!container) { return []; } const children = await container.getChildren(params); - return children.filter(isInstanceOf(Variable)).map(v => v.name); + return children.filter(isInstanceOf(Variable)); } /** Gets variables from a known {@link IVariableContainer} */ diff --git a/src/common/promiseUtil.ts b/src/common/promiseUtil.ts index d925491b4..379d7d386 100644 --- a/src/common/promiseUtil.ts +++ b/src/common/promiseUtil.ts @@ -46,6 +46,19 @@ export function some( }); } +export async function findIndexAsync( + array: ReadonlyArray, + predicate: (item: T) => Promise, +): Promise { + for (let i = 0; i < array.length; i++) { + if (await predicate(array[i])) { + return i; + } + } + + return -1; +} + export function getDeferred(): IDeferred { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion let resolve: IDeferred['resolve'] = null!; diff --git a/src/test/evaluate/evaluate-shadowed-variables.txt b/src/test/evaluate/evaluate-shadowed-variables.txt new file mode 100644 index 000000000..e58d6030c --- /dev/null +++ b/src/test/evaluate/evaluate-shadowed-variables.txt @@ -0,0 +1,15 @@ +result: 3 +line 1: +result: 3 +line 2: +result: 1 +line 3: +result: 1 +line 4: +result: 2 +line 5: +result: 2 +line 6: +result: 3 +line 7: +result: 3 diff --git a/src/test/evaluate/evaluate.ts b/src/test/evaluate/evaluate.ts index 1d7b76a1b..2721c7466 100644 --- a/src/test/evaluate/evaluate.ts +++ b/src/test/evaluate/evaluate.ts @@ -510,6 +510,40 @@ describe('evaluate', () => { r.assertLog(); }); + itIntegrates('shadowed variables', async ({ r }) => { + const p = await r.launchAndLoad('blank'); + + p.dap.evaluate({ + expression: `(function () { + let foo = 1; + if (true) { + let foo = 2; + if (true) { + let foo = 3; + debugger; + console.log(foo); + } + } + })();`, + }); + const sourcePromise = p.dap.once('loadedSource'); + const { threadId } = await p.dap.once('stopped'); + + const frameId = ( + await p.dap.stackTrace({ + threadId: threadId!, + }) + ).stackFrames[0].id; + const { source } = await sourcePromise; + await p.logger.evaluateAndLog('foo', { params: { frameId } }); + for (let line = 1; line <= 7; line++) { + p.log(`line ${line}:`); + await p.logger.evaluateAndLog('foo', { params: { frameId, line, column: 1, source } }); + } + + r.assertLog(); + }); + itIntegrates('supports bigint map keys (#1277)', async ({ r }) => { const p = await r.launchUrlAndLoad('index.html'); await p.logger.evaluateAndLog(`new Map([[1n, 'one'], [2n, 'two']])`);