Add isShadowed check to new-for-builtins (#540)

rules/new-for-builtins.js

@@ -1,52 +1,23 @@
 'use strict';
 const getDocumentationUrl = require('./utils/get-documentation-url');
+const builtins = require('./utils/builtins');
+const isShadowed = require('./utils/is-shadowed');
 
 const messages = {
 	enforce: 'Use `new {{name}}()` instead of `{{name}}()`.',
 	disallow: 'Use `{{name}}()` instead of `new {{name}}()`.'
 };
 
-const enforceNew = new Set([
-	'Object',
-	'Array',
-	'ArrayBuffer',
-	'BigInt64Array',
-	'BigUint64Array',
-	'DataView',
-	'Date',
-	'Error',
-	'Float32Array',
-	'Float64Array',
-	'Function',
-	'Int8Array',
-	'Int16Array',
-	'Int32Array',
-	'Map',
-	'WeakMap',
-	'Set',
-	'WeakSet',
-	'Promise',
-	'RegExp',
-	'Uint8Array',
-	'Uint16Array',
-	'Uint32Array',
-	'Uint8ClampedArray'
-]);
-
-const disallowNew = new Set([
-	'BigInt',
-	'Boolean',
-	'Number',
-	'String',
-	'Symbol'
-]);
+const enforceNew = new Set(builtins.enforceNew);
+const disallowNew = new Set(builtins.disallowNew);
 
 const create = context => {
 	return {
 		CallExpression: node => {
-			const {name} = node.callee;
+			const {callee} = node;
+			const {name} = callee;
 
-			if (enforceNew.has(name)) {
+			if (enforceNew.has(name) && !isShadowed(context.getScope(), callee)) {
 				context.report({
 					node,
 					messageId: 'enforce',
@@ -56,9 +27,10 @@ const create = context => {
 			}
 		},
 		NewExpression: node => {
-			const {name} = node.callee;
+			const {callee} = node;
+			const {name} = callee;
 
-			if (disallowNew.has(name)) {
+			if (disallowNew.has(name) && !isShadowed(context.getScope(), callee)) {
 				context.report({
 					node,
 					messageId: 'disallow',

rules/utils/builtins.js

@@ -0,0 +1,41 @@
+'use strict';
+
+const enforceNew = [
+	'Object',
+	'Array',
+	'ArrayBuffer',
+	'BigInt64Array',
+	'BigUint64Array',
+	'DataView',
+	'Date',
+	'Error',
+	'Float32Array',
+	'Float64Array',
+	'Function',
+	'Int8Array',
+	'Int16Array',
+	'Int32Array',
+	'Map',
+	'WeakMap',
+	'Set',
+	'WeakSet',
+	'Promise',
+	'RegExp',
+	'Uint8Array',
+	'Uint16Array',
+	'Uint32Array',
+	'Uint8ClampedArray'
+];
+
+const disallowNew = [
+	'BigInt',
+	'Boolean',
+	'Number',
+	'String',
+	'Symbol'
+];
+
+module.exports = {
+	enforceNew,
+	disallowNew
+};

rules/utils/is-shadowed.js

@@ -0,0 +1,35 @@
+'use strict';
+
+/**
+ * Finds the eslint-scope reference in the given scope.
+ * @param {Object} scope The scope to search.
+ * @param {ASTNode} node The identifier node.
+ * @returns {Reference|undefined} Returns the found reference or null if none were found.
+ */
+function findReference(scope, node) {
+	const references = scope.references
+		.filter(reference =>
+			reference.identifier.range[0] === node.range[0] &&
+			reference.identifier.range[1] === node.range[1]
+		);
+
+	return references.length === 1 ? references[0] : undefined;
+}
+
+/**
+ * Checks if the given identifier node is shadowed in the given scope.
+ * @param {Object} scope The current scope.
+ * @param {string} node The identifier node to check
+ * @returns {boolean} Whether or not the name is shadowed.
+ */
+function isShadowed(scope, node) {
+	const reference = findReference(scope, node);
+
+	return (
+		reference &&
+		reference.resolved &&
+		reference.resolved.defs.length > 0
+	);
+}
+
+module.exports = isShadowed;

test/new-for-builtins.js

@@ -1,10 +1,17 @@
 import test from 'ava';
 import avaRuleTester from 'eslint-ava-rule-tester';
 import rule from '../rules/new-for-builtins';
+import {enforceNew, disallowNew} from '../rules/utils/builtins';
 
 const ruleTester = avaRuleTester(test, {
-	env: {
-		es6: true
+	parserOptions: {
+		ecmaVersion: 2020,
+		sourceType: 'module'
+	},
+	// Make sure globals don't effect shadowed check result
+	globals: {
+		Map: 'off',
+		String: 'off'
 	}
 });
 
@@ -49,7 +56,45 @@ ruleTester.run('new-for-builtins', rule, {
 		'const foo = Boolean()',
 		'const foo = Number()',
 		'const foo = String()',
-		'const foo = Symbol()'
+		'const foo = Symbol()',
+		// Shadowed
+		...enforceNew.map(object => `
+			const ${object} = function() {};
+			const foo = ${object}();
+		`),
+		...disallowNew.map(object => `
+			const ${object} = function() {};
+			const foo = new ${object}();
+		`),
+		...enforceNew.map(object => `
+			function insideFunction() {
+				const ${object} = function() {};
+				const foo = ${object}();
+			}
+		`),
+		...disallowNew.map(object => `
+			function insideFunction() {
+				const ${object} = function() {};
+				const foo = new ${object}();
+			}
+		`),
+		// #122
+		`
+			import { Map } from 'immutable';
+			const m = Map();
+		`,
+		`
+			const {Map} = require('immutable');
+			const foo = Map();
+		`,
+		`
+			const {String} = require('guitar');
+			const lowE = new String();
+		`,
+		`
+			import {String} from 'guitar';
+			const lowE = new String();
+		`
 	],
 	invalid: [
 		{
@@ -211,6 +256,51 @@ ruleTester.run('new-for-builtins', rule, {
 			code: 'const foo = new Symbol()',
 			errors: [disallowNewError('Symbol')],
 			output: 'const foo = Symbol()'
+		},
+		{
+			code: `
+				function varCheck() {
+					{
+						var WeakMap = function() {};
+					}
+					// This should not reported
+					return WeakMap()
+				}
+				function constCheck() {
+					{
+						const Array = function() {};
+					}
+					return Array()
+				}
+				function letCheck() {
+					{
+						let Map = function() {};
+					}
+					return Map()
+				}
+			`,
+			errors: [enforceNewError('Array'), enforceNewError('Map')],
+			output: `
+				function varCheck() {
+					{
+						var WeakMap = function() {};
+					}
+					// This should not reported
+					return WeakMap()
+				}
+				function constCheck() {
+					{
+						const Array = function() {};
+					}
+					return new Array()
+				}
+				function letCheck() {
+					{
+						let Map = function() {};
+					}
+					return new Map()
+				}
+			`
 		}
 	]
 });