Skip to content

feat(eslint-plugin): add 'no-unnecessary-qualifier' rule #231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Feb 11, 2019
3 changes: 2 additions & 1 deletion packages/eslint-plugin/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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: | |
Expand All @@ -139,6 +139,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 `/// <reference path="" />` 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: | |
Expand Down
3 changes: 2 additions & 1 deletion packages/eslint-plugin/ROADMAP.md
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,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] |
Expand Down Expand Up @@ -606,6 +606,7 @@ Relevant plugins: [`chai-expect-keywords`](https://github.com/gavinaiken/eslint-
[`@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/prefer-function-type`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/prefer-function-type.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

<!-- eslint-plugin-import -->

Expand Down
79 changes: 79 additions & 0 deletions packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
# Warns when a namespace qualifier is unnecessary. (no-unnecessary-qualifier)

## Rule Details

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:

```ts
namespace A {
export type B = number;
const x: A.B = 3;
}
```

```ts
namespace A {
export const x = 3;
export const y = A.x;
}
```

```ts
enum A {
B,
C = A.B
}
```

```ts
namespace A {
export namespace B {
export type T = number;
const x: A.B.T = 3;
}
}
```

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;
namespace Y {
type T = string;
const x: X.T = 0;
}
}
```

## When Not To Use It

If you don't care about having unneeded namespace or enum qualifiers, then you don't need to use this rule.

## Further Reading

- TSLint: [no-unnecessary-qualifier](https://palantir.github.io/tslint/rules/no-unnecessary-qualifier/)
208 changes: 208 additions & 0 deletions packages/eslint-plugin/src/rules/no-unnecessary-qualifier.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,208 @@
/**
* @fileoverview Warns when a namespace qualifier is unnecessary.
* @author Benjamin Lichtman
*/

import { TSESTree } from '@typescript-eslint/typescript-estree';
import ts from 'typescript';
import * as tsutils from 'tsutils';
import * as util from '../util';

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

export default util.createRule({
name: 'no-unnecessary-qualifier',
meta: {
docs: {
category: 'Best Practices',
description: 'Warns when a namespace qualifier is unnecessary.',
recommended: false,
tslintName: 'no-unnecessary-qualifier'
},
fixable: 'code',
messages: {
unnecessaryQualifier:
"Qualifier is unnecessary since '{{ name }}' is in scope."
},
schema: [],
type: 'suggestion'
},
defaultOptions: [],
create(context) {
const namespacesInScope: ts.Node[] = [];
let currentFailedNamespaceExpression: TSESTree.Node | null = null;
const parserServices = util.getParserServices(context);
const esTreeNodeToTSNodeMap = parserServices.esTreeNodeToTSNodeMap;
const program = parserServices.program;
const checker = program.getTypeChecker();
const sourceCode = context.getSourceCode();

//----------------------------------------------------------------------
// Helpers
//----------------------------------------------------------------------

function tryGetAliasedSymbol(
symbol: ts.Symbol,
checker: ts.TypeChecker
): ts.Symbol | null {
return tsutils.isSymbolFlagSet(symbol, ts.SymbolFlags.Alias)
? checker.getAliasedSymbol(symbol)
: null;
}

function symbolIsNamespaceInScope(symbol: ts.Symbol): boolean {
const symbolDeclarations = symbol.getDeclarations() || [];

if (
symbolDeclarations.some(decl =>
namespacesInScope.some(ns => ns === decl)
)
) {
return true;
}

const alias = tryGetAliasedSymbol(symbol, checker);

return alias !== null && symbolIsNamespaceInScope(alias);
}

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);
}

function symbolsAreEqual(accessed: ts.Symbol, inScope: ts.Symbol): boolean {
return accessed === checker.getExportSymbolOfSymbol(inScope);
}

function qualifierIsUnnecessary(
qualifier: TSESTree.Node,
name: TSESTree.Identifier
): boolean {
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)
);
}

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 &&
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]]);
}
});
}
}

function enterDeclaration(node: TSESTree.Node): void {
const tsDeclaration = esTreeNodeToTSNodeMap.get(node);
if (tsDeclaration) {
namespacesInScope.push(tsDeclaration);
}
}

function exitDeclaration(node: TSESTree.Node) {
if (esTreeNodeToTSNodeMap.has(node)) {
namespacesInScope.pop();
}
}

function resetCurrentNamespaceExpression(node: TSESTree.Node): void {
if (node === currentFailedNamespaceExpression) {
currentFailedNamespaceExpression = null;
}
}

function isPropertyAccessExpression(
node: TSESTree.Node
): node is TSESTree.MemberExpression {
return node.type === 'MemberExpression' && !node.computed;
}

function isEntityNameExpression(node: TSESTree.Node): boolean {
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,
TSQualifiedName(node: TSESTree.TSQualifiedName): void {
visitNamespaceAccess(node, node.left, node.right);
},
'MemberExpression[computed=false]': function(
node: TSESTree.MemberExpression
): void {
const property = node.property as TSESTree.Identifier;
if (isEntityNameExpression(node.object)) {
visitNamespaceAccess(node, node.object, property);
}
},
'TSQualifiedName:exit': resetCurrentNamespaceExpression,
'MemberExpression:exit': resetCurrentNamespaceExpression
};
}
});
1 change: 1 addition & 0 deletions packages/eslint-plugin/tests/fixtures/foo.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export type T = number;
Loading