diff --git a/.markdownlint.json b/.markdownlint.json index 77f01355394..9dc1aa86002 100644 --- a/.markdownlint.json +++ b/.markdownlint.json @@ -58,7 +58,18 @@ // MD032/blanks-around-lists - Lists should be surrounded by blank lines "MD032": false, // MD033/no-inline-html - Inline HTML - "MD033": { "allowed_elements": ["a", "img", "br", "sup", "h1", "p"] }, + "MD033": { + "allowed_elements": [ + "a", + "img", + "br", + "sup", + "h1", + "p", + "details", + "summary" + ] + }, // MD034/no-bare-urls - Bare URL used "MD034": false, // MD035/hr-style - Horizontal rule style diff --git a/packages/eslint-plugin/docs/rules/ban-types.md b/packages/eslint-plugin/docs/rules/ban-types.md index e92e57b521f..e5eacd40c24 100644 --- a/packages/eslint-plugin/docs/rules/ban-types.md +++ b/packages/eslint-plugin/docs/rules/ban-types.md @@ -1,93 +1,61 @@ # Bans specific types from being used (`ban-types`) -This rule bans specific types and can suggest alternatives. It does not ban the -corresponding runtime objects from being used. +Some builtin types have aliases, some types are considered dangerous or harmful. +It's often a good idea to ban certain types to help with consistency and safety. ## Rule Details -Examples of **incorrect** code for this rule `"String": "Use string instead"` - -```ts -class Foo extends Bar implements Baz { - constructor(foo: String) {} - - exit(): Array { - const foo: String = 1 as String; - } -} -``` - -Examples of **correct** code for this rule `"String": "Use string instead"` - -```ts -class Foo extends Bar implements Baz { - constructor(foo: string) {} - - exit(): Array { - const foo: string = 1 as string; - } -} -``` +This rule bans specific types and can suggest alternatives. +Note that it does not ban the corresponding runtime objects from being used. ## Options -The banned type can either be a type name literal (`Foo`), a type name with generic parameter instantiations(s) (`Foo`), or the empty object literal (`{}`). - -```CJSON -{ - "@typescript-eslint/ban-types": ["error", { - "types": { - // report usages of the type using the default error message - "Foo": null, - - // add a custom message to help explain why not to use it - "Bar": "Don't use bar!", - - // add a custom message, AND tell the plugin how to fix it - "String": { - "message": "Use string instead", - "fixWith": "string" - } - - "{}": { - "message": "Use object instead", - "fixWith": "object" - } - } - }] -} +```ts +type Options = { + types?: { + [typeName: string]: + | string + | { + message: string; + fixWith?: string; + }; + }; + extendDefaults?: boolean; +}; ``` -By default, this rule includes types which are likely to be mistakes, such as `String` and `Number`. If you don't want these enabled, set the `extendDefaults` option to `false`: +The rule accepts a single object as options, with the following keys: -```CJSON -{ - "@typescript-eslint/ban-types": ["error", { - "types": { - // add a custom message, AND tell the plugin how to fix it - "String": { - "message": "Use string instead", - "fixWith": "string" - } - }, - "extendDefaults": false - }] -} -``` +- `types` - An object whose keys are the types you want to ban, and the values are error messages. + - The type can either be a type name literal (`Foo`), a type name with generic parameter instantiation(s) (`Foo`), or the empty object literal (`{}`). + - The values can be a string, which is the error message to be reported, + or it can be an object with the following properties: + - `message: string` - the message to display when the type is matched. + - `fixWith?: string` - a string to replace the banned type with when the fixer is run. If this is omitted, no fix will be done. +- `extendDefaults` - if you're specifying custom `types`, you can set this to `true` to extend the default `types` configuration. + - This is a convenience option to save you copying across the defaults when adding another type. + - If this is `false`, the rule will _only_ use the types defined in your configuration. -### Example +Example configuration: -```json +```jsonc { "@typescript-eslint/ban-types": [ "error", { "types": { - "Array": null, - "Object": "Use {} instead", + // add a custom message to help explain why not to use it + "Foo": "Don't use Far because it is unsafe", + + // add a custom message, AND tell the plugin how to fix it "String": { "message": "Use string instead", "fixWith": "string" + }, + + "{}": { + "message": "Use object instead", + "fixWith": "object" } } } @@ -95,6 +63,124 @@ By default, this rule includes types which are likely to be mistakes, such as `S } ``` +### Default Options + +The default options provide a set of "best practices", intended to provide safety and standardization in your codebase: + +- Don't use the upper-case primitive types, you should use the lower-case types for consistency. +- Avoid the `Function` type, as it provides little safety for the following reasons: + - It provides no type safety when calling the value, which means it's easy to provide the wrong arguments. + - It accepts class declarations, which will fail when called, as they are called without the `new` keyword. +- Avoid the `Object` and `{}` types, as they mean "any non-nullish value". + - This is a point of confusion for many developers, who think it means "any object type". +- Avoid the `object` type, as it is currently hard to use due to not being able to assert that keys exist. + - See [microsoft/TypeScript#21732](https://github.com/microsoft/TypeScript/issues/21732). + +**_Important note:_** the default options suggest using `Record`; this was a stylistic decision, as the built-in `Record` type is considered to look cleaner. + +
+Default Options + +```ts +const defaultTypes = { + String: { + message: 'Use string instead', + fixWith: 'string', + }, + Boolean: { + message: 'Use boolean instead', + fixWith: 'boolean', + }, + Number: { + message: 'Use number instead', + fixWith: 'number', + }, + Symbol: { + message: 'Use symbol instead', + fixWith: 'symbol', + }, + + Function: { + message: [ + 'The `Function` type accepts any function-like value.', + 'It provides no type safety when calling the function, which can be a common source of bugs.', + 'It also accepts things like class declarations, which will throw at runtime as they will not be called with `new`.', + 'If you are expecting the function to accept certain arguments, you should explicitly define the function shape.', + ].join('\n'), + }, + + // object typing + Object: { + message: [ + 'The `Object` type actually means "any non-nullish value", so it is marginally better than `unknown`.', + '- If you want a type meaning "any object", you probably want `Record` instead.', + '- If you want a type meaning "any value", you probably want `unknown` instead.', + ].join('\n'), + }, + '{}': { + message: [ + '`{}` actually means "any non-nullish value".', + '- If you want a type meaning "any object", you probably want `Record` instead.', + '- If you want a type meaning "any value", you probably want `unknown` instead.', + ].join('\n'), + }, + object: { + message: [ + 'The `object` type is currently hard to use ([see this issue](https://github.com/microsoft/TypeScript/issues/21732)).', + 'Consider using `Record` instead, as it allows you to more easily inspect and use the keys.', + ].join('\n'), + }, +}; +``` + +
+ +### Examples + +Examples of **incorrect** code with the default options: + +```ts +// use lower-case primitives for consistency +const str: String = 'foo'; +const bool: Boolean = true; +const num: Number = 1; +const symb: Symbol = Symbol('foo'); + +// use a proper function type +const func: Function = () => 1; + +// use safer object types +const lowerObj: object = {}; + +const capitalObj1: Object = 1; +const capitalObj2: Object = { a: 'string' }; + +const curly1: {} = 1; +const curly2: {} = { a: 'string' }; +``` + +Examples of **correct** code with the default options: + +```ts +// use lower-case primitives for consistency +const str: string = 'foo'; +const bool: boolean = true; +const num: number = 1; +const symb: symbol = Symbol('foo'); + +// use a proper function type +const func: () => number = () => 1; + +// use safer object types +const lowerObj: Record = {}; + +const capitalObj1: number = 1; +const capitalObj2: { a: string } = { a: 'string' }; + +const curly1: number = 1; +const curly2: Record<'a', string> = { a: 'string' }; +``` + ## Compatibility - TSLint: [ban-types](https://palantir.github.io/tslint/rules/ban-types/) diff --git a/packages/eslint-plugin/src/rules/ban-types.ts b/packages/eslint-plugin/src/rules/ban-types.ts index 7e056959a3f..51048df9a66 100644 --- a/packages/eslint-plugin/src/rules/ban-types.ts +++ b/packages/eslint-plugin/src/rules/ban-types.ts @@ -1,4 +1,8 @@ -import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils'; +import { + TSESLint, + TSESTree, + AST_NODE_TYPES, +} from '@typescript-eslint/experimental-utils'; import * as util from '../util'; type Types = Record< @@ -11,24 +15,20 @@ type Types = Record< } >; -type Options = [ +export type Options = [ { types?: Types; extendDefaults?: boolean; }, ]; -type MessageIds = 'bannedTypeMessage'; +export type MessageIds = 'bannedTypeMessage'; function removeSpaces(str: string): string { return str.replace(/ /g, ''); } function stringifyTypeName( - node: - | TSESTree.EntityName - | TSESTree.TSTypeLiteral - | TSESTree.TSNullKeyword - | TSESTree.TSUndefinedKeyword, + node: TSESTree.Node, sourceCode: TSESLint.SourceCode, ): string { return removeSpaces(sourceCode.getText(node)); @@ -52,13 +52,7 @@ function getCustomMessage( return ''; } -/* - Defaults for this rule should be treated as an "all or nothing" - merge, so we need special handling here. - - See: https://github.com/typescript-eslint/typescript-eslint/issues/686 - */ -const defaultTypes = { +const defaultTypes: Types = { String: { message: 'Use string instead', fixWith: 'string', @@ -71,14 +65,55 @@ const defaultTypes = { message: 'Use number instead', fixWith: 'number', }, - Object: { - message: 'Use Record instead', - fixWith: 'Record', - }, Symbol: { message: 'Use symbol instead', fixWith: 'symbol', }, + + Function: { + message: [ + 'The `Function` type accepts any function-like value.', + 'It provides no type safety when calling the function, which can be a common source of bugs.', + 'It also accepts things like class declarations, which will throw at runtime as they will not be called with `new`.', + 'If you are expecting the function to accept certain arguments, you should explicitly define the function shape.', + ].join('\n'), + }, + + // object typing + Object: { + message: [ + 'The `Object` type actually means "any non-nullish value", so it is marginally better than `unknown`.', + '- If you want a type meaning "any object", you probably want `Record` instead.', + '- If you want a type meaning "any value", you probably want `unknown` instead.', + ].join('\n'), + }, + '{}': { + message: [ + '`{}` actually means "any non-nullish value".', + '- If you want a type meaning "any object", you probably want `Record` instead.', + '- If you want a type meaning "any value", you probably want `unknown` instead.', + ].join('\n'), + }, + object: { + message: [ + 'The `object` type is currently hard to use ([see this issue](https://github.com/microsoft/TypeScript/issues/21732)).', + 'Consider using `Record` instead, as it allows you to more easily inspect and use the keys.', + ].join('\n'), + }, +}; + +export const TYPE_KEYWORDS = { + bigint: AST_NODE_TYPES.TSBigIntKeyword, + boolean: AST_NODE_TYPES.TSBooleanKeyword, + never: AST_NODE_TYPES.TSNeverKeyword, + null: AST_NODE_TYPES.TSNullKeyword, + number: AST_NODE_TYPES.TSNumberKeyword, + object: AST_NODE_TYPES.TSObjectKeyword, + string: AST_NODE_TYPES.TSStringKeyword, + symbol: AST_NODE_TYPES.TSSymbolKeyword, + undefined: AST_NODE_TYPES.TSUndefinedKeyword, + unknown: AST_NODE_TYPES.TSUnknownKeyword, + void: AST_NODE_TYPES.TSVoidKeyword, }; export default util.createRule({ @@ -92,7 +127,7 @@ export default util.createRule({ }, fixable: 'code', messages: { - bannedTypeMessage: "Don't use '{{name}}' as a type.{{customMessage}}", + bannedTypeMessage: "Don't use `{{name}}` as a type.{{customMessage}}", }, schema: [ { @@ -127,20 +162,17 @@ export default util.createRule({ create(context, [options]) { const extendDefaults = options.extendDefaults ?? true; const customTypes = options.types ?? {}; - const types: Types = { - ...(extendDefaults ? defaultTypes : {}), - ...customTypes, - }; + const types = Object.assign( + {}, + extendDefaults ? defaultTypes : {}, + customTypes, + ); const bannedTypes = new Map( Object.entries(types).map(([type, data]) => [removeSpaces(type), data]), ); function checkBannedTypes( - typeNode: - | TSESTree.EntityName - | TSESTree.TSTypeLiteral - | TSESTree.TSNullKeyword - | TSESTree.TSUndefinedKeyword, + typeNode: TSESTree.Node, name = stringifyTypeName(typeNode, context.getSourceCode()), ): void { const bannedType = bannedTypes.get(name); @@ -164,18 +196,21 @@ export default util.createRule({ } } - return { - ...(bannedTypes.has('null') && { - TSNullKeyword(node): void { - checkBannedTypes(node, 'null'); - }, - }), + const keywordSelectors = util.objectReduceKey( + TYPE_KEYWORDS, + (acc: TSESLint.RuleListener, keyword) => { + if (bannedTypes.has(keyword)) { + acc[TYPE_KEYWORDS[keyword]] = (node: TSESTree.Node): void => + checkBannedTypes(node, keyword); + } - ...(bannedTypes.has('undefined') && { - TSUndefinedKeyword(node): void { - checkBannedTypes(node, 'undefined'); - }, - }), + return acc; + }, + {}, + ); + + return { + ...keywordSelectors, TSTypeLiteral(node): void { if (node.members.length) { diff --git a/packages/eslint-plugin/src/rules/member-ordering.ts b/packages/eslint-plugin/src/rules/member-ordering.ts index 411823c8de7..de5b971f533 100644 --- a/packages/eslint-plugin/src/rules/member-ordering.ts +++ b/packages/eslint-plugin/src/rules/member-ordering.ts @@ -2,6 +2,7 @@ import { AST_NODE_TYPES, TSESLint, TSESTree, + JSONSchema, } from '@typescript-eslint/experimental-utils'; import * as util from '../util'; @@ -25,19 +26,19 @@ export type Options = [ }, ]; -const neverConfig = { +const neverConfig: JSONSchema.JSONSchema4 = { type: 'string', enum: ['never'], }; -const arrayConfig = (memberTypes: string[]): object => ({ +const arrayConfig = (memberTypes: string[]): JSONSchema.JSONSchema4 => ({ type: 'array', items: { enum: memberTypes, }, }); -const objectConfig = (memberTypes: string[]): object => ({ +const objectConfig = (memberTypes: string[]): JSONSchema.JSONSchema4 => ({ type: 'object', properties: { memberTypes: { diff --git a/packages/eslint-plugin/src/util/index.ts b/packages/eslint-plugin/src/util/index.ts index 381012acfec..2a0d891f5f1 100644 --- a/packages/eslint-plugin/src/util/index.ts +++ b/packages/eslint-plugin/src/util/index.ts @@ -5,6 +5,7 @@ export * from './createRule'; export * from './isTypeReadonly'; export * from './misc'; export * from './nullThrows'; +export * from './objectIterators'; export * from './types'; // this is done for convenience - saves migrating all of the old rules diff --git a/packages/eslint-plugin/src/util/objectIterators.ts b/packages/eslint-plugin/src/util/objectIterators.ts new file mode 100644 index 00000000000..474d64349ff --- /dev/null +++ b/packages/eslint-plugin/src/util/objectIterators.ts @@ -0,0 +1,34 @@ +function objectForEachKey>( + obj: T, + callback: (key: keyof T) => void, +): void { + const keys = Object.keys(obj); + for (const key of keys) { + callback(key); + } +} + +function objectMapKey, TReturn>( + obj: T, + callback: (key: keyof T) => TReturn, +): TReturn[] { + const values: TReturn[] = []; + objectForEachKey(obj, key => { + values.push(callback(key)); + }); + return values; +} + +function objectReduceKey, TAccumulator>( + obj: T, + callback: (acc: TAccumulator, key: keyof T) => TAccumulator, + initial: TAccumulator, +): TAccumulator { + let accumulator = initial; + objectForEachKey(obj, key => { + accumulator = callback(accumulator, key); + }); + return accumulator; +} + +export { objectForEachKey, objectMapKey, objectReduceKey }; diff --git a/packages/eslint-plugin/tests/rules/ban-types.test.ts b/packages/eslint-plugin/tests/rules/ban-types.test.ts index 772fa7716f8..9f11202a07f 100644 --- a/packages/eslint-plugin/tests/rules/ban-types.test.ts +++ b/packages/eslint-plugin/tests/rules/ban-types.test.ts @@ -1,12 +1,17 @@ -import rule from '../../src/rules/ban-types'; +import { TSESLint } from '@typescript-eslint/experimental-utils'; +import rule, { + MessageIds, + Options, + TYPE_KEYWORDS, +} from '../../src/rules/ban-types'; +import { objectReduceKey } from '../../src/util'; import { RuleTester, noFormat } from '../RuleTester'; -import { InferOptionsTypeFromRule } from '../../src/util'; const ruleTester = new RuleTester({ parser: '@typescript-eslint/parser', }); -const options: InferOptionsTypeFromRule = [ +const options: Options = [ { types: { String: { @@ -21,32 +26,13 @@ const options: InferOptionsTypeFromRule = [ fixWith: 'NS.Good', }, }, - }, -]; - -const options2: InferOptionsTypeFromRule = [ - { - types: { - null: { - message: 'Use undefined instead.', - fixWith: 'undefined', - }, - }, - }, -]; - -const options3: InferOptionsTypeFromRule = [ - { - types: { - undefined: null, - }, + extendDefaults: false, }, ]; ruleTester.run('ban-types', rule, { valid: [ 'let f = Object();', // Should not fail if there is no options set - 'let f: {} = {};', 'let f: { x: number; y: number } = { x: 1, y: 1 };', { code: 'let f = Object();', @@ -89,11 +75,27 @@ ruleTester.run('ban-types', rule, { }, { code: 'let a: undefined;', - options: options2, + options: [ + { + types: { + null: { + message: 'Use undefined instead.', + fixWith: 'undefined', + }, + }, + }, + ], }, { code: 'let a: null;', - options: options3, + options: [ + { + types: { + undefined: null, + }, + extendDefaults: false, + }, + ], }, ], invalid: [ @@ -122,30 +124,6 @@ ruleTester.run('ban-types', rule, { ], options, }, - { - code: 'let a: undefined;', - errors: [ - { - messageId: 'bannedTypeMessage', - data: { name: 'undefined', customMessage: '' }, - line: 1, - column: 8, - }, - ], - options: options3, - }, - { - code: 'let a: null;', - errors: [ - { - messageId: 'bannedTypeMessage', - data: { name: 'null', customMessage: ' Use undefined instead.' }, - line: 1, - column: 8, - }, - ], - options: options2, - }, { code: 'let aa: Foo;', errors: [ @@ -497,5 +475,34 @@ let bar: object = {}; }, ], }, + ...objectReduceKey( + TYPE_KEYWORDS, + (acc: TSESLint.InvalidTestCase[], key) => { + acc.push({ + code: `function foo(x: ${key}) {}`, + errors: [ + { + messageId: 'bannedTypeMessage', + data: { + name: key, + customMessage: '', + }, + line: 1, + column: 17, + }, + ], + options: [ + { + extendDefaults: false, + types: { + [key]: null, + }, + }, + ], + }); + return acc; + }, + [], + ), ], }); diff --git a/packages/experimental-utils/src/eslint-utils/applyDefault.ts b/packages/experimental-utils/src/eslint-utils/applyDefault.ts index 9b4f6513ecc..a9984b563a1 100644 --- a/packages/experimental-utils/src/eslint-utils/applyDefault.ts +++ b/packages/experimental-utils/src/eslint-utils/applyDefault.ts @@ -35,6 +35,8 @@ function applyDefault( return options; } -type AsMutable = { -readonly [TKey in keyof T]: T[TKey] }; +type AsMutable = { + -readonly [TKey in keyof T]: T[TKey]; +}; export { applyDefault }; diff --git a/packages/experimental-utils/src/ts-eslint-scope/analyze.ts b/packages/experimental-utils/src/ts-eslint-scope/analyze.ts index 9bbe1c1118e..51d69ba8cbe 100644 --- a/packages/experimental-utils/src/ts-eslint-scope/analyze.ts +++ b/packages/experimental-utils/src/ts-eslint-scope/analyze.ts @@ -1,5 +1,6 @@ import { analyze as ESLintAnalyze } from 'eslint-scope'; import { EcmaVersion } from '../ts-eslint'; +import { TSESTree } from '../ts-estree'; import { ScopeManager } from './ScopeManager'; interface AnalysisOptions { @@ -8,12 +9,12 @@ interface AnalysisOptions { ignoreEval?: boolean; nodejsScope?: boolean; impliedStrict?: boolean; - fallback?: string | ((node: {}) => string[]); + fallback?: string | ((node: TSESTree.Node) => string[]); sourceType?: 'script' | 'module'; ecmaVersion?: EcmaVersion; } const analyze = ESLintAnalyze as ( - ast: {}, + ast: TSESTree.Node, options?: AnalysisOptions, ) => ScopeManager; diff --git a/packages/parser/tests/lib/services.ts b/packages/parser/tests/lib/services.ts index 37ddf0fadcf..210cd39e1ea 100644 --- a/packages/parser/tests/lib/services.ts +++ b/packages/parser/tests/lib/services.ts @@ -1,6 +1,7 @@ import path from 'path'; import fs from 'fs'; import glob from 'glob'; +import { ParserOptions } from '../../src/parser-options'; import { createSnapshotTestBlock, formatSnapshotName, @@ -16,10 +17,9 @@ const testFiles = glob.sync(`**/*.src.ts`, { cwd: FIXTURES_DIR, }); -function createConfig(filename: string): object { +function createConfig(filename: string): ParserOptions { return { filePath: filename, - generateServices: true, project: './tsconfig.json', tsconfigRootDir: path.resolve(FIXTURES_DIR), }; diff --git a/packages/typescript-estree/src/parser-options.ts b/packages/typescript-estree/src/parser-options.ts index c55ace5adb0..e901ee74957 100644 --- a/packages/typescript-estree/src/parser-options.ts +++ b/packages/typescript-estree/src/parser-options.ts @@ -15,7 +15,7 @@ export interface Extra { filePath: string; jsx: boolean; loc: boolean; - log: Function; + log: (message: string) => void; preserveNodeMaps?: boolean; projects: string[]; range: boolean; @@ -81,7 +81,7 @@ interface ParseOptions { * When value is `false`, no logging will occur. * When value is not provided, `console.log()` will be used. */ - loggerFn?: Function | false; + loggerFn?: ((message: string) => void) | false; /** * Controls whether the `range` property is included on AST nodes. diff --git a/packages/typescript-estree/src/parser.ts b/packages/typescript-estree/src/parser.ts index 5e3e9b90261..069e8af1960 100644 --- a/packages/typescript-estree/src/parser.ts +++ b/packages/typescript-estree/src/parser.ts @@ -272,7 +272,7 @@ function applyParserOptionsToExtra(options: TSESTreeOptions): void { if (typeof options.loggerFn === 'function') { extra.log = options.loggerFn; } else if (options.loggerFn === false) { - extra.log = Function.prototype; + extra.log = (): void => {}; } if (typeof options.tsconfigRootDir === 'string') { @@ -337,9 +337,11 @@ function warnAboutTSVersion(): void { // Parser //------------------------------------------------------------------------------ +// eslint-disable-next-line @typescript-eslint/no-empty-interface +interface EmptyObject {} type AST = TSESTree.Program & - (T['tokens'] extends true ? { tokens: TSESTree.Token[] } : {}) & - (T['comment'] extends true ? { comments: TSESTree.Comment[] } : {}); + (T['tokens'] extends true ? { tokens: TSESTree.Token[] } : EmptyObject) & + (T['comment'] extends true ? { comments: TSESTree.Comment[] } : EmptyObject); interface ParseAndGenerateServicesResult { ast: AST; diff --git a/packages/typescript-estree/tests/lib/parse.ts b/packages/typescript-estree/tests/lib/parse.ts index c6435508eac..9c049ae882d 100644 --- a/packages/typescript-estree/tests/lib/parse.ts +++ b/packages/typescript-estree/tests/lib/parse.ts @@ -83,7 +83,7 @@ describe('parse()', () => { it('output tokens, comments, locs, and ranges when called with those options', () => { const spy = jest.spyOn(astConverter, 'astConverter'); - const loggerFn = jest.fn(() => true); + const loggerFn = jest.fn(() => {}); parser.parse('let foo = bar;', { loggerFn,