Add basic CommonJS support

[sandersn]

This PR adds basic CommonJS imports and exports. It's aiming to cover 80% of usage--a subset of TS semantics in most cases.

CommonJS imports are implemented basically the same way as in Strada. The binder and checker treat a variable declaration that matches a syntactic pattern as if it were the same as import x = require('x') in Typescript.

• const x = require('x') is supported.
• const { a, b, c } = require('x') is also supported (this is different than TS, but works the same as in Strada).
• const a = require('x').a is no longer supported.

CommonJS export assignments are implemented as a synthetic variant of Typescript export assignments. When the parser sees module.exports = ..., it emits a synthetic JS export = ....

• module.exports = x is supported, with the same semantics as export = x in TS.

CommonJS exports are implemented as a separate synthetic declaration. I don't create synthetic variable declarations because export var a = 1 both exports a and declares a local a. module.exports.a = 1 only does the former. More on that in Open Questions.

• module.exports.a = 1 exports a. It does not (yet) declare module.exports.a locally.

Other changes:

• I deleted some code that is no longer needed because of (1) simpler const x = require support (2) synthetic export support. But I haven't completely cleaned house, especially in the binder.
• I added JS support to the incomplete module resolution in fileloader/resolveImportsAndModuleAugmentations, which makes test results a lot better, even though it's still not how the finished port will work.
• I moved the code to reparser.go, and will move the JSDoc tag code there later.

Open questions

• Should we support local declaration of module.exports.x=? It is possible to hack the checker and name resolution to make it possible to refer to module.exports.x inside the CommonJS file, but it won't be pretty. I'm leaning toward not supporting this, at least until I can run top400 JS on Corsa after this PR. Based on the top100 JS corpus I looked at earlier, it's rare to reference an exported object within the exporting file.

• A secondary question is: should module, exports and require error without a declaration, suggesting that you install @types/node? Strada never does this for JS, but in Corsa I think it makes sense to ask people with checkJs on to install @types/node if they're using CommonJS. This does introduce lots of errors into the baselines, but remember that plain JS files report only syntax errors.

• How much of the ES/CJS interop failures in modulePreserve2 and 4, and importNonExportedMember12 are due to mistakes/omissions in this PR and how much are due to unfinished module exports code? I need somebody who understands ES/CJS interop well to look at these baselines.

• Naming: The synthetic node for module.exports= is KindJSExportAssignment, based on export='s KindExportAssignment. For module.exports.x=, I chose KindCommonJSExport, but maybe it should be KindCommonJSExportAssignment or KindCommonJSExportVariableDeclaration. Opinions?

• Should baselines skip all reparsed nodes? Just non-duplicate reparsed nodes? Skipping all of them makes the Strada baseline diffs smaller but omits, especially for @typedef, which was not baselined before.

Future work

• Move other reparsing code into reparser.go
• module.exports['illegal name']= -- requires generating a temp name to print it in a .d.ts file.
• The combination

/** @typedef {number} ImplicitlyExported */
module.exports = { a, b, c }

Requires exporting ImplicitlyExported with the rest of the exports a,b,c. The most straightforward way is to synthesise a temp name for {a,b,c} and a namespace with the same name that merges with it. The synthetic namespace contains the implicitly exported type.

[jakebailey]

const a = require('x').a is no longer supported.

This surprises me; how are we choosing to drop this?

A secondary question is: should module, exports and require error without a declaration, suggesting that you install @types/node?

I would not do this. It is not uncommon to depend on these working in CJS code without the node types.

How much of the ES/CJS interop failures in modulePreserve2 and 4, and importNonExportedMember12 are due to mistakes/omissions in this PR and how much are due to unfinished module exports code? I need somebody who understands ES/CJS interop well to look at these baselines.

I haven't looked either, but we're missing most of the code that makes externalModuleIndicator work, so a lot of this doesn't behave as expected.

Should baselines skip all reparsed nodes? Just non-duplicate reparsed nodes? Skipping all of them makes the Strada baseline diffs smaller

I would definitely be eliminating diffs.

[sandersn]

Some replies:

const a = require('x').a is no longer supported.

This surprises me; how are we choosing to drop this?

The only examples I found in top200 JS were webpack test cases.

A secondary question is: should module, exports and require error without a declaration, suggesting that you install @types/node?

I would not do this. It is not uncommon to depend on these working in CJS code without the node types.

Even if you have checkJs: true? But you're right, it is pretty annoying just in the baseline diffs.

Should baselines skip all reparsed nodes? Just non-duplicate reparsed nodes? Skipping all of them makes the Strada baseline diffs smaller

I would definitely be eliminating diffs.

I moved this up to the type tag PR and will consider re-adding non-duplicate reparsed nodes after Corsa is done and we don't care about baseline diffs anymore.

[andrewbranch]

Haven’t looked at the code yet, but from the description, I will note that

exports.a = "a";

is more common than

module.exports.a = "a";

in my experience. (Our own CommonJS emit, for example, does this.) (The way to reason about this, if it helps: the module system will use the value of module.exports as the source of truth, but both module and exports are free variables in the CJS script scope, with module.exports initialized to exports and exports initialized to an empty object. So by default, when you mutate the properties of exports, you mutate the properties of module.exports. But if you assign exports = ..., that rebinds the free variable and module.exports is unaffected. Similarly, if you reassign to module.exports = {}, subsequent mutations of exports.a will be ineffective.)

Should we support local declaration of module.exports.x=? It is possible to hack the checker and name resolution to make it possible to refer to module.exports.x inside the CommonJS file, but it won't be pretty. I'm leaning toward not supporting this, at least until I can run top400 JS on Corsa after this PR. Based on the top100 JS corpus I looked at earlier, it's rare to reference an exported object within the exporting file.

When searching for this, make sure you're looking for the exports.x form too. I think this is fairly common.

A secondary question is: should module, exports and require error without a declaration, suggesting that you install @types/node? Strada never does this for JS, but in Corsa I think it makes sense to ask people with checkJs on to install @types/node if they're using CommonJS. This does introduce lots of errors into the baselines, but remember that plain JS files report only syntax errors.

I would not do this—it seems like we’re making CommonJS much less powerful, so I’m not sure it makes sense to make it more nitpicky at the same time.

I think the broad questions about what can be dropped depends on the high level goal/philosophy here. If I were writing new CommonJS code (??), I would be happy with the level of support you have here. But existing code does all kinds of weird stuff, and I don’t think you’re going to find much of it in the topNNN suite. If we had a bottom10%, though... Are those users actively maintaining their code and upgrading TS versions? I doubt it.

[sandersn]

I will note that exports.a = "a"; is more common than module.exports.a = "a";in my experience.

Thanks, my intuitions about that are out-of-date.

I think the broad questions about what can be dropped depends on the high level goal/philosophy here.

The customers I'm trying to support are: people writing TS-in-JSDoc, people writing miscellaneous unchecked JS, and people browsing emitted JS in node_modules. The only code base in topNNN that I've seen with commonjs is webpack, and it uses only a single form (module.exports= + @typedef). CommonJS is basically for understanding JS in node_modules.

[andrewbranch]

people browsing emitted JS in node_modules

This contingency will likely be harmed by the removal of any require/exports-related analysis, unfortunately.

[sandersn]

The latest commit resolves module.exports and exports more or less the same way that Strada did -- by treating them as the source file's symbol.

I still need to look through the baselines for other bugs and limitations, and fix up the errors that you all have already pointed out.

[jakebailey]

Is there anything left in this PR? It's hard to view and the thread is long so I'm not sure where we are at; I tried to skim the code until the tab crashed, and I'm not sure there's anything that jumps out at me, especially if this is a net improvement over the current state that we can update later.

[sandersn]

@jakebailey You're probably right, I've just been fixing bugs that would be easier to review individually.

Still,

1. I want to look over the baselines one more time.
2. I have many many unused baselines I need to remove.
3. I'll ping you directly when I'm done with that.