Skip to content

Commit cc8f906

Browse files
feat(eslint-plugin): add 'no-unnecessary-qualifier' rule (typescript-eslint#231)
* feat(eslint-plugin): add no-unnecessary-qualifier rule * docs: add documentation * docs: update README and ROADMAP * test: increase code coverage * docs: add simple correct usage of qualifiers * chore: migrate to ts * refactor: use expressive selector
1 parent b50a68b commit cc8f906

File tree

7 files changed

+522
-2
lines changed

7 files changed

+522
-2
lines changed

Diff for: packages/eslint-plugin/README.md

+2-1
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e
128128
| [`@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: | |
129129
| [`@typescript-eslint/no-explicit-any`](./docs/rules/no-explicit-any.md) | Disallow usage of the `any` type (`no-any` from TSLint) | :heavy_check_mark: | |
130130
| [`@typescript-eslint/no-extraneous-class`](./docs/rules/no-extraneous-class.md) | Forbids the use of classes as namespaces (`no-unnecessary-class` from TSLint) | | |
131-
| [`@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) | | |
131+
| [`@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) | | |
132132
| [`@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: |
133133
| [`@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: | |
134134
| [`@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: | |
@@ -139,6 +139,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e
139139
| [`@typescript-eslint/no-this-alias`](./docs/rules/no-this-alias.md) | Disallow aliasing `this` (`no-this-assignment` from TSLint) | | |
140140
| [`@typescript-eslint/no-triple-slash-reference`](./docs/rules/no-triple-slash-reference.md) | Disallow `/// <reference path="" />` comments (`no-reference` from TSLint) | :heavy_check_mark: | |
141141
| [`@typescript-eslint/no-type-alias`](./docs/rules/no-type-alias.md) | Disallow the use of type aliases (`interface-over-type-literal` from TSLint) | | |
142+
| [`@typescript-eslint/no-unnecessary-qualifier`](./docs/rules/no-unnecessary-qualifier.md) | Warns when a namespace qualifier is unnecessary (`no-unnecessary-qualifier` from TSLint) | | :wrench: |
142143
| [`@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: |
143144
| [`@typescript-eslint/no-unused-vars`](./docs/rules/no-unused-vars.md) | Disallow unused variables (`no-unused-variable` from TSLint) | :heavy_check_mark: | |
144145
| [`@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 for: packages/eslint-plugin/ROADMAP.md

+2-1
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@
163163
| [`no-trailing-whitespace`] | 🌟 | [`no-trailing-spaces`][no-trailing-spaces] |
164164
| [`no-unnecessary-callback-wrapper`] | 🛑 | N/A and this might be unsafe (i.e. with `forEach`) |
165165
| [`no-unnecessary-initializer`] | 🌟 | [`no-undef-init`][no-undef-init] |
166-
| [`no-unnecessary-qualifier`] | 🛑 | N/A |
166+
| [`no-unnecessary-qualifier`] | | [`@typescript-eslint/no-unnecessary-qualifier`][no-unnecessary-qualifier] |
167167
| [`number-literal-format`] | 🛑 | N/A |
168168
| [`object-literal-key-quotes`] | 🌟 | [`quote-props`][quote-props] |
169169
| [`object-literal-shorthand`] | 🌟 | [`object-shorthand`][object-shorthand] |
@@ -606,6 +606,7 @@ Relevant plugins: [`chai-expect-keywords`](https://github.com/gavinaiken/eslint-
606606
[`@typescript-eslint/no-array-constructor`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-array-constructor.md
607607
[`@typescript-eslint/prefer-function-type`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/prefer-function-type.md
608608
[`@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
609+
[`@typescript-eslint/no-unnecessary-qualifier`]: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-unnecessary-qualifier.md
609610

610611
<!-- eslint-plugin-import -->
611612

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
# Warns when a namespace qualifier is unnecessary. (no-unnecessary-qualifier)
2+
3+
## Rule Details
4+
5+
This rule aims to let users know when a namespace or enum qualifier is unnecessary,
6+
whether used for a type or for a value.
7+
8+
Examples of **incorrect** code for this rule:
9+
10+
```ts
11+
namespace A {
12+
export type B = number;
13+
const x: A.B = 3;
14+
}
15+
```
16+
17+
```ts
18+
namespace A {
19+
export const x = 3;
20+
export const y = A.x;
21+
}
22+
```
23+
24+
```ts
25+
enum A {
26+
B,
27+
C = A.B
28+
}
29+
```
30+
31+
```ts
32+
namespace A {
33+
export namespace B {
34+
export type T = number;
35+
const x: A.B.T = 3;
36+
}
37+
}
38+
```
39+
40+
Examples of **correct** code for this rule:
41+
42+
```ts
43+
namespace X {
44+
export type T = number;
45+
}
46+
47+
namespace Y {
48+
export const x: X.T = 3;
49+
}
50+
```
51+
52+
```ts
53+
enum A {
54+
X,
55+
Y
56+
}
57+
58+
enum B {
59+
Z = A.X
60+
}
61+
```
62+
63+
```ts
64+
namespace X {
65+
export type T = number;
66+
namespace Y {
67+
type T = string;
68+
const x: X.T = 0;
69+
}
70+
}
71+
```
72+
73+
## When Not To Use It
74+
75+
If you don't care about having unneeded namespace or enum qualifiers, then you don't need to use this rule.
76+
77+
## Further Reading
78+
79+
- TSLint: [no-unnecessary-qualifier](https://palantir.github.io/tslint/rules/no-unnecessary-qualifier/)
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
/**
2+
* @fileoverview Warns when a namespace qualifier is unnecessary.
3+
* @author Benjamin Lichtman
4+
*/
5+
6+
import { TSESTree } from '@typescript-eslint/typescript-estree';
7+
import ts from 'typescript';
8+
import * as tsutils from 'tsutils';
9+
import * as util from '../util';
10+
11+
//------------------------------------------------------------------------------
12+
// Rule Definition
13+
//------------------------------------------------------------------------------
14+
15+
export default util.createRule({
16+
name: 'no-unnecessary-qualifier',
17+
meta: {
18+
docs: {
19+
category: 'Best Practices',
20+
description: 'Warns when a namespace qualifier is unnecessary.',
21+
recommended: false,
22+
tslintName: 'no-unnecessary-qualifier'
23+
},
24+
fixable: 'code',
25+
messages: {
26+
unnecessaryQualifier:
27+
"Qualifier is unnecessary since '{{ name }}' is in scope."
28+
},
29+
schema: [],
30+
type: 'suggestion'
31+
},
32+
defaultOptions: [],
33+
create(context) {
34+
const namespacesInScope: ts.Node[] = [];
35+
let currentFailedNamespaceExpression: TSESTree.Node | null = null;
36+
const parserServices = util.getParserServices(context);
37+
const esTreeNodeToTSNodeMap = parserServices.esTreeNodeToTSNodeMap;
38+
const program = parserServices.program;
39+
const checker = program.getTypeChecker();
40+
const sourceCode = context.getSourceCode();
41+
42+
//----------------------------------------------------------------------
43+
// Helpers
44+
//----------------------------------------------------------------------
45+
46+
function tryGetAliasedSymbol(
47+
symbol: ts.Symbol,
48+
checker: ts.TypeChecker
49+
): ts.Symbol | null {
50+
return tsutils.isSymbolFlagSet(symbol, ts.SymbolFlags.Alias)
51+
? checker.getAliasedSymbol(symbol)
52+
: null;
53+
}
54+
55+
function symbolIsNamespaceInScope(symbol: ts.Symbol): boolean {
56+
const symbolDeclarations = symbol.getDeclarations() || [];
57+
58+
if (
59+
symbolDeclarations.some(decl =>
60+
namespacesInScope.some(ns => ns === decl)
61+
)
62+
) {
63+
return true;
64+
}
65+
66+
const alias = tryGetAliasedSymbol(symbol, checker);
67+
68+
return alias !== null && symbolIsNamespaceInScope(alias);
69+
}
70+
71+
function getSymbolInScope(
72+
node: ts.Node,
73+
flags: ts.SymbolFlags,
74+
name: string
75+
): ts.Symbol | undefined {
76+
// TODO:PERF `getSymbolsInScope` gets a long list. Is there a better way?
77+
const scope = checker.getSymbolsInScope(node, flags);
78+
79+
return scope.find(scopeSymbol => scopeSymbol.name === name);
80+
}
81+
82+
function symbolsAreEqual(accessed: ts.Symbol, inScope: ts.Symbol): boolean {
83+
return accessed === checker.getExportSymbolOfSymbol(inScope);
84+
}
85+
86+
function qualifierIsUnnecessary(
87+
qualifier: TSESTree.Node,
88+
name: TSESTree.Identifier
89+
): boolean {
90+
const tsQualifier = esTreeNodeToTSNodeMap.get(qualifier);
91+
const tsName = esTreeNodeToTSNodeMap.get(name);
92+
93+
if (!(tsQualifier && tsName)) return false; // TODO: throw error?
94+
95+
const namespaceSymbol = checker.getSymbolAtLocation(tsQualifier);
96+
97+
if (
98+
typeof namespaceSymbol === 'undefined' ||
99+
!symbolIsNamespaceInScope(namespaceSymbol)
100+
) {
101+
return false;
102+
}
103+
104+
const accessedSymbol = checker.getSymbolAtLocation(tsName);
105+
106+
if (typeof accessedSymbol === 'undefined') {
107+
return false;
108+
}
109+
110+
// If the symbol in scope is different, the qualifier is necessary.
111+
const fromScope = getSymbolInScope(
112+
tsQualifier,
113+
accessedSymbol.flags,
114+
sourceCode.getText(name)
115+
);
116+
117+
return (
118+
typeof fromScope === 'undefined' ||
119+
symbolsAreEqual(accessedSymbol, fromScope)
120+
);
121+
}
122+
123+
function visitNamespaceAccess(
124+
node: TSESTree.Node,
125+
qualifier: TSESTree.Node,
126+
name: TSESTree.Identifier
127+
): void {
128+
// Only look for nested qualifier errors if we didn't already fail on the outer qualifier.
129+
if (
130+
!currentFailedNamespaceExpression &&
131+
qualifierIsUnnecessary(qualifier, name)
132+
) {
133+
currentFailedNamespaceExpression = node;
134+
context.report({
135+
node: qualifier,
136+
messageId: 'unnecessaryQualifier',
137+
data: {
138+
name: sourceCode.getText(name)
139+
},
140+
fix(fixer) {
141+
return fixer.removeRange([qualifier.range[0], name.range[0]]);
142+
}
143+
});
144+
}
145+
}
146+
147+
function enterDeclaration(node: TSESTree.Node): void {
148+
const tsDeclaration = esTreeNodeToTSNodeMap.get(node);
149+
if (tsDeclaration) {
150+
namespacesInScope.push(tsDeclaration);
151+
}
152+
}
153+
154+
function exitDeclaration(node: TSESTree.Node) {
155+
if (esTreeNodeToTSNodeMap.has(node)) {
156+
namespacesInScope.pop();
157+
}
158+
}
159+
160+
function resetCurrentNamespaceExpression(node: TSESTree.Node): void {
161+
if (node === currentFailedNamespaceExpression) {
162+
currentFailedNamespaceExpression = null;
163+
}
164+
}
165+
166+
function isPropertyAccessExpression(
167+
node: TSESTree.Node
168+
): node is TSESTree.MemberExpression {
169+
return node.type === 'MemberExpression' && !node.computed;
170+
}
171+
172+
function isEntityNameExpression(node: TSESTree.Node): boolean {
173+
return (
174+
node.type === 'Identifier' ||
175+
(isPropertyAccessExpression(node) &&
176+
isEntityNameExpression(node.object))
177+
);
178+
}
179+
180+
//----------------------------------------------------------------------
181+
// Public
182+
//----------------------------------------------------------------------
183+
184+
return {
185+
TSModuleDeclaration: enterDeclaration,
186+
TSEnumDeclaration: enterDeclaration,
187+
'ExportNamedDeclaration[declaration.type="TSModuleDeclaration"]': enterDeclaration,
188+
'ExportNamedDeclaration[declaration.type="TSEnumDeclaration"]': enterDeclaration,
189+
'TSModuleDeclaration:exit': exitDeclaration,
190+
'TSEnumDeclaration:exit': exitDeclaration,
191+
'ExportNamedDeclaration[declaration.type="TSModuleDeclaration"]:exit': exitDeclaration,
192+
'ExportNamedDeclaration[declaration.type="TSEnumDeclaration"]:exit': exitDeclaration,
193+
TSQualifiedName(node: TSESTree.TSQualifiedName): void {
194+
visitNamespaceAccess(node, node.left, node.right);
195+
},
196+
'MemberExpression[computed=false]': function(
197+
node: TSESTree.MemberExpression
198+
): void {
199+
const property = node.property as TSESTree.Identifier;
200+
if (isEntityNameExpression(node.object)) {
201+
visitNamespaceAccess(node, node.object, property);
202+
}
203+
},
204+
'TSQualifiedName:exit': resetCurrentNamespaceExpression,
205+
'MemberExpression:exit': resetCurrentNamespaceExpression
206+
};
207+
}
208+
});

Diff for: packages/eslint-plugin/tests/fixtures/foo.ts

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
export type T = number;

0 commit comments

Comments
 (0)