"Cannot read property 'lastIndexOf' of undefined" error in tsserver.js

[neoGeneva]

Bug Report

🔎 Search Terms

• lastIndexOf

🕗 Version & Regression Information

• This is a crash in tsserver.js
• This happens in 4.2.2, 4.2.3 and next (4.3.0-dev.20210305), but not version 4.1.5
• node version v14.15.4
• windows version 20H2

💻 Steps to reproduce

This happens consistently for me in the OmniSharp/omnisharp-vscode repository, when opening a typescript file with 4.2 installed.

1. git clone https://github.com/OmniSharp/omnisharp-vscode.git
2. cd omnisharp-vscode
3. npm install
4. npm install -g [email protected]
5. code .
6. Configure vscode to use the global ts server (for me this is "typescript.tsdk": "C:\\Users\\phil\\AppData\\Roaming\\npm\\node_modules\\typescript\\lib",)
7. Restart code
8. Open the TypeScript output to view errors

🙁 Actual behavior

The TypeScript language server crashes. Attached is a sample log:
tsserver.zip

🙂 Expected behavior

It to not crash.

Related Issues

There's a couple of related issues in vscode repo such as microsoft/vscode#115905

[IllusionMH]

@RyanCavanaugh is there a chance to reprioritize this issue: there is almost 20 reports (and growing) in just a week after release VS Code stable release?

Is there anything we should ask reporters to provide in order help with investigation?

[orta]

I wonder if this is a Windows only issue? I can't repro with the steps above ( tsserver: 4.3.0-dev.20210311 )

Judging on diffs - maybe this PR? #42150

[neoGeneva]

Looks likely it's windows related, I'm not familiar enough with tsserver.js to know exactly what's going on, but it looks like the undefined value originates here:

Where

a = "C:/Projects/OmniSharp/omnisharp-vscode/src/vscodeAdapter.ts"

and

cwd = "c:/Projects/OmniSharp/omnisharp-vscode"

So getPathComponents is returning the wrong thing?

edit:

The getPathComponents assumption is wrong sorry, looks like guessDirectorySymlink just steps down below 0.

[IllusionMH]

I wonder if this is a Windows only issue? I can't repro with the steps above ( tsserver: 4.3.0-dev.20210311 )

Yes, looks like it's Windows specific: I was able to reproduce in on Windows but not under WSL2

Still reproducible in latest nightly 4.3.0-dev.20210314

Did several checks and looks like:
First Bad is 4.2.0-dev.20201219
Last Good is 4.2.0-dev.20201211

Judging on diffs - maybe this PR? #42150

First bad is from December, but this PR was merged on January 20, so shouldn't be related.

[IllusionMH]

I wonder if this is a Windows only issue? I can't repro with the steps above ( tsserver: 4.3.0-dev.20210311 )

Yes, looks like it's Windows specific: I was able to reproduce in on Windows but not under WSL2

Still reproducible in latest nightly 4.3.0-dev.20210314

Did several checks and looks like:
First Bad is 4.2.0-dev.20201219
Last Good is 4.2.0-dev.20201211

Judging on diffs - maybe this PR? #42150

First bad is from December, but this PR was merged January 20, so shouldn't be related.

[mjbvz]

@orta @RyanCavanaugh We've seen a few reports of they on the VS Code side. Is this something that we could look into for a TS 4.2 recovery build if the fix isn't too risky?

[orta]

Just got back from a week off, I'll poke Daniel/Ryan about this issue and spin up a windows VM, from running on my Mac and in Windows these both pass:

// Difference case for C (as is the reference)
assert.deepEqual(ts.getPathComponents("C:/Projects/OmniSharp/omnisharp-vscode/src/vscodeAdapter.ts", "c:/Projects/OmniSharp/omnisharp-vscode"), 
    ["C:/", "Projects", "OmniSharp", "omnisharp-vscode", "src", "vscodeAdapter.ts"]);

// Same case of "c"
assert.deepEqual(ts.getPathComponents("c:/Projects/OmniSharp/omnisharp-vscode/src/vscodeAdapter.ts", "c:/Projects/OmniSharp/omnisharp-vscode"), 
    ["c:/", "Projects", "OmniSharp", "omnisharp-vscode", "src", "vscodeAdapter.ts"]);

And same for when they are normalized:

// Same case of "c"
assert.deepEqual(ts.getPathComponents(ts.getNormalizedAbsolutePath("c:/Projects/OmniSharp/omnisharp-vscode/src/vscodeAdapter.ts", "c:/Projects/OmniSharp/omnisharp-vscode")), 
  ["c:/", "Projects", "OmniSharp", "omnisharp-vscode", "src", "vscodeAdapter.ts"]);

// Difference case for C
assert.deepEqual(ts.getPathComponents(ts.getNormalizedAbsolutePath("C:/Projects/OmniSharp/omnisharp-vscode/src/vscodeAdapter.ts", "c:/Projects/OmniSharp/omnisharp-vscode")), 
  ["C:/", "Projects", "OmniSharp", "omnisharp-vscode", "src", "vscodeAdapter.ts"]);

Looking like ts.getPathComponents seems reliably gives back a useful set of path components with the expected values. Likely something to do with the while loop.

---

Given the time range when this was introduced, and that is it OS specific, it is likely that #41292 is probably the cause

[orta]

Confirmed that converting:

-            const realpathSync = _fs.realpathSync.native ?? _fs.realpathSync;
+           const realpathSync = _fs.realpathSync;

Which effectively undoes #41292 but does fix my repro, now to try figure out what triggers the difference in behavior

[amcasey]

Possibly unrelated, but there's a typo in the path here: https://github.com/OmniSharp/omnisharp-vscode/blob/a2c3a9d5024399ee65f77992dd81b63831d5cca6/test/unitTests/logging/OmnisharpChannelObserver.test.ts#L6

Edit: Fixing the typo mitigates the issue on my box.

[amcasey]

So, obviously, the path .,/../../src/vscodeAdapter can't be resolved. When normal resolution fails, we check if it's relative, conclude it's not (because it starts with neither ./, nor ../), and treat it as a module name. To find a module name (since we also recognize that it's not rooted), we walk up the directory hierarchy looking for node_modules folders. When we find one in the root, we check to see whether {project_root}/node_modules/.,/../../src/vscodeAdapter exists. As part of the existence check, we normalize the path and .,/ is cancelled out by the following ../, making the path syntactically valid, and node_modules/ cancels out with the remaining ../, accidentally leaving the correct path.

[Not fully confirmed] Downstream, we assume the path is in node_modules and fail to tidy it up when it turns out not to be.

How is realpathSync involved? I'm pretty sure there's a missing call to canonicalize the path in this bizarre corner case and we were just getting lucky because realpathSync doesn't modify the capitalization (whereas realpathSync.native does).

[amcasey]

Filed #43342. Still investigating why the reduced repro doesn't crash the server.

Edit: Looks like you need to have package.json and node_modules for the repro. I guess that makes sense, with a path in auto-imports.

Edit 2: node_modules\microsoft.aspnetcore.razor.vscode is important, for some reason

[amcasey]

Reduced repro

./tsconfig.json

{
}

./package.json

{
  "name": "csharp",
  "version": "1.23.10",
  "dependencies": {
    "microsoft.aspnetcore.razor.vscode": "6"
  }
}

./src/vscodeAdapter.ts

export {}

./test/unitTests/logging/OmnisharpChannelObserver.test.ts

import '.,/../../src/vscodeAdapter';

./node_modules/microsoft.aspnetcore.razor.vscode/package.json

{
  "name": "microsoft.aspnetcore.razor.vscode",
  "version": "6.0.0-alpha.1.20575.5",
  "types": "./extension.d.ts"
}

./node_modules/microsoft.aspnetcore.razor.vscode/extension.d.ts

export {}

[amcasey]

At least here and here, we depend on realpath to leave the path exactly the same if it doesn't include any symlinks.

[amcasey]

The bug - flagging a file as a symlink when it's not - applies more generally, but I think the crash will only happen if one of the root files is also resolved via a node_modules path (and there's an npm module with types for auto-import to pick up).

[amcasey]

Looks like 175K telemetry hits in the last day. (!!)

Edit: but they're from only 246 distinct machines.

[amcasey]

@sheetalkamat, @orta and I started trying to updating this location to be case-sensitivity aware by making Resolved.path, Resolved.originalPath and PathAndExtension.Path use Path, rather than string. We got into a bit of a mess updating hosts because some have getCanonicalFileName, some have useCaseSensitiveFileNames as a function, and some have useCaseSensitiveFileNames as a boolean. Before we tried to rationalize that, we thought we should double-check with you whether using Paths in the module resolver was even a sensible thing to do (our guess was "yes", since the cache appears to canonicalize paths already). Thoughts?

[sheetalkamat]

Result of module resolution should not be canonicalized.. you should canonicalize use instead. Since users can implement host providing custom resolution. Look at https://github.com/microsoft/TypeScript/blob/master/src/compiler/program.ts#L850 where host cannot assume everything will have extension.

[mjbvz]

Just a quick update on scheduling: last week we agreed to push this to 4.3 since telemetry shows that a limited number of machines are hitting this.

If you are impacted by this bug, once this issue is closed you should be able to install JavaScript and TypeScript nightly extension to pick up a fix instead of waiting on VS Code to pick up TS 4.3

[JakeTompkins]

I wonder if this is a Windows only issue? I can't repro with the steps above ( tsserver: 4.3.0-dev.20210311 )

Yes, looks like it's Windows specific: I was able to reproduce in on Windows but not under WSL2

Still reproducible in latest nightly 4.3.0-dev.20210314

Did several checks and looks like:
First Bad is 4.2.0-dev.20201219
Last Good is 4.2.0-dev.20201211

Judging on diffs - maybe this PR? #42150

First bad is from December, but this PR was merged January 20, so shouldn't be related.

I'm not sure I can be of much help here, but I want to let you all know that I've just gotten this error on a Mac. I've opened issue 128183 where you can see the stacktrace

[Stupidism]

Isn't this a serious but easy-to-fix bug that causes many people cannot using 4.3 at all? Do we have to wait until November?

[amcasey]

@Stupidism I'm actually not sure why this bug is still open, I believe the change that caused this issue was reverted in 4.2.4 and, I assume, 4.3. For November, we're attempting to re-implement the change without this bug.

Edit: 0d7b5e6 is in v4.2.4 and 6066eae is in v4.3-beta.

[amcasey]

@orta @DanielRosenwasser Any reason to keep this bug open?

[orta]

Nope, yeah I agree - it should be closed

[leanne1]

Just to flag i had this same error on version 4.4.2 on Mac OS 11.5.2. Downgraded to 4.3.5 and error not present.

[amcasey]

@leanne1 Can you please provide more details? I don't believe the code responsible for the original issue was present in 4.4 (though an improved implementation is available in the nightlies).

[stevoland]

@amcasey I am experiencing this issue on 4.4.3, MacOS 11.5. From logging, seems to be caused by this bit of our tsconfig:

"typeRoots": ["./typings", "./node_modules/@types"]

which I understand should be legit?

This causes guessDirectorySymlink to be called with /[CWD]/typings/index.d.ts, /[CWD]/node_modules/@types/../../typings/index.d.ts leading to the same paths to be compared (/[CWD]/typings/index.d.ts) in the while loop until Cannot read property 'lastIndexOf' of undefined

Update: git bisect suggests the issue was introduced in 4.4.0-dev.20210717

[doktordirk]

the last version without the error for me is 4.2.4 (using typedoc on a subdirectory)

[andrewbranch]

We’re now tracking this at #44953—a full repro would still be helpful 🙏