From 3bd499128cdaba2e279c5fbdf63bd6254a82cd05 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Tue, 1 Mar 2022 14:18:38 -0500 Subject: [PATCH 1/9] feat(eslint-plugin): allow 'prefer' in renamed parameter-properties rule --- packages/eslint-plugin/README.md | 2 +- packages/eslint-plugin/ROADMAP.md | 4 +- packages/eslint-plugin/docs/rules/README.md | 2 +- ...-properties.md => parameter-properties.md} | 4 +- packages/eslint-plugin/src/configs/all.ts | 2 +- packages/eslint-plugin/src/rules/index.ts | 4 +- .../src/rules/no-parameter-properties.ts | 110 --------- .../src/rules/parameter-properties.ts | 212 ++++++++++++++++++ ...s.test.ts => parameter-properties.test.ts} | 137 ++++++----- 9 files changed, 301 insertions(+), 176 deletions(-) rename packages/eslint-plugin/docs/rules/{no-parameter-properties.md => parameter-properties.md} (98%) delete mode 100644 packages/eslint-plugin/src/rules/no-parameter-properties.ts create mode 100644 packages/eslint-plugin/src/rules/parameter-properties.ts rename packages/eslint-plugin/tests/rules/{no-parameter-properties.test.ts => parameter-properties.test.ts} (78%) diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index 34209e621cb..31ee3dcfff8 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -135,7 +135,6 @@ Pro Tip: For larger codebases you may want to consider splitting our linting int | [`@typescript-eslint/no-non-null-asserted-nullish-coalescing`](./docs/rules/no-non-null-asserted-nullish-coalescing.md) | Disallows using a non-null assertion in the left operand of the nullish coalescing operator | | | | | [`@typescript-eslint/no-non-null-asserted-optional-chain`](./docs/rules/no-non-null-asserted-optional-chain.md) | Disallows using a non-null assertion after an optional chain expression | :white_check_mark: | | | | [`@typescript-eslint/no-non-null-assertion`](./docs/rules/no-non-null-assertion.md) | Disallows non-null assertions using the `!` postfix operator | :white_check_mark: | | | -| [`@typescript-eslint/no-parameter-properties`](./docs/rules/no-parameter-properties.md) | Disallow the use of parameter properties in class constructors | | | | | [`@typescript-eslint/no-redundant-type-constituents`](./docs/rules/no-redundant-type-constituents.md) | Disallow members of unions and intersections that do nothing or override type information | | | :thought_balloon: | | [`@typescript-eslint/no-require-imports`](./docs/rules/no-require-imports.md) | Disallows invocation of `require()` | | | | | [`@typescript-eslint/no-this-alias`](./docs/rules/no-this-alias.md) | Disallow aliasing `this` | :white_check_mark: | | | @@ -154,6 +153,7 @@ Pro Tip: For larger codebases you may want to consider splitting our linting int | [`@typescript-eslint/no-useless-empty-export`](./docs/rules/no-useless-empty-export.md) | Disallow empty exports that don't change anything in a module file | | :wrench: | | | [`@typescript-eslint/no-var-requires`](./docs/rules/no-var-requires.md) | Disallows the use of require statements except in import statements | :white_check_mark: | | | | [`@typescript-eslint/non-nullable-type-assertion-style`](./docs/rules/non-nullable-type-assertion-style.md) | Prefers a non-null assertion over explicit type cast when possible | | :wrench: | :thought_balloon: | +| [`@typescript-eslint/parameter-properties`](./docs/rules/parameter-properties.md) | Require or disallow the use of parameter properties in class constructors | | | | | [`@typescript-eslint/prefer-as-const`](./docs/rules/prefer-as-const.md) | Prefer usage of `as const` over literal type | :white_check_mark: | :wrench: | | | [`@typescript-eslint/prefer-enum-initializers`](./docs/rules/prefer-enum-initializers.md) | Prefer initializing each enums member value | | | | | [`@typescript-eslint/prefer-for-of`](./docs/rules/prefer-for-of.md) | Prefer a ‘for-of’ loop over a standard ‘for’ loop if the index is only used to access the array being iterated | | | | diff --git a/packages/eslint-plugin/ROADMAP.md b/packages/eslint-plugin/ROADMAP.md index d6f0cf393db..6ebff3867c6 100644 --- a/packages/eslint-plugin/ROADMAP.md +++ b/packages/eslint-plugin/ROADMAP.md @@ -169,7 +169,7 @@ It lists all TSLint rules along side rules from the ESLint ecosystem that are th | [`no-boolean-literal-compare`] | ✅ | [`@typescript-eslint/no-unnecessary-boolean-literal-compare`] | | [`no-consecutive-blank-lines`] | 🌟 | [`no-multiple-empty-lines`][no-multiple-empty-lines] | | [`no-irregular-whitespace`] | 🌟 | [`no-irregular-whitespace`][no-irregular-whitespace] with `skipStrings: false` | -| [`no-parameter-properties`] | ✅ | [`@typescript-eslint/no-parameter-properties`] | +| [`no-parameter-properties`] | ✅ | [`@typescript-eslint/parameter-properties`] | | [`no-redundant-jsdoc`] | 🔌 | [`jsdoc/no-types`] | | [`no-reference-import`] | ✅ | [`@typescript-eslint/triple-slash-reference`] | | [`no-trailing-whitespace`] | 🌟 | [`no-trailing-spaces`][no-trailing-spaces] | @@ -633,7 +633,7 @@ Relevant plugins: [`chai-expect-keywords`](https://github.com/gavinaiken/eslint- [`@typescript-eslint/naming-convention`]: https://typescript-eslint.io/rules/naming-convention [`@typescript-eslint/interface-name-prefix`]: https://typescript-eslint.io/rules/interface-name-prefix [`@typescript-eslint/naming-convention`]: https://typescript-eslint.io/rules/naming-convention -[`@typescript-eslint/no-parameter-properties`]: https://typescript-eslint.io/rules/no-parameter-properties +[`@typescript-eslint/parameter-properties`]: https://typescript-eslint.io/rules/parameter-properties [`@typescript-eslint/member-delimiter-style`]: https://typescript-eslint.io/rules/member-delimiter-style [`@typescript-eslint/prefer-for-of`]: https://typescript-eslint.io/rules/prefer-for-of [`@typescript-eslint/no-array-constructor`]: https://typescript-eslint.io/rules/no-array-constructor diff --git a/packages/eslint-plugin/docs/rules/README.md b/packages/eslint-plugin/docs/rules/README.md index 1145ec14aa1..3b723f22a90 100644 --- a/packages/eslint-plugin/docs/rules/README.md +++ b/packages/eslint-plugin/docs/rules/README.md @@ -59,7 +59,7 @@ slug: / | [`@typescript-eslint/no-non-null-asserted-nullish-coalescing`](./no-non-null-asserted-nullish-coalescing.md) | Disallows using a non-null assertion in the left operand of the nullish coalescing operator | | | | | [`@typescript-eslint/no-non-null-asserted-optional-chain`](./no-non-null-asserted-optional-chain.md) | Disallows using a non-null assertion after an optional chain expression | :white_check_mark: | | | | [`@typescript-eslint/no-non-null-assertion`](./no-non-null-assertion.md) | Disallows non-null assertions using the `!` postfix operator | :white_check_mark: | | | -| [`@typescript-eslint/no-parameter-properties`](./no-parameter-properties.md) | Disallow the use of parameter properties in class constructors | | | | +| [`@typescript-eslint/parameter-properties`](./parameter-properties.md) | Disallow the use of parameter properties in class constructors | | | | | [`@typescript-eslint/no-require-imports`](./no-require-imports.md) | Disallows invocation of `require()` | | | | | [`@typescript-eslint/no-this-alias`](./no-this-alias.md) | Disallow aliasing `this` | :white_check_mark: | | | | [`@typescript-eslint/no-type-alias`](./no-type-alias.md) | Disallow the use of type aliases | | | | diff --git a/packages/eslint-plugin/docs/rules/no-parameter-properties.md b/packages/eslint-plugin/docs/rules/parameter-properties.md similarity index 98% rename from packages/eslint-plugin/docs/rules/no-parameter-properties.md rename to packages/eslint-plugin/docs/rules/parameter-properties.md index e915acb1403..b746ebb51f6 100644 --- a/packages/eslint-plugin/docs/rules/no-parameter-properties.md +++ b/packages/eslint-plugin/docs/rules/parameter-properties.md @@ -1,6 +1,6 @@ -# `no-parameter-properties` +# `parameter-properties` -Disallow the use of parameter properties in class constructors. +Require or disallow the use of parameter properties in class constructors. Parameter properties can be confusing to those new to TypeScript as they are less explicit than other ways of declaring and initializing class members. diff --git a/packages/eslint-plugin/src/configs/all.ts b/packages/eslint-plugin/src/configs/all.ts index 03188b72464..c55e0e6ac37 100644 --- a/packages/eslint-plugin/src/configs/all.ts +++ b/packages/eslint-plugin/src/configs/all.ts @@ -85,7 +85,7 @@ export = { '@typescript-eslint/no-non-null-asserted-nullish-coalescing': 'error', '@typescript-eslint/no-non-null-asserted-optional-chain': 'error', '@typescript-eslint/no-non-null-assertion': 'error', - '@typescript-eslint/no-parameter-properties': 'error', + '@typescript-eslint/parameter-properties': 'error', 'no-redeclare': 'off', '@typescript-eslint/no-redeclare': 'error', '@typescript-eslint/no-redundant-type-constituents': 'error', diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts index c2f90319e38..283eeee0b76 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -58,7 +58,6 @@ import noNamespace from './no-namespace'; import noNonNullAssertedNullishCoalescing from './no-non-null-asserted-nullish-coalescing'; import noNonNullAssertedOptionalChain from './no-non-null-asserted-optional-chain'; import noNonNullAssertion from './no-non-null-assertion'; -import noParameterProperties from './no-parameter-properties'; import noRedeclare from './no-redeclare'; import noRedundantTypeConstituents from './no-redundant-type-constituents'; import noRequireImports from './no-require-imports'; @@ -87,6 +86,7 @@ import noVarRequires from './no-var-requires'; import nonNullableTypeAssertionStyle from './non-nullable-type-assertion-style'; import objectCurlySpacing from './object-curly-spacing'; import paddingLineBetweenStatements from './padding-line-between-statements'; +import parameterProperties from './parameter-properties'; import preferAsConst from './prefer-as-const'; import preferEnumInitializers from './prefer-enum-initializers'; import preferForOf from './prefer-for-of'; @@ -184,7 +184,7 @@ export default { 'no-non-null-asserted-nullish-coalescing': noNonNullAssertedNullishCoalescing, 'no-non-null-asserted-optional-chain': noNonNullAssertedOptionalChain, 'no-non-null-assertion': noNonNullAssertion, - 'no-parameter-properties': noParameterProperties, + 'parameter-properties': parameterProperties, 'no-redeclare': noRedeclare, 'no-redundant-type-constituents': noRedundantTypeConstituents, 'no-require-imports': noRequireImports, diff --git a/packages/eslint-plugin/src/rules/no-parameter-properties.ts b/packages/eslint-plugin/src/rules/no-parameter-properties.ts deleted file mode 100644 index 3ab01614e72..00000000000 --- a/packages/eslint-plugin/src/rules/no-parameter-properties.ts +++ /dev/null @@ -1,110 +0,0 @@ -import { TSESTree, AST_NODE_TYPES } from '@typescript-eslint/utils'; -import * as util from '../util'; - -type Modifier = - | 'readonly' - | 'private' - | 'protected' - | 'public' - | 'private readonly' - | 'protected readonly' - | 'public readonly'; -type Options = [ - { - allows: Modifier[]; - }, -]; -type MessageIds = 'noParamProp'; - -export default util.createRule({ - name: 'no-parameter-properties', - meta: { - type: 'problem', - docs: { - description: - 'Disallow the use of parameter properties in class constructors', - // too opinionated to be recommended - recommended: false, - }, - messages: { - noParamProp: - 'Property {{parameter}} cannot be declared in the constructor.', - }, - schema: [ - { - type: 'object', - properties: { - allows: { - type: 'array', - items: { - enum: [ - 'readonly', - 'private', - 'protected', - 'public', - 'private readonly', - 'protected readonly', - 'public readonly', - ], - }, - minItems: 1, - }, - }, - additionalProperties: false, - }, - ], - }, - defaultOptions: [ - { - allows: [], - }, - ], - create(context, [{ allows }]) { - /** - * Gets the modifiers of `node`. - * @param node the node to be inspected. - */ - function getModifiers(node: TSESTree.TSParameterProperty): Modifier { - const modifiers: Modifier[] = []; - - if (node.accessibility) { - modifiers.push(node.accessibility); - } - if (node.readonly) { - modifiers.push('readonly'); - } - - return modifiers.filter(Boolean).join(' ') as Modifier; - } - - return { - TSParameterProperty(node): void { - const modifiers = getModifiers(node); - - if (!allows.includes(modifiers)) { - // HAS to be an identifier or assignment or TSC will throw - if ( - node.parameter.type !== AST_NODE_TYPES.Identifier && - node.parameter.type !== AST_NODE_TYPES.AssignmentPattern - ) { - return; - } - - const name = - node.parameter.type === AST_NODE_TYPES.Identifier - ? node.parameter.name - : // has to be an Identifier or TSC will throw an error - (node.parameter.left as TSESTree.Identifier).name; - - context.report({ - node, - messageId: 'noParamProp', - data: { - parameter: name, - }, - }); - } - }, - }; - }, -}); diff --git a/packages/eslint-plugin/src/rules/parameter-properties.ts b/packages/eslint-plugin/src/rules/parameter-properties.ts new file mode 100644 index 00000000000..db623c92480 --- /dev/null +++ b/packages/eslint-plugin/src/rules/parameter-properties.ts @@ -0,0 +1,212 @@ +import { TSESTree, AST_NODE_TYPES } from '@typescript-eslint/utils'; +import * as util from '../util'; + +type Modifier = + | 'readonly' + | 'private' + | 'protected' + | 'public' + | 'private readonly' + | 'protected readonly' + | 'public readonly'; + +type Prefer = 'class-property' | 'parameter-property'; + +type Options = [ + { + allow?: Modifier[]; + prefer?: Prefer; + }, +]; + +type MessageIds = 'preferClassProperty' | 'preferParameterProperty'; + +export default util.createRule({ + name: 'parameter-properties', + meta: { + type: 'problem', + docs: { + description: + 'Require or disallow the use of parameter properties in class constructors', + recommended: false, + }, + messages: { + preferClassProperty: + 'Property {{parameter}} should be declared as a class property.', + preferParameterProperty: + 'Property {{parameter}} should be declared as a parameter property.', + }, + schema: [ + { + type: 'object', + properties: { + allow: { + type: 'array', + items: { + enum: [ + 'readonly', + 'private', + 'protected', + 'public', + 'private readonly', + 'protected readonly', + 'public readonly', + ], + }, + minItems: 1, + }, + prefer: { + enum: ['class-property', 'parameter-property'], + }, + }, + additionalProperties: false, + }, + ], + }, + defaultOptions: [ + { + allow: [], + prefer: 'class-property', + }, + ], + create(context, [{ allow = [], prefer = 'class-property' }]) { + /** + * Gets the modifiers of `node`. + * @param node the node to be inspected. + */ + function getModifiers(node: TSESTree.TSParameterProperty): Modifier { + const modifiers: Modifier[] = []; + + if (node.accessibility) { + modifiers.push(node.accessibility); + } + if (node.readonly) { + modifiers.push('readonly'); + } + + return modifiers.filter(Boolean).join(' ') as Modifier; + } + + if (prefer === 'class-property') { + return { + TSParameterProperty(node): void { + const modifiers = getModifiers(node); + + if (!allow.includes(modifiers)) { + // HAS to be an identifier or assignment or TSC will throw + if ( + node.parameter.type !== AST_NODE_TYPES.Identifier && + node.parameter.type !== AST_NODE_TYPES.AssignmentPattern + ) { + return; + } + + const name = + node.parameter.type === AST_NODE_TYPES.Identifier + ? node.parameter.name + : // has to be an Identifier or TSC will throw an error + (node.parameter.left as TSESTree.Identifier).name; + + context.report({ + node, + messageId: 'preferClassProperty', + data: { + parameter: name, + }, + }); + } + }, + }; + } + + interface PropertyNodes { + classProperty?: TSESTree.PropertyDefinition; + constructorAssignment?: TSESTree.AssignmentExpression; + constructorParameter?: TSESTree.Identifier; + } + + const propertyNodesByNameStack: Map[] = []; + + function getNodesByName(name: string): PropertyNodes { + const propertyNodesByName = propertyNodesByNameStack.at(-1)!; + const existing = propertyNodesByName.get(name); + if (existing) { + return existing; + } + + const created: PropertyNodes = {}; + propertyNodesByName.set(name, created); + return created; + } + + return { + 'ClassDeclaration, ClassExpression'(): void { + propertyNodesByNameStack.push(new Map()); + }, + + ':matches(ClassDeclaration, ClassExpression):exit'(): void { + const propertyNodesByName = propertyNodesByNameStack.pop()!; + + for (const [name, nodes] of propertyNodesByName) { + if ( + nodes.classProperty && + nodes.constructorAssignment && + nodes.constructorParameter + ) { + context.report({ + data: { + parameter: name, + }, + messageId: 'preferParameterProperty', + node: nodes.classProperty, + }); + } + } + }, + + ClassBody(node): void { + for (const element of node.body) { + if ( + element.type === AST_NODE_TYPES.PropertyDefinition && + element.key.type === AST_NODE_TYPES.Identifier + ) { + getNodesByName(element.key.name).classProperty = element; + } + } + }, + + 'MethodDefinition[kind="constructor"]'( + node: TSESTree.MethodDefinition, + ): void { + for (const parameter of node.value.params) { + if ( + parameter.type === AST_NODE_TYPES.Identifier && + parameter.typeAnnotation + ) { + getNodesByName(parameter.name).constructorParameter = parameter; + } + } + + for (const statement of node.value.body?.body ?? []) { + if ( + statement.type !== AST_NODE_TYPES.ExpressionStatement || + statement.expression.type !== AST_NODE_TYPES.AssignmentExpression || + statement.expression.left.type !== + AST_NODE_TYPES.MemberExpression || + statement.expression.left.object.type !== + AST_NODE_TYPES.ThisExpression || + statement.expression.left.property.type !== + AST_NODE_TYPES.Identifier || + statement.expression.right.type !== AST_NODE_TYPES.Identifier + ) { + break; + } + + getNodesByName( + statement.expression.right.name, + ).constructorAssignment = statement.expression; + } + }, + }; + }, +}); diff --git a/packages/eslint-plugin/tests/rules/no-parameter-properties.test.ts b/packages/eslint-plugin/tests/rules/parameter-properties.test.ts similarity index 78% rename from packages/eslint-plugin/tests/rules/no-parameter-properties.test.ts rename to packages/eslint-plugin/tests/rules/parameter-properties.test.ts index f4f7fb03d2d..55afcdd086a 100644 --- a/packages/eslint-plugin/tests/rules/no-parameter-properties.test.ts +++ b/packages/eslint-plugin/tests/rules/parameter-properties.test.ts @@ -1,11 +1,11 @@ -import rule from '../../src/rules/no-parameter-properties'; +import rule from '../../src/rules/parameter-properties'; import { RuleTester } from '../RuleTester'; const ruleTester = new RuleTester({ parser: '@typescript-eslint/parser', }); -ruleTester.run('no-parameter-properties', rule, { +ruleTester.run('parameter-properties', rule, { valid: [ ` class Foo { @@ -34,7 +34,7 @@ class Foo { constructor(readonly name: string) {} } `, - options: [{ allows: ['readonly'] }], + options: [{ allow: ['readonly'] }], }, { code: ` @@ -42,7 +42,7 @@ class Foo { constructor(private name: string) {} } `, - options: [{ allows: ['private'] }], + options: [{ allow: ['private'] }], }, { code: ` @@ -50,7 +50,7 @@ class Foo { constructor(protected name: string) {} } `, - options: [{ allows: ['protected'] }], + options: [{ allow: ['protected'] }], }, { code: ` @@ -58,7 +58,7 @@ class Foo { constructor(public name: string) {} } `, - options: [{ allows: ['public'] }], + options: [{ allow: ['public'] }], }, { code: ` @@ -66,7 +66,7 @@ class Foo { constructor(private readonly name: string) {} } `, - options: [{ allows: ['private readonly'] }], + options: [{ allow: ['private readonly'] }], }, { code: ` @@ -74,7 +74,7 @@ class Foo { constructor(protected readonly name: string) {} } `, - options: [{ allows: ['protected readonly'] }], + options: [{ allow: ['protected readonly'] }], }, { code: ` @@ -82,7 +82,7 @@ class Foo { constructor(public readonly name: string) {} } `, - options: [{ allows: ['public readonly'] }], + options: [{ allow: ['public readonly'] }], }, { code: ` @@ -90,7 +90,7 @@ class Foo { constructor(readonly name: string, private age: number) {} } `, - options: [{ allows: ['readonly', 'private'] }], + options: [{ allow: ['readonly', 'private'] }], }, { code: ` @@ -98,7 +98,7 @@ class Foo { constructor(public readonly name: string, private age: number) {} } `, - options: [{ allows: ['public readonly', 'private'] }], + options: [{ allow: ['public readonly', 'private'] }], }, // Semantically invalid test case ` @@ -122,7 +122,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -139,7 +139,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -156,7 +156,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -173,7 +173,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -190,7 +190,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -207,7 +207,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -224,7 +224,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -241,7 +241,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -258,7 +258,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -266,7 +266,7 @@ class Foo { column: 15, }, { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'age', }, @@ -283,7 +283,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -291,7 +291,7 @@ class Foo { column: 15, }, { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'age', }, @@ -308,7 +308,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -316,7 +316,7 @@ class Foo { column: 15, }, { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'age', }, @@ -334,7 +334,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -352,7 +352,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -360,7 +360,7 @@ class Foo { column: 15, }, { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -378,7 +378,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -386,7 +386,7 @@ class Foo { column: 15, }, { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -394,7 +394,7 @@ class Foo { column: 15, }, { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'age', }, @@ -412,7 +412,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -430,7 +430,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -438,7 +438,7 @@ class Foo { column: 15, }, { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -456,7 +456,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -464,7 +464,7 @@ class Foo { column: 15, }, { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -472,7 +472,7 @@ class Foo { column: 15, }, { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'age', }, @@ -490,7 +490,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -508,7 +508,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -516,7 +516,7 @@ class Foo { column: 15, }, { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -534,7 +534,7 @@ class Foo { `, errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -542,7 +542,7 @@ class Foo { column: 15, }, { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -550,7 +550,7 @@ class Foo { column: 15, }, { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'age', }, @@ -566,10 +566,10 @@ class Foo { constructor(readonly name: string) {} } `, - options: [{ allows: ['private'] }], + options: [{ allow: ['private'] }], errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -584,10 +584,10 @@ class Foo { constructor(private name: string) {} } `, - options: [{ allows: ['readonly'] }], + options: [{ allow: ['readonly'] }], errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -604,12 +604,12 @@ class Foo { `, options: [ { - allows: ['readonly', 'private', 'public', 'protected readonly'], + allow: ['readonly', 'private', 'public', 'protected readonly'], }, ], errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -626,7 +626,7 @@ class Foo { `, options: [ { - allows: [ + allow: [ 'readonly', 'private', 'protected', @@ -637,7 +637,7 @@ class Foo { ], errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -652,10 +652,10 @@ class Foo { constructor(private readonly name: string) {} } `, - options: [{ allows: ['readonly', 'private'] }], + options: [{ allow: ['readonly', 'private'] }], errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -672,7 +672,7 @@ class Foo { `, options: [ { - allows: [ + allow: [ 'readonly', 'protected', 'private readonly', @@ -682,7 +682,7 @@ class Foo { ], errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'name', }, @@ -698,10 +698,10 @@ class Foo { constructor(private name: string, protected age?: number) {} } `, - options: [{ allows: ['private'] }], + options: [{ allow: ['private'] }], errors: [ { - messageId: 'noParamProp', + messageId: 'preferClassProperty', data: { parameter: 'age', }, @@ -710,5 +710,28 @@ class Foo { }, ], }, + { + code: ` +class Foo { + member: string; + + constructor(member: string) { + this.member = member; + } +} + `, + errors: [ + { + messageId: 'preferParameterProperty', + data: { + parameter: 'member', + }, + line: 3, + column: 3, + }, + ], + only: true, + options: [{ prefer: 'parameter-property' }], + }, ], }); From 1ac6b87ec74fab4bf91af0d56fa5d4683a1698e1 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Thu, 3 Mar 2022 07:15:48 -0500 Subject: [PATCH 2/9] test: add test cases --- packages/eslint-plugin/src/rules/index.ts | 2 +- .../src/rules/parameter-properties.ts | 43 +++- .../tests/rules/parameter-properties.test.ts | 234 +++++++++++++++++- 3 files changed, 269 insertions(+), 10 deletions(-) diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts index 283eeee0b76..14351ac0284 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -184,7 +184,6 @@ export default { 'no-non-null-asserted-nullish-coalescing': noNonNullAssertedNullishCoalescing, 'no-non-null-asserted-optional-chain': noNonNullAssertedOptionalChain, 'no-non-null-assertion': noNonNullAssertion, - 'parameter-properties': parameterProperties, 'no-redeclare': noRedeclare, 'no-redundant-type-constituents': noRedundantTypeConstituents, 'no-require-imports': noRequireImports, @@ -213,6 +212,7 @@ export default { 'non-nullable-type-assertion-style': nonNullableTypeAssertionStyle, 'object-curly-spacing': objectCurlySpacing, 'padding-line-between-statements': paddingLineBetweenStatements, + 'parameter-properties': parameterProperties, 'prefer-as-const': preferAsConst, 'prefer-enum-initializers': preferEnumInitializers, 'prefer-for-of': preferForOf, diff --git a/packages/eslint-plugin/src/rules/parameter-properties.ts b/packages/eslint-plugin/src/rules/parameter-properties.ts index db623c92480..4e3532a2fc7 100644 --- a/packages/eslint-plugin/src/rules/parameter-properties.ts +++ b/packages/eslint-plugin/src/rules/parameter-properties.ts @@ -74,7 +74,9 @@ export default util.createRule({ * Gets the modifiers of `node`. * @param node the node to be inspected. */ - function getModifiers(node: TSESTree.TSParameterProperty): Modifier { + function getModifiers( + node: TSESTree.PropertyDefinition | TSESTree.TSParameterProperty, + ): Modifier { const modifiers: Modifier[] = []; if (node.accessibility) { @@ -128,7 +130,8 @@ export default util.createRule({ const propertyNodesByNameStack: Map[] = []; function getNodesByName(name: string): PropertyNodes { - const propertyNodesByName = propertyNodesByNameStack.at(-1)!; + const propertyNodesByName = + propertyNodesByNameStack[propertyNodesByNameStack.length - 1]; const existing = propertyNodesByName.get(name); if (existing) { return existing; @@ -139,6 +142,27 @@ export default util.createRule({ return created; } + const sourceCode = context.getSourceCode(); + + function typeAnnotationsMatch( + classProperty: TSESTree.PropertyDefinition, + constructorParameter: TSESTree.Identifier, + ): boolean { + if ( + !classProperty.typeAnnotation || + !constructorParameter.typeAnnotation + ) { + return ( + classProperty.typeAnnotation === constructorParameter.typeAnnotation + ); + } + + return ( + sourceCode.getText(classProperty.typeAnnotation) === + sourceCode.getText(constructorParameter.typeAnnotation) + ); + } + return { 'ClassDeclaration, ClassExpression'(): void { propertyNodesByNameStack.push(new Map()); @@ -151,7 +175,11 @@ export default util.createRule({ if ( nodes.classProperty && nodes.constructorAssignment && - nodes.constructorParameter + nodes.constructorParameter && + typeAnnotationsMatch( + nodes.classProperty, + nodes.constructorParameter, + ) ) { context.report({ data: { @@ -168,7 +196,9 @@ export default util.createRule({ for (const element of node.body) { if ( element.type === AST_NODE_TYPES.PropertyDefinition && - element.key.type === AST_NODE_TYPES.Identifier + element.key.type === AST_NODE_TYPES.Identifier && + !element.value && + !allow.includes(getModifiers(element)) ) { getNodesByName(element.key.name).classProperty = element; } @@ -179,10 +209,7 @@ export default util.createRule({ node: TSESTree.MethodDefinition, ): void { for (const parameter of node.value.params) { - if ( - parameter.type === AST_NODE_TYPES.Identifier && - parameter.typeAnnotation - ) { + if (parameter.type === AST_NODE_TYPES.Identifier) { getNodesByName(parameter.name).constructorParameter = parameter; } } diff --git a/packages/eslint-plugin/tests/rules/parameter-properties.test.ts b/packages/eslint-plugin/tests/rules/parameter-properties.test.ts index 55afcdd086a..d30e746a319 100644 --- a/packages/eslint-plugin/tests/rules/parameter-properties.test.ts +++ b/packages/eslint-plugin/tests/rules/parameter-properties.test.ts @@ -12,6 +12,14 @@ class Foo { constructor(name: string) {} } `, + { + code: ` +class Foo { + constructor(name: string) {} +} + `, + options: [{ prefer: 'class-property' }], + }, ` class Foo { constructor(...name: string[]) {} @@ -112,6 +120,162 @@ class Foo { constructor(private [test]: [string]) {} } `, + { + code: ` +class Foo { + constructor(private name: string[]) {} +} + `, + options: [{ prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + constructor(...name: string[]) {} +} + `, + options: [{ prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + constructor(age: string, ...name: string[]) {} +} + `, + options: [{ prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + constructor(private age: string, ...name: string[]) {} +} + `, + options: [{ prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + public age: number; + constructor(age: string) { + this.age = age; + } +} + `, + options: [{ prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + public age = ''; + constructor(age: string) { + this.age = age; + } +} + `, + options: [{ prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + public age; + constructor(age: string) { + this.age = age; + } +} + `, + options: [{ prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + public age: string; + constructor(age) { + this.age = age; + } +} + `, + options: [{ prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + public age: string; + constructor(age: string) { + console.log('unrelated'); + this.age = age; + } +} + `, + options: [{ prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + public age: string; + constructor(age: string) { + this.age = age; + } +} + `, + options: [{ allow: ['public'], prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + public readonly age: string; + constructor(age: string) { + this.age = age; + } +} + `, + options: [{ allow: ['public readonly'], prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + protected age: string; + constructor(age: string) { + this.age = age; + } +} + `, + options: [{ allow: ['protected'], prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + protected readonly age: string; + constructor(age: string) { + this.age = age; + } +} + `, + options: [ + { allow: ['protected readonly'], prefer: 'parameter-property' }, + ], + }, + { + code: ` +class Foo { + private age: string; + constructor(age: string) { + this.age = age; + } +} + `, + options: [{ allow: ['private'], prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + private readonly age: string; + constructor(age: string) { + this.age = age; + } +} + `, + options: [{ allow: ['private readonly'], prefer: 'parameter-property' }], + }, ], invalid: [ { @@ -730,8 +894,76 @@ class Foo { column: 3, }, ], - only: true, options: [{ prefer: 'parameter-property' }], }, + { + code: ` +class Foo { + constructor(member: string) { + this.member = member; + } + + member: string; +} + `, + errors: [ + { + messageId: 'preferParameterProperty', + data: { + parameter: 'member', + }, + line: 7, + column: 3, + }, + ], + options: [{ prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + member; + constructor(member) { + this.member = member; + } +} + `, + errors: [ + { + messageId: 'preferParameterProperty', + data: { + parameter: 'member', + }, + line: 3, + column: 3, + }, + ], + options: [{ prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + public member: string; + constructor(member: string) { + this.member = member; + } +} + `, + errors: [ + { + messageId: 'preferParameterProperty', + data: { + parameter: 'member', + }, + line: 3, + column: 3, + }, + ], + options: [ + { + allow: ['protected', 'private', 'readonly'], + prefer: 'parameter-property', + }, + ], + }, ], }); From 19f2e76f6f89e326148b2b33ea1773090e9bc5f1 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 5 Mar 2022 13:05:37 -0500 Subject: [PATCH 3/9] chore: docs and a bit more testing --- .../docs/rules/parameter-properties.md | 87 ++++++++++++++++++- .../tests/rules/parameter-properties.test.ts | 36 ++++++++ 2 files changed, 122 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin/docs/rules/parameter-properties.md b/packages/eslint-plugin/docs/rules/parameter-properties.md index b746ebb51f6..642b0e93391 100644 --- a/packages/eslint-plugin/docs/rules/parameter-properties.md +++ b/packages/eslint-plugin/docs/rules/parameter-properties.md @@ -13,7 +13,14 @@ declare all properties in the class. ## Options This rule, in its default state, does not require any argument and would completely disallow the use of parameter properties. -If you would like to allow certain types of parameter properties then you may pass an object with the following options: +It may take an options object containing either or both of: + +- `"allows"`: allowing certain kinds of properties to be ignored +- `"prefer"`: either `"class-properties"` _(default)_ or `"parameter-properties"` + +### `"allows"` + +If you would like to ignore certain kinds of properties then you may pass an object containing `"allows"` as an array of any of the following options: - `allows`, an array containing one or more of the allowed modifiers. Valid values are: - `readonly`, allows **readonly** parameter properties. @@ -24,6 +31,27 @@ If you would like to allow certain types of parameter properties then you may pa - `protected readonly`, allows **protected readonly** parameter properties. - `public readonly`, allows **public readonly** parameter properties. +For example, to ignore `public` properties: + +```json +{ + "@typescript-eslint/parameter-properties": [ + true, + { + "allow": ["public"] + } + ] +} +``` + +### `"prefer"` + +By default, the rule prefers class properties (`"class-properties"`). +You can switch it to instead preferring parameter properties when: + +- A class property and constructor parameter have the same name and type +- The constructor parameter is assigned to the class property at the beginning of the constructor + ### default Examples of code for this rule with no options at all: @@ -392,6 +420,63 @@ class Foo { } ``` +### `"parameter-properties"` + +Examples of code for the `{ "prefer": ["parameter-properties"] }` option: + + + +#### ❌ Incorrect + +```ts +class Foo { + private name: string; + constructor(name: string) { + this.name = name; + } +} + +class Foo { + public readonly name: string; + constructor(name: string) { + this.name = name; + } +} + +class Foo { + constructor(name: string) { + this.name = name; + } + name: string; +} +``` + +#### ✅ Correct + +```ts +class Foo { + private differentName: string; + constructor(name: string) { + this.differentName = name; + } +} + +class Foo { + private differentType: number | undefined; + constructor(differentType: number) { + this.differentType = differentType; + } +} + +class Foo { + protected logicInConstructor: string; + constructor(logicInConstructor: string) { + console.log('Hello, world!'); + this.logicInConstructor = logicInConstructor; + } +} +``` + ## When Not To Use It If you don't care about the using parameter properties in constructors, then you will not need this rule. diff --git a/packages/eslint-plugin/tests/rules/parameter-properties.test.ts b/packages/eslint-plugin/tests/rules/parameter-properties.test.ts index d30e746a319..44580ad1b92 100644 --- a/packages/eslint-plugin/tests/rules/parameter-properties.test.ts +++ b/packages/eslint-plugin/tests/rules/parameter-properties.test.ts @@ -210,6 +210,42 @@ class Foo { }, { code: ` +class Foo { + other: string; + constructor(age: string) { + this.other = age; + } +} + `, + options: [{ prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + age: string; + constructor(age: string) { + this.age = ''; + console.log(age); + } +} + `, + options: [{ prefer: 'parameter-property' }], + }, + { + code: ` +class Foo { + age() { + return ''; + } + constructor(age: string) { + this.age = age; + } +} + `, + options: [{ prefer: 'parameter-property' }], + }, + { + code: ` class Foo { public age: string; constructor(age: string) { From e7bba70cb7967e3a0e923a9d3f26586561e28add Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 5 Mar 2022 13:08:13 -0500 Subject: [PATCH 4/9] docs: reword --- packages/eslint-plugin/docs/rules/parameter-properties.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin/docs/rules/parameter-properties.md b/packages/eslint-plugin/docs/rules/parameter-properties.md index 642b0e93391..0974300bce3 100644 --- a/packages/eslint-plugin/docs/rules/parameter-properties.md +++ b/packages/eslint-plugin/docs/rules/parameter-properties.md @@ -47,7 +47,9 @@ For example, to ignore `public` properties: ### `"prefer"` By default, the rule prefers class properties (`"class-properties"`). -You can switch it to instead preferring parameter properties when: +You can switch it to instead preferring parameter properties with (`"parameter-properties"`). + +In `"parameter-properties"` mode, the rule will issue a report when: - A class property and constructor parameter have the same name and type - The constructor parameter is assigned to the class property at the beginning of the constructor From 698b760f9a95892ef17d36e77858eb25f35f97ee Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 26 Mar 2022 11:43:08 -0500 Subject: [PATCH 5/9] chore: added back old rule as deprecated --- .../src/rules/no-parameter-properties.ts | 111 +++ .../rules/no-parameter-properties.test.ts | 714 ++++++++++++++++++ 2 files changed, 825 insertions(+) create mode 100644 packages/eslint-plugin/src/rules/no-parameter-properties.ts create mode 100644 packages/eslint-plugin/tests/rules/no-parameter-properties.test.ts diff --git a/packages/eslint-plugin/src/rules/no-parameter-properties.ts b/packages/eslint-plugin/src/rules/no-parameter-properties.ts new file mode 100644 index 00000000000..d7abe9ed159 --- /dev/null +++ b/packages/eslint-plugin/src/rules/no-parameter-properties.ts @@ -0,0 +1,111 @@ +import { TSESTree, AST_NODE_TYPES } from '@typescript-eslint/utils'; +import * as util from '../util'; + +type Modifier = + | 'readonly' + | 'private' + | 'protected' + | 'public' + | 'private readonly' + | 'protected readonly' + | 'public readonly'; +type Options = [ + { + allows: Modifier[]; + }, +]; +type MessageIds = 'noParamProp'; + +export default util.createRule({ + name: 'no-parameter-properties', + meta: { + deprecated: true, + type: 'problem', + docs: { + description: + 'Disallow the use of parameter properties in class constructors', + // too opinionated to be recommended + recommended: false, + }, + messages: { + noParamProp: + 'Property {{parameter}} cannot be declared in the constructor.', + }, + schema: [ + { + type: 'object', + properties: { + allows: { + type: 'array', + items: { + enum: [ + 'readonly', + 'private', + 'protected', + 'public', + 'private readonly', + 'protected readonly', + 'public readonly', + ], + }, + minItems: 1, + }, + }, + additionalProperties: false, + }, + ], + }, + defaultOptions: [ + { + allows: [], + }, + ], + create(context, [{ allows }]) { + /** + * Gets the modifiers of `node`. + * @param node the node to be inspected. + */ + function getModifiers(node: TSESTree.TSParameterProperty): Modifier { + const modifiers: Modifier[] = []; + + if (node.accessibility) { + modifiers.push(node.accessibility); + } + if (node.readonly) { + modifiers.push('readonly'); + } + + return modifiers.filter(Boolean).join(' ') as Modifier; + } + + return { + TSParameterProperty(node): void { + const modifiers = getModifiers(node); + + if (!allows.includes(modifiers)) { + // HAS to be an identifier or assignment or TSC will throw + if ( + node.parameter.type !== AST_NODE_TYPES.Identifier && + node.parameter.type !== AST_NODE_TYPES.AssignmentPattern + ) { + return; + } + + const name = + node.parameter.type === AST_NODE_TYPES.Identifier + ? node.parameter.name + : // has to be an Identifier or TSC will throw an error + (node.parameter.left as TSESTree.Identifier).name; + + context.report({ + node, + messageId: 'noParamProp', + data: { + parameter: name, + }, + }); + } + }, + }; + }, +}); diff --git a/packages/eslint-plugin/tests/rules/no-parameter-properties.test.ts b/packages/eslint-plugin/tests/rules/no-parameter-properties.test.ts new file mode 100644 index 00000000000..f4f7fb03d2d --- /dev/null +++ b/packages/eslint-plugin/tests/rules/no-parameter-properties.test.ts @@ -0,0 +1,714 @@ +import rule from '../../src/rules/no-parameter-properties'; +import { RuleTester } from '../RuleTester'; + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', +}); + +ruleTester.run('no-parameter-properties', rule, { + valid: [ + ` +class Foo { + constructor(name: string) {} +} + `, + ` +class Foo { + constructor(...name: string[]) {} +} + `, + ` +class Foo { + constructor(name: string, age: number) {} +} + `, + ` +class Foo { + constructor(name: string); + constructor(name: string, age?: number) {} +} + `, + { + code: ` +class Foo { + constructor(readonly name: string) {} +} + `, + options: [{ allows: ['readonly'] }], + }, + { + code: ` +class Foo { + constructor(private name: string) {} +} + `, + options: [{ allows: ['private'] }], + }, + { + code: ` +class Foo { + constructor(protected name: string) {} +} + `, + options: [{ allows: ['protected'] }], + }, + { + code: ` +class Foo { + constructor(public name: string) {} +} + `, + options: [{ allows: ['public'] }], + }, + { + code: ` +class Foo { + constructor(private readonly name: string) {} +} + `, + options: [{ allows: ['private readonly'] }], + }, + { + code: ` +class Foo { + constructor(protected readonly name: string) {} +} + `, + options: [{ allows: ['protected readonly'] }], + }, + { + code: ` +class Foo { + constructor(public readonly name: string) {} +} + `, + options: [{ allows: ['public readonly'] }], + }, + { + code: ` +class Foo { + constructor(readonly name: string, private age: number) {} +} + `, + options: [{ allows: ['readonly', 'private'] }], + }, + { + code: ` +class Foo { + constructor(public readonly name: string, private age: number) {} +} + `, + options: [{ allows: ['public readonly', 'private'] }], + }, + // Semantically invalid test case + ` +class Foo { + constructor(private ...name: string[]) {} +} + `, + // Semantically invalid test case + ` +class Foo { + constructor(private [test]: [string]) {} +} + `, + ], + invalid: [ + { + code: ` +class Foo { + constructor(readonly name: string) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(private name: string) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(protected name: string) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(public name: string) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(private readonly name: string) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(protected readonly name: string) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(public readonly name: string) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(public name: string, age: number) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(private name: string, private age: number) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + { + messageId: 'noParamProp', + data: { + parameter: 'age', + }, + line: 3, + column: 37, + }, + ], + }, + { + code: ` +class Foo { + constructor(protected name: string, protected age: number) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + { + messageId: 'noParamProp', + data: { + parameter: 'age', + }, + line: 3, + column: 39, + }, + ], + }, + { + code: ` +class Foo { + constructor(public name: string, public age: number) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + { + messageId: 'noParamProp', + data: { + parameter: 'age', + }, + line: 3, + column: 36, + }, + ], + }, + { + code: ` +class Foo { + constructor(name: string); + constructor(private name: string, age?: number) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 4, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(private name: string); + constructor(private name: string, age?: number) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 4, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(private name: string); + constructor(private name: string, private age?: number) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 4, + column: 15, + }, + { + messageId: 'noParamProp', + data: { + parameter: 'age', + }, + line: 4, + column: 37, + }, + ], + }, + { + code: ` +class Foo { + constructor(name: string); + constructor(protected name: string, age?: number) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 4, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(protected name: string); + constructor(protected name: string, age?: number) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 4, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(protected name: string); + constructor(protected name: string, protected age?: number) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 4, + column: 15, + }, + { + messageId: 'noParamProp', + data: { + parameter: 'age', + }, + line: 4, + column: 39, + }, + ], + }, + { + code: ` +class Foo { + constructor(name: string); + constructor(public name: string, age?: number) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 4, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(public name: string); + constructor(public name: string, age?: number) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 4, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(public name: string); + constructor(public name: string, public age?: number) {} +} + `, + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 4, + column: 15, + }, + { + messageId: 'noParamProp', + data: { + parameter: 'age', + }, + line: 4, + column: 36, + }, + ], + }, + + { + code: ` +class Foo { + constructor(readonly name: string) {} +} + `, + options: [{ allows: ['private'] }], + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(private name: string) {} +} + `, + options: [{ allows: ['readonly'] }], + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(protected name: string) {} +} + `, + options: [ + { + allows: ['readonly', 'private', 'public', 'protected readonly'], + }, + ], + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(public name: string) {} +} + `, + options: [ + { + allows: [ + 'readonly', + 'private', + 'protected', + 'protected readonly', + 'public readonly', + ], + }, + ], + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(private readonly name: string) {} +} + `, + options: [{ allows: ['readonly', 'private'] }], + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(protected readonly name: string) {} +} + `, + options: [ + { + allows: [ + 'readonly', + 'protected', + 'private readonly', + 'public readonly', + ], + }, + ], + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'name', + }, + line: 3, + column: 15, + }, + ], + }, + { + code: ` +class Foo { + constructor(private name: string); + constructor(private name: string, protected age?: number) {} +} + `, + options: [{ allows: ['private'] }], + errors: [ + { + messageId: 'noParamProp', + data: { + parameter: 'age', + }, + line: 4, + column: 37, + }, + ], + }, + ], +}); From 295fa9aba0082014ed2f40c3594371a1c2254f0f Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 26 Mar 2022 12:07:20 -0500 Subject: [PATCH 6/9] chore: add rule back to index.ts --- packages/eslint-plugin/src/rules/index.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts index 14351ac0284..bdb80d580b0 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -60,6 +60,7 @@ import noNonNullAssertedOptionalChain from './no-non-null-asserted-optional-chai import noNonNullAssertion from './no-non-null-assertion'; import noRedeclare from './no-redeclare'; import noRedundantTypeConstituents from './no-redundant-type-constituents'; +import noParameterProperties from './no-parameter-properties'; import noRequireImports from './no-require-imports'; import noRestrictedImports from './no-restricted-imports'; import noShadow from './no-shadow'; @@ -184,6 +185,7 @@ export default { 'no-non-null-asserted-nullish-coalescing': noNonNullAssertedNullishCoalescing, 'no-non-null-asserted-optional-chain': noNonNullAssertedOptionalChain, 'no-non-null-assertion': noNonNullAssertion, + 'no-parameter-properties': noParameterProperties, 'no-redeclare': noRedeclare, 'no-redundant-type-constituents': noRedundantTypeConstituents, 'no-require-imports': noRequireImports, From 4f3409aab64d497a9f07a31defb599b718c06460 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 26 Mar 2022 12:36:15 -0500 Subject: [PATCH 7/9] chore: add rule md, and replacedBy --- .../docs/rules/no-parameter-properties.md | 407 ++++++++++++++++++ .../src/rules/no-parameter-properties.ts | 1 + 2 files changed, 408 insertions(+) create mode 100644 packages/eslint-plugin/docs/rules/no-parameter-properties.md diff --git a/packages/eslint-plugin/docs/rules/no-parameter-properties.md b/packages/eslint-plugin/docs/rules/no-parameter-properties.md new file mode 100644 index 00000000000..e915acb1403 --- /dev/null +++ b/packages/eslint-plugin/docs/rules/no-parameter-properties.md @@ -0,0 +1,407 @@ +# `no-parameter-properties` + +Disallow the use of parameter properties in class constructors. + +Parameter properties can be confusing to those new to TypeScript as they are less explicit than other ways +of declaring and initializing class members. + +## Rule Details + +This rule disallows the use of parameter properties in constructors, forcing the user to explicitly +declare all properties in the class. + +## Options + +This rule, in its default state, does not require any argument and would completely disallow the use of parameter properties. +If you would like to allow certain types of parameter properties then you may pass an object with the following options: + +- `allows`, an array containing one or more of the allowed modifiers. Valid values are: + - `readonly`, allows **readonly** parameter properties. + - `private`, allows **private** parameter properties. + - `protected`, allows **protected** parameter properties. + - `public`, allows **public** parameter properties. + - `private readonly`, allows **private readonly** parameter properties. + - `protected readonly`, allows **protected readonly** parameter properties. + - `public readonly`, allows **public readonly** parameter properties. + +### default + +Examples of code for this rule with no options at all: + + + +#### ❌ Incorrect + +```ts +class Foo { + constructor(readonly name: string) {} +} + +class Foo { + constructor(private name: string) {} +} + +class Foo { + constructor(protected name: string) {} +} + +class Foo { + constructor(public name: string) {} +} + +class Foo { + constructor(private readonly name: string) {} +} + +class Foo { + constructor(protected readonly name: string) {} +} + +class Foo { + constructor(public readonly name: string) {} +} +``` + +#### ✅ Correct + +```ts +class Foo { + constructor(name: string) {} +} +``` + +### readonly + +Examples of code for the `{ "allows": ["readonly"] }` options: + + + +#### ❌ Incorrect + +```ts +class Foo { + constructor(private name: string) {} +} + +class Foo { + constructor(protected name: string) {} +} + +class Foo { + constructor(public name: string) {} +} + +class Foo { + constructor(private readonly name: string) {} +} + +class Foo { + constructor(protected readonly name: string) {} +} + +class Foo { + constructor(public readonly name: string) {} +} +``` + +#### ✅ Correct + +```ts +class Foo { + constructor(name: string) {} +} + +class Foo { + constructor(readonly name: string) {} +} +``` + +### private + +Examples of code for the `{ "allows": ["private"] }` options: + + + +#### ❌ Incorrect + +```ts +class Foo { + constructor(readonly name: string) {} +} + +class Foo { + constructor(protected name: string) {} +} + +class Foo { + constructor(public name: string) {} +} + +class Foo { + constructor(private readonly name: string) {} +} + +class Foo { + constructor(protected readonly name: string) {} +} + +class Foo { + constructor(public readonly name: string) {} +} +``` + +#### ✅ Correct + +```ts +class Foo { + constructor(name: string) {} +} + +class Foo { + constructor(private name: string) {} +} +``` + +### protected + +Examples of code for the `{ "allows": ["protected"] }` options: + + + +#### ❌ Incorrect + +```ts +class Foo { + constructor(readonly name: string) {} +} + +class Foo { + constructor(private name: string) {} +} + +class Foo { + constructor(public name: string) {} +} + +class Foo { + constructor(private readonly name: string) {} +} + +class Foo { + constructor(protected readonly name: string) {} +} + +class Foo { + constructor(public readonly name: string) {} +} +``` + +#### ✅ Correct + +```ts +class Foo { + constructor(name: string) {} +} + +class Foo { + constructor(protected name: string) {} +} +``` + +### public + +Examples of code for the `{ "allows": ["public"] }` options: + + + +#### ❌ Incorrect + +```ts +class Foo { + constructor(readonly name: string) {} +} + +class Foo { + constructor(private name: string) {} +} + +class Foo { + constructor(protected name: string) {} +} + +class Foo { + constructor(private readonly name: string) {} +} + +class Foo { + constructor(protected readonly name: string) {} +} + +class Foo { + constructor(public readonly name: string) {} +} +``` + +#### ✅ Correct + +```ts +class Foo { + constructor(name: string) {} +} + +class Foo { + constructor(public name: string) {} +} +``` + +### private readonly + +Examples of code for the `{ "allows": ["private readonly"] }` options: + + + +#### ❌ Incorrect + +```ts +class Foo { + constructor(readonly name: string) {} +} + +class Foo { + constructor(private name: string) {} +} + +class Foo { + constructor(protected name: string) {} +} + +class Foo { + constructor(public name: string) {} +} + +class Foo { + constructor(protected readonly name: string) {} +} + +class Foo { + constructor(public readonly name: string) {} +} +``` + +#### ✅ Correct + +```ts +class Foo { + constructor(name: string) {} +} + +class Foo { + constructor(private readonly name: string) {} +} +``` + +### protected readonly + +Examples of code for the `{ "allows": ["protected readonly"] }` options: + + + +#### ❌ Incorrect + +```ts +class Foo { + constructor(readonly name: string) {} +} + +class Foo { + constructor(private name: string) {} +} + +class Foo { + constructor(protected name: string) {} +} + +class Foo { + constructor(public name: string) {} +} + +class Foo { + constructor(private readonly name: string) {} +} + +class Foo { + constructor(public readonly name: string) {} +} +``` + +#### ✅ Correct + +```ts +class Foo { + constructor(name: string) {} +} + +class Foo { + constructor(protected readonly name: string) {} +} +``` + +### public readonly + +Examples of code for the `{ "allows": ["public readonly"] }` options: + + + +#### ❌ Incorrect + +```ts +class Foo { + constructor(readonly name: string) {} +} + +class Foo { + constructor(private name: string) {} +} + +class Foo { + constructor(protected name: string) {} +} + +class Foo { + constructor(public name: string) {} +} + +class Foo { + constructor(private readonly name: string) {} +} + +class Foo { + constructor(protected readonly name: string) {} +} +``` + +#### ✅ Correct + +```ts +class Foo { + constructor(name: string) {} +} + +class Foo { + constructor(public readonly name: string) {} +} +``` + +## When Not To Use It + +If you don't care about the using parameter properties in constructors, then you will not need this rule. + +## Related To + +- TSLint: [no-parameter-properties](https://palantir.github.io/tslint/rules/no-parameter-properties/) + +## Attributes + +- [ ] ✅ Recommended +- [ ] 🔧 Fixable +- [ ] 💭 Requires type information diff --git a/packages/eslint-plugin/src/rules/no-parameter-properties.ts b/packages/eslint-plugin/src/rules/no-parameter-properties.ts index d7abe9ed159..d510332f0c0 100644 --- a/packages/eslint-plugin/src/rules/no-parameter-properties.ts +++ b/packages/eslint-plugin/src/rules/no-parameter-properties.ts @@ -20,6 +20,7 @@ export default util.createRule({ name: 'no-parameter-properties', meta: { deprecated: true, + replacedBy: ['@typescript-eslint/parameter-properties'], type: 'problem', docs: { description: From b96865d8463f498ccad685fa583964cee992d95f Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 26 Mar 2022 12:37:53 -0500 Subject: [PATCH 8/9] chore: import order --- packages/eslint-plugin/src/rules/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts index bdb80d580b0..fb48ee5cadc 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -58,9 +58,9 @@ import noNamespace from './no-namespace'; import noNonNullAssertedNullishCoalescing from './no-non-null-asserted-nullish-coalescing'; import noNonNullAssertedOptionalChain from './no-non-null-asserted-optional-chain'; import noNonNullAssertion from './no-non-null-assertion'; +import noParameterProperties from './no-parameter-properties'; import noRedeclare from './no-redeclare'; import noRedundantTypeConstituents from './no-redundant-type-constituents'; -import noParameterProperties from './no-parameter-properties'; import noRequireImports from './no-require-imports'; import noRestrictedImports from './no-restricted-imports'; import noShadow from './no-shadow'; From e67dd44aae924b595f3a7c2bc398ed8437009244 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Sat, 26 Mar 2022 12:41:20 -0500 Subject: [PATCH 9/9] chore: ...and eslint-plugin/docs/rules/README.md --- packages/eslint-plugin/docs/rules/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/docs/rules/README.md b/packages/eslint-plugin/docs/rules/README.md index 60c19816fa4..af0c386ee00 100644 --- a/packages/eslint-plugin/docs/rules/README.md +++ b/packages/eslint-plugin/docs/rules/README.md @@ -59,7 +59,6 @@ slug: / | [`@typescript-eslint/no-non-null-asserted-nullish-coalescing`](./no-non-null-asserted-nullish-coalescing.md) | Disallows using a non-null assertion in the left operand of the nullish coalescing operator | | | | | [`@typescript-eslint/no-non-null-asserted-optional-chain`](./no-non-null-asserted-optional-chain.md) | Disallows using a non-null assertion after an optional chain expression | :white_check_mark: | | | | [`@typescript-eslint/no-non-null-assertion`](./no-non-null-assertion.md) | Disallows non-null assertions using the `!` postfix operator | :white_check_mark: | | | -| [`@typescript-eslint/parameter-properties`](./parameter-properties.md) | Disallow the use of parameter properties in class constructors | | | | | [`@typescript-eslint/no-require-imports`](./no-require-imports.md) | Disallows invocation of `require()` | | | | | [`@typescript-eslint/no-this-alias`](./no-this-alias.md) | Disallow aliasing `this` | :white_check_mark: | | | | [`@typescript-eslint/no-type-alias`](./no-type-alias.md) | Disallow the use of type aliases | | | | @@ -76,6 +75,7 @@ slug: / | [`@typescript-eslint/no-unsafe-return`](./no-unsafe-return.md) | Disallows returning any from a function | :white_check_mark: | | :thought_balloon: | | [`@typescript-eslint/no-var-requires`](./no-var-requires.md) | Disallows the use of require statements except in import statements | :white_check_mark: | | | | [`@typescript-eslint/non-nullable-type-assertion-style`](./non-nullable-type-assertion-style.md) | Prefers a non-null assertion over explicit type cast when possible | | :wrench: | :thought_balloon: | +| [`@typescript-eslint/parameter-properties`](./parameter-properties.md) | Require or disallow the use of parameter properties in class constructors | | | | | [`@typescript-eslint/prefer-as-const`](./prefer-as-const.md) | Prefer usage of `as const` over literal type | :white_check_mark: | :wrench: | | | [`@typescript-eslint/prefer-enum-initializers`](./prefer-enum-initializers.md) | Prefer initializing each enums member value | | | | | [`@typescript-eslint/prefer-for-of`](./prefer-for-of.md) | Prefer a ‘for-of’ loop over a standard ‘for’ loop if the index is only used to access the array being iterated | | | |