From 2a22528c81978af2e86e4bfb2cf079ac4e41fd3f Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Mon, 1 Aug 2022 13:57:53 -0700 Subject: [PATCH] Check for require when binding commonjs Previously, the binder didn't check for a declared function named 'require' when binding require in JS. It unconditionally treated 'require' as the commonjs function, even when a local function named 'require' existed and even in ES modules. Now, the binder checks for a local function named require and avoids binding in that case. Both before and after, actually importing the module causes the checker to issue an error saying that top-level functions named 'require' are illegal. But this error doesn't show up until you've imported from the module, so you won't notice it in the editor, where JS errors are most useful. Within-binder checks for declared functions are dependent on binding order, but function binding simulates hoisting by binding all function declarations first, so it should be pretty reliable. --- src/compiler/binder.ts | 1 + .../requireAsDeclaredFunction.errors.txt | 39 ++++++++++ .../reference/requireAsDeclaredFunction.js | 73 +++++++++++++++++++ .../requireAsDeclaredFunction.symbols | 63 ++++++++++++++++ .../reference/requireAsDeclaredFunction.types | 69 ++++++++++++++++++ .../salsa/requireAsDeclaredFunction.ts | 31 ++++++++ 6 files changed, 276 insertions(+) create mode 100644 tests/baselines/reference/requireAsDeclaredFunction.errors.txt create mode 100644 tests/baselines/reference/requireAsDeclaredFunction.js create mode 100644 tests/baselines/reference/requireAsDeclaredFunction.symbols create mode 100644 tests/baselines/reference/requireAsDeclaredFunction.types create mode 100644 tests/cases/conformance/salsa/requireAsDeclaredFunction.ts diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts index 52f99bcb8d7..7817d59b0c1 100644 --- a/src/compiler/binder.ts +++ b/src/compiler/binder.ts @@ -3260,6 +3260,7 @@ namespace ts { if (isInJSFile(node) && isVariableDeclarationInitializedToBareOrAccessedRequire(possibleVariableDecl) && !getJSDocTypeTag(node) && + !(lookupSymbolForName(container, "require" as __String)) && !(getCombinedModifierFlags(node) & ModifierFlags.Export) ) { declareSymbolAndAddToSymbolTable(node as Declaration, SymbolFlags.Alias, SymbolFlags.AliasExcludes); diff --git a/tests/baselines/reference/requireAsDeclaredFunction.errors.txt b/tests/baselines/reference/requireAsDeclaredFunction.errors.txt new file mode 100644 index 00000000000..f9d74d3ad52 --- /dev/null +++ b/tests/baselines/reference/requireAsDeclaredFunction.errors.txt @@ -0,0 +1,39 @@ +tests/cases/conformance/salsa/first.js(5,10): error TS2441: Duplicate identifier 'require'. Compiler reserves name 'require' in top level scope of a module. +tests/cases/conformance/salsa/global.js(5,10): error TS2441: Duplicate identifier 'require'. Compiler reserves name 'require' in top level scope of a module. +tests/cases/conformance/salsa/last.js(6,10): error TS2441: Duplicate identifier 'require'. Compiler reserves name 'require' in top level scope of a module. + + +==== tests/cases/conformance/salsa/first.js (1 errors) ==== + /** + * @param {string} str + * @return {number} + */ + function require(str) { return str.length } + ~~~~~~~ +!!! error TS2441: Duplicate identifier 'require'. Compiler reserves name 'require' in top level scope of a module. + const someModule = require("google-closure-compiler"); + export {someModule}; +==== tests/cases/conformance/salsa/last.js (1 errors) ==== + const someModule = require("google-closure-compiler"); + /** + * @param {string} str + * @return {number} + */ + function require(str) { return str.length } + ~~~~~~~ +!!! error TS2441: Duplicate identifier 'require'. Compiler reserves name 'require' in top level scope of a module. + export {someModule}; +==== tests/cases/conformance/salsa/global.js (1 errors) ==== + /** + * @param {string} str + * @return {number} + */ + function require(str) { return str.length } + ~~~~~~~ +!!! error TS2441: Duplicate identifier 'require'. Compiler reserves name 'require' in top level scope of a module. + const someModule = require("google-closure-compiler"); + +==== tests/cases/conformance/salsa/main.ts (0 errors) ==== + import { someModule as m1 } from './first' + import { someModule as m2 } from './last' + \ No newline at end of file diff --git a/tests/baselines/reference/requireAsDeclaredFunction.js b/tests/baselines/reference/requireAsDeclaredFunction.js new file mode 100644 index 00000000000..daf784cdb4d --- /dev/null +++ b/tests/baselines/reference/requireAsDeclaredFunction.js @@ -0,0 +1,73 @@ +//// [tests/cases/conformance/salsa/requireAsDeclaredFunction.ts] //// + +//// [first.js] +/** + * @param {string} str + * @return {number} + */ +function require(str) { return str.length } +const someModule = require("google-closure-compiler"); +export {someModule}; +//// [last.js] +const someModule = require("google-closure-compiler"); +/** + * @param {string} str + * @return {number} + */ +function require(str) { return str.length } +export {someModule}; +//// [global.js] +/** + * @param {string} str + * @return {number} + */ +function require(str) { return str.length } +const someModule = require("google-closure-compiler"); + +//// [main.ts] +import { someModule as m1 } from './first' +import { someModule as m2 } from './last' + + +//// [first.js] +"use strict"; +exports.__esModule = true; +exports.someModule = void 0; +/** + * @param {string} str + * @return {number} + */ +function require(str) { return str.length; } +var someModule = require("google-closure-compiler"); +exports.someModule = someModule; +//// [last.js] +"use strict"; +exports.__esModule = true; +exports.someModule = void 0; +var someModule = require("google-closure-compiler"); +exports.someModule = someModule; +/** + * @param {string} str + * @return {number} + */ +function require(str) { return str.length; } +//// [global.js] +/** + * @param {string} str + * @return {number} + */ +function require(str) { return str.length; } +var someModule = require("google-closure-compiler"); +//// [main.js] +"use strict"; +exports.__esModule = true; + + +//// [first.d.ts] +export const someModule: number; +//// [last.d.ts] +export const someModule: number; +//// [global.d.ts] +export {}; +//// [main.d.ts] +export {}; diff --git a/tests/baselines/reference/requireAsDeclaredFunction.symbols b/tests/baselines/reference/requireAsDeclaredFunction.symbols new file mode 100644 index 00000000000..510651afa74 --- /dev/null +++ b/tests/baselines/reference/requireAsDeclaredFunction.symbols @@ -0,0 +1,63 @@ +=== tests/cases/conformance/salsa/first.js === +/** + * @param {string} str + * @return {number} + */ +function require(str) { return str.length } +>require : Symbol(require, Decl(first.js, 0, 0)) +>str : Symbol(str, Decl(first.js, 4, 17)) +>str.length : Symbol(String.length, Decl(lib.es5.d.ts, --, --)) +>str : Symbol(str, Decl(first.js, 4, 17)) +>length : Symbol(String.length, Decl(lib.es5.d.ts, --, --)) + +const someModule = require("google-closure-compiler"); +>someModule : Symbol(someModule, Decl(first.js, 5, 5)) +>require : Symbol(require, Decl(first.js, 0, 0)) + +export {someModule}; +>someModule : Symbol(someModule, Decl(first.js, 6, 8)) + +=== tests/cases/conformance/salsa/last.js === +const someModule = require("google-closure-compiler"); +>someModule : Symbol(someModule, Decl(last.js, 0, 5)) +>require : Symbol(require, Decl(last.js, 0, 54)) + +/** + * @param {string} str + * @return {number} + */ +function require(str) { return str.length } +>require : Symbol(require, Decl(last.js, 0, 54)) +>str : Symbol(str, Decl(last.js, 5, 17)) +>str.length : Symbol(String.length, Decl(lib.es5.d.ts, --, --)) +>str : Symbol(str, Decl(last.js, 5, 17)) +>length : Symbol(String.length, Decl(lib.es5.d.ts, --, --)) + +export {someModule}; +>someModule : Symbol(someModule, Decl(last.js, 6, 8)) + +=== tests/cases/conformance/salsa/global.js === +/** + * @param {string} str + * @return {number} + */ +function require(str) { return str.length } +>require : Symbol(require, Decl(global.js, 0, 0)) +>str : Symbol(str, Decl(global.js, 4, 17)) +>str.length : Symbol(String.length, Decl(lib.es5.d.ts, --, --)) +>str : Symbol(str, Decl(global.js, 4, 17)) +>length : Symbol(String.length, Decl(lib.es5.d.ts, --, --)) + +const someModule = require("google-closure-compiler"); +>someModule : Symbol(someModule, Decl(global.js, 5, 5)) +>require : Symbol(require, Decl(global.js, 0, 0)) + +=== tests/cases/conformance/salsa/main.ts === +import { someModule as m1 } from './first' +>someModule : Symbol(someModule, Decl(first.js, 6, 8)) +>m1 : Symbol(m1, Decl(main.ts, 0, 8)) + +import { someModule as m2 } from './last' +>someModule : Symbol(someModule, Decl(last.js, 6, 8)) +>m2 : Symbol(m2, Decl(main.ts, 1, 8)) + diff --git a/tests/baselines/reference/requireAsDeclaredFunction.types b/tests/baselines/reference/requireAsDeclaredFunction.types new file mode 100644 index 00000000000..073fc4747be --- /dev/null +++ b/tests/baselines/reference/requireAsDeclaredFunction.types @@ -0,0 +1,69 @@ +=== tests/cases/conformance/salsa/first.js === +/** + * @param {string} str + * @return {number} + */ +function require(str) { return str.length } +>require : (str: string) => number +>str : string +>str.length : number +>str : string +>length : number + +const someModule = require("google-closure-compiler"); +>someModule : number +>require("google-closure-compiler") : number +>require : (str: string) => number +>"google-closure-compiler" : "google-closure-compiler" + +export {someModule}; +>someModule : number + +=== tests/cases/conformance/salsa/last.js === +const someModule = require("google-closure-compiler"); +>someModule : number +>require("google-closure-compiler") : number +>require : (str: string) => number +>"google-closure-compiler" : "google-closure-compiler" + +/** + * @param {string} str + * @return {number} + */ +function require(str) { return str.length } +>require : (str: string) => number +>str : string +>str.length : number +>str : string +>length : number + +export {someModule}; +>someModule : number + +=== tests/cases/conformance/salsa/global.js === +/** + * @param {string} str + * @return {number} + */ +function require(str) { return str.length } +>require : (str: string) => number +>str : string +>str.length : number +>str : string +>length : number + +const someModule = require("google-closure-compiler"); +>someModule : number +>require("google-closure-compiler") : number +>require : (str: string) => number +>"google-closure-compiler" : "google-closure-compiler" + +=== tests/cases/conformance/salsa/main.ts === +import { someModule as m1 } from './first' +>someModule : number +>m1 : number + +import { someModule as m2 } from './last' +>someModule : number +>m2 : number + diff --git a/tests/cases/conformance/salsa/requireAsDeclaredFunction.ts b/tests/cases/conformance/salsa/requireAsDeclaredFunction.ts new file mode 100644 index 00000000000..efead945aef --- /dev/null +++ b/tests/cases/conformance/salsa/requireAsDeclaredFunction.ts @@ -0,0 +1,31 @@ +// @outDir: out/ +// @declaration: true +// @checkJs: true +// @filename: first.js +/** + * @param {string} str + * @return {number} + */ +function require(str) { return str.length } +const someModule = require("google-closure-compiler"); +export {someModule}; +// @filename: last.js +const someModule = require("google-closure-compiler"); +/** + * @param {string} str + * @return {number} + */ +function require(str) { return str.length } +export {someModule}; +// @filename: global.js + +/** + * @param {string} str + * @return {number} + */ +function require(str) { return str.length } +const someModule = require("google-closure-compiler"); + +// @filename: main.ts +import { someModule as m1 } from './first' +import { someModule as m2 } from './last'