Skip to content

Don’t recognize non-top-level module.exports as special assignments #34805

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Andrew Branch (andrewbranch) wants to merge 1 commit into from
Closed

Don’t recognize non-top-level module.exports as special assignments #34805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2365,7 +2365,10 @@ namespace ts {
}
break;
case SyntaxKind.BinaryExpression:
const specialKind = getAssignmentDeclarationKind(node as BinaryExpression);
const binaryExpression = node as BinaryExpression;
binaryExpression.left.parent = binaryExpression;
binaryExpression.right.parent = binaryExpression;
const specialKind = getAssignmentDeclarationKind(binaryExpression);
switch (specialKind) {
case AssignmentDeclarationKind.ExportsProperty:
bindExportsPropertyAssignment(node as BindableStaticPropertyAssignmentExpression);
Expand Down
16 changes: 13 additions & 3 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2044,6 +2044,15 @@ namespace ts {
&& getElementOrPropertyAccessName(node) === "exports";
}

export function isTopLevelInSourceFileOrIIFE(node: Node): boolean {
const container = getEnclosingBlockScopeContainer(node);
if (container.kind === SyntaxKind.SourceFile) {
return true;
}
const iife = getImmediatelyInvokedFunctionExpression(container);
return !!iife && isTopLevelInSourceFileOrIIFE(iife);
}

/// Given a BinaryExpression, returns SpecialPropertyAssignmentKind for the various kinds of property
/// assignments we treat as special in the binder
export function getAssignmentDeclarationKind(expr: BinaryExpression | CallExpression): AssignmentDeclarationKind {
Expand Down Expand Up @@ -2104,7 +2113,7 @@ namespace ts {
return AssignmentDeclarationKind.None;
}
const entityName = expr.arguments[0];
if (isExportsIdentifier(entityName) || isModuleExportsAccessExpression(entityName)) {
if ((isExportsIdentifier(entityName) || isModuleExportsAccessExpression(entityName)) && isTopLevelInSourceFileOrIIFE(entityName)) {

@sandersn Nathan Shively-Sanders (sandersn) Oct 29, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously, isTopLevel calls occurred outside getAssignmentDeclarationKind, mostly [1] so that we could call that once in the binder and not have to walk up the tree too much elsewhere -- at least at one point, getAssignmentDeclarationKind checked fairly local syntax.

So, three questions:

  1. Does this make a difference in practise? We don't have any JS projects in the performance suite so have no way to measure this.
  2. Is getAssignmentDeclarationKind actually called a lot in the checker?
  3. Is getAssignmentDeclarationKind actually still pretty local?
  4. Four. Four questions: Is this change worth it even if those answers are all "yes"? Probably, but I wanted to have you think about them too.

[1] it was partly chance. But lots of things are partly chance.

@andrewbranch Andrew Branch (andrewbranch) Oct 29, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. You mean perf difference? I think it shouldn’t, since we check a lot of other things before resorting to looking up the tree. This only happens when we see an assignment to something related to exports. But you’re right, we don’t have metrics on this.
  2. Yes, and a lot in services too. My initial instinct was to do this top-down from the binder, but I realized it would produce inconsistent results everywhere else :/
  3. I don’t understand the question?
  4. I’m actually not sure, honestly. It feels fairly unimportant since literally nobody has ever asked for this; it just came up as a correctness suggestion in Conditional assignments to exports should not mark a js file as a commonjs module #33765, which we only noticed because there was a regression related to element access assignments. The heuristics around what should be global feel really fuzzy to me in scenarios like this, which likely only appear in bundled or otherwise module-faking browser code. I’m not attached to this PR at all, and was actually hoping that putting it up would solicit enlightening feedback about why/whether this issue matters.

@sandersn Nathan Shively-Sanders (sandersn) Oct 31, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Do the existing parts of getAssignmentDeclarationKind examine parents or children up and down the tree much? Or is this the first time it has started walking up the tree more than, say, one or two parents deep?

@andrewbranch Andrew Branch (andrewbranch) Nov 1, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, this is the first time we look way up the tree.

return AssignmentDeclarationKind.ObjectDefinePropertyExports;
}
if (isBindableStaticAccessExpression(entityName) && getElementOrPropertyAccessName(entityName) === "prototype") {
Expand Down Expand Up @@ -2161,7 +2170,7 @@ namespace ts {
if (lhs.expression.kind === SyntaxKind.ThisKeyword) {
return AssignmentDeclarationKind.ThisProperty;
}
else if (isModuleExportsAccessExpression(lhs)) {
else if (isModuleExportsAccessExpression(lhs) && isTopLevelInSourceFileOrIIFE(lhs)) {
// module.exports = expr
return AssignmentDeclarationKind.ModuleExports;
}
Expand All @@ -2179,7 +2188,8 @@ namespace ts {
if ((id.escapedText === "exports" ||
id.escapedText === "module" && getElementOrPropertyAccessName(nextToLast) === "exports") &&
// ExportsProperty does not support binding with computed names
isBindableStaticAccessExpression(lhs)) {
isBindableStaticAccessExpression(lhs) &&
isTopLevelInSourceFileOrIIFE(lhs)) {
// exports.name = expr OR module.exports.name = expr OR exports["name"] = expr ...
return AssignmentDeclarationKind.ExportsProperty;
}
Expand Down
36 changes: 36 additions & 0 deletions tests/baselines/reference/nestedModuleExports.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
tests/cases/conformance/jsdoc/not-iife.js(5,9): error TS2580: Cannot find name 'module'. Do you need to install type definitions for node? Try `npm i @types/node`.
tests/cases/conformance/jsdoc/not-iife.js(6,9): error TS2580: Cannot find name 'module'. Do you need to install type definitions for node? Try `npm i @types/node`.
tests/cases/conformance/jsdoc/not-iife.js(8,9): error TS2580: Cannot find name 'module'. Do you need to install type definitions for node? Try `npm i @types/node`.
tests/cases/conformance/jsdoc/test.js(2,1): error TS2304: Cannot find name 'ShouldNotBeGlobal'.


==== tests/cases/conformance/jsdoc/not-iife.js (3 errors) ====
var ShouldBeGlobal = 0;
function f(type, ctor, exports) {
if (typeof exports !== "undefined") {
exports.a = ctor;
module.exports.b = ctor;
~~~~~~
!!! error TS2580: Cannot find name 'module'. Do you need to install type definitions for node? Try `npm i @types/node`.
module.exports = { ctor };
~~~~~~
!!! error TS2580: Cannot find name 'module'. Do you need to install type definitions for node? Try `npm i @types/node`.
exports = { ctor };
module.exports["c"] = type;
~~~~~~
!!! error TS2580: Cannot find name 'module'. Do you need to install type definitions for node? Try `npm i @types/node`.
}
}

==== tests/cases/conformance/jsdoc/iife.js (0 errors) ====
var ShouldNotBeGlobal = 0;
(function () {
exports.a = "b";
})();

==== tests/cases/conformance/jsdoc/test.js (1 errors) ====
ShouldBeGlobal;
ShouldNotBeGlobal;
~~~~~~~~~~~~~~~~~
!!! error TS2304: Cannot find name 'ShouldNotBeGlobal'.

50 changes: 50 additions & 0 deletions tests/baselines/reference/nestedModuleExports.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
=== tests/cases/conformance/jsdoc/not-iife.js ===
var ShouldBeGlobal = 0;
>ShouldBeGlobal : Symbol(ShouldBeGlobal, Decl(not-iife.js, 0, 3))

function f(type, ctor, exports) {
>f : Symbol(f, Decl(not-iife.js, 0, 23))
>type : Symbol(type, Decl(not-iife.js, 1, 11))
>ctor : Symbol(ctor, Decl(not-iife.js, 1, 16))
>exports : Symbol(exports, Decl(not-iife.js, 1, 22))

if (typeof exports !== "undefined") {
>exports : Symbol(exports, Decl(not-iife.js, 1, 22))

exports.a = ctor;
>exports : Symbol(exports, Decl(not-iife.js, 1, 22))
>ctor : Symbol(ctor, Decl(not-iife.js, 1, 16))

module.exports.b = ctor;
>ctor : Symbol(ctor, Decl(not-iife.js, 1, 16))

module.exports = { ctor };
>ctor : Symbol(ctor, Decl(not-iife.js, 5, 26))

exports = { ctor };
>exports : Symbol(exports, Decl(not-iife.js, 1, 22))
>ctor : Symbol(ctor, Decl(not-iife.js, 6, 19))

module.exports["c"] = type;
>type : Symbol(type, Decl(not-iife.js, 1, 11))
}
}

=== tests/cases/conformance/jsdoc/iife.js ===
var ShouldNotBeGlobal = 0;
>ShouldNotBeGlobal : Symbol(ShouldNotBeGlobal, Decl(iife.js, 0, 3))

(function () {
exports.a = "b";
>exports.a : Symbol(a, Decl(iife.js, 1, 14))
>exports : Symbol(a, Decl(iife.js, 1, 14))
>a : Symbol(a, Decl(iife.js, 1, 14))

})();

=== tests/cases/conformance/jsdoc/test.js ===
ShouldBeGlobal;
>ShouldBeGlobal : Symbol(ShouldBeGlobal, Decl(not-iife.js, 0, 3))

ShouldNotBeGlobal;

84 changes: 84 additions & 0 deletions tests/baselines/reference/nestedModuleExports.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
=== tests/cases/conformance/jsdoc/not-iife.js ===
var ShouldBeGlobal = 0;
>ShouldBeGlobal : number
>0 : 0

function f(type, ctor, exports) {
>f : (type: any, ctor: any, exports: any) => void
>type : any
>ctor : any
>exports : any

if (typeof exports !== "undefined") {
>typeof exports !== "undefined" : boolean
>typeof exports : "string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "object" | "function"
>exports : any
>"undefined" : "undefined"

exports.a = ctor;
>exports.a = ctor : any
>exports.a : any
>exports : any
>a : any
>ctor : any

module.exports.b = ctor;
>module.exports.b = ctor : any
>module.exports.b : any
>module.exports : any
>module : any
>exports : any
>b : any
>ctor : any

module.exports = { ctor };
>module.exports = { ctor } : { ctor: any; }
>module.exports : any
>module : any
>exports : any
>{ ctor } : { ctor: any; }
>ctor : any

exports = { ctor };
>exports = { ctor } : { ctor: any; }
>exports : any
>{ ctor } : { ctor: any; }
>ctor : any

module.exports["c"] = type;
>module.exports["c"] = type : any
>module.exports["c"] : any
>module.exports : any
>module : any
>exports : any
>"c" : "c"
>type : any
}
}

=== tests/cases/conformance/jsdoc/iife.js ===
var ShouldNotBeGlobal = 0;
>ShouldNotBeGlobal : number
>0 : 0

(function () {
>(function () { exports.a = "b";})() : void
>(function () { exports.a = "b";}) : () => void
>function () { exports.a = "b";} : () => void

exports.a = "b";
>exports.a = "b" : "b"
>exports.a : string
>exports : typeof import("tests/cases/conformance/jsdoc/iife")
>a : string
>"b" : "b"

})();

=== tests/cases/conformance/jsdoc/test.js ===
ShouldBeGlobal;
>ShouldBeGlobal : number

ShouldNotBeGlobal;
>ShouldNotBeGlobal : any

25 changes: 25 additions & 0 deletions tests/cases/conformance/jsdoc/nestedModuleExports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// @noEmit: true
// @checkJs: true
// @allowJs: true

// @Filename: not-iife.js
var ShouldBeGlobal = 0;
function f(type, ctor, exports) {
if (typeof exports !== "undefined") {
exports.a = ctor;
module.exports.b = ctor;
module.exports = { ctor };
exports = { ctor };
module.exports["c"] = type;
}
}

// @Filename: iife.js
var ShouldNotBeGlobal = 0;
(function () {
exports.a = "b";
})();

// @Filename: test.js
ShouldBeGlobal;
ShouldNotBeGlobal;