From fd0d8780a9dcaa42917231afa10967e50bd5fc80 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sun, 26 May 2019 15:04:09 -0400 Subject: [PATCH 1/8] feat(eslint-plugin): added new rule prefer-readonly Adds the equivalent of TSLint's `prefer-readonly` rule. --- .../src/rules/prefer-readonly.ts | 320 +++++++++++++ packages/eslint-plugin/src/util/types.ts | 21 + .../tests/rules/prefer-readonly.test.ts | 421 ++++++++++++++++++ 3 files changed, 762 insertions(+) create mode 100644 packages/eslint-plugin/src/rules/prefer-readonly.ts create mode 100644 packages/eslint-plugin/tests/rules/prefer-readonly.test.ts diff --git a/packages/eslint-plugin/src/rules/prefer-readonly.ts b/packages/eslint-plugin/src/rules/prefer-readonly.ts new file mode 100644 index 000000000000..f24dbbb76731 --- /dev/null +++ b/packages/eslint-plugin/src/rules/prefer-readonly.ts @@ -0,0 +1,320 @@ +import * as tsutils from 'tsutils'; +import ts from 'typescript'; +import * as util from '../util'; +import { typeIsOrHasBaseType } from '../util'; +import { TSESTree, AST_NODE_TYPES } from '@typescript-eslint/typescript-estree'; + +type MessageIds = 'preferReadonly'; + +type Options = [ + { + onlyInlineLambdas?: boolean; + } +]; + +const functionScopeBoundaries = [ + 'ArrowFunctionExpression', + 'FunctionDeclaration', + 'FunctionExpression', + 'GetAccessor', + 'MethodDefinition', + 'SetAccessor', +].join(', '); + +export default util.createRule({ + name: 'prefer-readonly', + meta: { + docs: { + description: + "Requires that private variables are marked as `readonly` if they're never modified outside of the constructor.", + category: 'Best Practices', + recommended: false, + tslintRuleName: 'prefer-readonly', + }, + fixable: 'code', + messages: { + preferReadonly: + "Member '{{name}}' is never reassigned; mark it as `readonly`.", + }, + schema: [ + { + type: 'object', + properties: { + onlyInlineLambdas: { + type: 'boolean', + }, + }, + }, + ], + type: 'suggestion', + }, + defaultOptions: [{}], + create(context, [{ onlyInlineLambdas }]) { + const parserServices = util.getParserServices(context); + const checker = parserServices.program.getTypeChecker(); + const classScopeStack: ClassScope[] = []; + + function handlePropertyAccessExpression( + node: ts.PropertyAccessExpression, + parent: ts.Node, + classScope: ClassScope, + ) { + if (ts.isBinaryExpression(parent)) { + handleParentBinaryExpression(node, parent, classScope); + return; + } + + if (ts.isDeleteExpression(parent)) { + classScope.addVariableModification(node); + return; + } + + if ( + ts.isPostfixUnaryExpression(parent) || + ts.isPrefixUnaryExpression(parent) + ) { + handleParentPostfixOrPrefixUnaryExpression(parent, classScope); + } + } + + function handleParentBinaryExpression( + node: ts.PropertyAccessExpression, + parent: ts.BinaryExpression, + classScope: ClassScope, + ) { + if ( + parent.left === node && + tsutils.isAssignmentKind(parent.operatorToken.kind) + ) { + classScope.addVariableModification(node); + } + } + + function handleParentPostfixOrPrefixUnaryExpression( + node: ts.PostfixUnaryExpression | ts.PrefixUnaryExpression, + classScope: ClassScope, + ) { + if ( + node.operator === ts.SyntaxKind.PlusPlusToken || + node.operator === ts.SyntaxKind.MinusMinusToken + ) { + classScope.addVariableModification( + node.operand as ts.PropertyAccessExpression, + ); + } + } + + function isConstructor(node: TSESTree.Node) { + return ( + node.type === AST_NODE_TYPES.MethodDefinition && + node.kind === 'constructor' + ); + } + + function isFunctionScopeBoundaryInStack(node: TSESTree.Node) { + if (classScopeStack.length === 0) { + return false; + } + + const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node); + if (ts.isConstructorDeclaration(tsNode)) { + return false; + } + + return tsutils.isFunctionScopeBoundary(tsNode); + } + + return { + 'ClassDeclaration, ClassExpression'( + node: TSESTree.ClassDeclaration | TSESTree.ClassExpression, + ) { + classScopeStack.push( + new ClassScope( + checker, + parserServices.esTreeNodeToTSNodeMap.get(node), + onlyInlineLambdas, + ), + ); + }, + 'ClassDeclaration, ClassExpression:exit'() { + const finalizedClassScope = classScopeStack.pop()!; + const sourceCode = context.getSourceCode(); + + for (const violatingNode of finalizedClassScope.finalizeUnmodifiedPrivateNonReadonlys()) { + const esNode = parserServices.tsNodeToESTreeNodeMap.get( + violatingNode, + ); + context.report({ + data: { + name: sourceCode.getText( + esNode.type === 'TSParameterProperty' + ? (esNode.parameter as TSESTree.AssignmentPattern).left + : (esNode as TSESTree.Property).key, + ), + }, + messageId: 'preferReadonly', + node: esNode, + }); + } + }, + MemberExpression(node) { + const tsNode = parserServices.esTreeNodeToTSNodeMap.get( + node, + ) as ts.PropertyAccessExpression; + if (classScopeStack.length !== 0) { + handlePropertyAccessExpression( + tsNode, + tsNode.parent, + classScopeStack[classScopeStack.length - 1], + ); + } + }, + [functionScopeBoundaries](node: TSESTree.Node) { + if (isConstructor(node)) { + classScopeStack[classScopeStack.length - 1].enterConstructor( + parserServices.esTreeNodeToTSNodeMap.get( + node, + ) as ts.ConstructorDeclaration, + ); + } else if (isFunctionScopeBoundaryInStack(node)) { + classScopeStack[classScopeStack.length - 1].enterNonConstructor(); + } + }, + [`${functionScopeBoundaries}:exit`](node: TSESTree.Node) { + if (isConstructor(node)) { + classScopeStack[classScopeStack.length - 1].exitConstructor(); + } else if (isFunctionScopeBoundaryInStack(node)) { + classScopeStack[classScopeStack.length - 1].exitNonConstructor(); + } + }, + }; + }, +}); + +type ParameterOrPropertyDeclaration = + | ts.ParameterDeclaration + | ts.PropertyDeclaration; + +const OUTSIDE_CONSTRUCTOR = -1; +const DIRECTLY_INSIDE_CONSTRUCTOR = 0; + +class ClassScope { + private readonly privateModifiableMembers = new Map< + string, + ParameterOrPropertyDeclaration + >(); + private readonly privateModifiableStatics = new Map< + string, + ParameterOrPropertyDeclaration + >(); + private readonly memberVariableModifications = new Set(); + private readonly staticVariableModifications = new Set(); + + private readonly classType: ts.Type; + + private constructorScopeDepth = OUTSIDE_CONSTRUCTOR; + + public constructor( + private readonly checker: ts.TypeChecker, + classNode: ts.ClassLikeDeclaration, + private readonly onlyInlineLambdas?: boolean, + ) { + this.checker = checker; + this.classType = checker.getTypeAtLocation(classNode); + + for (const member of classNode.members) { + if (ts.isPropertyDeclaration(member)) { + this.addDeclaredVariable(member); + } + } + } + + public addDeclaredVariable(node: ParameterOrPropertyDeclaration) { + if ( + !tsutils.isModifierFlagSet(node, ts.ModifierFlags.Private) || + tsutils.isModifierFlagSet(node, ts.ModifierFlags.Readonly) || + ts.isComputedPropertyName(node.name) + ) { + return; + } + + if ( + this.onlyInlineLambdas && + node.initializer !== undefined && + !ts.isArrowFunction(node.initializer) + ) { + return; + } + + (tsutils.isModifierFlagSet(node, ts.ModifierFlags.Static) + ? this.privateModifiableStatics + : this.privateModifiableMembers + ).set(node.name.getText(), node); + } + + public addVariableModification(node: ts.PropertyAccessExpression) { + const modifierType = this.checker.getTypeAtLocation(node.expression); + if ( + modifierType.symbol === undefined || + !typeIsOrHasBaseType(modifierType, this.classType) + ) { + return; + } + + const modifyingStatic = + tsutils.isObjectType(modifierType) && + tsutils.isObjectFlagSet(modifierType, ts.ObjectFlags.Anonymous); + if ( + !modifyingStatic && + this.constructorScopeDepth === DIRECTLY_INSIDE_CONSTRUCTOR + ) { + return; + } + + (modifyingStatic + ? this.staticVariableModifications + : this.memberVariableModifications + ).add(node.name.text); + } + + public enterConstructor(node: ts.ConstructorDeclaration) { + this.constructorScopeDepth = DIRECTLY_INSIDE_CONSTRUCTOR; + + for (const parameter of node.parameters) { + if (tsutils.isModifierFlagSet(parameter, ts.ModifierFlags.Private)) { + this.addDeclaredVariable(parameter); + } + } + } + + public exitConstructor() { + this.constructorScopeDepth = OUTSIDE_CONSTRUCTOR; + } + + public enterNonConstructor() { + if (this.constructorScopeDepth !== OUTSIDE_CONSTRUCTOR) { + this.constructorScopeDepth += 1; + } + } + + public exitNonConstructor() { + if (this.constructorScopeDepth !== OUTSIDE_CONSTRUCTOR) { + this.constructorScopeDepth -= 1; + } + } + + public finalizeUnmodifiedPrivateNonReadonlys() { + this.memberVariableModifications.forEach(variableName => { + this.privateModifiableMembers.delete(variableName); + }); + + this.staticVariableModifications.forEach(variableName => { + this.privateModifiableStatics.delete(variableName); + }); + + return [ + ...Array.from(this.privateModifiableMembers.values()), + ...Array.from(this.privateModifiableStatics.values()), + ]; + } +} diff --git a/packages/eslint-plugin/src/util/types.ts b/packages/eslint-plugin/src/util/types.ts index 4e5d455926ac..7faa4e0da598 100644 --- a/packages/eslint-plugin/src/util/types.ts +++ b/packages/eslint-plugin/src/util/types.ts @@ -35,3 +35,24 @@ export function containsTypeByName( bases.some(t => containsTypeByName(t, allowedNames)) ); } + +export const typeIsOrHasBaseType = (type: ts.Type, parentType: ts.Type) => { + if (type.symbol === undefined || parentType.symbol === undefined) { + return false; + } + + const typeAndBaseTypes = [type]; + const ancestorTypes = type.getBaseTypes(); + + if (ancestorTypes !== undefined) { + typeAndBaseTypes.push(...ancestorTypes); + } + + for (const baseType of typeAndBaseTypes) { + if (baseType.symbol !== undefined && baseType.symbol.name === parentType.symbol.name) { + return true; + } + } + + return false; +}; diff --git a/packages/eslint-plugin/tests/rules/prefer-readonly.test.ts b/packages/eslint-plugin/tests/rules/prefer-readonly.test.ts new file mode 100644 index 000000000000..a54b4ed4958e --- /dev/null +++ b/packages/eslint-plugin/tests/rules/prefer-readonly.test.ts @@ -0,0 +1,421 @@ +import rule from '../../src/rules/prefer-readonly'; +import { RuleTester, getFixturesRootDir } from '../RuleTester'; + +const rootDir = getFixturesRootDir(); +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + parserOptions: { + ecmaVersion: 2015, + tsconfigRootDir: rootDir, + project: './tsconfig.json', + }, +}); + +ruleTester.run('prefer-readonly', rule, { + valid: [ + `class TestEmpty { }`, + `class TestReadonlyStatic { + private static readonly correctlyReadonlyStatic = 7; + }`, + `class TestModifiableStatic { + private static correctlyModifiableStatic = 7; + + public constructor() { + TestModifiableStatic.correctlyModifiableStatic += 1; + } + }`, + `class TestModifiableByParameterProperty { + private static readonly correctlyModifiableByParameterProperty = 7; + + public constructor( + public correctlyModifiablePublicParameter: number = (() => { + return TestModifiableStatic.correctlyModifiableByParameterProperty += 1; + })() + ) { } + }`, + `class TestReadonlyInline { + private readonly correctlyReadonlyInline = 7; + }`, + `class TestReadonlyDelayed { + private readonly correctlyReadonlyDelayed = 7; + + public constructor() { + this.correctlyReadonlyDelayed += 1; + } + }`, + `class TestModifiableInline { + private correctlyModifiableInline = 7; + + public mutate() { + this.correctlyModifiableInline += 1; + + return class { + private correctlyModifiableInline = 7; + + mutate() { + this.correctlyModifiableInline += 1; + } + }; + } + }`, + `class TestModifiableDelayed { + private correctlyModifiableDelayed = 7; + + public mutate() { + this.correctlyModifiableDelayed += 1; + } + }`, + `class TestModifiableDeleted { + private correctlyModifiableDeleted = 7; + + public mutate() { + delete this.correctlyModifiableDeleted; + } + }`, + `class TestModifiableWithinConstructor { + private correctlyModifiableWithinConstructor = 7; + + public constructor() { + (() => { + this.correctlyModifiableWithinConstructor += 1; + })(); + } + }`, + `class TestModifiableWithinConstructorArrowFunction { + private correctlyModifiableWithinConstructorArrowFunction = 7; + + public constructor() { + (() => { + this.correctlyModifiableWithinConstructorArrowFunction += 1; + })(); + } + }`, + `class TestModifiableWithinConstructorInFunctionExpression { + private correctlyModifiableWithinConstructorInFunctionExpression = 7; + + public constructor() { + const self = this; + + (() => { + self.correctlyModifiableWithinConstructorInFunctionExpression += 1; + })(); + } + }`, + `class TestModifiableWithinConstructorInGetAccessor { + private correctlyModifiableWithinConstructorInGetAccessor = 7; + + public constructor() { + const self = this; + + const confusingObject = { + get accessor() { + return self.correctlyModifiableWithinConstructorInGetAccessor += 1; + }, + }; + } + }`, + `class TestModifiableWithinConstructorInMethodDeclaration { + private correctlyModifiableWithinConstructorInMethodDeclaration = 7; + + public constructor() { + const self = this; + + const confusingObject = { + methodDeclaration() { + self.correctlyModifiableWithinConstructorInMethodDeclaration = 7; + } + }; + } + }`, + `class TestModifiableWithinConstructorInSetAccessor { + private correctlyModifiableWithinConstructorInSetAccessor = 7; + + public constructor() { + const self = this; + + const confusingObject = { + set accessor(value: number) { + self.correctlyModifiableWithinConstructorInSetAccessor += value; + }, + }; + } + }`, + `class TestModifiablePostDecremented { + private correctlyModifiablePostDecremented = 7; + + public mutate() { + this.correctlyModifiablePostDecremented -= 1; + } + }`, + `class TestyModifiablePostIncremented { + private correctlyModifiablePostIncremented = 7; + + public mutate() { + this.correctlyModifiablePostIncremented += 1; + } + }`, + `class TestModifiablePreDecremented { + private correctlyModifiablePreDecremented = 7; + + public mutate() { + --this.correctlyModifiablePreDecremented; + } + }`, + `class TestModifiablePreIncremented { + private correctlyModifiablePreIncremented = 7; + + public mutate() { + ++this.correctlyModifiablePreIncremented; + } + }`, + `class TestProtectedModifiable { + protected protectedModifiable = 7; + }`, + `class TestPublicModifiable { + public publicModifiable = 7; + }`, + `class TestReadonlyParameter { + public constructor( + private readonly correctlyReadonlyParameter = 7, + ) { } + }`, + `class TestCorrectlyModifiableParameter { + public constructor( + private correctlyModifiableParameter = 7, + ) { } + + public mutate() { + this.correctlyModifiableParameter += 1; + } + }`, + { + code: `class TestCorrectlyNonInlineLambdas { + private correctlyNonInlineLambda = 7; + }`, + options: [{ + onlyInlineLambdas: true, + }] + }, + ], + invalid: [ + { + code: `class TestIncorrectlyModifiableStatic { + private static incorrectlyModifiableStatic = 7; + }`, + errors: [ + { + data: { + name: 'incorrectlyModifiableStatic', + }, + messageId: 'preferReadonly', + }, + ], + }, + { + code: `class TestIncorrectlyModifiableStaticArrow { + private static incorrectlyModifiableStaticArrow = () => 7; + }`, + errors: [ + { + data: { + name: 'incorrectlyModifiableStaticArrow', + }, + messageId: 'preferReadonly', + }, + ], + }, + { + code: `class TestIncorrectlyModifiableInline { + private incorrectlyModifiableInline = 7; + + public createConfusingChildClass() { + return class { + private incorrectlyModifiableInline = 7; + } + } + }`, + errors: [ + { + data: { + name: 'incorrectlyModifiableInline', + }, + line: 2, + messageId: 'preferReadonly', + }, + { + data: { + name: 'incorrectlyModifiableInline', + }, + line: 6, + messageId: 'preferReadonly', + }, + ], + }, + { + code: `class TestIncorrectlyModifiableDelayed { + private incorrectlyModifiableDelayed = 7; + + public constructor() { + this.incorrectlyModifiableDelayed = 7; + } + }`, + errors: [ + { + data: { + name: 'incorrectlyModifiableDelayed', + }, + messageId: 'preferReadonly', + }, + ], + }, + { + code: `class TestChildClassExpressionModifiable { + private childClassExpressionModifiable = 7; + + public createConfusingChildClass() { + return class { + mutate() { + this.childClassExpressionModifiable = 7; + } + } + } + }`, + errors: [ + { + data: { + name: 'childClassExpressionModifiable', + }, + line: 2, + messageId: 'preferReadonly', + }, + ], + }, + { + code: `class TestIncorrectlyModifiablePostMinus { + private incorrectlyModifiablePostMinus = 7; + + public mutate() { + this.incorrectlyModifiablePostMinus - 1; + } + }`, + errors: [ + { + data: { + name: 'incorrectlyModifiablePostMinus', + }, + line: 2, + messageId: 'preferReadonly', + }, + ], + }, + { + code: `class TestIncorrectlyModifiablePostPlus { + private incorrectlyModifiablePostPlus = 7; + + public mutate() { + this.incorrectlyModifiablePostPlus + 1; + } + }`, + errors: [ + { + data: { + name: 'incorrectlyModifiablePostPlus', + }, + line: 2, + messageId: 'preferReadonly', + }, + ], + }, + { + code: `class TestIncorrectlyModifiablePreMinus { + private incorrectlyModifiablePreMinus = 7; + + public mutate() { + -this.incorrectlyModifiablePreMinus; + } + }`, + errors: [ + { + data: { + name: 'incorrectlyModifiablePreMinus', + }, + line: 2, + messageId: 'preferReadonly', + }, + ], + }, + { + code: `class TestIncorrectlyModifiablePrePlus { + private incorrectlyModifiablePrePlus = 7; + + public mutate() { + +this.incorrectlyModifiablePrePlus; + } + }`, + errors: [ + { + data: { + name: 'incorrectlyModifiablePrePlus', + }, + line: 2, + messageId: 'preferReadonly', + }, + ], + }, + { + code: `class TestOverlappingClassVariable { + private overlappingClassVariable = 7; + + public workWithSimilarClass(other: SimilarClass) { + other.overlappingClassVariable = 7; + } + } + + class SimilarClass { + public overlappingClassVariable = 7; + }`, + errors: [ + { + data: { + name: 'overlappingClassVariable', + }, + line: 2, + messageId: 'preferReadonly', + }, + ], + }, + { + code: `class TestIncorrectlyModifiableParameter { + public constructor( + private incorrectlyModifiableParameter = 7, + ) { } + }`, + errors: [ + { + data: { + name: 'incorrectlyModifiableParameter', + }, + line: 3, + messageId: 'preferReadonly', + }, + ], + }, + { + code: `class TestCorrectlyNonInlineLambdas { + private incorrectlyInlineLambda = () => 7; + }`, + errors: [ + { + data: { + name: 'incorrectlyInlineLambda', + }, + line: 2, + messageId: 'preferReadonly', + }, + ], + options: [{ + onlyInlineLambdas: true, + }] + }, + ], +}); From 572881880e535971b6d604997dba72eb28a8cfe6 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sun, 26 May 2019 23:50:57 -0400 Subject: [PATCH 2/8] Added docs, auto-fixing --- packages/eslint-plugin/README.md | 1 + packages/eslint-plugin/ROADMAP.md | 3 +- .../docs/rules/prefer-readonly.md | 81 +++++++++++++++++++ .../src/rules/prefer-readonly.ts | 13 +-- .../tests/rules/prefer-readonly.test.ts | 16 ++-- 5 files changed, 101 insertions(+), 13 deletions(-) create mode 100644 packages/eslint-plugin/docs/rules/prefer-readonly.md diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index 6b605d31b57b..f3175be01f59 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -158,6 +158,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e | [`@typescript-eslint/prefer-interface`](./docs/rules/prefer-interface.md) | Prefer an interface declaration over a type literal (type T = { ... }) | :heavy_check_mark: | :wrench: | | | [`@typescript-eslint/prefer-namespace-keyword`](./docs/rules/prefer-namespace-keyword.md) | Require the use of the `namespace` keyword instead of the `module` keyword to declare custom TypeScript modules | :heavy_check_mark: | :wrench: | | | [`@typescript-eslint/prefer-regexp-exec`](./docs/rules/prefer-regexp-exec.md) | Prefer RegExp#exec() over String#match() if no global flag is provided | | | :thought_balloon: | +| [`@typescript-eslint/prefer-readonly`](./docs/rules/prefer-readonly.md) | Requires that private members are marked as `readonly` if they're never modified outside of the constructor | :heavy_check_mark: | :wrench: | | | [`@typescript-eslint/prefer-string-starts-ends-with`](./docs/rules/prefer-string-starts-ends-with.md) | Enforce the use of `String#startsWith` and `String#endsWith` instead of other equivalent methods of checking substrings | | :wrench: | :thought_balloon: | | [`@typescript-eslint/promise-function-async`](./docs/rules/promise-function-async.md) | Requires any function or method that returns a Promise to be marked async | | | :thought_balloon: | | [`@typescript-eslint/require-array-sort-compare`](./docs/rules/require-array-sort-compare.md) | Enforce giving `compare` argument to `Array#sort` | | | :thought_balloon: | diff --git a/packages/eslint-plugin/ROADMAP.md b/packages/eslint-plugin/ROADMAP.md index b2567afe070e..1457e4f73543 100644 --- a/packages/eslint-plugin/ROADMAP.md +++ b/packages/eslint-plugin/ROADMAP.md @@ -120,7 +120,7 @@ | [`no-require-imports`] | ✅ | [`@typescript-eslint/no-require-imports`] | | [`object-literal-sort-keys`] | 🌓 | [`sort-keys`][sort-keys] [2] | | [`prefer-const`] | 🌟 | [`prefer-const`][prefer-const] | -| [`prefer-readonly`] | 🛑 | N/A | +| [`prefer-readonly`] | ✅ | [`@typescript-eslint/prefer-readonly`] | | [`trailing-comma`] | 🌓 | [`comma-dangle`][comma-dangle] or [Prettier] | [1] Only warns when importing deprecated symbols
@@ -609,6 +609,7 @@ Relevant plugins: [`chai-expect-keywords`](https://github.com/gavinaiken/eslint- [`@typescript-eslint/prefer-interface`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/prefer-interface.md [`@typescript-eslint/no-array-constructor`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-array-constructor.md [`@typescript-eslint/prefer-function-type`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/prefer-function-type.md +[`@typescript-eslint/prefer-readonly`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/prefer-readonly.md [`@typescript-eslint/no-for-in-array`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-for-in-array.md [`@typescript-eslint/no-unnecessary-qualifier`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md [`@typescript-eslint/semi`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/semi.md diff --git a/packages/eslint-plugin/docs/rules/prefer-readonly.md b/packages/eslint-plugin/docs/rules/prefer-readonly.md new file mode 100644 index 000000000000..b3f0a5420109 --- /dev/null +++ b/packages/eslint-plugin/docs/rules/prefer-readonly.md @@ -0,0 +1,81 @@ +# require never-modified private members be marked as `readonly` + +This rule enforces that private members are marked as `readonly` if they're never modified outside of the constructor. + +## Rule Details + +Member variables with the privacy `private` are never permitted to be modified outside of their declaring class. +If that class never modifies their value, they may safely be marked as `readonly`. + +Examples of **incorrect** code for this rule: + +```ts +class Container { + // These member variables could be marked as readonly + private neverModifiedMember = true; + private onlyModifiedInConstructor: number; + + public constructor( + onlyModifiedInConstructor: number, + // Private parameter properties can also be marked as reaodnly + private neverModifiedParameter: string, + ) { + this.onlyModifiedInConstructor = onlyModifiedInConstructor; + } +} +``` + +Examples of **correct** code for this rule: + +```ts +class Container { + // Public members might be modified externally + public publicMember: boolean; + + // Protected members might be modified by child classes + protected protectedMember: number; + + // This is modified later on by the class + private modifiedLater = 'unchanged'; + + public mutate() { + this.modifiedLater = 'mutated'; + } +} +``` + +## Options + +This rule, in its default state, does not require any argument. + +### onlyInlineLambdas + +You may pass `"onlyInlineLambdas": true` as a rule option within an object to restrict checking only to members immediately assigned a lambda value. + +```cjson +{ + "@typescript-eslint/prefer-readonly": ["error", { "onlyInlineLambdas": true }] +} +``` + +Example of **correct** code for the `{ "onlyInlineLambdas": true }` options: + +```ts +class Container { + private neverModifiedPrivate = 'unchanged'; +} +``` + +Example of **incorrect** code for the `{ "onlyInlineLambdas": true }` options: + +```ts +class Container { + private onClick = () => { + /* ... */ + }; +} +``` + +## Related to + +- TSLint: ['prefer-readonly'](https://palantir.github.io/tslint/rules/prefer-readonly) diff --git a/packages/eslint-plugin/src/rules/prefer-readonly.ts b/packages/eslint-plugin/src/rules/prefer-readonly.ts index f24dbbb76731..330c6a442d58 100644 --- a/packages/eslint-plugin/src/rules/prefer-readonly.ts +++ b/packages/eslint-plugin/src/rules/prefer-readonly.ts @@ -26,7 +26,7 @@ export default util.createRule({ meta: { docs: { description: - "Requires that private variables are marked as `readonly` if they're never modified outside of the constructor.", + "Requires that private members are marked as `readonly` if they're never modified outside of the constructor.", category: 'Best Practices', recommended: false, tslintRuleName: 'prefer-readonly', @@ -144,14 +144,15 @@ export default util.createRule({ const esNode = parserServices.tsNodeToESTreeNodeMap.get( violatingNode, ); + const nameNode = + esNode.type === 'TSParameterProperty' + ? (esNode.parameter as TSESTree.AssignmentPattern).left + : (esNode as TSESTree.Property).key; context.report({ data: { - name: sourceCode.getText( - esNode.type === 'TSParameterProperty' - ? (esNode.parameter as TSESTree.AssignmentPattern).left - : (esNode as TSESTree.Property).key, - ), + name: sourceCode.getText(nameNode), }, + fix: fixer => fixer.insertTextBefore(nameNode, 'readonly '), messageId: 'preferReadonly', node: esNode, }); diff --git a/packages/eslint-plugin/tests/rules/prefer-readonly.test.ts b/packages/eslint-plugin/tests/rules/prefer-readonly.test.ts index a54b4ed4958e..e76f3cff0494 100644 --- a/packages/eslint-plugin/tests/rules/prefer-readonly.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-readonly.test.ts @@ -192,9 +192,11 @@ ruleTester.run('prefer-readonly', rule, { code: `class TestCorrectlyNonInlineLambdas { private correctlyNonInlineLambda = 7; }`, - options: [{ - onlyInlineLambdas: true, - }] + options: [ + { + onlyInlineLambdas: true, + }, + ], }, ], invalid: [ @@ -413,9 +415,11 @@ ruleTester.run('prefer-readonly', rule, { messageId: 'preferReadonly', }, ], - options: [{ - onlyInlineLambdas: true, - }] + options: [ + { + onlyInlineLambdas: true, + }, + ], }, ], }); From a10f82d7377225a3e37a00dc6e88d51590968846 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Mon, 27 May 2019 16:47:55 -0400 Subject: [PATCH 3/8] Updated docs; love the new build time checks! --- packages/eslint-plugin/README.md | 2 +- packages/eslint-plugin/src/configs/all.json | 1 + packages/eslint-plugin/src/configs/recommended.json | 1 + packages/eslint-plugin/src/rules/index.ts | 2 ++ packages/eslint-plugin/src/rules/prefer-readonly.ts | 4 ++-- 5 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index f3175be01f59..c1524fce8389 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -157,8 +157,8 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e | [`@typescript-eslint/prefer-includes`](./docs/rules/prefer-includes.md) | Enforce `includes` method over `indexOf` method | | :wrench: | :thought_balloon: | | [`@typescript-eslint/prefer-interface`](./docs/rules/prefer-interface.md) | Prefer an interface declaration over a type literal (type T = { ... }) | :heavy_check_mark: | :wrench: | | | [`@typescript-eslint/prefer-namespace-keyword`](./docs/rules/prefer-namespace-keyword.md) | Require the use of the `namespace` keyword instead of the `module` keyword to declare custom TypeScript modules | :heavy_check_mark: | :wrench: | | +| [`@typescript-eslint/prefer-readonly`](./docs/rules/prefer-readonly.md) | Requires that private members are marked as `readonly` if they're never modified outside of the constructor | :heavy_check_mark: | :wrench: | :thought_balloon: | | [`@typescript-eslint/prefer-regexp-exec`](./docs/rules/prefer-regexp-exec.md) | Prefer RegExp#exec() over String#match() if no global flag is provided | | | :thought_balloon: | -| [`@typescript-eslint/prefer-readonly`](./docs/rules/prefer-readonly.md) | Requires that private members are marked as `readonly` if they're never modified outside of the constructor | :heavy_check_mark: | :wrench: | | | [`@typescript-eslint/prefer-string-starts-ends-with`](./docs/rules/prefer-string-starts-ends-with.md) | Enforce the use of `String#startsWith` and `String#endsWith` instead of other equivalent methods of checking substrings | | :wrench: | :thought_balloon: | | [`@typescript-eslint/promise-function-async`](./docs/rules/promise-function-async.md) | Requires any function or method that returns a Promise to be marked async | | | :thought_balloon: | | [`@typescript-eslint/require-array-sort-compare`](./docs/rules/require-array-sort-compare.md) | Enforce giving `compare` argument to `Array#sort` | | | :thought_balloon: | diff --git a/packages/eslint-plugin/src/configs/all.json b/packages/eslint-plugin/src/configs/all.json index cc2f97786718..5d20c767b9bc 100644 --- a/packages/eslint-plugin/src/configs/all.json +++ b/packages/eslint-plugin/src/configs/all.json @@ -55,6 +55,7 @@ "@typescript-eslint/prefer-includes": "error", "@typescript-eslint/prefer-interface": "error", "@typescript-eslint/prefer-namespace-keyword": "error", + "@typescript-eslint/prefer-readonly": "error", "@typescript-eslint/prefer-regexp-exec": "error", "@typescript-eslint/prefer-string-starts-ends-with": "error", "@typescript-eslint/promise-function-async": "error", diff --git a/packages/eslint-plugin/src/configs/recommended.json b/packages/eslint-plugin/src/configs/recommended.json index d26fd25d01c1..417e581fc5ed 100644 --- a/packages/eslint-plugin/src/configs/recommended.json +++ b/packages/eslint-plugin/src/configs/recommended.json @@ -32,6 +32,7 @@ "@typescript-eslint/no-var-requires": "error", "@typescript-eslint/prefer-interface": "error", "@typescript-eslint/prefer-namespace-keyword": "error", + "@typescript-eslint/prefer-readonly": "warn", "@typescript-eslint/type-annotation-spacing": "error" } } diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts index d62f58d6af3f..770504ff0cdb 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -43,6 +43,7 @@ import preferFunctionType from './prefer-function-type'; import preferIncludes from './prefer-includes'; import preferInterface from './prefer-interface'; import preferNamespaceKeyword from './prefer-namespace-keyword'; +import preferReadonly from './prefer-readonly'; import preferRegexpExec from './prefer-regexp-exec'; import preferStringStartsEndsWith from './prefer-string-starts-ends-with'; import promiseFunctionAsync from './promise-function-async'; @@ -99,6 +100,7 @@ export default { 'prefer-includes': preferIncludes, 'prefer-interface': preferInterface, 'prefer-namespace-keyword': preferNamespaceKeyword, + 'prefer-readonly': preferReadonly, 'prefer-regexp-exec': preferRegexpExec, 'prefer-string-starts-ends-with': preferStringStartsEndsWith, 'promise-function-async': promiseFunctionAsync, diff --git a/packages/eslint-plugin/src/rules/prefer-readonly.ts b/packages/eslint-plugin/src/rules/prefer-readonly.ts index 330c6a442d58..d4a826896971 100644 --- a/packages/eslint-plugin/src/rules/prefer-readonly.ts +++ b/packages/eslint-plugin/src/rules/prefer-readonly.ts @@ -26,9 +26,9 @@ export default util.createRule({ meta: { docs: { description: - "Requires that private members are marked as `readonly` if they're never modified outside of the constructor.", + "Requires that private members are marked as `readonly` if they're never modified outside of the constructor", category: 'Best Practices', - recommended: false, + recommended: 'warn', tslintRuleName: 'prefer-readonly', }, fixable: 'code', From c80f1f4bfd00f00f1dda2aa679f5094b309d5629 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Mon, 27 May 2019 20:22:25 -0400 Subject: [PATCH 4/8] Fixed linting errors (ha) and corrected internal source --- .../eslint-plugin-tslint/src/custom-linter.ts | 2 +- .../indent-new-do-not-use/OffsetStorage.ts | 14 +-- .../rules/indent-new-do-not-use/TokenInfo.ts | 2 +- .../src/rules/prefer-readonly.ts | 24 +++- .../tests/rules/prefer-readonly.test.ts | 109 +++++++++++++++++- packages/typescript-estree/src/convert.ts | 6 +- 6 files changed, 139 insertions(+), 18 deletions(-) diff --git a/packages/eslint-plugin-tslint/src/custom-linter.ts b/packages/eslint-plugin-tslint/src/custom-linter.ts index 892e12f1adf0..833fda00ca92 100644 --- a/packages/eslint-plugin-tslint/src/custom-linter.ts +++ b/packages/eslint-plugin-tslint/src/custom-linter.ts @@ -4,7 +4,7 @@ import { Program } from 'typescript'; const TSLintLinter = Linter as any; export class CustomLinter extends TSLintLinter { - constructor(options: ILinterOptions, private program: Program) { + constructor(options: ILinterOptions, private readonly program: Program) { super(options, program); } diff --git a/packages/eslint-plugin/src/rules/indent-new-do-not-use/OffsetStorage.ts b/packages/eslint-plugin/src/rules/indent-new-do-not-use/OffsetStorage.ts index deacc272bb33..475e3c35d7b0 100644 --- a/packages/eslint-plugin/src/rules/indent-new-do-not-use/OffsetStorage.ts +++ b/packages/eslint-plugin/src/rules/indent-new-do-not-use/OffsetStorage.ts @@ -9,13 +9,13 @@ import { TokenInfo } from './TokenInfo'; * A class to store information on desired offsets of tokens from each other */ export class OffsetStorage { - private tokenInfo: TokenInfo; - private indentSize: number; - private indentType: string; - private tree: BinarySearchTree; - private lockedFirstTokens: WeakMap; - private desiredIndentCache: WeakMap; - private ignoredTokens: WeakSet; + private readonly tokenInfo: TokenInfo; + private readonly indentSize: number; + private readonly indentType: string; + private readonly tree: BinarySearchTree; + private readonly lockedFirstTokens: WeakMap; + private readonly desiredIndentCache: WeakMap; + private readonly ignoredTokens: WeakSet; /** * @param tokenInfo a TokenInfo instance * @param indentSize The desired size of each indentation level diff --git a/packages/eslint-plugin/src/rules/indent-new-do-not-use/TokenInfo.ts b/packages/eslint-plugin/src/rules/indent-new-do-not-use/TokenInfo.ts index 9b7d345fe3d6..16d15c4ae5f1 100644 --- a/packages/eslint-plugin/src/rules/indent-new-do-not-use/TokenInfo.ts +++ b/packages/eslint-plugin/src/rules/indent-new-do-not-use/TokenInfo.ts @@ -8,7 +8,7 @@ import { TokenOrComment } from './BinarySearchTree'; * A helper class to get token-based info related to indentation */ export class TokenInfo { - private sourceCode: TSESLint.SourceCode; + private readonly sourceCode: TSESLint.SourceCode; public firstTokensByLineNumber: Map; constructor(sourceCode: TSESLint.SourceCode) { diff --git a/packages/eslint-plugin/src/rules/prefer-readonly.ts b/packages/eslint-plugin/src/rules/prefer-readonly.ts index d4a826896971..115a73c43db7 100644 --- a/packages/eslint-plugin/src/rules/prefer-readonly.ts +++ b/packages/eslint-plugin/src/rules/prefer-readonly.ts @@ -124,6 +124,24 @@ export default util.createRule({ return tsutils.isFunctionScopeBoundary(tsNode); } + function getEsNodesFromViolatingNode( + violatingNode: ParameterOrPropertyDeclaration, + ) { + if (ts.isParameterPropertyDeclaration(violatingNode)) { + return { + esNode: parserServices.tsNodeToESTreeNodeMap.get(violatingNode.name), + nameNode: parserServices.tsNodeToESTreeNodeMap.get( + violatingNode.name, + ), + }; + } + + return { + esNode: parserServices.tsNodeToESTreeNodeMap.get(violatingNode), + nameNode: parserServices.tsNodeToESTreeNodeMap.get(violatingNode.name), + }; + } + return { 'ClassDeclaration, ClassExpression'( node: TSESTree.ClassDeclaration | TSESTree.ClassExpression, @@ -141,13 +159,9 @@ export default util.createRule({ const sourceCode = context.getSourceCode(); for (const violatingNode of finalizedClassScope.finalizeUnmodifiedPrivateNonReadonlys()) { - const esNode = parserServices.tsNodeToESTreeNodeMap.get( + const { esNode, nameNode } = getEsNodesFromViolatingNode( violatingNode, ); - const nameNode = - esNode.type === 'TSParameterProperty' - ? (esNode.parameter as TSESTree.AssignmentPattern).left - : (esNode as TSESTree.Property).key; context.report({ data: { name: sourceCode.getText(nameNode), diff --git a/packages/eslint-plugin/tests/rules/prefer-readonly.test.ts b/packages/eslint-plugin/tests/rules/prefer-readonly.test.ts index e76f3cff0494..3c8499c2fc51 100644 --- a/packages/eslint-plugin/tests/rules/prefer-readonly.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-readonly.test.ts @@ -212,6 +212,9 @@ ruleTester.run('prefer-readonly', rule, { messageId: 'preferReadonly', }, ], + output: `class TestIncorrectlyModifiableStatic { + private static readonly incorrectlyModifiableStatic = 7; + }`, }, { code: `class TestIncorrectlyModifiableStaticArrow { @@ -225,6 +228,9 @@ ruleTester.run('prefer-readonly', rule, { messageId: 'preferReadonly', }, ], + output: `class TestIncorrectlyModifiableStaticArrow { + private static readonly incorrectlyModifiableStaticArrow = () => 7; + }`, }, { code: `class TestIncorrectlyModifiableInline { @@ -252,6 +258,15 @@ ruleTester.run('prefer-readonly', rule, { messageId: 'preferReadonly', }, ], + output: `class TestIncorrectlyModifiableInline { + private readonly incorrectlyModifiableInline = 7; + + public createConfusingChildClass() { + return class { + private readonly incorrectlyModifiableInline = 7; + } + } + }`, }, { code: `class TestIncorrectlyModifiableDelayed { @@ -269,6 +284,13 @@ ruleTester.run('prefer-readonly', rule, { messageId: 'preferReadonly', }, ], + output: `class TestIncorrectlyModifiableDelayed { + private readonly incorrectlyModifiableDelayed = 7; + + public constructor() { + this.incorrectlyModifiableDelayed = 7; + } + }`, }, { code: `class TestChildClassExpressionModifiable { @@ -276,8 +298,10 @@ ruleTester.run('prefer-readonly', rule, { public createConfusingChildClass() { return class { + private childClassExpressionModifiable = 7; + mutate() { - this.childClassExpressionModifiable = 7; + this.childClassExpressionModifiable += 1; } } } @@ -291,6 +315,19 @@ ruleTester.run('prefer-readonly', rule, { messageId: 'preferReadonly', }, ], + output: `class TestChildClassExpressionModifiable { + private readonly childClassExpressionModifiable = 7; + + public createConfusingChildClass() { + return class { + private childClassExpressionModifiable = 7; + + mutate() { + this.childClassExpressionModifiable += 1; + } + } + } + }`, }, { code: `class TestIncorrectlyModifiablePostMinus { @@ -309,6 +346,13 @@ ruleTester.run('prefer-readonly', rule, { messageId: 'preferReadonly', }, ], + output: `class TestIncorrectlyModifiablePostMinus { + private readonly incorrectlyModifiablePostMinus = 7; + + public mutate() { + this.incorrectlyModifiablePostMinus - 1; + } + }`, }, { code: `class TestIncorrectlyModifiablePostPlus { @@ -327,6 +371,13 @@ ruleTester.run('prefer-readonly', rule, { messageId: 'preferReadonly', }, ], + output: `class TestIncorrectlyModifiablePostPlus { + private readonly incorrectlyModifiablePostPlus = 7; + + public mutate() { + this.incorrectlyModifiablePostPlus + 1; + } + }`, }, { code: `class TestIncorrectlyModifiablePreMinus { @@ -345,6 +396,13 @@ ruleTester.run('prefer-readonly', rule, { messageId: 'preferReadonly', }, ], + output: `class TestIncorrectlyModifiablePreMinus { + private readonly incorrectlyModifiablePreMinus = 7; + + public mutate() { + -this.incorrectlyModifiablePreMinus; + } + }`, }, { code: `class TestIncorrectlyModifiablePrePlus { @@ -363,6 +421,13 @@ ruleTester.run('prefer-readonly', rule, { messageId: 'preferReadonly', }, ], + output: `class TestIncorrectlyModifiablePrePlus { + private readonly incorrectlyModifiablePrePlus = 7; + + public mutate() { + +this.incorrectlyModifiablePrePlus; + } + }`, }, { code: `class TestOverlappingClassVariable { @@ -385,6 +450,17 @@ ruleTester.run('prefer-readonly', rule, { messageId: 'preferReadonly', }, ], + output: `class TestOverlappingClassVariable { + private readonly overlappingClassVariable = 7; + + public workWithSimilarClass(other: SimilarClass) { + other.overlappingClassVariable = 7; + } + } + + class SimilarClass { + public overlappingClassVariable = 7; + }`, }, { code: `class TestIncorrectlyModifiableParameter { @@ -401,6 +477,34 @@ ruleTester.run('prefer-readonly', rule, { messageId: 'preferReadonly', }, ], + output: `class TestIncorrectlyModifiableParameter { + public constructor( + private readonly incorrectlyModifiableParameter = 7, + ) { } + }`, + }, + { + code: `class TestIncorrectlyModifiableParameter { + public constructor( + public ignore: boolean, + private incorrectlyModifiableParameter = 7, + ) { } + }`, + errors: [ + { + data: { + name: 'incorrectlyModifiableParameter', + }, + line: 4, + messageId: 'preferReadonly', + }, + ], + output: `class TestIncorrectlyModifiableParameter { + public constructor( + public ignore: boolean, + private readonly incorrectlyModifiableParameter = 7, + ) { } + }`, }, { code: `class TestCorrectlyNonInlineLambdas { @@ -420,6 +524,9 @@ ruleTester.run('prefer-readonly', rule, { onlyInlineLambdas: true, }, ], + output: `class TestCorrectlyNonInlineLambdas { + private readonly incorrectlyInlineLambda = () => 7; + }`, }, ], }); diff --git a/packages/typescript-estree/src/convert.ts b/packages/typescript-estree/src/convert.ts index f8151e1b268c..cdf6b184150f 100644 --- a/packages/typescript-estree/src/convert.ts +++ b/packages/typescript-estree/src/convert.ts @@ -43,9 +43,9 @@ export function convertError(error: any) { export class Converter { private readonly ast: ts.SourceFile; - private options: ConverterOptions; - private esTreeNodeToTSNodeMap = new WeakMap(); - private tsNodeToESTreeNodeMap = new WeakMap(); + private readonly options: ConverterOptions; + private readonly esTreeNodeToTSNodeMap = new WeakMap(); + private readonly tsNodeToESTreeNodeMap = new WeakMap(); private allowPattern: boolean = false; private inTypeMode: boolean = false; From 40678979930dff859292e5fe9934cc20948b945b Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 1 Jun 2019 09:20:12 -0400 Subject: [PATCH 5/8] PR feedback: non recommended; :exit; some test coverage --- .../eslint-plugin/src/configs/recommended.json | 1 - .../eslint-plugin/src/rules/prefer-readonly.ts | 18 +++++++++--------- .../tests/rules/prefer-readonly.test.ts | 17 +++++++++++++++++ 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/packages/eslint-plugin/src/configs/recommended.json b/packages/eslint-plugin/src/configs/recommended.json index 417e581fc5ed..d26fd25d01c1 100644 --- a/packages/eslint-plugin/src/configs/recommended.json +++ b/packages/eslint-plugin/src/configs/recommended.json @@ -32,7 +32,6 @@ "@typescript-eslint/no-var-requires": "error", "@typescript-eslint/prefer-interface": "error", "@typescript-eslint/prefer-namespace-keyword": "error", - "@typescript-eslint/prefer-readonly": "warn", "@typescript-eslint/type-annotation-spacing": "error" } } diff --git a/packages/eslint-plugin/src/rules/prefer-readonly.ts b/packages/eslint-plugin/src/rules/prefer-readonly.ts index 115a73c43db7..3ece4a282c5f 100644 --- a/packages/eslint-plugin/src/rules/prefer-readonly.ts +++ b/packages/eslint-plugin/src/rules/prefer-readonly.ts @@ -28,7 +28,6 @@ export default util.createRule({ description: "Requires that private members are marked as `readonly` if they're never modified outside of the constructor", category: 'Best Practices', - recommended: 'warn', tslintRuleName: 'prefer-readonly', }, fixable: 'code', @@ -38,17 +37,18 @@ export default util.createRule({ }, schema: [ { - type: 'object', + allowAdditionalProperties: false, properties: { onlyInlineLambdas: { type: 'boolean', }, }, + type: 'object', }, ], type: 'suggestion', }, - defaultOptions: [{}], + defaultOptions: [{ onlyInlineLambdas: false }], create(context, [{ onlyInlineLambdas }]) { const parserServices = util.getParserServices(context); const checker = parserServices.program.getTypeChecker(); @@ -154,7 +154,7 @@ export default util.createRule({ ), ); }, - 'ClassDeclaration, ClassExpression:exit'() { + 'ClassDeclaration:exit, ClassExpression:exit'() { const finalizedClassScope = classScopeStack.pop()!; const sourceCode = context.getSourceCode(); @@ -173,9 +173,9 @@ export default util.createRule({ } }, MemberExpression(node) { - const tsNode = parserServices.esTreeNodeToTSNodeMap.get( - node, - ) as ts.PropertyAccessExpression; + const tsNode = parserServices.esTreeNodeToTSNodeMap.get< + ts.PropertyAccessExpression + >(node); if (classScopeStack.length !== 0) { handlePropertyAccessExpression( tsNode, @@ -187,9 +187,9 @@ export default util.createRule({ [functionScopeBoundaries](node: TSESTree.Node) { if (isConstructor(node)) { classScopeStack[classScopeStack.length - 1].enterConstructor( - parserServices.esTreeNodeToTSNodeMap.get( + parserServices.esTreeNodeToTSNodeMap.get( node, - ) as ts.ConstructorDeclaration, + ), ); } else if (isFunctionScopeBoundaryInStack(node)) { classScopeStack[classScopeStack.length - 1].enterNonConstructor(); diff --git a/packages/eslint-plugin/tests/rules/prefer-readonly.test.ts b/packages/eslint-plugin/tests/rules/prefer-readonly.test.ts index 3c8499c2fc51..a5ff4734e1c5 100644 --- a/packages/eslint-plugin/tests/rules/prefer-readonly.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-readonly.test.ts @@ -13,6 +13,23 @@ const ruleTester = new RuleTester({ ruleTester.run('prefer-readonly', rule, { valid: [ + `function ignore() { }`, + `const ignore = function () { }`, + `const ignore = () => { }`, + `const container = { member: true }; + container.member;`, + `const container = { member: 1 }; + +container.member;`, + `const container = { member: 1 }; + ++container.member;`, + `const container = { member: 1 }; + container.member++;`, + `const container = { member: 1 }; + -container.member;`, + `const container = { member: 1 }; + --container.member;`, + `const container = { member: 1 }; + container.member--;`, `class TestEmpty { }`, `class TestReadonlyStatic { private static readonly correctlyReadonlyStatic = 7; From 88e21fb504457514bd3d01c8b0bdc5a292824c4d Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 1 Jun 2019 09:31:02 -0400 Subject: [PATCH 6/8] I guess tslintRuleName isn't allowed now? --- packages/eslint-plugin/src/rules/prefer-readonly.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/prefer-readonly.ts b/packages/eslint-plugin/src/rules/prefer-readonly.ts index 3ece4a282c5f..d08f5f135917 100644 --- a/packages/eslint-plugin/src/rules/prefer-readonly.ts +++ b/packages/eslint-plugin/src/rules/prefer-readonly.ts @@ -28,7 +28,6 @@ export default util.createRule({ description: "Requires that private members are marked as `readonly` if they're never modified outside of the constructor", category: 'Best Practices', - tslintRuleName: 'prefer-readonly', }, fixable: 'code', messages: { From f0f75578e014ed597f53647f42c0df5d2c454832 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 1 Jun 2019 09:36:44 -0400 Subject: [PATCH 7/8] Added back recommended as false --- packages/eslint-plugin/src/rules/prefer-readonly.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/eslint-plugin/src/rules/prefer-readonly.ts b/packages/eslint-plugin/src/rules/prefer-readonly.ts index d08f5f135917..969b38521040 100644 --- a/packages/eslint-plugin/src/rules/prefer-readonly.ts +++ b/packages/eslint-plugin/src/rules/prefer-readonly.ts @@ -28,6 +28,7 @@ export default util.createRule({ description: "Requires that private members are marked as `readonly` if they're never modified outside of the constructor", category: 'Best Practices', + recommended: false, }, fixable: 'code', messages: { From f70bbe17a66b6985074b82a80a5e90cd24ea91de Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 1 Jun 2019 09:44:42 -0400 Subject: [PATCH 8/8] Removed :exit; fixed README.md table --- packages/eslint-plugin/README.md | 2 +- packages/eslint-plugin/src/rules/prefer-readonly.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index c1524fce8389..84a23bfa6b6e 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -157,7 +157,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e | [`@typescript-eslint/prefer-includes`](./docs/rules/prefer-includes.md) | Enforce `includes` method over `indexOf` method | | :wrench: | :thought_balloon: | | [`@typescript-eslint/prefer-interface`](./docs/rules/prefer-interface.md) | Prefer an interface declaration over a type literal (type T = { ... }) | :heavy_check_mark: | :wrench: | | | [`@typescript-eslint/prefer-namespace-keyword`](./docs/rules/prefer-namespace-keyword.md) | Require the use of the `namespace` keyword instead of the `module` keyword to declare custom TypeScript modules | :heavy_check_mark: | :wrench: | | -| [`@typescript-eslint/prefer-readonly`](./docs/rules/prefer-readonly.md) | Requires that private members are marked as `readonly` if they're never modified outside of the constructor | :heavy_check_mark: | :wrench: | :thought_balloon: | +| [`@typescript-eslint/prefer-readonly`](./docs/rules/prefer-readonly.md) | Requires that private members are marked as `readonly` if they're never modified outside of the constructor | | :wrench: | :thought_balloon: | | [`@typescript-eslint/prefer-regexp-exec`](./docs/rules/prefer-regexp-exec.md) | Prefer RegExp#exec() over String#match() if no global flag is provided | | | :thought_balloon: | | [`@typescript-eslint/prefer-string-starts-ends-with`](./docs/rules/prefer-string-starts-ends-with.md) | Enforce the use of `String#startsWith` and `String#endsWith` instead of other equivalent methods of checking substrings | | :wrench: | :thought_balloon: | | [`@typescript-eslint/promise-function-async`](./docs/rules/promise-function-async.md) | Requires any function or method that returns a Promise to be marked async | | | :thought_balloon: | diff --git a/packages/eslint-plugin/src/rules/prefer-readonly.ts b/packages/eslint-plugin/src/rules/prefer-readonly.ts index 969b38521040..24685d306762 100644 --- a/packages/eslint-plugin/src/rules/prefer-readonly.ts +++ b/packages/eslint-plugin/src/rules/prefer-readonly.ts @@ -154,7 +154,7 @@ export default util.createRule({ ), ); }, - 'ClassDeclaration:exit, ClassExpression:exit'() { + 'ClassDeclaration, ClassExpression:exit'() { const finalizedClassScope = classScopeStack.pop()!; const sourceCode = context.getSourceCode();