projectVersion not bumped when rootFileNames changes

[cspotcode]

Expected Behavior

getProjectVersion() is incremented when a new file is appended to getScriptFileNames() array.

Actual Behavior

getProjectVersion() is not incremented when a file is added to getScriptFileNames() during typechecking, before it has been require()d.

@blakeembrey it looks like this was changed here:
#985
That pull request means that all files added to the in-memory cache are also added to getScriptFileNames() so they need to trigger a bump of getProjectVersion() Otherwise it breaks the TS compiler's expectations, which might cause bugs down the road.

Steps to reproduce the problem

Omitted a rigorous reproduction until I double-check with you that this change was, indeed, accidental.

If you have a index.ts that imports a foo.ts, and you do ts-node ./index.ts:
typechecking will see the import statement, and updateMemoryCache will be called to pull foo.ts into the cache, adding it to the fileVersions Map.

Specifications

• ts-node version: 8.8.1
• TypeScript version: 3.8.3
• tsconfig.json, if you're using one: no tsconfig
• node version: 13.8.0
• Operating system and version: tested on Windows & Ubuntu via WSL2

[blakeembrey]

Hmm, I believe I purposely reverted this in d133b39 because I couldn't replicate any issue and wanted to avoid changing the logic you had added 😄 I can make this change again.

[blakeembrey]

Actually, I take that back - it looks like I did change this in 2e9a2f1 and I believe it follows your expectation.

[cspotcode]

Here's where I think we're missing a projectVersion++:
https://github.com/TypeStrong/ts-node/blob/master/src/index.ts#L489

That line is effectively adding a new entry to the getScriptFileNames array, which as far as I understand, is the set of "root files." So every time we change that array, we're changing the Program, and we need to indicate this by bumping the getProjectVersion. Otherwise TS will reuse an invalid Program instance.

Right now ts-node adds all require()d files to the getScriptFileNames array because, if we don't, the compiler might consider them "external" and refuse to emit .js. Unfortunately when we don't bump the getProjectVersion when we do this, TypeScript reuses an old Program instance where it still considers these files to be external and doesn't give us transpiled output. If we did not implement getProjectVersion, then TypeScript would manually look at getScriptFileNames, notice the change, and rebuild the Program.

I created an example: https://github.com/TypeStrong/ts-node-repros/tree/issues-996

[cspotcode]

I addressed this in #998. Unfortunately, it resurfaces #884.

#884 was caused by getScriptFileNames: () => Array.from(memoryCache.fileVersions.keys())
We're only supposed to be returning rootFileNames; we can't including a bunch of internal lib files because it messes up typechecking. (I haven't thoroughly dug into this yet, but this is my understanding at the moment)

The regression was being masked because we weren't properly bumping projectVersion. So sometimes the compiler was getting into a bad state, but the bug wasn't triggering because TypeScript was erroneously reusing an old Program. I'm pretty I can tweak the #884 regression test so it triggers reliably on master. EDIT done in #999. I tweaked the test and now it's failing on master.

[blakeembrey]

Interesting, thanks for the reproduction!

We're only supposed to be returning rootFileNames; we can't including a bunch of internal lib files because it messes up typechecking. (I haven't thoroughly dug into this yet, but this is my understanding at the moment)

How do we reconcile this change with the fact that it seems to speed things up for people in #754? Is that just an accident or side effect of skipping new programs effectively? Or does your projectVersion++ in that position alone fix the issue for now without the perf regression? Unfortunately I don't have a large enough project using ts-node to test the perf issues locally.

[blakeembrey]

One possibility, which I had considered before but didn't believe, is that the act of creating a new identity for getScriptFileNames is the thing that's actually improving performance (e.g. Array.from or .slice()). I believe I used to do .slice() for this reason, but there's been so much hacky code that I can't remember anymore, sorry.

[cspotcode]

I need to do some benchmarking to be sure. I've used codegen to create a huge project for testing in the past; I just need to make sure it's representative of a real-world project. Last time I generated 1000 empty files; this time I need to fill them with code so it takes time to parse.

Is that just an ... side effect of skipping new programs effectively?

I'm not sure, but I'm worried it might be.

Maybe it's related to #997? If there were filename inconsistencies, TypeScript would be internally discovering a C:\bar\baz.ts, but then we'd be adding a C:/bar/baz.ts to getScriptFileNames(). Does that mean TypeScript would be compiling the same file twice? Or would have to re-parse the SourceFile a bunch, as the filename switched back and forth between \ and /?

#970 might help. With #970, I'm pretty sure we'll be able to avoid many cases where the Program gets rebuilt, by avoiding unnecessarily changing getScriptFileNames().

[blakeembrey]

Thanks @cspotcode. Feel free to revert any of my hacks anytime! 😄

[cspotcode]

One possibility... is that the act of creating a new identity for getScriptFileNames is the thing that's actually improving performance

Weird, I just checked the compiler source code, and ts.createProgram does appear to keep a reference to that array!

program = ts.createProgram(a = ['foo.ts'], {});
program.getRootFileNames() === a

When I have time for benchmarking, I'll test that theory.