From 5cf7100ad3c7b2fd3be62d94b7b4708460590a97 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Mon, 19 Jun 2017 08:51:24 -0700 Subject: [PATCH] Added no-this-reassignment rule (#2931) --- src/configs/all.ts | 1 + src/configs/latest.ts | 1 + src/rules/noThisAssignmentRule.ts | 147 ++++++++++++++++++ .../allow-destructuring/test.ts.lint | 13 ++ .../allow-destructuring/tslint.json | 7 + .../allowed-names/test.ts.lint | 8 + .../allowed-names/tslint.json | 7 + .../no-this-assignment/default/test.ts.lint | 46 ++++++ .../no-this-assignment/default/tslint.json | 5 + 9 files changed, 235 insertions(+) create mode 100644 src/rules/noThisAssignmentRule.ts create mode 100644 test/rules/no-this-assignment/allow-destructuring/test.ts.lint create mode 100644 test/rules/no-this-assignment/allow-destructuring/tslint.json create mode 100644 test/rules/no-this-assignment/allowed-names/test.ts.lint create mode 100644 test/rules/no-this-assignment/allowed-names/tslint.json create mode 100644 test/rules/no-this-assignment/default/test.ts.lint create mode 100644 test/rules/no-this-assignment/default/tslint.json diff --git a/src/configs/all.ts b/src/configs/all.ts index f4dff8e4daa..415fe430547 100644 --- a/src/configs/all.ts +++ b/src/configs/all.ts @@ -51,6 +51,7 @@ export const rules = { "no-namespace": true, "no-non-null-assertion": true, "no-reference": true, + "no-this-assignment": true, "no-var-requires": true, "only-arrow-functions": true, "prefer-for-of": true, diff --git a/src/configs/latest.ts b/src/configs/latest.ts index 52e3c35dc2b..cae2c38f598 100644 --- a/src/configs/latest.ts +++ b/src/configs/latest.ts @@ -41,6 +41,7 @@ export const rules = { true, "check-parameters", ], + "no-this-assignment": true, }; // tslint:enable object-literal-sort-keys diff --git a/src/rules/noThisAssignmentRule.ts b/src/rules/noThisAssignmentRule.ts new file mode 100644 index 00000000000..0ca388c0bf4 --- /dev/null +++ b/src/rules/noThisAssignmentRule.ts @@ -0,0 +1,147 @@ +/** + * @license + * Copyright 2017 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 utils from "tsutils"; +import * as ts from "typescript"; + +import * as Lint from "../index"; + +const ALLOW_THIS_DESTRUCTURING = "allow-destructuring"; +const ALLOWED_THIS_NAMES = "allowed-names"; + +interface Options { + allowedNames: string[]; + allowDestructuring: boolean; +} + +interface ConfigOptions { + "allow-destructuring"?: boolean; + "allowed-names"?: string[]; +} + +const parseConfigOptions = (configOptions: ConfigOptions | undefined): Options => { + const allowedNames: string[] = []; + let allowDestructuring = false; + + if (configOptions !== undefined) { + allowDestructuring = !!configOptions[ALLOW_THIS_DESTRUCTURING]; + + if (configOptions[ALLOWED_THIS_NAMES] !== undefined) { + allowedNames.push(...configOptions[ALLOWED_THIS_NAMES]!); + } + } + + return { allowedNames, allowDestructuring }; +}; + +export class Rule extends Lint.Rules.AbstractRule { + public static metadata: Lint.IRuleMetadata = { + description: "Disallows unnecessary references to `this`.", + optionExamples: [ + true, + [ + true, + { + [ALLOWED_THIS_NAMES]: ["^self$"], + [ALLOW_THIS_DESTRUCTURING]: true, + }, + ], + ], + options: { + additionalProperties: false, + properties: { + [ALLOW_THIS_DESTRUCTURING]: { + type: "boolean", + }, + [ALLOWED_THIS_NAMES]: { + listType: "string", + type: "list", + }, + }, + type: "object", + }, + optionsDescription: Lint.Utils.dedent` + Two options may be provided on an object: + + * \`${ALLOW_THIS_DESTRUCTURING}\` allows using destructuring to access members of \`this\` (e.g. \`{ foo, bar } = this;\`). + * \`${ALLOWED_THIS_NAMES}\` may be specified as a list of regular expressions to match allowed variable names.`, + rationale: "Assigning a variable to `this` instead of properly using arrow lambdas" + + "may be a symptom of pre-ES6 practices or not manging scope well.", + ruleName: "no-this-assignment", + type: "functionality", + typescriptOnly: false, + }; + + public static FAILURE_STRING_BINDINGS = "Don't assign members of `this` to local variables."; + + public static FAILURE_STRING_FACTORY_IDENTIFIERS(name: string) { + return `Assigning \`this\` reference to local variable not allowed: ${name}.`; + } + + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + const options = parseConfigOptions((this.ruleArguments as [ConfigOptions])[0]); + const noThisAssignmentWalker = new NoThisAssignmentWalker(sourceFile, this.ruleName, options); + + return this.applyWithWalker(noThisAssignmentWalker); + } +} + +class NoThisAssignmentWalker extends Lint.AbstractWalker { + private readonly allowedThisNameTesters = this.options.allowedNames.map( + (allowedThisName) => new RegExp(allowedThisName)); + + public walk(sourceFile: ts.SourceFile): void { + ts.forEachChild(sourceFile, this.visitNode); + } + + private visitNode = (node: ts.Node): void => { + if (utils.isVariableDeclaration(node)) { + this.visitVariableDeclaration(node); + } + + ts.forEachChild(node, this.visitNode); + } + + private visitVariableDeclaration(node: ts.VariableDeclaration): void { + if (node.initializer === undefined || node.initializer.kind !== ts.SyntaxKind.ThisKeyword) { + return; + } + + switch (node.name.kind) { + case ts.SyntaxKind.Identifier: + if (this.variableNameIsBanned(node.name.text)) { + this.addFailureAtNode(node, Rule.FAILURE_STRING_FACTORY_IDENTIFIERS(node.name.text)); + } + break; + + default: + if (!this.options.allowDestructuring) { + this.addFailureAtNode(node, Rule.FAILURE_STRING_BINDINGS); + } + } + } + + private variableNameIsBanned(name: string): boolean { + for (const tester of this.allowedThisNameTesters) { + if (tester.test(name)) { + return false; + } + } + + return true; + } +} diff --git a/test/rules/no-this-assignment/allow-destructuring/test.ts.lint b/test/rules/no-this-assignment/allow-destructuring/test.ts.lint new file mode 100644 index 00000000000..3be40991478 --- /dev/null +++ b/test/rules/no-this-assignment/allow-destructuring/test.ts.lint @@ -0,0 +1,13 @@ +const { length } = this; + +const { length, toString } = this; + +const [foo] = this; + +const [foo, bar] = this; + +const self = this; + ~~~~~~~~~~~ [name % ('self')] + +[name]: Assigning `this` reference to local variable not allowed: %s. + diff --git a/test/rules/no-this-assignment/allow-destructuring/tslint.json b/test/rules/no-this-assignment/allow-destructuring/tslint.json new file mode 100644 index 00000000000..ba6ce5a4fd2 --- /dev/null +++ b/test/rules/no-this-assignment/allow-destructuring/tslint.json @@ -0,0 +1,7 @@ +{ + "rules": { + "no-this-assignment": [true, { + "allow-destructuring": true + }] + } +} diff --git a/test/rules/no-this-assignment/allowed-names/test.ts.lint b/test/rules/no-this-assignment/allowed-names/test.ts.lint new file mode 100644 index 00000000000..7411f550af8 --- /dev/null +++ b/test/rules/no-this-assignment/allowed-names/test.ts.lint @@ -0,0 +1,8 @@ +const start = this; + +const startEnd = this; + +const endStart = this; + ~~~~~~~~~~~~~~~ [name % ('endStart')] + +[name]: Assigning `this` reference to local variable not allowed: %s. diff --git a/test/rules/no-this-assignment/allowed-names/tslint.json b/test/rules/no-this-assignment/allowed-names/tslint.json new file mode 100644 index 00000000000..8435d774dc5 --- /dev/null +++ b/test/rules/no-this-assignment/allowed-names/tslint.json @@ -0,0 +1,7 @@ +{ + "rules": { + "no-this-assignment": [true, { + "allowed-names": ["^start"] + }] + } +} diff --git a/test/rules/no-this-assignment/default/test.ts.lint b/test/rules/no-this-assignment/default/test.ts.lint new file mode 100644 index 00000000000..94a9141c709 --- /dev/null +++ b/test/rules/no-this-assignment/default/test.ts.lint @@ -0,0 +1,46 @@ +var unscoped = this; + ~~~~~~~~~~~~~~~ [identifier % ('unscoped')] + +function testFunction() { + let inFunction = this; + ~~~~~~~~~~~~~~~~~ [identifier % ('inFunction')] +} + +const testLambda = () => { + const inLambda = this; + ~~~~~~~~~~~~~~~ [identifier % ('inLambda')] +}; + +class TestClass { + constructor() { + const inConstructor = this; + ~~~~~~~~~~~~~~~~~~~~ [identifier % ('inConstructor')] + + const asThis: this = this; + ~~~~~~~~~~~~~~~~~~~ [identifier % ('asThis')] + + const asString = "this"; + const asArray = [this]; + const asArrayString = ["this"]; + } + + public act(scope: this = this) { + const inMemberFunction = this; + ~~~~~~~~~~~~~~~~~~~~~~~ [identifier % ('inMemberFunction')] + + const { act } = this; + ~~~~~~~~~~~~~~ [binding] + + const { act, constructor } = this; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~ [binding] + + const [foo] = this; + ~~~~~~~~~~~~ [binding] + + const [foo, bar] = this; + ~~~~~~~~~~~~~~~~~ [binding] + } +} + +[binding]: Don't assign members of `this` to local variables. +[identifier]: Assigning `this` reference to local variable not allowed: %s. diff --git a/test/rules/no-this-assignment/default/tslint.json b/test/rules/no-this-assignment/default/tslint.json new file mode 100644 index 00000000000..3fb515282cd --- /dev/null +++ b/test/rules/no-this-assignment/default/tslint.json @@ -0,0 +1,5 @@ +{ + "rules": { + "no-this-assignment": true + } +}