From ae7e0fd955afcda04dbde3d23a955bca8fcc9dc3 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Wed, 6 Feb 2019 22:11:49 -0800 Subject: [PATCH 1/7] feat(eslint-plugin): add no-unnecessary-qualifier rule --- .../docs/rules/no-unnecessary-qualifier.md | 31 ++ .../lib/rules/no-unnecessary-qualifier.js | 297 ++++++++++++++++++ packages/eslint-plugin/lib/util.js | 14 + .../lib/rules/no-unnecessary-qualifier.js | 185 +++++++++++ 4 files changed, 527 insertions(+) create mode 100644 packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md create mode 100644 packages/eslint-plugin/lib/rules/no-unnecessary-qualifier.js create mode 100644 packages/eslint-plugin/tests/lib/rules/no-unnecessary-qualifier.js diff --git a/packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md b/packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md new file mode 100644 index 000000000000..70a10bc9ceab --- /dev/null +++ b/packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md @@ -0,0 +1,31 @@ +# Warns when a namespace qualifier is unnecessary. (no-unnecessary-qualifier) + +Please describe the origin of the rule here. + +## Rule Details + +This rule aims to... + +Examples of **incorrect** code for this rule: + +```js +// fill me in +``` + +Examples of **correct** code for this rule: + +```js +// fill me in +``` + +### Options + +If there are any options, describe them here. Otherwise, delete this section. + +## When Not To Use It + +Give a short description of when it would be appropriate to turn off this rule. + +## Further Reading + +If there are other links that describe the issue this rule addresses, please include them here in a bulleted list. diff --git a/packages/eslint-plugin/lib/rules/no-unnecessary-qualifier.js b/packages/eslint-plugin/lib/rules/no-unnecessary-qualifier.js new file mode 100644 index 000000000000..ec242c7795b9 --- /dev/null +++ b/packages/eslint-plugin/lib/rules/no-unnecessary-qualifier.js @@ -0,0 +1,297 @@ +/** + * @fileoverview Warns when a namespace qualifier is unnecessary. + * @author Benjamin Lichtman + */ +'use strict'; + +/** + * @typedef {import("eslint").Rule.RuleModule} RuleModule + * @typedef {import("estree").Node} ESTreeNode + * @typedef {import("estree").Expression} Expression + * @typedef {import("estree").MemberExpression} MemberExpression + * @typedef {import("estree").Identifier} Identifier + * @typedef {Identifier | QualifiedName} EntityName + * @typedef {{ left: EntityName, right: Identifier } & ESTreeNode} QualifiedName + */ + +const ts = require('typescript'); +const tsutils = require('tsutils'); +const utils = require('../util'); + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +/** + * @type {RuleModule} + */ +module.exports = { + meta: { + docs: { + description: 'Warns when a namespace qualifier is unnecessary.', + category: 'Fill me in', + recommended: false + }, + fixable: 'code', + messages: { + unnecessaryQualifier: + "Qualifier is unnecessary since '{{ name }}' is in scope." + }, + schema: [ + // fill in your schema + ], + type: 'suggestion' + }, + + create(context) { + // variables should be defined here + /** + * @type {(ts.ModuleDeclaration | ts.EnumDeclaration)[]} + */ + const namespacesInScope = []; + + /** + * Track failing namespace expression so nested qualifier errors are not reported + * @type {ESTreeNode | null} + */ + let currentFailedNamespaceExpression = null; + + const parserServices = utils.getParserServices(context); + + /** + * @type {WeakMap} + */ + const esTreeNodeToTSNodeMap = parserServices.esTreeNodeToTSNodeMap; + + /** + * @type {ts.Program} + */ + const program = parserServices.program; + const checker = program.getTypeChecker(); + + const sourceCode = context.getSourceCode(); + + //---------------------------------------------------------------------- + // Helpers + //---------------------------------------------------------------------- + + /** + * @param {ts.Symbol} symbol the symbol being inspected + * @param {ts.TypeChecker} checker The program's type checker + * @returns {ts.Symbol | null} Returns the aliased symbol of symbol if it exists, or undefined otherwise + */ + function tryGetAliasedSymbol(symbol, checker) { + return tsutils.isSymbolFlagSet(symbol, ts.SymbolFlags.Alias) + ? checker.getAliasedSymbol(symbol) + : null; + } + + /** + * @param {ts.Symbol} symbol The symbol being checked + * @returns {boolean} True iff the symbol is a namespace that is in scope + */ + function symbolIsNamespaceInScope(symbol) { + const symbolDeclarations = symbol.getDeclarations(); + + if (typeof symbolDeclarations === 'undefined') { + return false; + } + if ( + symbolDeclarations.some(decl => + namespacesInScope.some(ns => ns === decl) + ) + ) { + return true; + } + + const alias = tryGetAliasedSymbol(symbol, checker); + + return alias !== null && symbolIsNamespaceInScope(alias); + } + + /** + * @param {ts.Node} node The node whose in-scope symbols are to be searched + * @param {ts.SymbolFlags} flags The symbol flags being inspected + * @param {any} name The name of the symbol being searched for + * @returns {ts.Symbol | undefined} The symbol in the scope of node with the provided name, or null if one does not exist + */ + function getSymbolInScope(node, flags, name) { + // TODO:PERF `getSymbolsInScope` gets a long list. Is there a better way? + const scope = checker.getSymbolsInScope(node, flags); + + return scope.find(scopeSymbol => scopeSymbol.name === name); + } + + /** + * @param {ts.Symbol} accessed The symbol of the namespace that's accessed + * @param {ts.Symbol} inScope The symbol in scope that has the same text as accessed + * @returns {boolean} Returns true iff the symbols are equal + */ + function symbolsAreEqual(accessed, inScope) { + // Available starting in typescript@2.6 + if (typeof checker.getExportSymbolOfSymbol !== 'undefined') { + return accessed === checker.getExportSymbolOfSymbol(inScope); + } + return ( + accessed === inScope || + // For compatibility with typescript@2.5: compare declarations because the symbols don't have the same reference + utils.arraysAreEqual( + accessed.declarations, + inScope.declarations, + (a, b) => a === b + ) + ); + } + + /** + * @param {ESTreeNode} qualifier The qualifier being checked + * @param {Identifier} name The name being accessed from the qualifier + * @returns {boolean} True iff the qualifier is unnecessary + */ + function qualifierIsUnnecessary(qualifier, name) { + const tsQualifier = esTreeNodeToTSNodeMap.get(qualifier); + const tsName = esTreeNodeToTSNodeMap.get(name); + + if (!(tsQualifier && tsName)) return false; // TODO: throw error? + + const namespaceSymbol = checker.getSymbolAtLocation(tsQualifier); + + if ( + typeof namespaceSymbol === 'undefined' || + !symbolIsNamespaceInScope(namespaceSymbol) + ) { + return false; + } + + const accessedSymbol = checker.getSymbolAtLocation(tsName); + + if (typeof accessedSymbol === 'undefined') { + return false; + } + + // If the symbol in scope is different, the qualifier is necessary. + const fromScope = getSymbolInScope( + tsQualifier, + accessedSymbol.flags, + sourceCode.getText(name) + ); + + return ( + typeof fromScope === 'undefined' || + symbolsAreEqual(accessedSymbol, fromScope) + ); + } + + /** + * @param {ESTreeNode} node The namespace access node + * @param {ESTreeNode} qualifier The qualifier of the namespace access + * @param {Identifier} name The name being accessed in the namespace + * @returns {void} + */ + function visitNamespaceAccess(node, qualifier, name) { + // Only look for nested qualifier errors if we didn't already fail on the outer qualifier. + if ( + !currentFailedNamespaceExpression && + qualifierIsUnnecessary(qualifier, name) + ) { + currentFailedNamespaceExpression = node; + context.report({ + node: qualifier, + messageId: 'unnecessaryQualifier', + data: { + name: sourceCode.getText(name) + }, + fix(fixer) { + return fixer.removeRange([qualifier.range[0], name.range[0]]); + } + }); + } + } + + /** + * @param {ESTreeNode} node The TSModuleDeclaration or TSEnumDeclaration being visited + * @returns {void} + */ + function enterDeclaration(node) { + const tsDeclaration = esTreeNodeToTSNodeMap.get(node); + if (tsDeclaration) { + namespacesInScope.push(tsDeclaration); + } + } + + /** + * @returns {void} + */ + function exitDeclaration(node) { + if (esTreeNodeToTSNodeMap.has(node)) { + namespacesInScope.pop(); + } + } + + /** + * @param {ESTreeNode} node The current node being exited + * @returns {void} + */ + function resetCurrentNamespaceExpression(node) { + if (node === currentFailedNamespaceExpression) { + currentFailedNamespaceExpression = null; + } + } + + /** + * @param {ESTreeNode} node An ESTree AST node + * @returns {boolean} Returns true if node is a PropertyAccessExpression + */ + function isPropertyAccessExpression(node) { + return node.type === 'MemberExpression' && !node.computed; + } + + /** + * @param {ESTreeNode} node An ESTree AST node + * @returns {boolean} Returns true if node is an EntityNameExpression + */ + function isEntityNameExpression(node) { + return ( + node.type === 'Identifier' || + (isPropertyAccessExpression(node) && + isEntityNameExpression(node.object)) + ); + } + + //---------------------------------------------------------------------- + // Public + //---------------------------------------------------------------------- + + return { + TSModuleDeclaration: enterDeclaration, + TSEnumDeclaration: enterDeclaration, + 'ExportNamedDeclaration[declaration.type="TSModuleDeclaration"]': enterDeclaration, + 'ExportNamedDeclaration[declaration.type="TSEnumDeclaration"]': enterDeclaration, + 'TSModuleDeclaration:exit': exitDeclaration, + 'TSEnumDeclaration:exit': exitDeclaration, + 'ExportNamedDeclaration[declaration.type="TSModuleDeclaration"]:exit': exitDeclaration, + 'ExportNamedDeclaration[declaration.type="TSEnumDeclaration"]:exit': exitDeclaration, + /** + * @param {QualifiedName} node The node being inspected + * @returns {void} + */ + TSQualifiedName(node) { + visitNamespaceAccess(node, node.left, node.right); + }, + /** + * @param {MemberExpression} node The node being inspected + * @returns {void} + */ + MemberExpression(node) { + if (node.computed) return; + const property = /** @type {Identifier} */ (node.property); + + if (isEntityNameExpression(node.object)) { + visitNamespaceAccess(node, node.object, property); + } + }, + 'TSQualifiedName:exit': resetCurrentNamespaceExpression, + 'MemberExpression:exit': resetCurrentNamespaceExpression + }; + } +}; diff --git a/packages/eslint-plugin/lib/util.js b/packages/eslint-plugin/lib/util.js index dac41fe21bb7..789a50925e19 100644 --- a/packages/eslint-plugin/lib/util.js +++ b/packages/eslint-plugin/lib/util.js @@ -127,3 +127,17 @@ exports.getParserServices = context => { } return context.parserServices; }; + +/** + * @template T + * @param {ReadonlyArray | undefined} a An array + * @param {ReadonlyArray | undefined} b Another array + * @param {function(T, T): boolean} eq Comparison function + * @returns {boolean} Returns true iff the arrays are equal + */ +exports.arraysAreEqual = (a, b, eq) => + a === b || + (typeof a !== 'undefined' && + typeof b !== 'undefined' && + a.length === b.length && + a.every((x, idx) => eq(x, b[idx]))); diff --git a/packages/eslint-plugin/tests/lib/rules/no-unnecessary-qualifier.js b/packages/eslint-plugin/tests/lib/rules/no-unnecessary-qualifier.js new file mode 100644 index 000000000000..c39a02054ff3 --- /dev/null +++ b/packages/eslint-plugin/tests/lib/rules/no-unnecessary-qualifier.js @@ -0,0 +1,185 @@ +/** + * @fileoverview Warns when a namespace qualifier is unnecessary. + * @author Benjamin Lichtman + */ +'use strict'; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ +const path = require('path'); + +const rule = require('../../../lib/rules/no-unnecessary-qualifier'), + RuleTester = require('eslint').RuleTester; + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +const rootPath = path.join(process.cwd(), 'tests/fixtures/'); + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + parserOptions: { + tsconfigRootDir: rootPath, + project: './tsconfig.json' + } +}); + +ruleTester.run('no-unnecessary-qualifier', rule, { + valid: [ + ` +namespace X { + export type T = number; + namespace Y { + type T = string; + const x: X.T = 0; + } +}` + ], + + invalid: [ + { + code: ` +namespace A { + export type B = number; + const x: A.B = 3; +}`, + errors: [ + { + messageId: 'unnecessaryQualifier', + type: 'Identifier' + } + ], + output: ` +namespace A { + export type B = number; + const x: B = 3; +}` + }, + { + code: ` +namespace A { + export const x = 3; + export const y = A.x; +}`, + errors: [ + { + messageId: 'unnecessaryQualifier', + type: 'Identifier' + } + ], + output: ` +namespace A { + export const x = 3; + export const y = x; +}` + }, + { + code: ` +namespace A { + export type T = number; + export namespace B { + const x: A.T = 3; + } +}`, + errors: [ + { + messageId: 'unnecessaryQualifier', + type: 'Identifier' + } + ], + output: ` +namespace A { + export type T = number; + export namespace B { + const x: T = 3; + } +}` + }, + { + code: ` +namespace A { + export namespace B { + export type T = number; + const x: A.B.T = 3; + } +}`, + errors: [ + { + messageId: 'unnecessaryQualifier', + type: 'TSQualifiedName' + } + ], + output: ` +namespace A { + export namespace B { + export type T = number; + const x: T = 3; + } +}` + }, + { + code: ` +namespace A { + export namespace B { + export const x = 3; + const y = A.B.x; + } +}`, + errors: [ + { + messageId: 'unnecessaryQualifier', + type: 'MemberExpression' + } + ], + output: ` +namespace A { + export namespace B { + export const x = 3; + const y = x; + } +}` + }, + { + code: ` +enum A { + B, + C = A.B +}`, + errors: [ + { + messageId: 'unnecessaryQualifier', + type: 'Identifier' + } + ], + output: ` +enum A { + B, + C = B +}` + }, + { + code: ` +namespace Foo { + export enum A { + B, + C = Foo.A.B + } +}`, + errors: [ + { + messageId: 'unnecessaryQualifier', + type: 'MemberExpression' + } + ], + output: ` +namespace Foo { + export enum A { + B, + C = B + } +}` + } + ] +}); From ef32e1d0386a7b4dddf6b5bc7fc811655cd596b8 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Fri, 8 Feb 2019 14:58:18 -0800 Subject: [PATCH 2/7] docs: add documentation --- .../docs/rules/no-unnecessary-qualifier.md | 51 ++++++++++++++----- .../lib/rules/no-unnecessary-qualifier.js | 10 ++-- 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md b/packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md index 70a10bc9ceab..7ac44c14395c 100644 --- a/packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md +++ b/packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md @@ -1,31 +1,58 @@ # Warns when a namespace qualifier is unnecessary. (no-unnecessary-qualifier) -Please describe the origin of the rule here. - ## Rule Details -This rule aims to... +This rule aims to let users know when a namespace or enum qualifier is unnecessary, +whether used for a type or for a value. Examples of **incorrect** code for this rule: -```js -// fill me in +```ts +namespace A { + export type B = number; + const x: A.B = 3; +} ``` -Examples of **correct** code for this rule: +```ts +namespace A { + export const x = 3; + export const y = A.x; +} +``` -```js -// fill me in +```ts +enum A { + B, + C = A.B +} ``` -### Options +```ts +namespace A { + export namespace B { + export type T = number; + const x: A.B.T = 3; + } +} +``` -If there are any options, describe them here. Otherwise, delete this section. +Examples of **correct** code for this rule: + +```ts +namespace X { + export type T = number; + namespace Y { + type T = string; + const x: X.T = 0; + } +} +``` ## When Not To Use It -Give a short description of when it would be appropriate to turn off this rule. +If you don't care about having unneeded namespace or enum qualifiers, then you don't need to use this rule. ## Further Reading -If there are other links that describe the issue this rule addresses, please include them here in a bulleted list. +- TSLint: [no-unnecessary-qualifier](https://palantir.github.io/tslint/rules/no-unnecessary-qualifier/) diff --git a/packages/eslint-plugin/lib/rules/no-unnecessary-qualifier.js b/packages/eslint-plugin/lib/rules/no-unnecessary-qualifier.js index ec242c7795b9..655c79f06a7b 100644 --- a/packages/eslint-plugin/lib/rules/no-unnecessary-qualifier.js +++ b/packages/eslint-plugin/lib/rules/no-unnecessary-qualifier.js @@ -29,17 +29,17 @@ module.exports = { meta: { docs: { description: 'Warns when a namespace qualifier is unnecessary.', - category: 'Fill me in', - recommended: false + category: 'TypeScript', + recommended: false, + extraDescription: [utils.tslintRule('no-unnecessary-qualifier')], + url: utils.metaDocsUrl('no-unnecessary-qualifier') }, fixable: 'code', messages: { unnecessaryQualifier: "Qualifier is unnecessary since '{{ name }}' is in scope." }, - schema: [ - // fill in your schema - ], + schema: [], type: 'suggestion' }, From 8dfaf030cda74942f48e4b0aec6f624e82dcb519 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Fri, 8 Feb 2019 15:10:17 -0800 Subject: [PATCH 3/7] docs: update README and ROADMAP --- packages/eslint-plugin/README.md | 3 ++- packages/eslint-plugin/ROADMAP.md | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index 3807877a073e..37dcbca4aa0c 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -128,7 +128,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e | [`@typescript-eslint/no-empty-interface`](./docs/rules/no-empty-interface.md) | Disallow the declaration of empty interfaces (`no-empty-interface` from TSLint) | :heavy_check_mark: | | | [`@typescript-eslint/no-explicit-any`](./docs/rules/no-explicit-any.md) | Disallow usage of the `any` type (`no-any` from TSLint) | :heavy_check_mark: | | | [`@typescript-eslint/no-extraneous-class`](./docs/rules/no-extraneous-class.md) | Forbids the use of classes as namespaces (`no-unnecessary-class` from TSLint) | | | -| [`@typescript-eslint/no-for-in-array`](./docs/rules/no-for-in-array.md) | Disallow iterating over an array with a for-in loop (`no-for-in-array` from TSLint) | | | +| [`@typescript-eslint/no-for-in-array`](./docs/rules/no-for-in-array.md) | Disallow iterating over an array with a for-in loop (`no-for-in-array` from TSLint) | | | | [`@typescript-eslint/no-inferrable-types`](./docs/rules/no-inferrable-types.md) | Disallows explicit type declarations for variables or parameters initialized to a number, string, or boolean. (`no-inferrable-types` from TSLint) | :heavy_check_mark: | :wrench: | | [`@typescript-eslint/no-misused-new`](./docs/rules/no-misused-new.md) | Enforce valid definition of `new` and `constructor`. (`no-misused-new` from TSLint) | :heavy_check_mark: | | | [`@typescript-eslint/no-namespace`](./docs/rules/no-namespace.md) | Disallow the use of custom TypeScript modules and namespaces (`no-namespace` from TSLint) | :heavy_check_mark: | | @@ -138,6 +138,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e | [`@typescript-eslint/no-this-alias`](./docs/rules/no-this-alias.md) | Disallow aliasing `this` (`no-this-assignment` from TSLint) | | | | [`@typescript-eslint/no-triple-slash-reference`](./docs/rules/no-triple-slash-reference.md) | Disallow `/// ` comments (`no-reference` from TSLint) | :heavy_check_mark: | | | [`@typescript-eslint/no-type-alias`](./docs/rules/no-type-alias.md) | Disallow the use of type aliases (`interface-over-type-literal` from TSLint) | | | +| [`@typescript-eslint/no-unnecessary-qualifier`](./docs/rules/no-unnecessary-qualifier.md) | Warns when a namespace qualifier is unnecessary (`no-unnecessary-qualifier` from TSLint) | | :wrench: | | [`@typescript-eslint/no-unnecessary-type-assertion`](./docs/rules/no-unnecessary-type-assertion.md) | Warns if a type assertion does not change the type of an expression (`no-unnecessary-type-assertion` from TSLint) | | :wrench: | | [`@typescript-eslint/no-unused-vars`](./docs/rules/no-unused-vars.md) | Disallow unused variables (`no-unused-variable` from TSLint) | :heavy_check_mark: | | | [`@typescript-eslint/no-use-before-define`](./docs/rules/no-use-before-define.md) | Disallow the use of variables before they are defined | :heavy_check_mark: | | diff --git a/packages/eslint-plugin/ROADMAP.md b/packages/eslint-plugin/ROADMAP.md index 48ccaa900b63..c71e6629b628 100644 --- a/packages/eslint-plugin/ROADMAP.md +++ b/packages/eslint-plugin/ROADMAP.md @@ -157,7 +157,7 @@ | [`no-trailing-whitespace`] | 🌟 | [`no-trailing-spaces`][no-trailing-spaces] | | [`no-unnecessary-callback-wrapper`] | 🛑 | N/A and this might be unsafe (i.e. with `forEach`) | | [`no-unnecessary-initializer`] | 🌟 | [`no-undef-init`][no-undef-init] | -| [`no-unnecessary-qualifier`] | 🛑 | N/A | +| [`no-unnecessary-qualifier`] | ✅ | [`@typescript-eslint/no-unnecessary-qualifier`][no-unnecessary-qualifier] | | [`number-literal-format`] | 🛑 | N/A | | [`object-literal-key-quotes`] | 🌟 | [`quote-props`][quote-props] | | [`object-literal-shorthand`] | 🌟 | [`object-shorthand`][object-shorthand] | @@ -587,6 +587,7 @@ Relevant plugins: [`chai-expect-keywords`](https://github.com/gavinaiken/eslint- [`@typescript-eslint/prefer-interface`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/prefer-interface.md [`@typescript-eslint/no-array-constructor`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-array-constructor.md [`@typescript-eslint/no-for-in-array`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-for-in-array.md +[`@typescript-eslint/no-unnecessary-qualifier`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md From 47f45042edde120be8c5dc84e0a7c2efc6b06d6e Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Fri, 8 Feb 2019 16:57:56 -0800 Subject: [PATCH 4/7] test: increase code coverage --- .../lib/rules/no-unnecessary-qualifier.js | 19 ++----------- packages/eslint-plugin/lib/util.js | 14 ---------- packages/eslint-plugin/tests/fixtures/foo.ts | 1 + .../lib/rules/no-unnecessary-qualifier.js | 28 ++++++++++++++++++- 4 files changed, 30 insertions(+), 32 deletions(-) create mode 100644 packages/eslint-plugin/tests/fixtures/foo.ts diff --git a/packages/eslint-plugin/lib/rules/no-unnecessary-qualifier.js b/packages/eslint-plugin/lib/rules/no-unnecessary-qualifier.js index 655c79f06a7b..0031034e3785 100644 --- a/packages/eslint-plugin/lib/rules/no-unnecessary-qualifier.js +++ b/packages/eslint-plugin/lib/rules/no-unnecessary-qualifier.js @@ -91,11 +91,8 @@ module.exports = { * @returns {boolean} True iff the symbol is a namespace that is in scope */ function symbolIsNamespaceInScope(symbol) { - const symbolDeclarations = symbol.getDeclarations(); + const symbolDeclarations = symbol.getDeclarations() || []; - if (typeof symbolDeclarations === 'undefined') { - return false; - } if ( symbolDeclarations.some(decl => namespacesInScope.some(ns => ns === decl) @@ -128,19 +125,7 @@ module.exports = { * @returns {boolean} Returns true iff the symbols are equal */ function symbolsAreEqual(accessed, inScope) { - // Available starting in typescript@2.6 - if (typeof checker.getExportSymbolOfSymbol !== 'undefined') { - return accessed === checker.getExportSymbolOfSymbol(inScope); - } - return ( - accessed === inScope || - // For compatibility with typescript@2.5: compare declarations because the symbols don't have the same reference - utils.arraysAreEqual( - accessed.declarations, - inScope.declarations, - (a, b) => a === b - ) - ); + return accessed === checker.getExportSymbolOfSymbol(inScope); } /** diff --git a/packages/eslint-plugin/lib/util.js b/packages/eslint-plugin/lib/util.js index 762741a8ab8b..6714e600ee65 100644 --- a/packages/eslint-plugin/lib/util.js +++ b/packages/eslint-plugin/lib/util.js @@ -127,17 +127,3 @@ exports.getParserServices = context => { } return context.parserServices; }; - -/** - * @template T - * @param {ReadonlyArray | undefined} a An array - * @param {ReadonlyArray | undefined} b Another array - * @param {function(T, T): boolean} eq Comparison function - * @returns {boolean} Returns true iff the arrays are equal - */ -exports.arraysAreEqual = (a, b, eq) => - a === b || - (typeof a !== 'undefined' && - typeof b !== 'undefined' && - a.length === b.length && - a.every((x, idx) => eq(x, b[idx]))); diff --git a/packages/eslint-plugin/tests/fixtures/foo.ts b/packages/eslint-plugin/tests/fixtures/foo.ts new file mode 100644 index 000000000000..fda4aa0a1c50 --- /dev/null +++ b/packages/eslint-plugin/tests/fixtures/foo.ts @@ -0,0 +1 @@ +export type T = number; diff --git a/packages/eslint-plugin/tests/lib/rules/no-unnecessary-qualifier.js b/packages/eslint-plugin/tests/lib/rules/no-unnecessary-qualifier.js index c39a02054ff3..c789c6b708c6 100644 --- a/packages/eslint-plugin/tests/lib/rules/no-unnecessary-qualifier.js +++ b/packages/eslint-plugin/tests/lib/rules/no-unnecessary-qualifier.js @@ -22,7 +22,9 @@ const ruleTester = new RuleTester({ parser: '@typescript-eslint/parser', parserOptions: { tsconfigRootDir: rootPath, - project: './tsconfig.json' + project: './tsconfig.json', + sourceType: 'module', + ecmaVersion: 6 } }); @@ -35,6 +37,11 @@ namespace X { type T = string; const x: X.T = 0; } +}`, + `const x: A.B = 3;`, + ` +namespace X { + const z = X.y; }` ], @@ -179,6 +186,25 @@ namespace Foo { B, C = B } +}` + }, + { + code: ` +import * as Foo from './foo'; +declare module './foo' { + const x: Foo.T = 3; +}`, + filename: path.join(rootPath, 'bar.ts'), + errors: [ + { + messageId: 'unnecessaryQualifier', + type: 'Identifier' + } + ], + output: ` +import * as Foo from './foo'; +declare module './foo' { + const x: T = 3; }` } ] From 51f3e70a8400ffc74a6703d29c86b189f4222d8d Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Fri, 8 Feb 2019 17:09:52 -0800 Subject: [PATCH 5/7] docs: add simple correct usage of qualifiers --- .../docs/rules/no-unnecessary-qualifier.md | 21 +++++++++++++++++++ .../lib/rules/no-unnecessary-qualifier.js | 17 +++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md b/packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md index 7ac44c14395c..2f356e0eac3a 100644 --- a/packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md +++ b/packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md @@ -39,6 +39,27 @@ namespace A { Examples of **correct** code for this rule: +```ts +namespace X { + export type T = number; +} + +namespace Y { + export const x: X.T = 3; +} +``` + +```ts +enum A { + X, + Y +} + +enum B { + Z = A.X +} +``` + ```ts namespace X { export type T = number; diff --git a/packages/eslint-plugin/tests/lib/rules/no-unnecessary-qualifier.js b/packages/eslint-plugin/tests/lib/rules/no-unnecessary-qualifier.js index c789c6b708c6..90218cfef83f 100644 --- a/packages/eslint-plugin/tests/lib/rules/no-unnecessary-qualifier.js +++ b/packages/eslint-plugin/tests/lib/rules/no-unnecessary-qualifier.js @@ -31,6 +31,23 @@ const ruleTester = new RuleTester({ ruleTester.run('no-unnecessary-qualifier', rule, { valid: [ ` +namespace X { + export type T = number; +} + +namespace Y { + export const x: X.T = 3; +}`, + ` +enum A { + X, + Y +} + +enum B { + Z = A.X +}`, + ` namespace X { export type T = number; namespace Y { From 0fcb5f948180dd598d467f65e1b090f64a87db62 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Mon, 11 Feb 2019 11:14:48 -0800 Subject: [PATCH 6/7] chore: migrate to ts --- .../rules/no-unnecessary-qualifier.ts} | 160 +++++------------- .../no-unnecessary-qualifier.test.ts} | 41 ++--- .../typescript-estree/src/parser-options.ts | 1 + 3 files changed, 65 insertions(+), 137 deletions(-) rename packages/eslint-plugin/{lib/rules/no-unnecessary-qualifier.js => src/rules/no-unnecessary-qualifier.ts} (54%) rename packages/eslint-plugin/tests/{lib/rules/no-unnecessary-qualifier.js => rules/no-unnecessary-qualifier.test.ts} (79%) diff --git a/packages/eslint-plugin/lib/rules/no-unnecessary-qualifier.js b/packages/eslint-plugin/src/rules/no-unnecessary-qualifier.ts similarity index 54% rename from packages/eslint-plugin/lib/rules/no-unnecessary-qualifier.js rename to packages/eslint-plugin/src/rules/no-unnecessary-qualifier.ts index 0031034e3785..cff7b6b8cacb 100644 --- a/packages/eslint-plugin/lib/rules/no-unnecessary-qualifier.js +++ b/packages/eslint-plugin/src/rules/no-unnecessary-qualifier.ts @@ -2,37 +2,24 @@ * @fileoverview Warns when a namespace qualifier is unnecessary. * @author Benjamin Lichtman */ -'use strict'; -/** - * @typedef {import("eslint").Rule.RuleModule} RuleModule - * @typedef {import("estree").Node} ESTreeNode - * @typedef {import("estree").Expression} Expression - * @typedef {import("estree").MemberExpression} MemberExpression - * @typedef {import("estree").Identifier} Identifier - * @typedef {Identifier | QualifiedName} EntityName - * @typedef {{ left: EntityName, right: Identifier } & ESTreeNode} QualifiedName - */ - -const ts = require('typescript'); -const tsutils = require('tsutils'); -const utils = require('../util'); +import { TSESTree } from '@typescript-eslint/typescript-estree'; +import ts from 'typescript'; +import * as tsutils from 'tsutils'; +import * as util from '../util'; //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ -/** - * @type {RuleModule} - */ -module.exports = { +export default util.createRule({ + name: 'no-unnecessary-qualifier', meta: { docs: { + category: 'Best Practices', description: 'Warns when a namespace qualifier is unnecessary.', - category: 'TypeScript', recommended: false, - extraDescription: [utils.tslintRule('no-unnecessary-qualifier')], - url: utils.metaDocsUrl('no-unnecessary-qualifier') + tslintName: 'no-unnecessary-qualifier' }, fixable: 'code', messages: { @@ -42,55 +29,30 @@ module.exports = { schema: [], type: 'suggestion' }, - + defaultOptions: [], create(context) { - // variables should be defined here - /** - * @type {(ts.ModuleDeclaration | ts.EnumDeclaration)[]} - */ - const namespacesInScope = []; - - /** - * Track failing namespace expression so nested qualifier errors are not reported - * @type {ESTreeNode | null} - */ - let currentFailedNamespaceExpression = null; - - const parserServices = utils.getParserServices(context); - - /** - * @type {WeakMap} - */ + const namespacesInScope: ts.Node[] = []; + let currentFailedNamespaceExpression: TSESTree.Node | null = null; + const parserServices = util.getParserServices(context); const esTreeNodeToTSNodeMap = parserServices.esTreeNodeToTSNodeMap; - - /** - * @type {ts.Program} - */ const program = parserServices.program; const checker = program.getTypeChecker(); - const sourceCode = context.getSourceCode(); //---------------------------------------------------------------------- // Helpers //---------------------------------------------------------------------- - /** - * @param {ts.Symbol} symbol the symbol being inspected - * @param {ts.TypeChecker} checker The program's type checker - * @returns {ts.Symbol | null} Returns the aliased symbol of symbol if it exists, or undefined otherwise - */ - function tryGetAliasedSymbol(symbol, checker) { + function tryGetAliasedSymbol( + symbol: ts.Symbol, + checker: ts.TypeChecker + ): ts.Symbol | null { return tsutils.isSymbolFlagSet(symbol, ts.SymbolFlags.Alias) ? checker.getAliasedSymbol(symbol) : null; } - /** - * @param {ts.Symbol} symbol The symbol being checked - * @returns {boolean} True iff the symbol is a namespace that is in scope - */ - function symbolIsNamespaceInScope(symbol) { + function symbolIsNamespaceInScope(symbol: ts.Symbol): boolean { const symbolDeclarations = symbol.getDeclarations() || []; if ( @@ -106,34 +68,25 @@ module.exports = { return alias !== null && symbolIsNamespaceInScope(alias); } - /** - * @param {ts.Node} node The node whose in-scope symbols are to be searched - * @param {ts.SymbolFlags} flags The symbol flags being inspected - * @param {any} name The name of the symbol being searched for - * @returns {ts.Symbol | undefined} The symbol in the scope of node with the provided name, or null if one does not exist - */ - function getSymbolInScope(node, flags, name) { + function getSymbolInScope( + node: ts.Node, + flags: ts.SymbolFlags, + name: string + ): ts.Symbol | undefined { // TODO:PERF `getSymbolsInScope` gets a long list. Is there a better way? const scope = checker.getSymbolsInScope(node, flags); return scope.find(scopeSymbol => scopeSymbol.name === name); } - /** - * @param {ts.Symbol} accessed The symbol of the namespace that's accessed - * @param {ts.Symbol} inScope The symbol in scope that has the same text as accessed - * @returns {boolean} Returns true iff the symbols are equal - */ - function symbolsAreEqual(accessed, inScope) { + function symbolsAreEqual(accessed: ts.Symbol, inScope: ts.Symbol): boolean { return accessed === checker.getExportSymbolOfSymbol(inScope); } - /** - * @param {ESTreeNode} qualifier The qualifier being checked - * @param {Identifier} name The name being accessed from the qualifier - * @returns {boolean} True iff the qualifier is unnecessary - */ - function qualifierIsUnnecessary(qualifier, name) { + function qualifierIsUnnecessary( + qualifier: TSESTree.Node, + name: TSESTree.Identifier + ): boolean { const tsQualifier = esTreeNodeToTSNodeMap.get(qualifier); const tsName = esTreeNodeToTSNodeMap.get(name); @@ -167,13 +120,11 @@ module.exports = { ); } - /** - * @param {ESTreeNode} node The namespace access node - * @param {ESTreeNode} qualifier The qualifier of the namespace access - * @param {Identifier} name The name being accessed in the namespace - * @returns {void} - */ - function visitNamespaceAccess(node, qualifier, name) { + function visitNamespaceAccess( + node: TSESTree.Node, + qualifier: TSESTree.Node, + name: TSESTree.Identifier + ): void { // Only look for nested qualifier errors if we didn't already fail on the outer qualifier. if ( !currentFailedNamespaceExpression && @@ -193,49 +144,32 @@ module.exports = { } } - /** - * @param {ESTreeNode} node The TSModuleDeclaration or TSEnumDeclaration being visited - * @returns {void} - */ - function enterDeclaration(node) { + function enterDeclaration(node: TSESTree.Node): void { const tsDeclaration = esTreeNodeToTSNodeMap.get(node); if (tsDeclaration) { namespacesInScope.push(tsDeclaration); } } - /** - * @returns {void} - */ - function exitDeclaration(node) { + function exitDeclaration(node: TSESTree.Node) { if (esTreeNodeToTSNodeMap.has(node)) { namespacesInScope.pop(); } } - /** - * @param {ESTreeNode} node The current node being exited - * @returns {void} - */ - function resetCurrentNamespaceExpression(node) { + function resetCurrentNamespaceExpression(node: TSESTree.Node): void { if (node === currentFailedNamespaceExpression) { currentFailedNamespaceExpression = null; } } - /** - * @param {ESTreeNode} node An ESTree AST node - * @returns {boolean} Returns true if node is a PropertyAccessExpression - */ - function isPropertyAccessExpression(node) { + function isPropertyAccessExpression( + node: TSESTree.Node + ): node is TSESTree.MemberExpression { return node.type === 'MemberExpression' && !node.computed; } - /** - * @param {ESTreeNode} node An ESTree AST node - * @returns {boolean} Returns true if node is an EntityNameExpression - */ - function isEntityNameExpression(node) { + function isEntityNameExpression(node: TSESTree.Node): boolean { return ( node.type === 'Identifier' || (isPropertyAccessExpression(node) && @@ -256,20 +190,12 @@ module.exports = { 'TSEnumDeclaration:exit': exitDeclaration, 'ExportNamedDeclaration[declaration.type="TSModuleDeclaration"]:exit': exitDeclaration, 'ExportNamedDeclaration[declaration.type="TSEnumDeclaration"]:exit': exitDeclaration, - /** - * @param {QualifiedName} node The node being inspected - * @returns {void} - */ - TSQualifiedName(node) { + TSQualifiedName(node: TSESTree.TSQualifiedName): void { visitNamespaceAccess(node, node.left, node.right); }, - /** - * @param {MemberExpression} node The node being inspected - * @returns {void} - */ - MemberExpression(node) { + MemberExpression(node: TSESTree.MemberExpression): void { if (node.computed) return; - const property = /** @type {Identifier} */ (node.property); + const property = node.property as TSESTree.Identifier; if (isEntityNameExpression(node.object)) { visitNamespaceAccess(node, node.object, property); @@ -279,4 +205,4 @@ module.exports = { 'MemberExpression:exit': resetCurrentNamespaceExpression }; } -}; +}); diff --git a/packages/eslint-plugin/tests/lib/rules/no-unnecessary-qualifier.js b/packages/eslint-plugin/tests/rules/no-unnecessary-qualifier.test.ts similarity index 79% rename from packages/eslint-plugin/tests/lib/rules/no-unnecessary-qualifier.js rename to packages/eslint-plugin/tests/rules/no-unnecessary-qualifier.test.ts index 90218cfef83f..3e9587bb6c79 100644 --- a/packages/eslint-plugin/tests/lib/rules/no-unnecessary-qualifier.js +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-qualifier.test.ts @@ -2,20 +2,21 @@ * @fileoverview Warns when a namespace qualifier is unnecessary. * @author Benjamin Lichtman */ -'use strict'; //------------------------------------------------------------------------------ // Requirements //------------------------------------------------------------------------------ -const path = require('path'); -const rule = require('../../../lib/rules/no-unnecessary-qualifier'), - RuleTester = require('eslint').RuleTester; +import path from 'path'; +import rule from '../../src/rules/no-unnecessary-qualifier'; +import { RuleTester } from '../RuleTester'; +import { AST_NODE_TYPES } from '@typescript-eslint/typescript-estree'; //------------------------------------------------------------------------------ // Tests //------------------------------------------------------------------------------ +const messageId = 'unnecessaryQualifier'; const rootPath = path.join(process.cwd(), 'tests/fixtures/'); const ruleTester = new RuleTester({ @@ -71,8 +72,8 @@ namespace A { }`, errors: [ { - messageId: 'unnecessaryQualifier', - type: 'Identifier' + messageId, + type: AST_NODE_TYPES.Identifier } ], output: ` @@ -89,8 +90,8 @@ namespace A { }`, errors: [ { - messageId: 'unnecessaryQualifier', - type: 'Identifier' + messageId, + type: AST_NODE_TYPES.Identifier } ], output: ` @@ -109,8 +110,8 @@ namespace A { }`, errors: [ { - messageId: 'unnecessaryQualifier', - type: 'Identifier' + messageId, + type: AST_NODE_TYPES.Identifier } ], output: ` @@ -131,8 +132,8 @@ namespace A { }`, errors: [ { - messageId: 'unnecessaryQualifier', - type: 'TSQualifiedName' + messageId, + type: AST_NODE_TYPES.TSQualifiedName } ], output: ` @@ -153,8 +154,8 @@ namespace A { }`, errors: [ { - messageId: 'unnecessaryQualifier', - type: 'MemberExpression' + messageId, + type: AST_NODE_TYPES.MemberExpression } ], output: ` @@ -173,8 +174,8 @@ enum A { }`, errors: [ { - messageId: 'unnecessaryQualifier', - type: 'Identifier' + messageId, + type: AST_NODE_TYPES.Identifier } ], output: ` @@ -193,8 +194,8 @@ namespace Foo { }`, errors: [ { - messageId: 'unnecessaryQualifier', - type: 'MemberExpression' + messageId, + type: AST_NODE_TYPES.MemberExpression } ], output: ` @@ -214,8 +215,8 @@ declare module './foo' { filename: path.join(rootPath, 'bar.ts'), errors: [ { - messageId: 'unnecessaryQualifier', - type: 'Identifier' + messageId, + type: AST_NODE_TYPES.Identifier } ], output: ` diff --git a/packages/typescript-estree/src/parser-options.ts b/packages/typescript-estree/src/parser-options.ts index 8a30b27b89ff..9dadb456483b 100644 --- a/packages/typescript-estree/src/parser-options.ts +++ b/packages/typescript-estree/src/parser-options.ts @@ -38,6 +38,7 @@ export interface ParserOptions { export interface ParserWeakMap { get(key: TKey): TValue; + has(key: any): boolean; } export interface ParserServices { From eee94b191f3e3b617d5413255a057c99e7918c14 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Mon, 11 Feb 2019 11:34:23 -0800 Subject: [PATCH 7/7] refactor: use expressive selector --- .../eslint-plugin/src/rules/no-unnecessary-qualifier.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-qualifier.ts b/packages/eslint-plugin/src/rules/no-unnecessary-qualifier.ts index cff7b6b8cacb..f13be913cbb3 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-qualifier.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-qualifier.ts @@ -193,10 +193,10 @@ export default util.createRule({ TSQualifiedName(node: TSESTree.TSQualifiedName): void { visitNamespaceAccess(node, node.left, node.right); }, - MemberExpression(node: TSESTree.MemberExpression): void { - if (node.computed) return; + 'MemberExpression[computed=false]': function( + node: TSESTree.MemberExpression + ): void { const property = node.property as TSESTree.Identifier; - if (isEntityNameExpression(node.object)) { visitNamespaceAccess(node, node.object, property); }