feat(eslint-plugin): [ban-types] rework default options (#848)

This commit is contained in:
Brad Zacher
2020-05-09 16:16:27 -07:00
parent 13915cdf47
commit 62235ce889
13 changed files with 353 additions and 173 deletions
+12 -1
View File
@@ -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
+155 -69
View File
@@ -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<F = String> extends Bar<String> implements Baz<String> {
constructor(foo: String) {}
exit(): Array<String> {
const foo: String = 1 as String;
}
}
```
Examples of **correct** code for this rule `"String": "Use string instead"`
```ts
class Foo<F = string> extends Bar<string> implements Baz<string> {
constructor(foo: string) {}
exit(): Array<string> {
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<Bar>`), 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<Bar>`), 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<string, unknown>`; this was a stylistic decision, as the built-in `Record` type is considered to look cleaner.
<details>
<summary>Default Options</summary>
```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<string, unknown>` 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<string, unknown>` 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<string, unknown>` instead, as it allows you to more easily inspect and use the keys.',
].join('\n'),
},
};
```
</details>
### 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<string, unknown> = {};
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/)
+75 -40
View File
@@ -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<string, any> instead',
fixWith: 'Record<string, any>',
},
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<string, unknown>` 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<string, unknown>` 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<string, unknown>` 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<Options, MessageIds>({
@@ -92,7 +127,7 @@ export default util.createRule<Options, MessageIds>({
},
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<Options, MessageIds>({
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<Options, MessageIds>({
}
}
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) {
@@ -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: {
+1
View File
@@ -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
@@ -0,0 +1,34 @@
function objectForEachKey<T extends Record<string, unknown>>(
obj: T,
callback: (key: keyof T) => void,
): void {
const keys = Object.keys(obj);
for (const key of keys) {
callback(key);
}
}
function objectMapKey<T extends Record<string, unknown>, TReturn>(
obj: T,
callback: (key: keyof T) => TReturn,
): TReturn[] {
const values: TReturn[] = [];
objectForEachKey(obj, key => {
values.push(callback(key));
});
return values;
}
function objectReduceKey<T extends Record<string, unknown>, 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 };
@@ -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<typeof rule> = [
const options: Options = [
{
types: {
String: {
@@ -21,32 +26,13 @@ const options: InferOptionsTypeFromRule<typeof rule> = [
fixWith: 'NS.Good',
},
},
},
];
const options2: InferOptionsTypeFromRule<typeof rule> = [
{
types: {
null: {
message: 'Use undefined instead.',
fixWith: 'undefined',
},
},
},
];
const options3: InferOptionsTypeFromRule<typeof rule> = [
{
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<MessageIds, Options>[], 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;
},
[],
),
],
});
@@ -35,6 +35,8 @@ function applyDefault<TUser extends readonly unknown[], TDefault extends TUser>(
return options;
}
type AsMutable<T extends {}> = { -readonly [TKey in keyof T]: T[TKey] };
type AsMutable<T extends readonly unknown[]> = {
-readonly [TKey in keyof T]: T[TKey];
};
export { applyDefault };
@@ -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;
+2 -2
View File
@@ -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),
};
@@ -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.
+5 -3
View File
@@ -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<T extends TSESTreeOptions> = 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<T extends TSESTreeOptions> {
ast: AST<T>;
@@ -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,