Skip to content

Commit

Permalink
[New] no-namespace: Make rule fixable
Browse files Browse the repository at this point in the history
 - Add guards to avoid crashing older versions of ESLint
 - Note that no-namespace's --fix requires ESLint 5+
 - Prevent no-namespace --fix tests from running under ESLint < 5
  • Loading branch information
Trevor Burnham authored and ljharb committed Jun 29, 2019
1 parent 3704801 commit d030d8e
Show file tree
Hide file tree
Showing 5 changed files with 228 additions and 20 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel

### Added
- [`group-exports`]: make aggregate module exports valid ([#1472], thanks [@atikenny])
- [`no-namespace`]: Make rule fixable ([#1401], thanks [@TrevorBurnham])

### Added
- support `parseForESLint` from custom parser ([#1435], thanks [@JounQin])
Expand Down Expand Up @@ -617,6 +618,7 @@ for info on changes for earlier releases.
[#1412]: https://github.com/benmosher/eslint-plugin-import/pull/1412
[#1409]: https://github.com/benmosher/eslint-plugin-import/pull/1409
[#1404]: https://github.com/benmosher/eslint-plugin-import/pull/1404
[#1401]: https://github.com/benmosher/eslint-plugin-import/pull/1401
[#1393]: https://github.com/benmosher/eslint-plugin-import/pull/1393
[#1389]: https://github.com/benmosher/eslint-plugin-import/pull/1389
[#1377]: https://github.com/benmosher/eslint-plugin-import/pull/1377
Expand Down Expand Up @@ -989,3 +991,4 @@ for info on changes for earlier releases.
[@JounQin]: https://github.com/JounQin
[@atikenny]: https://github.com/atikenny
[@schmidsi]: https://github.com/schmidsi
[@TrevorBurnham]: https://github.com/TrevorBurnham
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
* Ensure all imports appear before other statements ([`first`])
* Ensure all exports appear after other statements ([`exports-last`])
* Report repeated import of the same module in multiple places ([`no-duplicates`])
* Report namespace imports ([`no-namespace`])
* Forbid namespace (a.k.a. "wildcard" `*`) imports ([`no-namespace`])
* Ensure consistent use of file extension within the import path ([`extensions`])
* Enforce a convention in module import order ([`order`])
* Enforce a newline after import statements ([`newline-after-import`])
Expand Down
10 changes: 8 additions & 2 deletions docs/rules/no-namespace.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
# import/no-namespace

Reports if namespace import is used.
Enforce a convention of not using namespace (a.k.a. "wildcard" `*`) imports.

+(fixable) The `--fix` option on the [command line] automatically fixes problems reported by this rule, provided that the namespace object is only used for direct member access, e.g. `namespace.a`.
The `--fix` functionality for this rule requires ESLint 5 or newer.

## Rule Details

Expand All @@ -12,10 +15,13 @@ import { a, b } from './bar'
import defaultExport, { a, b } from './foobar'
```

...whereas here imports will be reported:
Invalid:

```js
import * as foo from 'foo';
```

```js
import defaultExport, * as foo from 'foo';
```

Expand Down
132 changes: 131 additions & 1 deletion src/rules/no-namespace.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,143 @@ module.exports = {
docs: {
url: docsUrl('no-namespace'),
},
fixable: 'code',
},

create: function (context) {
return {
'ImportNamespaceSpecifier': function (node) {
context.report(node, `Unexpected namespace import.`)
const scopeVariables = context.getScope().variables
const namespaceVariable = scopeVariables.find((variable) =>
variable.defs[0].node === node
)
const namespaceReferences = namespaceVariable.references
const namespaceIdentifiers = namespaceReferences.map(reference => reference.identifier)
const canFix = namespaceIdentifiers.length > 0 && !usesNamespaceAsObject(namespaceIdentifiers)

context.report({
node,
message: `Unexpected namespace import.`,
fix: canFix && (fixer => {
const scopeManager = context.getSourceCode().scopeManager
const fixes = []

// Pass 1: Collect variable names that are already in scope for each reference we want
// to transform, so that we can be sure that we choose non-conflicting import names
const importNameConflicts = {}
namespaceIdentifiers.forEach((identifier) => {
const parent = identifier.parent
if (parent && parent.type === 'MemberExpression') {
const importName = getMemberPropertyName(parent)
const localConflicts = getVariableNamesInScope(scopeManager, parent)
if (!importNameConflicts[importName]) {
importNameConflicts[importName] = localConflicts
} else {
localConflicts.forEach((c) => importNameConflicts[importName].add(c))
}
}
})

// Choose new names for each import
const importNames = Object.keys(importNameConflicts)
const importLocalNames = generateLocalNames(
importNames,
importNameConflicts,
namespaceVariable.name
)

// Replace the ImportNamespaceSpecifier with a list of ImportSpecifiers
const namedImportSpecifiers = importNames.map((importName) =>
importName === importLocalNames[importName]
? importName
: `${importName} as ${importLocalNames[importName]}`
)
fixes.push(fixer.replaceText(node, `{ ${namedImportSpecifiers.join(', ')} }`))

// Pass 2: Replace references to the namespace with references to the named imports
namespaceIdentifiers.forEach((identifier) => {
const parent = identifier.parent
if (parent && parent.type === 'MemberExpression') {
const importName = getMemberPropertyName(parent)
fixes.push(fixer.replaceText(parent, importLocalNames[importName]))
}
})

return fixes
}),
})
},
}
},
}

/**
* @param {Identifier[]} namespaceIdentifiers
* @returns {boolean} `true` if the namespace variable is more than just a glorified constant
*/
function usesNamespaceAsObject(namespaceIdentifiers) {
return !namespaceIdentifiers.every((identifier) => {
const parent = identifier.parent

// `namespace.x` or `namespace['x']`
return (
parent && parent.type === 'MemberExpression' &&
(parent.property.type === 'Identifier' || parent.property.type === 'Literal')
)
})
}

/**
* @param {MemberExpression} memberExpression
* @returns {string} the name of the member in the object expression, e.g. the `x` in `namespace.x`
*/
function getMemberPropertyName(memberExpression) {
return memberExpression.property.type === 'Identifier'
? memberExpression.property.name
: memberExpression.property.value
}

/**
* @param {ScopeManager} scopeManager
* @param {ASTNode} node
* @return {Set<string>}
*/
function getVariableNamesInScope(scopeManager, node) {
let currentNode = node
let scope = scopeManager.acquire(currentNode)
while (scope == null) {
currentNode = currentNode.parent
scope = scopeManager.acquire(currentNode, true)
}
return new Set([
...scope.variables.map(variable => variable.name),
...scope.upper.variables.map(variable => variable.name),
])
}

/**
*
* @param {*} names
* @param {*} nameConflicts
* @param {*} namespaceName
*/
function generateLocalNames(names, nameConflicts, namespaceName) {
const localNames = {}
names.forEach((name) => {
let localName
if (!nameConflicts[name].has(name)) {
localName = name
} else if (!nameConflicts[name].has(`${namespaceName}_${name}`)) {
localName = `${namespaceName}_${name}`
} else {
for (let i = 1; i < Infinity; i++) {
if (!nameConflicts[name].has(`${namespaceName}_${name}_${i}`)) {
localName = `${namespaceName}_${name}_${i}`
break
}
}
}
localNames[name] = localName
})
return localNames
}
101 changes: 85 additions & 16 deletions tests/src/rules/no-namespace.js
Original file line number Diff line number Diff line change
@@ -1,44 +1,113 @@
import { RuleTester } from 'eslint'
import eslintPkg from 'eslint/package.json'
import semver from 'semver'
import { test } from '../utils'

const ERROR_MESSAGE = 'Unexpected namespace import.'

const ruleTester = new RuleTester()

// --fix functionality requires ESLint 5+
const FIX_TESTS = semver.satisfies(eslintPkg.version, '>5.0.0') ? [
test({
code: `
import * as foo from './foo';
florp(foo.bar);
florp(foo['baz']);
`.trim(),
output: `
import { bar, baz } from './foo';
florp(bar);
florp(baz);
`.trim(),
errors: [ {
line: 1,
column: 8,
message: ERROR_MESSAGE,
}],
}),
test({
code: `
import * as foo from './foo';
const bar = 'name conflict';
const baz = 'name conflict';
const foo_baz = 'name conflict';
florp(foo.bar);
florp(foo['baz']);
`.trim(),
output: `
import { bar as foo_bar, baz as foo_baz_1 } from './foo';
const bar = 'name conflict';
const baz = 'name conflict';
const foo_baz = 'name conflict';
florp(foo_bar);
florp(foo_baz_1);
`.trim(),
errors: [ {
line: 1,
column: 8,
message: ERROR_MESSAGE,
}],
}),
test({
code: `
import * as foo from './foo';
function func(arg) {
florp(foo.func);
florp(foo['arg']);
}
`.trim(),
output: `
import { func as foo_func, arg as foo_arg } from './foo';
function func(arg) {
florp(foo_func);
florp(foo_arg);
}
`.trim(),
errors: [ {
line: 1,
column: 8,
message: ERROR_MESSAGE,
}],
}),
] : []

ruleTester.run('no-namespace', require('rules/no-namespace'), {
valid: [
{ code: "import { a, b } from 'foo';", parserOptions: { ecmaVersion: 2015, sourceType: 'module' } },
{ code: "import { a, b } from './foo';", parserOptions: { ecmaVersion: 2015, sourceType: 'module' } },
{ code: "import bar from 'bar';", parserOptions: { ecmaVersion: 2015, sourceType: 'module' } },
{ code: "import bar from './bar';", parserOptions: { ecmaVersion: 2015, sourceType: 'module' } },
{ code: 'import { a, b } from \'foo\';', parserOptions: { ecmaVersion: 2015, sourceType: 'module' } },
{ code: 'import { a, b } from \'./foo\';', parserOptions: { ecmaVersion: 2015, sourceType: 'module' } },
{ code: 'import bar from \'bar\';', parserOptions: { ecmaVersion: 2015, sourceType: 'module' } },
{ code: 'import bar from \'./bar\';', parserOptions: { ecmaVersion: 2015, sourceType: 'module' } },
],

invalid: [
{
code: "import * as foo from 'foo';",
test({
code: 'import * as foo from \'foo\';',
output: 'import * as foo from \'foo\';',
errors: [ {
line: 1,
column: 8,
message: ERROR_MESSAGE,
} ],
parserOptions: { ecmaVersion: 2015, sourceType: 'module' },
},
{
code: "import defaultExport, * as foo from 'foo';",
}),
test({
code: 'import defaultExport, * as foo from \'foo\';',
output: 'import defaultExport, * as foo from \'foo\';',
errors: [ {
line: 1,
column: 23,
message: ERROR_MESSAGE,
} ],
parserOptions: { ecmaVersion: 2015, sourceType: 'module' },
},
{
code: "import * as foo from './foo';",
}),
test({
code: 'import * as foo from \'./foo\';',
output: 'import * as foo from \'./foo\';',
errors: [ {
line: 1,
column: 8,
message: ERROR_MESSAGE,
} ],
parserOptions: { ecmaVersion: 2015, sourceType: 'module' },
},
}),
...FIX_TESTS,
],
})

0 comments on commit d030d8e

Please sign in to comment.