From 4c0622b26aa6375832bec2bf80f5c5522481c4ea Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Tue, 2 Oct 2018 17:21:52 -0700 Subject: [PATCH 1/2] Added function-constructor rule Blocks using the `Function()`/`new Function()` methods to create new functions. Fixes #3554. --- src/configs/all.ts | 144 +++++++-------- src/configs/recommended.ts | 171 ++++++++---------- .../functionConstructor.examples.ts | 45 +++++ src/rules/functionConstructorRule.ts | 73 ++++++++ test/rules/function-constructor/test.ts.lint | 19 ++ test/rules/function-constructor/tslint.json | 5 + 6 files changed, 290 insertions(+), 167 deletions(-) create mode 100644 src/rules/code-examples/functionConstructor.examples.ts create mode 100644 src/rules/functionConstructorRule.ts create mode 100644 test/rules/function-constructor/test.ts.lint create mode 100644 test/rules/function-constructor/tslint.json diff --git a/src/configs/all.ts b/src/configs/all.ts index 47b9556543a..b90c3118c68 100644 --- a/src/configs/all.ts +++ b/src/configs/all.ts @@ -29,19 +29,25 @@ export const rules = { "ban-types": { options: [ ["Object", "Avoid using the `Object` type. Did you mean `object`?"], - ["Function", "Avoid using the `Function` type. Prefer a specific function type, like `() => void`."], + [ + "Function", + "Avoid using the `Function` type. Prefer a specific function type, like `() => void`." + ], ["Boolean", "Avoid using the `Boolean` type. Did you mean `boolean`?"], ["Number", "Avoid using the `Number` type. Did you mean `number`?"], ["String", "Avoid using the `String` type. Did you mean `string`?"], - ["Symbol", "Avoid using the `Symbol` type. Did you mean `symbol`?"], - ], + ["Symbol", "Avoid using the `Symbol` type. Did you mean `symbol`?"] + ] }, "ban-ts-ignore": true, "member-access": [true, "check-accessor", "check-constructor", "check-parameter-property"], - "member-ordering": [true, { - "order": "statics-first", - "alphabetize": true, - }], + "member-ordering": [ + true, + { + order: "statics-first", + alphabetize: true + } + ], "no-any": true, "no-empty-interface": true, "no-import-side-effect": true, @@ -58,7 +64,7 @@ export const rules = { "prefer-for-of": true, "prefer-readonly": true, "promise-function-async": true, - "typedef": [ + typedef: [ true, "call-signature", "arrow-call-signature", @@ -66,24 +72,24 @@ export const rules = { "arrow-parameter", "property-declaration", "variable-declaration", - "member-variable-declaration", + "member-variable-declaration" ], "typedef-whitespace": [ true, { "call-signature": "nospace", "index-signature": "nospace", - "parameter": "nospace", + parameter: "nospace", "property-declaration": "nospace", - "variable-declaration": "nospace", + "variable-declaration": "nospace" }, { "call-signature": "onespace", "index-signature": "onespace", - "parameter": "onespace", + parameter: "onespace", "property-declaration": "onespace", - "variable-declaration": "onespace", - }, + "variable-declaration": "onespace" + } ], "unified-signatures": true, @@ -91,8 +97,9 @@ export const rules = { "await-promise": true, // "ban": no sensible default "ban-comma-operator": true, - "curly": true, - "forin": true, + curly: true, + forin: true, + "function-constructor": true, // "import-blacklist": no sensible default "label-position": true, "no-arg": true, @@ -103,10 +110,7 @@ export const rules = { "no-debugger": true, "no-duplicate-super": true, "no-duplicate-switch-case": true, - "no-duplicate-variable": [ - true, - "check-parameters", - ], + "no-duplicate-variable": [true, "check-parameters"], "no-dynamic-delete": true, "no-empty": true, "no-eval": true, @@ -135,7 +139,7 @@ export const rules = { "no-var-keyword": true, "no-void-expression": true, "prefer-conditional-expression": true, - "radix": true, + radix: true, "restrict-plus-operands": true, "strict-boolean-expressions": true, "strict-type-predicates": true, @@ -147,8 +151,8 @@ export const rules = { // Maintainability "cyclomatic-complexity": true, - "eofline": true, - "indent": [true, "spaces"], + eofline: true, + indent: [true, "spaces"], "linebreak-style": [true, "LF"], "max-classes-per-file": [true, 1], "max-file-line-count": [true, 1000], @@ -162,38 +166,30 @@ export const rules = { "no-trailing-whitespace": true, "object-literal-sort-keys": true, "prefer-const": true, - "trailing-comma": [true, { - "esSpecCompliant": true, - "multiline": "always", - "singleline": "never", - }], + "trailing-comma": [ + true, + { + esSpecCompliant: true, + multiline: "always", + singleline: "never" + } + ], // Style - "align": [ - true, - "parameters", - "arguments", - "statements", - "elements", - "members", - ], + align: [true, "parameters", "arguments", "statements", "elements", "members"], "array-type": [true, "array-simple"], "arrow-parens": true, "arrow-return-shorthand": [true, "multiline"], "binary-expression-operand-order": true, "callable-types": true, "class-name": true, - "comment-format": [ - true, - "check-space", - "check-uppercase", - ], + "comment-format": [true, "check-space", "check-uppercase"], "comment-type": [true, "singleline", "multiline", "doc", "directive"], "completed-docs": true, // "file-header": No sensible default - "deprecation": true, - "encoding": true, + deprecation: true, + encoding: true, "file-name-casing": [true, "camel-case"], "import-spacing": true, "increment-decrement": true, @@ -223,44 +219,41 @@ export const rules = { "check-else", "check-finally", "check-open-brace", - "check-whitespace", + "check-whitespace" ], "one-variable-per-declaration": true, - "ordered-imports": [true, { - "import-sources-order": "case-insensitive", - "named-imports-order": "case-insensitive", - "module-source-path": "full", - }], + "ordered-imports": [ + true, + { + "import-sources-order": "case-insensitive", + "named-imports-order": "case-insensitive", + "module-source-path": "full" + } + ], "prefer-function-over-method": true, "prefer-method-signature": true, "prefer-object-spread": true, "prefer-switch": true, "prefer-template": true, "prefer-while": true, - "quotemark": [ + quotemark: [true, "double", "avoid-escape", "avoid-template"], + "return-undefined": true, + semicolon: [true, "always"], + "space-before-function-paren": [ true, - "double", - "avoid-escape", - "avoid-template", + { + anonymous: "never", + asyncArrow: "always", + constructor: "never", + method: "never", + named: "never" + } ], - "return-undefined": true, - "semicolon": [true, "always"], - "space-before-function-paren": [true, { - "anonymous": "never", - "asyncArrow": "always", - "constructor": "never", - "method": "never", - "named": "never", - }], "space-within-parens": [true, 0], "switch-final-break": true, "type-literal-delimiter": true, - "variable-name": [ - true, - "ban-keywords", - "check-format", - ], - "whitespace": [ + "variable-name": [true, "ban-keywords", "check-format"], + whitespace: [ true, "check-branch", "check-decl", @@ -271,12 +264,19 @@ export const rules = { "check-typecast", "check-preblock", "check-type-operator", - "check-rest-spread", - ], + "check-rest-spread" + ] }; -export const RULES_EXCLUDED_FROM_ALL_CONFIG = - ["ban", "fileHeader", "importBlacklist", "noInvalidThis", "noSwitchCaseFallThrough", "typeofCompare", "noUnusedVariable"]; +export const RULES_EXCLUDED_FROM_ALL_CONFIG = [ + "ban", + "fileHeader", + "importBlacklist", + "noInvalidThis", + "noSwitchCaseFallThrough", + "typeofCompare", + "noUnusedVariable" +]; // Exclude typescript-only rules from jsRules, otherwise it's identical. export const jsRules: { [key: string]: any } = {}; diff --git a/src/configs/recommended.ts b/src/configs/recommended.ts index e2dfc369281..1f864386731 100644 --- a/src/configs/recommended.ts +++ b/src/configs/recommended.ts @@ -17,57 +17,58 @@ export const rules = { "adjacent-overload-signatures": true, - "align": { - options: [ - "parameters", - "statements", - ], + align: { + options: ["parameters", "statements"] }, "array-type": { - options: ["array-simple"], + options: ["array-simple"] }, "arrow-parens": true, "arrow-return-shorthand": true, "ban-types": { options: [ ["Object", "Avoid using the `Object` type. Did you mean `object`?"], - ["Function", "Avoid using the `Function` type. Prefer a specific function type, like `() => void`."], + [ + "Function", + "Avoid using the `Function` type. Prefer a specific function type, like `() => void`." + ], ["Boolean", "Avoid using the `Boolean` type. Did you mean `boolean`?"], ["Number", "Avoid using the `Number` type. Did you mean `number`?"], ["String", "Avoid using the `String` type. Did you mean `string`?"], - ["Symbol", "Avoid using the `Symbol` type. Did you mean `symbol`?"], - ], + ["Symbol", "Avoid using the `Symbol` type. Did you mean `symbol`?"] + ] }, "callable-types": true, "class-name": true, "comment-format": { - options: ["check-space"], + options: ["check-space"] }, - "curly": true, + curly: true, "cyclomatic-complexity": false, - "eofline": true, - "forin": true, + eofline: true, + forin: true, + "function-constructor": true, "import-spacing": true, - "indent": { - options: ["spaces"], + indent: { + options: ["spaces"] }, "interface-name": { - options: ["always-prefix"], + options: ["always-prefix"] }, "interface-over-type-literal": true, "jsdoc-format": true, "label-position": true, "max-classes-per-file": { - options: [1], + options: [1] }, "max-line-length": { - options: [120], + options: [120] }, "member-access": true, "member-ordering": { options: { - order: "statics-first", - }, + order: "statics-first" + } }, "new-parens": true, "no-angle-bracket-type-assertion": true, @@ -103,7 +104,7 @@ export const rules = { "no-var-keyword": true, "no-var-requires": true, "object-literal-key-quotes": { - options: ["consistent-as-needed"], + options: ["consistent-as-needed"] }, "object-literal-shorthand": true, "object-literal-sort-keys": true, @@ -113,36 +114,30 @@ export const rules = { "check-else", "check-finally", "check-open-brace", - "check-whitespace", - ], + "check-whitespace" + ] }, "one-variable-per-declaration": { - options: ["ignore-for-loop"], + options: ["ignore-for-loop"] }, "only-arrow-functions": { - options: [ - "allow-declarations", - "allow-named-functions", - ], + options: ["allow-declarations", "allow-named-functions"] }, "ordered-imports": { options: { "import-sources-order": "case-insensitive", "module-source-path": "full", - "named-imports-order": "case-insensitive", - }, + "named-imports-order": "case-insensitive" + } }, "prefer-const": true, "prefer-for-of": true, - "quotemark": { - options: [ - "double", - "avoid-escape", - ], + quotemark: { + options: ["double", "avoid-escape"] }, - "radix": true, - "semicolon": { - options: ["always"], + radix: true, + semicolon: { + options: ["always"] }, "space-before-function-paren": { options: { @@ -150,78 +145,71 @@ export const rules = { asyncArrow: "always", constructor: "never", method: "never", - named: "never", - }, + named: "never" + } }, "trailing-comma": { options: { esSpecCompliant: true, multiline: "always", - singleline: "never", - }, + singleline: "never" + } }, "triple-equals": { - options: ["allow-null-check"], + options: ["allow-null-check"] }, - "typedef": false, + typedef: false, "typedef-whitespace": { options: [ { "call-signature": "nospace", "index-signature": "nospace", - "parameter": "nospace", + parameter: "nospace", "property-declaration": "nospace", - "variable-declaration": "nospace", + "variable-declaration": "nospace" }, { "call-signature": "onespace", "index-signature": "onespace", - "parameter": "onespace", + parameter: "onespace", "property-declaration": "onespace", - "variable-declaration": "onespace", - }, - ], + "variable-declaration": "onespace" + } + ] }, "typeof-compare": false, // deprecated in TSLint 5.9.0 "unified-signatures": true, "use-isnan": true, "variable-name": { - options: [ - "ban-keywords", - "check-format", - "allow-pascal-case", - ], + options: ["ban-keywords", "check-format", "allow-pascal-case"] }, - "whitespace": { + whitespace: { options: [ "check-branch", "check-decl", "check-operator", "check-separator", "check-type", - "check-typecast", - ], - }, + "check-typecast" + ] + } }; export const jsRules = { - "align": { - options: [ - "parameters", - "statements", - ], + align: { + options: ["parameters", "statements"] }, "class-name": true, - "curly": true, - "eofline": true, - "forin": true, + curly: true, + eofline: true, + forin: true, "import-spacing": true, - "indent": { - options: ["spaces"], + indent: { + options: ["spaces"] }, "jsdoc-format": true, "label-position": true, "max-line-length": { - options: [120], + options: [120] }, "new-parens": true, "no-arg": true, @@ -251,21 +239,18 @@ export const jsRules = { "check-else", "check-finally", "check-open-brace", - "check-whitespace", - ], + "check-whitespace" + ] }, "one-variable-per-declaration": { - options: ["ignore-for-loop"], + options: ["ignore-for-loop"] }, - "quotemark": { - options: [ - "double", - "avoid-escape", - ], + quotemark: { + options: ["double", "avoid-escape"] }, - "radix": true, - "semicolon": { - options: ["always"], + radix: true, + semicolon: { + options: ["always"] }, "space-before-function-paren": { options: { @@ -273,34 +258,30 @@ export const jsRules = { asyncArrow: "always", constructor: "never", method: "never", - named: "never", - }, + named: "never" + } }, "trailing-comma": { options: { multiline: "always", - singleline: "never", - }, + singleline: "never" + } }, "triple-equals": { - options: ["allow-null-check"], + options: ["allow-null-check"] }, "use-isnan": true, "variable-name": { - options: [ - "ban-keywords", - "check-format", - "allow-pascal-case", - ], + options: ["ban-keywords", "check-format", "allow-pascal-case"] }, - "whitespace": { + whitespace: { options: [ "check-branch", "check-decl", "check-operator", "check-separator", "check-type", - "check-typecast", - ], - }, + "check-typecast" + ] + } }; diff --git a/src/rules/code-examples/functionConstructor.examples.ts b/src/rules/code-examples/functionConstructor.examples.ts new file mode 100644 index 00000000000..47b222b5595 --- /dev/null +++ b/src/rules/code-examples/functionConstructor.examples.ts @@ -0,0 +1,45 @@ +/** + * @license + * Copyright 2013 Palantir Technologies, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as Lint from "../../index"; + +export const codeExamples = [ + { + config: Lint.Utils.dedent` + "rules": { "function-constructor": true } + `, + description: "Use inline lambdas instead of calling Function", + fail: Lint.Utils.dedent` + let doesNothing = new Function(); + `, + pass: Lint.Utils.dedent` + let doesNothing = () => {}; + ` + }, + { + config: Lint.Utils.dedent` + "rules": { "function-constructor": true } + `, + description: "Use parameters instead of constructor strings", + fail: Lint.Utils.dedent` + let addNumbers = new Function("a", "b", "return a + b"); + `, + pass: Lint.Utils.dedent` + let addNumbers = (a, b) => a + b; + ` + } +]; diff --git a/src/rules/functionConstructorRule.ts b/src/rules/functionConstructorRule.ts new file mode 100644 index 00000000000..98b442c008b --- /dev/null +++ b/src/rules/functionConstructorRule.ts @@ -0,0 +1,73 @@ +/** + * @license + * Copyright 2013 Palantir Technologies, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { isCallExpression, isNewExpression, isIdentifier } from "tsutils"; +import * as ts from "typescript"; + +import * as Lint from ".."; +import { codeExamples } from "./code-examples/functionConstructor.examples"; + +export class Rule extends Lint.Rules.AbstractRule { + public static metadata: Lint.IRuleMetadata = { + ruleName: "function-constructor", + description: Lint.Utils.dedent` + Prevents using the built-in Function constructor. + `, + rationale: Lint.Utils.dedent` + Calling the constructor directly is similar to \`eval\`, which is a symptom of design issues. + String inputs don't receive type checking and can cause performance issues, particularly when dynamically created. + + If you need to dynamically create functions, use "factory" functions that themselves return functions. + `, + optionsDescription: "Not configurable.", + options: null, + optionExamples: [true], + type: "functionality", + typescriptOnly: false, + codeExamples + }; + + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + return this.applyWithFunction(sourceFile, walk); + } + + public static FAILURE = "Do not use the Function constructor to create functions."; +} + +function walk(context: Lint.WalkContext): void { + ts.forEachChild(context.sourceFile, function cb(node): void { + if (isFunctionCallOrNewExpression(node)) { + addFailureAtNode(node); + } + + ts.forEachChild(node, cb); + }); + + function addFailureAtNode(node: CallOrNewExpression) { + context.addFailureAtNode(node, Rule.FAILURE); + } +} + +function isFunctionCallOrNewExpression(node: ts.Node): node is CallOrNewExpression { + if (isCallExpression(node) || isNewExpression(node)) { + return isIdentifier(node.expression) && node.expression.text === "Function"; + } + + return false; +} + +type CallOrNewExpression = ts.CallExpression | ts.NewExpression; diff --git a/test/rules/function-constructor/test.ts.lint b/test/rules/function-constructor/test.ts.lint new file mode 100644 index 00000000000..cc2f3829427 --- /dev/null +++ b/test/rules/function-constructor/test.ts.lint @@ -0,0 +1,19 @@ +function one() { } +const two = function () { }; +const three = () => {}; + +const noParametersNew = Function(); + ~~~~~~~~~~ [0] +const oneParameterNew = Function("a"); + ~~~~~~~~~~~~~ [0] +const twoParametersNew = Function("a", "b"); + ~~~~~~~~~~~~~~~~~~ [0] + +const noParametersNew = new Function(); + ~~~~~~~~~~~~~~ [0] +const oneParameterNew = new Function("a"); + ~~~~~~~~~~~~~~~~~ [0] +const twoParametersNew = new Function("a", "b"); + ~~~~~~~~~~~~~~~~~~~~~~ [0] + +[0]: Do not use the Function constructor to create functions. diff --git a/test/rules/function-constructor/tslint.json b/test/rules/function-constructor/tslint.json new file mode 100644 index 00000000000..9f48491ff28 --- /dev/null +++ b/test/rules/function-constructor/tslint.json @@ -0,0 +1,5 @@ +{ + "rules": { + "function-constructor": true + } +} From 7cae22f77d933e83db1de09917343eb48ee25842 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Tue, 2 Oct 2018 17:25:15 -0700 Subject: [PATCH 2/2] Lint fixups --- src/rules/functionConstructorRule.ts | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/rules/functionConstructorRule.ts b/src/rules/functionConstructorRule.ts index 98b442c008b..eacbb928dac 100644 --- a/src/rules/functionConstructorRule.ts +++ b/src/rules/functionConstructorRule.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { isCallExpression, isNewExpression, isIdentifier } from "tsutils"; +import { isCallExpression, isIdentifier, isNewExpression } from "tsutils"; import * as ts from "typescript"; import * as Lint from ".."; @@ -23,29 +23,29 @@ import { codeExamples } from "./code-examples/functionConstructor.examples"; export class Rule extends Lint.Rules.AbstractRule { public static metadata: Lint.IRuleMetadata = { - ruleName: "function-constructor", + codeExamples, description: Lint.Utils.dedent` Prevents using the built-in Function constructor. `, + optionExamples: [true], + options: null, + optionsDescription: "Not configurable.", rationale: Lint.Utils.dedent` Calling the constructor directly is similar to \`eval\`, which is a symptom of design issues. String inputs don't receive type checking and can cause performance issues, particularly when dynamically created. - + If you need to dynamically create functions, use "factory" functions that themselves return functions. `, - optionsDescription: "Not configurable.", - options: null, - optionExamples: [true], + ruleName: "function-constructor", type: "functionality", - typescriptOnly: false, - codeExamples + typescriptOnly: false }; + public static FAILURE = "Do not use the Function constructor to create functions."; + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { return this.applyWithFunction(sourceFile, walk); } - - public static FAILURE = "Do not use the Function constructor to create functions."; } function walk(context: Lint.WalkContext): void {