Variable renamed incorrectly, leading to thrown error

[jakebailey]

Describe the bug

When debugging the TS codebase (which is bundled with esbuild), I periodically hit cases where the variable name I'm using in the debug console doesn't work and ends up throwing.

To Reproduce
Steps to reproduce the behavior:

1. Clone https://github.com/microsoft/TypeScript, npm install, etc
2. Set breakpoint on the line after const links = ... in getTypeOfVariableOrParameterOrProperty in checker.ts.
3. With .vscode/launch.template.json copied to .vscode/launch.json, open 2dArrays.ts and run the test via the "mocha tests" launcher.
4. Once the breakpoint is hit, open the debug console and type links.
5. Note the stack trace which mentions links2.
6. Disable source mapped stepping; note the variable is called links, not links2, and now the debug console works.

Log File

vscode-debugadapter-07d5029d.json.gz

VS Code Version: 1.74.3 with debugger v2023.1.3017

Additional context

Also, the error that's thrown bypasses #1259, somehow; the error is huge.

[jakebailey]

This also appears to be related; when at the same breakpoint (and anywhere in debugging, really), if I write:

(node) => node

Into the debug console, I get:

Uncaught SyntaxError SyntaxError: Malformed arrow function parameter list

But disabling source mapped stepping makes the problem go away.

[connor4312]

I think we may try to rename node in the parameter, which is not correct. Will check on that

Renames are not AST-aware, we just try to get the closest rename. In general I really want to avoid having to parse source AST's. Maybe it's sufficient to generate a set of fallbacks, like typeof links2 === 'undefined ? (typeof links1 === 'undefined' ? links : links1) : links2. Renames that share the underlying source name will usually be a result of shadowing, and thus the non-relevant links aliases won't be available in that scope since they wouldn't have been used...

[jakebailey]

To be clear, you mean avoiding parsing the AST of the debug console input? I would expect that to always be small so not actually expensive in practice.

Maybe it's sufficient to generate a set of fallbacks, like typeof links2 === 'undefined ? (typeof links1 === 'undefined' ? links : links1) : links2.

That set of fallbacks won't work if the variable happens to be undefined already, though.

I'm a little confused, though; in this case, node is not something that's actually in scope, so why is it getting a renaming at all? This to me sounds like the first part of the issue where it's spuriously using the name links2.

E.g. without source mapping:

node
Uncaught ReferenceError ReferenceError: node is not defined
    at eval (repl:1:1)

[connor4312]

We do parse debug console input, but we don't parse sources in the runtime. Sourcemaps just say "the thing at this location is now named something else", so to deal with renames we just pull the closest rename to the current stackframe for each identifier we find in the evaluate input.

That set of fallbacks won't work if the variable happens to be undefined already, though.

If only one of the aliases is defined, it would still return undefined. Alternatively we could do a big try/catch to access each variable. Which would probably be fine...

node is not something that's actually in scope

If it's anywhere in the source before the current stackframe, we'll try to rename it.

[jakebailey]

Hm, I guess I thought that the info from whatever powers the "variables" panel was in use here, since that seems to have the up-to-date scope information. But, obviously I'm not an expert at this.

[connor4312]

Yea, we could be a bit smarter about not renaming things that aren't in scopes. But, assuming we don't rename things in a way that generates invalid syntax, renaming things that are contained within the REPL input should be harmless. const node=42;console.log(node) turning into const node2=42;console.log(node2) for example.

[jakebailey]

I suppose that's true, but you'd have to be careful about something like (node, node2) => node + node2 and similar, and I'm not quite sure how to do that without moving past a simple replacement.

[connor4312]

This also appears to be related; when at the same breakpoint (and anywhere in debugging, really), if I write:

(node) => node

Into the debug console, I get:

Uncaught SyntaxError SyntaxError: Malformed arrow function parameter list

But disabling source mapped stepping makes the problem go away.

For me, doing that generates (renamedIndentifier) => typeof renamedIdentifier === 'undefined' ? node : renamedIdentifier. Which, regardless of the merits, is syntactically valid. Could you grab a trace log so I can see how it's getting transformed for you?

[connor4312]

I played with actually doing the parsing to make renames scope-aware. As expected, it can be slow for large files. But if I put it in a worker, it should be okay: #1548

A faster approach for just this would be doing curly-brace matching instead of full AST parsing, which could be sufficient, but not as correct, since we want to actually include everything in the containing statement rather than just variables declared in each block (such as function parameters)

[jakebailey]

Here's a trace log of the (node) => node:

vscode-debugadapter-9e1ab71a.json.gz

[jakebailey]

Wow, very odd, it's transforming it to:

(node2.kind => typeof node2.kind !== "undefined" ? node2.kind : node);

[connor4312]

Ahh, yep, that would do it. We added support for member access renames, but I didn't update the replacer to account for that.

[jakebailey]

I'm hitting what I think is still this bug more and more. For example, I was debugging our parser and wanted completions on the scanner but didn't get any. Checking the variable, the debug console instead gave me the wrong thing entirely; compare source mapped versus not.

I think @andrewbranch hit a case just like this with hover yesterday afternoon.

[andrewbranch]

Yep, I spent like 20 minutes trying to figure out why something was undefined when it turned out to be a hover showing me the value of a completely different variable 😦

[connor4312]

I'm on vacation currently and for the duration of next week, so this will not be fixed immediately, but soon.

My current thinking is that I still really dislike parsing the AST of compiled code. Even in a worker, taking a second to do that means an (extra) second of delay before variables are available. One thing I'm considering is writing a script that:

1. Knows about JavaScript string syntax (to ignore their contents) and
2. Tracks braces and parenthesis to generate rename scopes

(or use a lexer that can do this; currently we use acorn for parsing and manipulation, but it doesn't have a 'lex-only' mode)

[jakebailey]

So far the best workaround I've had is to follow the instructions to install nightly, then downgrade the nightly verison to v2022.12.1317. This version doesn't appear to have the bug, but does thankfully contain the performance optimizations needed to make debugging responsive in the TypeScript repo.

EDIT: except, breakpoints don't seem to work in this revision, so, not sure what's happening there.

[andrewbranch]

Here’s another interesting one; apparently it’s not just renaming identifiers; I don’t know what it’s doing but it’s mangling the syntax here

[jakebailey]

Yeah, I think that one is somewhat the same as: #1542 (comment)

[connor4312]

Looked at a few solutions. Using swc in webassembly is a bit faster than Acorn's parser, running 2.5 ops/s instead of acorn.parse()'s 0.9 ops/s for TypeScript's run.js. But it also is a decently sized wasm file (350KB / 150KB gzipped).

In my search I found https://github.com/guybedford/es-module-lexer, which is a lexer dedicated to the purpose of extracting import/export statements. It's widely used (12M downloads/month), tiny, and incredibly fast, handling run.js at 1,500,000 ops/s. My plan is to fork and repurpose that module for extracting the relevant information we need for scopes--or write something similar to it in rust or javascript. I have a 6 hour train ride today, so we'll see how far I can get :)

[jakebailey]

Given a user's going to also have the TypeScript extension running, I wonder if it'd be possible to grab the info from there instead, given it should be already parsed and unlikely to change (so maybe quick).

[jakebailey]

Although I guess these are output files so they wouldn't be immediate. I am curious about our parser's perf, of course.

[connor4312]

This is gone in #1566

[jakebailey]

Thanks! Looking forward to the next nightly or stable so I can try it out.