From bfe255fde0cb5fe5e32c02eb5ba35d27fb23d9ea Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Wed, 2 Sep 2020 10:14:40 -0700 Subject: [PATCH] feat(eslint-plugin): [no-shadow] add option `ignoreFunctionTypeParameterNameValueShadow` (#2470) --- package.json | 3 + .../eslint-plugin/docs/rules/no-shadow.md | 39 +++++- packages/eslint-plugin/src/rules/no-shadow.ts | 50 +++++-- .../tests/rules/no-shadow.test.ts | 122 +++++++++++++++++- 4 files changed, 197 insertions(+), 17 deletions(-) diff --git a/package.json b/package.json index 89e20c04d80..5d02e9b5e43 100644 --- a/package.json +++ b/package.json @@ -57,6 +57,9 @@ "footer-max-length": [ 0 ], + "footer-max-line-length": [ + 0 + ], "header-max-length": [ 0 ] diff --git a/packages/eslint-plugin/docs/rules/no-shadow.md b/packages/eslint-plugin/docs/rules/no-shadow.md index d4b5294696a..ed241b80788 100644 --- a/packages/eslint-plugin/docs/rules/no-shadow.md +++ b/packages/eslint-plugin/docs/rules/no-shadow.md @@ -23,17 +23,21 @@ This rule adds the following options: ```ts interface Options extends BaseNoShadowOptions { ignoreTypeValueShadow?: boolean; + ignoreFunctionTypeParameterNameValueShadow?: boolean; } const defaultOptions: Options = { ...baseNoShadowDefaultOptions, ignoreTypeValueShadow: true, + ignoreFunctionTypeParameterNameValueShadow: true, }; ``` ### `ignoreTypeValueShadow` -When set to `true`, the rule will ignore when you name a type and a variable with the same name. +When set to `true`, the rule will ignore the case when you name a type the same as a variable. + +TypeScript allows types and variables to shadow one-another. This is generally safe because you cannot use variables in type locations without a `typeof` operator, so there's little risk of confusion. Examples of **correct** code with `{ ignoreTypeValueShadow: true }`: @@ -47,4 +51,37 @@ interface Bar { const Bar = 'test'; ``` +### `ignoreFunctionTypeParameterNameValueShadow` + +When set to `true`, the rule will ignore the case when you name a function type argument the same as a variable. + +Each of a function type's arguments creates a value variable within the scope of the function type. This is done so that you can reference the type later using the `typeof` operator: + +```ts +type Func = (test: string) => typeof test; + +declare const fn: Func; +const result = fn('str'); // typeof result === string +``` + +This means that function type arguments shadow value variable names in parent scopes: + +```ts +let test = 1; +type TestType = typeof test; // === number +type Func = (test: string) => typeof test; // this "test" references the argument, not the variable + +declare const fn: Func; +const result = fn('str'); // typeof result === string +``` + +If you do not use the `typeof` operator in a function type return type position, you can safely turn this option on. + +Examples of **correct** code with `{ ignoreFunctionTypeParameterNameValueShadow: true }`: + +```ts +const test = 1; +type Func = (test: string) => typeof test; +``` + Taken with ❤️ [from ESLint core](https://github.com/eslint/eslint/blob/master/docs/rules/no-shadow.md) diff --git a/packages/eslint-plugin/src/rules/no-shadow.ts b/packages/eslint-plugin/src/rules/no-shadow.ts index 9c77ca321fd..05761572b21 100644 --- a/packages/eslint-plugin/src/rules/no-shadow.ts +++ b/packages/eslint-plugin/src/rules/no-shadow.ts @@ -12,6 +12,7 @@ type Options = [ builtinGlobals?: boolean; hoist?: 'all' | 'functions' | 'never'; ignoreTypeValueShadow?: boolean; + ignoreFunctionTypeParameterNameValueShadow?: boolean; }, ]; @@ -45,6 +46,9 @@ export default util.createRule({ ignoreTypeValueShadow: { type: 'boolean', }, + ignoreFunctionTypeParameterNameValueShadow: { + type: 'boolean', + }, }, additionalProperties: false, }, @@ -59,6 +63,7 @@ export default util.createRule({ builtinGlobals: false, hoist: 'functions', ignoreTypeValueShadow: true, + ignoreFunctionTypeParameterNameValueShadow: true, }, ], create(context, [options]) { @@ -77,15 +82,37 @@ export default util.createRule({ return false; } - if ( - !('isValueVariable' in shadowed) || - !('isValueVariable' in variable) - ) { - // one of them is an eslint global variable + if (!('isValueVariable' in variable)) { + // this shouldn't happen... + return false; + } + + const isShadowedValue = + 'isValueVariable' in shadowed ? shadowed.isValueVariable : true; + return variable.isValueVariable !== isShadowedValue; + } + + function isFunctionTypeParameterNameValueShadow( + variable: TSESLint.Scope.Variable, + shadowed: TSESLint.Scope.Variable, + ): boolean { + if (options.ignoreFunctionTypeParameterNameValueShadow !== true) { + return false; + } + + if (!('isValueVariable' in variable)) { + // this shouldn't happen... + return false; + } + + const isShadowedValue = + 'isValueVariable' in shadowed ? shadowed.isValueVariable : true; + if (!isShadowedValue) { return false; } - return variable.isValueVariable !== shadowed.isValueVariable; + const id = variable.identifiers[0]; + return util.isFunctionType(id.parent); } /** @@ -117,12 +144,12 @@ export default util.createRule({ } /** - * Checks if a variable of the class name in the class scope of ClassDeclaration. + * Checks if a variable of the class name in the class scope of TSEnumDeclaration. * - * ClassDeclaration creates two variables of its name into its outer scope and its class scope. + * TSEnumDeclaration creates two variables of its name into its outer scope and its class scope. * So we should ignore the variable in the class scope. * @param variable The variable to check. - * @returns Whether or not the variable of the class name in the class scope of ClassDeclaration. + * @returns Whether or not the variable of the class name in the class scope of TSEnumDeclaration. */ function isDuplicatedEnumNameVariable( variable: TSESLint.Scope.Variable, @@ -273,6 +300,11 @@ export default util.createRule({ continue; } + // ignore function type parameter name shadowing if configured + if (isFunctionTypeParameterNameValueShadow(variable, shadowed)) { + continue; + } + const isESLintGlobal = 'writeable' in shadowed; if ( (shadowed.identifiers.length > 0 || diff --git a/packages/eslint-plugin/tests/rules/no-shadow.test.ts b/packages/eslint-plugin/tests/rules/no-shadow.test.ts index 54f9851be84..bbaad211a25 100644 --- a/packages/eslint-plugin/tests/rules/no-shadow.test.ts +++ b/packages/eslint-plugin/tests/rules/no-shadow.test.ts @@ -56,6 +56,58 @@ const x = 1; type x = string; } `, + { + code: ` +type Foo = 1; + `, + options: [{ ignoreTypeValueShadow: true }], + globals: { + Foo: 'writable', + }, + }, + { + code: ` +type Foo = 1; + `, + options: [ + { + ignoreTypeValueShadow: false, + builtinGlobals: false, + }, + ], + globals: { + Foo: 'writable', + }, + }, + // https://github.com/typescript-eslint/typescript-eslint/issues/2360 + ` +enum Direction { + left = 'left', + right = 'right', +} + `, + // https://github.com/typescript-eslint/typescript-eslint/issues/2447 + { + code: ` +const test = 1; +type Fn = (test: string) => typeof test; + `, + options: [{ ignoreFunctionTypeParameterNameValueShadow: true }], + }, + { + code: ` +type Fn = (Foo: string) => typeof Foo; + `, + options: [ + { + ignoreFunctionTypeParameterNameValueShadow: true, + builtinGlobals: false, + }, + ], + globals: { + Foo: 'writable', + }, + }, ], invalid: [ { @@ -109,6 +161,69 @@ const x = 1; }, ], }, + { + code: ` +type Foo = 1; + `, + options: [ + { + ignoreTypeValueShadow: false, + builtinGlobals: true, + }, + ], + globals: { + Foo: 'writable', + }, + errors: [ + { + messageId: 'noShadow', + data: { + name: 'Foo', + }, + line: 2, + }, + ], + }, + // https://github.com/typescript-eslint/typescript-eslint/issues/2447 + { + code: ` +const test = 1; +type Fn = (test: string) => typeof test; + `, + options: [{ ignoreFunctionTypeParameterNameValueShadow: false }], + errors: [ + { + messageId: 'noShadow', + data: { + name: 'test', + }, + line: 3, + }, + ], + }, + { + code: ` +type Fn = (Foo: string) => typeof Foo; + `, + options: [ + { + ignoreFunctionTypeParameterNameValueShadow: false, + builtinGlobals: true, + }, + ], + globals: { + Foo: 'writable', + }, + errors: [ + { + messageId: 'noShadow', + data: { + name: 'Foo', + }, + line: 2, + }, + ], + }, ], }); @@ -431,13 +546,6 @@ function foo(cb) { `, options: [{ allow: ['cb'] }], }, - // https://github.com/typescript-eslint/typescript-eslint/issues/2360 - ` -enum Direction { - left = 'left', - right = 'right', -} - `, ], invalid: [ {