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

Question

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

neoGeneva opened this issue 5 years ago · 28 comments

neoGeneva commented 5 years ago

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.

git clone https://github.com/OmniSharp/omnisharp-vscode.git
cd omnisharp-vscode
npm install
npm install -g typescript@4.2.2
code .
Configure vscode to use the global ts server (for me this is "typescript.tsdk": "C:\\Users\\phil\\AppData\\Roaming\\npm\\node_modules\\typescript\\lib",)
Restart code
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

Answer 1 · 2021-03-12T13:07:32.000Z

@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?

Answer 2 · 2021-03-12T14:24:51.000Z

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

Answer 3 · 2021-03-13T03:15:37.000Z

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.

Answer 4 · 2021-03-15T02:13:09.000Z

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.

Answer 5 · 2021-03-15T02:13:26.000Z

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.

Answer 6 · 2021-03-18T20:12:18.000Z

@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?

Answer 7 · 2021-03-22T12:45:59.000Z

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

Answer 8 · 2021-03-22T16:46:07.000Z

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

Answer 9 · 2021-03-22T22:48:13.000Z

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.

Answer 10 · 2021-03-22T23:35:55.000Z

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).

Answer 11 · 2021-03-23T00:12:56.000Z

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

Answer 12 · 2021-03-23T16:27:49.000Z

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 {}

Answer 13 · 2021-03-23T21:28:11.000Z

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

Answer 14 · 2021-03-23T22:12:15.000Z

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).

Answer 15 · 2021-03-23T22:40:25.000Z

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

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

Answer 16 · 2021-03-29T17:42:07.000Z

@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?

Answer 17 · 2021-03-29T18:07:00.000Z

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.

Answer 18 · 2021-03-31T04:30:45.000Z

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

Answer 19 · 2021-07-08T07:20:10.000Z

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

Answer 20 · 2021-08-18T00:05:09.000Z

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?

Answer 21 · 2021-08-18T00:32:40.000Z

@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.

Answer 22 · 2021-08-18T00:40:58.000Z

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

Answer 23 · 2021-08-18T09:17:27.000Z

Nope, yeah I agree - it should be closed

Answer 24 · 2021-09-10T13:35:50.000Z

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.

Answer 25 · 2021-09-10T14:55:28.000Z

@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).

Answer 26 · 2021-09-13T16:49:14.000Z

@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

Answer 27 · 2021-09-16T11:46:54.000Z

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

Answer 28 · 2021-09-16T16:21:40.000Z

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