Opening a file causes subsequent completion failures

[DanielRosenwasser]

// @filename: classes.ts
import { Component } from "./utils.js";

export class MyComponent extends Component {
    render/**/
}

// @filename: utils.ts
export class Element {
    // ...
}

export abstract class Component {
    abstract render(): Element;
}

// @filename: tsconfig.json
{
    "compilerOptions": {
        "target": "es2022",
        "module": "nodenext",
        "strict": true,
        "outDir": "../out",
        "moduleDetection": "force"
    }
}

1. Request completions with auto-imports and class member completions at /**/
2. Open up utils.ts so that it is not just in the program, but is also an open file.
3. Erase a single character at the marker.
4. Request completions.

Response received: completionInfo (32). Request took 54 ms. Success: false . Message: Error processing request. Debug Failure.
Error: Debug Failure.
    at Object.addImportFromExportedSymbol (tsserver.js:141295:13)
    at tsserver.js:147019:38
    at Array.forEach (<anonymous>)
    at importSymbols (tsserver.js:147019:11)
    at createSignatureDeclarationFromSignature (tsserver.js:146653:9)
    at outputMethod (tsserver.js:146541:20)
    at Object.addNewNodeForMemberSymbol (tsserver.js:146523:9)
    at getEntryForMemberCompletion (tsserver.js:148878:22)
    at createCompletionEntry (tsserver.js:148775:64)
    at getCompletionEntriesFromSymbols (tsserver.js:149316:19)
    at completionInfoFromData (tsserver.js:148442:23)
    at Object.getCompletionsAtPosition (tsserver.js:148264:24)
    at Object.getCompletionsAtPosition2 [as getCompletionsAtPosition] (tsserver.js:135381:35)
    at IpcIOSession.getCompletions (tsserver.js:178887:54)
    at completionInfo (tsserver.js:177298:43)
    at tsserver.js:179651:69
    at IpcIOSession.executeWithRequestId (tsserver.js:179643:14)
    at IpcIOSession.executeCommand (tsserver.js:179651:29)
    at IpcIOSession.onMessage (tsserver.js:179693:51)
    at process.<anonymous> (tsserver.js:181261:14)
    at process.emit (node:events:526:28)
    at emit (node:internal/child_process:938:14)

[andrewbranch]

I followed these steps exactly, and can’t reproduce.

[DanielRosenwasser]

Let me work on a better repro.

[DanielRosenwasser]

I'm able to consistently repro this - even at the root of my file system. The key to the repro is that utils.ts has to start out as a closed file, but then is opened. Maybe it's an OS specific thing?

[andrewbranch]

Do you have a package.json?

[DanielRosenwasser]

I've tried it with and without package.jsons, and I just tried it in a Codespace and was able to repro it.

[andrewbranch]

On 5.0.0-dev.20230215?

[DanielRosenwasser]

Yup!

classMemberCompletionCrash.webm

[andrewbranch]

Writing up the results of my investigation since I have some vacation coming up. It’s possible that @sheetalkamat will be able to fix this while I’m out.

I initially thought this might be a regression from #52686, but I was wrong. I verified this crashes before that PR. Here’s what’s happening:

1. When you load the project, we parse and bind all the files in the project. We also set up a sort of backing store for the contents of each file. Closed files get a simple, memory-efficient backing store, while open files get a more complex backing store that optimizes edits and queries on the file text.
2. When you trigger completions in classes.ts, we build and cache an ExportInfoMap which contains, among other things, non-transient symbols of modules and exports other than classes.ts. There are a few things that can invalidate this cache, but the big picture is that it’s built specifically to serve the file you’re currently editing. If you were to make an edit to another file, we would wipe the cache, in part because that file’s symbol would change, leaving the old one stale and retained in the cache. So the underlying assumption is that (non-transient) symbols don’t change identities when none of their declarations are changing.
3. When you open utils.ts, the backing store for that file changes from the simple one to the edit-optimizing one.
4. When you come back to classes.ts and edit its text, an edit is made in its backing store, incrementing its version and marking the project as dirty. A throttled amount of time later, or when you next request completions (whichever comes first), we update the program. We go through and see which files need to be updated by checking whether the version on the existing source file matches the version on the backing store. Obviously, classes.ts is out of date because you just made an edit. The twist is that utils.ts also appears out of date, because the version strings used by the two different kinds of backing store are different. Any time we swap backing stores for a file (which happens when files are opened and closed), next time the program is updated, those files will appear to be out of date even if their text contents didn’t change.
5. Any files that appeared to be out of date get reparsed and rebound. For utils.ts, this is unnecessary work. But it also means that the file has a new symbol, making the one in the ExportInfoMap an incorrect identity (and also a minor memory leak).
6. When you request completions again, we use the ExportInfoMap, and as part of the filtering, we look for equality between the cached module symbol and the one attached to the up-to-date source file we need to resolve a module specifier to. But since the identity of the source file’s symbol changed, this filter unexpectedly comes up empty.

I talked to @sheetalkamat about this, and she said

I guess we would need to simplify the versioning logic in server (which I have always wanted to do since its from so long ago patched to work in most scenarios)

I have a failing test at 6396ba4 that I think should start passing—or at least stop crashing—if the extra parse and bind stop happening.

I investigated whether the cache invalidation for the ExportInfoMap could take the source file’s version into account, but there’s not really a good place to slot that in right now. Since this is not a regression, I think we should pursue the versioning fix that would have the added benefit of skipping some unnecessary parsing and binding.

[DanielRosenwasser]

For utils.ts, this is unnecessary work. But it also means that the file has a new symbol, making the one in the ExportInfoMap an incorrect identity (and also a minor memory leak).

I wonder if this is possibly why there's a few crawler sessions that seem to suddenly crash.