Check module.exports #25732

sandersn · 2018-07-17T17:31:14Z

Create a unique symbol and type for module in each commonjs file. Previously, all references to module used the same symbol, which had type any. Now module has type { exports: typeof import("current-file") }. This module symbol is no longer identified by identity but by CheckFlags.ModuleExports.

A few notes:

I'm not sure that getSourceFileOfNode is the best way to get the symbol of the source file.
module has a fresh anonymous type for each file. This may have to change in order to fix const x = require("m") should create an alias symbol #25533, although the outline of the solution will remain the same.
module.exports assignment is now unconditionally a Special Property Assignment for the purposes of checking. That means assignments to module.exports (1) will not have assignability errors (2) return the type of module.exports, not its initializer. This should work fine because references to module.exports now refer to the source file's type.
In order to make control flow right for merged aliases, getExportSymbolOfValueSymbolIfExported needs to unconditionally get the merged symbol of its return. Previously it only merged symbol.exportSymbol.
Constructs like exports.default = { blah: 1 }; module.exports = exports['default'] mean that getTypeOfFuncClassEnumModule now needs to use push/popTypeResolution to prevent infinite loops.

Fixes #25687
Fixes #25621

…itive (#25619)"" This reverts commit 16676f2.

…nsitive (#25688)"" This reverts commit ff8c30d.

It burns a check flags. Probably necessary, but perhaps not. I haven't accepted baselines, but they are a bit questionable. I'm not sure the synthetic type is right, because I expected to see { "exports": typeof import("x") } but instead see { "x": typeof import("x") }.

Conflicts between exports property assignments and exports assignments should get a union type instead of an error.

sandersn · 2018-07-17T18:24:40Z

Looks like the build found some lint. I'll fix it in my next commit.

sandersn · 2018-07-17T18:42:22Z

Notes from user tests:

Good

adonis-framework: Aliases of module.exports are now typed, so we find that Helpers._makePath should have its third parameter optional, but is not annotated as such in its jsdoc, and that Route.route should have it second parameter verb: string | string[] not just string.
assert: property assignments to an alias declaration (var assert = module.exports = ok) are now recognised. The type of this in those property assignments is still not right, but hasn't gotten worse at least.
npmlog: Same as assert, but the difference is much more dramatic. Also strict null checks catches a rogue undefined.
chrome-devtools-frontend: (1) includes a copy of assert (2) now has a contextual type for some assignments that sets this: any. This removes some errors! TODO: Finish later.

Bad

axios: Assignment to the default property (ironically commented as "Support typescript") has an incorrect "variable 'axios' is used before being assigned error"

var axios = {}
module.exports = axios // ok
module.exports.default = axios // should be ok, currently an error

npm: Alias of module.exports isn't callable in 3 different places. (Probably related to const x = require("m") should create an alias symbol #25533).

var npm = module.exports = function (tree) {
}
module.exports.asReadInstalled = function (tree) {
    npm(tree) // should be callable, but isn't
}

npmlog: The alias of module.exports is missing the properties from its initialiser.

var EE = require('events').EventEmitter
var log = exports = module.exports = new EE()
log.on('error', function() { })

Ugly

create-react-app: No more "can't find module 'browserslist'." I have no idea why.
graceful-fs: Duplicate assignability error trying to overwrite fs.close. Maybe because of the special handling in checkBinaryLikeExpression? But this should have fewer assignability checks.

These currently fail.

Still quite messy and full of notes

This allows exports like `module.exports = new EE` to have properties added to them. Still messy, but I'm going to run user tests and look for regressions.

getExportSymbolOfValueSymbolIfExported should always merge its returned symbol, whether it's symbol.exportSymbol or just symbol.

sandersn · 2018-07-19T22:35:57Z

I fixed all the user test failures. The only incorrect change is to npm, which uses the same pattern as npmlog, but wrapped in an IIFE. Because the exports assignments aren't at top-level they aren't recognised. Now the error message is wrong as well &mdash it says typeof EventEmitter instead of EventEmitter. However, I don't think it's worth the additional effort to fix until we also make this pattern work inside an IIFE.

ghost · 2018-07-19T23:38:40Z

src/compiler/checker.ts

@@ -1412,6 +1411,14 @@ namespace ts {
                        return lastLocation.symbol;
                    }
                }
+                if (originalLocation && isInJavaScriptFile(originalLocation) && originalLocation.parent) {


There are 7 conditions here -- why put 3 in one if and 4 in another?

Also, no need to test && originalLocation.parent if we know it's an Identifier.

Oops, that's a symptom of the development history. It looks a lot better with isModuleExportsPropertyAccessExpression.

ghost · 2018-07-19T23:39:58Z

src/compiler/checker.ts

@@ -1412,6 +1411,14 @@ namespace ts {
                        return lastLocation.symbol;
                    }
                }
+                if (originalLocation && isInJavaScriptFile(originalLocation) && originalLocation.parent) {
+                    if (isIdentifier(originalLocation) && isPropertyAccessExpression(originalLocation.parent) &&
+                        originalLocation.escapedText === "module" && originalLocation.parent.name.escapedText === "exports") {


isModuleExportsPropertyAccessExpression does something similar to this.

ghost · 2018-07-19T23:41:18Z

src/compiler/checker.ts

+                if (originalLocation && isInJavaScriptFile(originalLocation) && originalLocation.parent) {
+                    if (isIdentifier(originalLocation) && isPropertyAccessExpression(originalLocation.parent) &&
+                        originalLocation.escapedText === "module" && originalLocation.parent.name.escapedText === "exports") {
+                        const moduleSymbol = createSymbol(SymbolFlags.FunctionScopedVariable, "module" as __String, CheckFlags.ModuleExports);


I think the creation should be cached somewhere, since we base several other caches on the symbol. If we see the name module 100 times (not that unlikely in a commonjs module), we'll create 100 different symbols, and we'll also create a new type for each of those symbols.

Yikes! Good catch.

This is equivalent to always binding module in module.exports to a specific symbol, so I just moved it to the binder and moved ModuleExports from CheckFlags to SymbolFlags.

ghost · 2018-07-19T23:42:34Z

src/compiler/checker.ts

@@ -4710,7 +4711,7 @@ namespace ts {
            return undefined;
        }

-        function getWidenedTypeFromJSSpecialPropertyDeclarations(symbol: Symbol) {
+        function getWidenedTypeFromJSSpecialPropertyDeclarations(symbol: Symbol, aliased: Symbol) {


This function's getting pretty big (over 120 lines), any way you could break it down in a future PR?

ghost · 2018-07-19T23:44:18Z

src/compiler/checker.ts

@@ -4766,15 +4767,19 @@ namespace ts {
                }
                else if (!jsDocType && isBinaryExpression(expression)) {
                    // If we don't have an explicit JSDoc type, get the type from the expression.
-                    let type = getWidenedLiteralType(checkExpressionCached(expression.right));
+                    const isModuleExportsAlias = aliased !== symbol;


Might be neater to make aliased optional.

I agree. Done.

ghost · 2018-07-19T23:45:12Z

src/compiler/checker.ts

+                        if (!aliased.exports) {
+                            aliased.exports = createSymbolTable();
+                        }
+                        (isModuleExportsAlias ? aliased : symbol).exports!.forEach((s, name) => {


If !isModuleExportsAlias, symbol the same as aliased. So this should always equal aliased.
Tested with Debug.assert((isModuleExportsAlias ? aliased : symbol) === aliased);.

This is now aliased || symbol now that aliased is optional instead of sometimes duplicated.

ghost · 2018-07-19T23:54:05Z

src/compiler/checker.ts

-                        links.type = baseTypeVariable ? getIntersectionType([type, baseTypeVariable]) : type;
+                    if (symbol.flags & SymbolFlags.ValueModule && symbol.valueDeclaration && isSourceFile(symbol.valueDeclaration) && symbol.valueDeclaration.commonJsModuleIndicator) {
+                        if (!pushTypeResolution(symbol, TypeSystemPropertyName.Type)) {
+                            return errorType;


Why not set links.type in this case?
This whole function might be neater if it just called getTypeOfFuncClassEnumModuleUncached which used return x instead of links.type = x. Then we would be sure to be setting the value and not forgetting some control flow path.

The original reason is that getTypeOfVariableOrParameterOrProperty does. I think that's because you can call getTypeOfSymbol in situations that are speculative, and you may successfully be able to get the type at a later time.

I tried this and nothing broke, even when I changed the equivalent line in getTypeOfVariableOrParameterOrProperty. So it looks like my guess is wrong. I think I will merge this for now and try that refactor separately, for both.

ghost · 2018-07-19T23:55:59Z

src/compiler/checker.ts

@@ -19295,7 +19322,7 @@ namespace ts {
            if (node.expression.kind === SyntaxKind.SuperKeyword) {
                const superType = checkSuperExpression(node.expression);
                if (isTypeAny(superType)) {
-                    forEach(node.arguments, checkExpresionNoReturn); // Still visit arguments so they get marked for visibility, etc
+                    forEach(node.arguments, checkExpressionNoReturn); // Still visit arguments so they get marked for visibility, etc


This seems like a complex way to avoid writing a for loop...

Previously it was also special, but created during name resolution in the checker. It made sense when there was only one special symbol for all files, but now there is one `module` symbol per file.

sandersn added 6 commits July 16, 2018 14:53

Revert "Revert "Explicitly typed special assignments are context sens…

b7e466b

…itive (#25619)"" This reverts commit 16676f2.

Revert "Revert "Explicitly typed prototype assignments are context se…

2ba6ecd

…nsitive (#25688)"" This reverts commit ff8c30d.

Update some baselines

0488063

module.exports= always uses lhs type

cf373ee

Conflicts between exports property assignments and exports assignments should get a union type instead of an error.

Merge branch 'master' into js/check-module-exports

29fb86a

sandersn added 9 commits July 17, 2018 12:46

Fix lint and accept good user baselines

e17f98a

Add tests based on user tests.

aba56b8

These currently fail.

Fix all but 1 of user test bugs found by typing module.exports

bb225c5

Still quite messy and full of notes

Merge branch 'master' into js/check-module-exports

af8cc03

Follow merged symbols+allow any object type

a490b37

This allows exports like `module.exports = new EE` to have properties added to them. Still messy, but I'm going to run user tests and look for regressions.

Update improved user baselines

27dcb56

Fix infinite recursion when checking module.exports

2360565

Fix bogus use-before-def error

37a3a82

getExportSymbolOfValueSymbolIfExported should always merge its returned symbol, whether it's symbol.exportSymbol or just symbol.

Update user test baselines

2714c10

Cleanup

9f5d60b

sandersn requested review from a user and mhegazy July 19, 2018 23:21

More small cleanup

87c187e

ghost reviewed Jul 19, 2018

View reviewed changes

Bind module of module.exports as a special symbol

ee96f65

Previously it was also special, but created during name resolution in the checker. It made sense when there was only one special symbol for all files, but now there is one `module` symbol per file.

ghost approved these changes Jul 20, 2018

View reviewed changes

sandersn merged commit 31d599a into master Jul 20, 2018

ghost deleted the js/check-module-exports branch July 20, 2018 18:50

This was referenced Aug 7, 2018

🤖 User test baselines have changed #26256

Closed

In JS, extra circular reference errors when referring to a missing property on module #26268

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check module.exports #25732

Check module.exports #25732

sandersn commented Jul 17, 2018 •

edited

Loading

sandersn commented Jul 17, 2018

sandersn commented Jul 17, 2018

sandersn commented Jul 19, 2018

ghost Jul 19, 2018

ghost Jul 19, 2018

sandersn Jul 20, 2018

ghost Jul 19, 2018

ghost Jul 19, 2018 •

edited by ghost

Loading

sandersn Jul 20, 2018

sandersn Jul 20, 2018

ghost Jul 19, 2018

ghost Jul 19, 2018

sandersn Jul 20, 2018

ghost Jul 19, 2018

sandersn Jul 20, 2018

ghost Jul 19, 2018

sandersn Jul 20, 2018

ghost Jul 19, 2018

sandersn Jul 20, 2018

Check module.exports #25732

Check module.exports #25732

Conversation

sandersn commented Jul 17, 2018 • edited Loading

sandersn commented Jul 17, 2018

sandersn commented Jul 17, 2018

Good

Bad

Ugly

sandersn commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 19, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Jul 17, 2018 •

edited

Loading

ghost Jul 19, 2018 •

edited by ghost

Loading