From 4b3d82022a45eeb52a94446454e5809fbb5812eb Mon Sep 17 00:00:00 2001 From: Gavin Barron Date: Sun, 7 Apr 2019 01:07:22 -0700 Subject: [PATCH] feat(eslint-plugin): [member-accessibility] add more options (#322) --- .../rules/explicit-member-accessibility.md | 235 ++++++++++++- .../rules/explicit-member-accessibility.ts | 199 +++++++++-- .../explicit-member-accessibility.test.ts | 322 ++++++++++++++++++ 3 files changed, 719 insertions(+), 37 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/explicit-member-accessibility.md b/packages/eslint-plugin/docs/rules/explicit-member-accessibility.md index ac3e8b1d7fc..a672b766c6c 100644 --- a/packages/eslint-plugin/docs/rules/explicit-member-accessibility.md +++ b/packages/eslint-plugin/docs/rules/explicit-member-accessibility.md @@ -10,28 +10,247 @@ be easier to use. This rule aims to make code more readable and explicit about who can use which properties. -The following patterns are considered warnings: +## Options + +```ts +type AccessibilityLevel = + | 'explicit' // require an accessor (including public) + | 'no-public' // don't require public + | 'off'; // don't check + +interface Config { + accessibility?: AccessibilityLevel; + overrides?: { + accessors?: AccessibilityLevel; + constructors?: AccessibilityLevel; + methods?: AccessibilityLevel; + properties?: AccessibilityLevel; + parameterProperties?: AccessibilityLevel; + }; +} +``` + +Default config: + +```JSON +{ "accessibility": "explicit" } +``` + +This rule in it's default state requires no configuration and will enforce that every class member has an accessibility modifier. If you would like to allow for some implicit public members then you have the following options: +A possible configuration could be: + +```ts +{ + accessibility: 'explicit', + overrides { + accessors: 'explicit', + constructors: 'no-public', + methods: 'explicit', + properties: 'off', + parameterProperties: 'explicit' + } +} +``` + +The following patterns are considered incorrect code if no options are provided: + +```ts +class Animal { + constructor(name) { + // No accessibility modifier + this.animalName = name; + } + animalName: string; // No accessibility modifier + get name(): string { + // No accessibility modifier + return this.animalName; + } + set name(value: string) { + // No accessibility modifier + this.animalName = value; + } + walk() { + // method + } +} +``` + +The following patterns are considered correct with the default options `{ accessibility: 'explicit' }`: + +```ts +class Animal { + public constructor(public breed, animalName) { + // Parameter property and constructor + this.animalName = name; + } + private animalName: string; // Property + get name(): string { + // get accessor + return this.animalName; + } + set name(value: string) { + // set accessor + this.animalName = value; + } + public walk() { + // method + } +} +``` + +The following patterns are considered incorrect with the accessibility set to **no-public** `[{ accessibility: 'no-public' }]`: + +```ts +class Animal { + public constructor(public breed, animalName) { + // Parameter property and constructor + this.animalName = name; + } + public animalName: string; // Property + public get name(): string { + // get accessor + return this.animalName; + } + public set name(value: string) { + // set accessor + this.animalName = value; + } + public walk() { + // method + } +} +``` + +The following patterns are considered correct with the accessibility set to **no-public** `[{ accessibility: 'no-public' }]`: + +```ts +class Animal { + constructor(protected breed, animalName) { + // Parameter property and constructor + this.name = name; + } + private animalName: string; // Property + get name(): string { + // get accessor + return this.animalName; + } + private set name(value: string) { + // set accessor + this.animalName = value; + } + protected walk() { + // method + } +} +``` + +### Overrides + +There are three ways in which an override can be used. + +- To disallow the use of public on a given member. +- To enforce explicit member accessibility when the root has allowed implicit public accessibility +- To disable any checks on given member type + +#### Disallow the use of public on a given member + +e.g. `[ { overrides: { constructor: 'no-public' } } ]` + +The following patterns are considered incorrect with the example override + +```ts +class Animal { + public constructor(protected animalName) {} + public get name() { + return this.animalName; + } +} +``` + +The following patterns are considered correct with the example override + +```ts +class Animal { + constructor(protected animalName) {} + public get name() { + return this.animalName; + } +} +``` + +#### Require explicit accessibility for a given member + +e.g. `[ { accessibility: 'no-public', overrides: { properties: 'explicit' } } ]` + +The following patterns are considered incorrect with the example override + +```ts +class Animal { + constructor(protected animalName) {} + get name() { + return this.animalName; + } + protected set name(value: string) { + this.animalName = value; + } + legs: number; + private hasFleas: boolean; +} +``` + +The following patterns are considered correct with the example override + +```ts +class Animal { + constructor(protected animalName) {} + get name() { + return this.animalName; + } + protected set name(value: string) { + this.animalName = value; + } + public legs: number; + private hasFleas: boolean; +} +``` + +#### Disable any checks on given member type + +e.g. `[{ overrides: { accessors : 'off' } } ]` + +As no checks on the overridden member type are performed all permutations of visibility are permitted for that member type + +The follow pattern is considered incorrect for the given configuration ```ts class Animal { - name: string; // No accessibility modifier - getName(): string {} // No accessibility modifier + constructor(protected animalName) {} + public get name() { + return this.animalName; + } + get legs() { + return this.legCount; + } } ``` -The following patterns are not warnings: +The following patterns are considered correct with the example override ```ts class Animal { - private name: string; // explicit accessibility modifier - public getName(): string {} // explicit accessibility modifier + public constructor(protected animalName) {} + public get name() { + return this.animalName; + } + get legs() { + return this.legCount; + } } ``` ## When Not To Use It -If you think defaulting to public is a good default, then you will not need -this rule. +If you think defaulting to public is a good default, then you should consider using the `no-public` setting. If you want to mix implicit and explicit public members then disable this rule. ## Further Reading diff --git a/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts b/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts index 66029faa501..be79d684a69 100644 --- a/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts +++ b/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts @@ -1,7 +1,29 @@ -import { TSESTree } from '@typescript-eslint/typescript-estree'; +import { TSESTree, AST_NODE_TYPES } from '@typescript-eslint/typescript-estree'; import * as util from '../util'; -export default util.createRule({ +type AccessibilityLevel = + | 'explicit' // require an accessor (including public) + | 'no-public' // don't require public + | 'off'; // don't check + +interface Config { + accessibility?: AccessibilityLevel; + overrides?: { + accessors?: AccessibilityLevel; + constructors?: AccessibilityLevel; + methods?: AccessibilityLevel; + properties?: AccessibilityLevel; + parameterProperties?: AccessibilityLevel; + }; +} + +type Options = [Config]; + +type MessageIds = 'unwantedPublicAccessibility' | 'missingAccessibility'; + +const accessibilityLevel = { enum: ['explicit', 'no-public', 'off'] }; + +export default util.createRule({ name: 'explicit-member-accessibility', meta: { type: 'problem', @@ -15,12 +37,60 @@ export default util.createRule({ messages: { missingAccessibility: 'Missing accessibility modifier on {{type}} {{name}}.', + unwantedPublicAccessibility: + 'Public accessibility modifier on {{type}} {{name}}.', }, - schema: [], + schema: [ + { + type: 'object', + properties: { + accessibility: accessibilityLevel, + overrides: { + type: 'object', + properties: { + accessors: accessibilityLevel, + constructors: accessibilityLevel, + methods: accessibilityLevel, + properties: accessibilityLevel, + parameterProperties: accessibilityLevel, + }, + additionalProperties: false, + }, + }, + additionalProperties: false, + }, + ], }, - defaultOptions: [], - create(context) { + defaultOptions: [{ accessibility: 'explicit' }], + create(context, [option]) { const sourceCode = context.getSourceCode(); + const baseCheck: AccessibilityLevel = option.accessibility || 'explicit'; + const overrides = option.overrides || {}; + const ctorCheck = overrides.constructors || baseCheck; + const accessorCheck = overrides.accessors || baseCheck; + const methodCheck = overrides.methods || baseCheck; + const propCheck = overrides.properties || baseCheck; + const paramPropCheck = overrides.parameterProperties || baseCheck; + + /** + * Generates the report for rule violations + */ + function reportIssue( + messageId: MessageIds, + nodeType: string, + node: TSESTree.Node, + nodeName: string, + ) { + context.report({ + node: node, + messageId: messageId, + data: { + type: nodeType, + name: nodeName, + }, + }); + } + /** * Checks if a method declaration has an accessibility modifier. * @param methodDefinition The node representing a MethodDefinition. @@ -28,18 +98,49 @@ export default util.createRule({ function checkMethodAccessibilityModifier( methodDefinition: TSESTree.MethodDefinition, ): void { - if ( - !methodDefinition.accessibility && - util.isTypeScriptFile(context.getFilename()) - ) { - context.report({ - node: methodDefinition, - messageId: 'missingAccessibility', - data: { - type: 'method definition', - name: util.getNameFromClassMember(methodDefinition, sourceCode), - }, - }); + let nodeType = 'method definition'; + let check = baseCheck; + switch (methodDefinition.kind) { + case 'method': + check = methodCheck; + break; + case 'constructor': + check = ctorCheck; + break; + case 'get': + case 'set': + check = accessorCheck; + nodeType = `${methodDefinition.kind} property accessor`; + break; + } + if (check === 'off') { + return; + } + + if (util.isTypeScriptFile(context.getFilename())) { + // const methodName = util.getNameFromPropertyName(methodDefinition.key); + const methodName = util.getNameFromClassMember( + methodDefinition, + sourceCode, + ); + if ( + check === 'no-public' && + methodDefinition.accessibility === 'public' + ) { + reportIssue( + 'unwantedPublicAccessibility', + nodeType, + methodDefinition, + methodName, + ); + } else if (check === 'explicit' && !methodDefinition.accessibility) { + reportIssue( + 'missingAccessibility', + nodeType, + methodDefinition, + methodName, + ); + } } } @@ -50,22 +151,62 @@ export default util.createRule({ function checkPropertyAccessibilityModifier( classProperty: TSESTree.ClassProperty, ): void { - if ( - !classProperty.accessibility && - util.isTypeScriptFile(context.getFilename()) - ) { - context.report({ - node: classProperty, - messageId: 'missingAccessibility', - data: { - type: 'class property', - name: util.getNameFromPropertyName(classProperty.key), - }, - }); + const nodeType = 'class property'; + + if (util.isTypeScriptFile(context.getFilename())) { + const propertyName = util.getNameFromPropertyName(classProperty.key); + if ( + propCheck === 'no-public' && + classProperty.accessibility === 'public' + ) { + reportIssue( + 'unwantedPublicAccessibility', + nodeType, + classProperty, + propertyName, + ); + } else if (propCheck === 'explicit' && !classProperty.accessibility) { + reportIssue( + 'missingAccessibility', + nodeType, + classProperty, + propertyName, + ); + } + } + } + + /** + * Checks that the parameter property has the desired accessibility modifiers set. + * @param {TSESTree.TSParameterProperty} node The node representing a Parameter Property + */ + function checkParameterPropertyAccessibilityModifier( + node: TSESTree.TSParameterProperty, + ) { + const nodeType = 'parameter property'; + if (util.isTypeScriptFile(context.getFilename())) { + // 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 nodeName = + 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; + + if (paramPropCheck === 'no-public' && node.accessibility === 'public') { + reportIssue('unwantedPublicAccessibility', nodeType, node, nodeName); + } } } return { + TSParameterProperty: checkParameterPropertyAccessibilityModifier, ClassProperty: checkPropertyAccessibilityModifier, MethodDefinition: checkMethodAccessibilityModifier, }; diff --git a/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts b/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts index e630450689c..51a94a19de2 100644 --- a/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts +++ b/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts @@ -39,6 +39,129 @@ class Test { } `, }, + { + filename: 'test.ts', + code: ` +class Test { + protected name: string + protected foo?: string + public getX () { + return this.x + } +} + `, + options: [{ accessibility: 'explicit' }], + }, + { + filename: 'test.ts', + code: ` +class Test { + protected name: string + protected foo?: string + getX () { + return this.x + } +} + `, + options: [{ accessibility: 'no-public' }], + }, + { + filename: 'test.ts', + code: ` +class Test { + name: string + foo?: string + getX () { + return this.x + } + get fooName(): string { + return this.foo + ' ' + this.name + } +} + `, + options: [{ accessibility: 'no-public' }], + }, + { + filename: 'test.ts', + code: ` +class Test { + private x: number; + constructor (x: number) { + this.x = x; + } + get internalValue() { + return this.x; + } + private set internalValue(value: number) { + this.x = value; + } + public square (): number { + return this.x * this.x; + } +} + `, + options: [{ overrides: { constructors: 'off', accessors: 'off' } }], + }, + { + filename: 'test.ts', + code: ` +class Test { + private x: number; + public constructor (x: number) { + this.x = x; + } + public get internalValue() { + return this.x; + } + public set internalValue(value: number) { + this.x = value; + } + public square (): number { + return this.x * this.x; + } + half (): number { + return this.x / 2; + } +} + `, + options: [{ overrides: { methods: 'off' } }], + }, + { + filename: 'test.ts', + code: ` +class Test { + constructor(private x: number){} +} + `, + options: [{ accessibility: 'no-public' }], + }, + { + filename: 'test.ts', + code: ` +class Test { + constructor(public x: number){} +} + `, + options: [ + { + accessibility: 'no-public', + overrides: { parameterProperties: 'off' }, + }, + ], + }, + { + filename: 'test.js', + code: ` +class Test { + constructor(public x: number){} +} + `, + options: [ + { + accessibility: 'no-public', + }, + ], + }, ], invalid: [ { @@ -116,5 +239,204 @@ class Test { }, ], }, + { + filename: 'test.ts', + code: ` +class Test { + protected name: string + protected foo?: string + public getX () { + return this.x + } +} + `, + options: [{ accessibility: 'no-public' }], + errors: [ + { + messageId: 'unwantedPublicAccessibility', + data: { + type: 'method definition', + name: 'getX', + }, + line: 5, + column: 3, + }, + ], + }, + { + filename: 'test.ts', + code: ` +class Test { + protected name: string + public foo?: string + getX () { + return this.x + } +} + `, + options: [{ accessibility: 'no-public' }], + errors: [ + { + messageId: 'unwantedPublicAccessibility', + data: { + type: 'class property', + name: 'foo', + }, + line: 4, + column: 3, + }, + ], + }, + { + filename: 'test.ts', + code: ` +class Test { + public x: number + public getX () { + return this.x + } +} + `, + errors: [ + { + messageId: 'unwantedPublicAccessibility', + line: 3, + column: 3, + }, + { + messageId: 'unwantedPublicAccessibility', + line: 4, + column: 3, + }, + ], + options: [{ accessibility: 'no-public' }], + }, + { + filename: 'test.ts', + code: ` +class Test { + private x: number; + constructor (x: number) { + this.x = x; + } + get internalValue() { + return this.x; + } + set internalValue(value: number) { + this.x = value; + } +} + `, + errors: [ + { + messageId: 'missingAccessibility', + line: 7, + column: 3, + }, + { + messageId: 'missingAccessibility', + line: 10, + column: 3, + }, + ], + options: [{ overrides: { constructors: 'no-public' } }], + }, + { + filename: 'test.ts', + code: ` +class Test { + private x: number; + constructor (x: number) { + this.x = x; + } + get internalValue() { + return this.x; + } + set internalValue(value: number) { + this.x = value; + } +} + `, + errors: [ + { + messageId: 'missingAccessibility', + line: 4, + column: 3, + }, + { + messageId: 'missingAccessibility', + line: 7, + column: 3, + }, + { + messageId: 'missingAccessibility', + line: 10, + column: 3, + }, + ], + }, + { + filename: 'test.ts', + code: ` +class Test { + constructor(public x: number){} +} + `, + errors: [ + { + messageId: 'unwantedPublicAccessibility', + line: 3, + column: 15, + }, + ], + options: [ + { + accessibility: 'no-public', + }, + ], + }, + { + filename: 'test.ts', + code: ` +class Test { + constructor(public x: number){} + public foo(): string { + return 'foo'; + } +} + `, + errors: [ + { + messageId: 'missingAccessibility', + line: 3, + column: 3, + }, + { + messageId: 'unwantedPublicAccessibility', + line: 3, + column: 15, + }, + ], + options: [ + { + overrides: { parameterProperties: 'no-public' }, + }, + ], + }, + { + filename: 'test.ts', + code: ` +class Test { + constructor(public x: number){} +} + `, + errors: [ + { + messageId: 'missingAccessibility', + line: 3, + column: 3, + }, + ], + }, ], });