From 692c0aa63daa2dadada2fda7acbf8692b00199fa Mon Sep 17 00:00:00 2001 From: Ethan Resnick Date: Wed, 23 May 2018 01:04:50 -0400 Subject: [PATCH 1/2] import-blacklist: support banning specific named imports --- src/rules/importBlacklistRule.ts | 189 +++++++++++++++++++++-- test/rules/import-blacklist/test.ts.lint | 41 ++++- test/rules/import-blacklist/tslint.json | 2 +- 3 files changed, 213 insertions(+), 19 deletions(-) diff --git a/src/rules/importBlacklistRule.ts b/src/rules/importBlacklistRule.ts index c8a3dcb95a0..d6530fd2e4a 100644 --- a/src/rules/importBlacklistRule.ts +++ b/src/rules/importBlacklistRule.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { findImports, ImportKind } from "tsutils"; +import { findImports, ImportKind, isExportDeclaration, isImportDeclaration, isNamedImports } from "tsutils"; import * as ts from "typescript"; import * as Lint from "../index"; @@ -24,25 +24,59 @@ export class Rule extends Lint.Rules.AbstractRule { public static metadata: Lint.IRuleMetadata = { ruleName: "import-blacklist", description: Lint.Utils.dedent` - Disallows importing the specified modules directly via \`import\` and \`require\`. - Instead only sub modules may be imported from that module.`, + Disallows importing the specified modules via \`import\` and \`require\`, + or importing specific named exports of the specified modules.`, rationale: Lint.Utils.dedent` - Some libraries allow importing their submodules instead of the entire module. - This is good practise as it avoids loading unused modules.`, - optionsDescription: "A list of blacklisted modules.", + For some libraries, importing the library directly can cause unused + submodules to be loaded, so you may want to block these imports and + require that users directly import only the submodules they need. + In other cases, you may simply want to ban an import because using + it is undesirable or unsafe.`, + optionsDescription: "A list of blacklisted modules or named imports.", options: { type: "array", items: { - type: "string", + oneOf: [ + { + type: "string", + minLength: 1, + }, + { + type: "object", + additionalProperties: { + type: "array", + minItems: 1, + items: { + type: "string", + minLength: 1, + }, + }, + }, + ], }, - minLength: 1, }, - optionExamples: [true, [true, "rxjs", "lodash"]], + optionExamples: [ + true, + [true, "rxjs", "lodash"], + [true, "lodash", { lodash: ["pull", "pullAll"] }], + ], type: "functionality", typescriptOnly: false, }; - public static FAILURE_STRING = "This import is blacklisted, import a submodule instead"; + public static WHOLE_MODULE_FAILURE_STRING = + "Importing this module is blacklisted. Try importing a submodule instead."; + + public static IMPLICIT_NAMED_IMPORT_FAILURE_STRING = + "Some named exports from this module are blacklisted for importing " + + "(or re-exporting). Import/re-export only the specific values you want, " + + "instead of the whole module."; + + public static MAKE_NAMED_IMPORT_FAILURE_STRING(importName: string) { + return importName === "default" + ? "Importing (or re-exporting) the default export is blacklisted." + : `The export "${importName}" is blacklisted.`; + } public isEnabled(): boolean { return super.isEnabled() && this.ruleArguments.length > 0; @@ -54,9 +88,140 @@ export class Rule extends Lint.Rules.AbstractRule { } function walk(ctx: Lint.WalkContext) { + type Options = Array; + interface BannedImports { + [moduleName: string]: true | Set; + } + + // Merge/normalize options. + // E.g., ["a", { "b": ["c"], "d": ["e"] }, "f", { "f": ["g"] }] + // becomes { "a": true, "b": Set(["c"]), "d": Set(["e"]), "f": true }. + const bannedImports = (ctx.options as Options).reduce( + (acc, it) => { + if (typeof it === "string") { + acc[it] = true; + } else { + Object.keys(it).forEach((moduleName) => { + if (acc[moduleName] instanceof Set) { + it[moduleName].forEach((bannedImport) => { + (acc[moduleName] as Set).add(bannedImport); + }); + } else if (acc[moduleName] !== true) { + acc[moduleName] = new Set(it[moduleName]); + } + }); + } + return acc; + }, + Object.create(null) as BannedImports, + ); + for (const name of findImports(ctx.sourceFile, ImportKind.All)) { - if (ctx.options.indexOf(name.text) !== -1) { - ctx.addFailure(name.getStart(ctx.sourceFile) + 1, name.end - 1, Rule.FAILURE_STRING); + // TODO #3963: Resolve/normalize relative file imports to a canonical path? + const importedModule = name.text; + const bansForModule = bannedImports[importedModule]; + + // Check if at least some imports from this module are banned. + if (bansForModule !== undefined) { + // If importing this module is totally banned, we can error now, + // without determining whether the user is importing the whole + // module or named exports. + if (bansForModule === true) { + ctx.addFailure( + name.getStart(ctx.sourceFile) + 1, + name.end - 1, + Rule.WHOLE_MODULE_FAILURE_STRING, + ); + continue; + } + + // Otherwise, find the named imports, if any, and fail if the + // user tried to import any of them. We don't have named imports + // when the user is importing the whole module, which includes: + // + // - ImportKind.Require (i.e., `require('module-specifier')`), + // - ImportKind.DynamicImport (i.e., `import("module-specifier")`), + // - ImportKind.ImportEquals (i.e., `import x = require()`), + // - and ImportKind.ImportDeclaration, where there's a full namespace + // import (i.e. `import * as x from "module-specifier"`) + // + // However, namedImports will be an array when we have one of the + // various permutations of `import x, { a, b as c } from "y"`. + // + // We treat re-exports from other modules the same as attempting to + // import the re-exported binding(s), as the re-export is essentially + // an import followed by an export, and not treating these as an + // import would allow backdoor imports of the banned bindings. So, + // our last case is `ImportKind.ExportFrom`, and for that: + // + // - `export nameForDefault from "module"` isn't part of the ESM + // syntax (yet), so we only have to handle two cases below: + // `export { x } from "y"` and `export * from "specifier"`. + const parentNode = name.parent; + + // Disable strict-boolean-expressions for the next few lines so our && + // checks can help type inference figure out if when don't have undefined. + // tslint:disable strict-boolean-expressions + + const importClause = parentNode && isImportDeclaration(parentNode) + ? parentNode.importClause + : undefined; + + const importsDefaultExport = importClause && Boolean(importClause.name); + + // Below, check isNamedImports to rule out the + // `import * as ns from "..."` case. + const importsSpecificNamedExports = importClause + && importClause.namedBindings + && isNamedImports(importClause.namedBindings); + + // If parentNode is an ExportDeclaration, it must represent an + // `export from`, as findImports verifies that. Then, if exportClause + // is undefined, we're dealing with `export * from ...`. + const reExportsSpecificNamedExports = parentNode + && isExportDeclaration(parentNode) + && Boolean(parentNode.exportClause); + + // tslint:enable strict-boolean-expressions + + if (importsDefaultExport || importsSpecificNamedExports || reExportsSpecificNamedExports) { + // Add an import for the default import and any named bindings. + // For the named bindings, we use the name of the export from the + // module (i.e., .propertyName) over its alias in the import when + // the two diverge. + const toExportName = (it: ts.ImportSpecifier | ts.ExportSpecifier) => + (it.propertyName || it.name).text; // tslint:disable-line strict-boolean-expressions + + const exportClause = reExportsSpecificNamedExports + ? (parentNode as ts.ExportDeclaration).exportClause! + : undefined; + + const namedImportsOrReExports = [ + ...(importsDefaultExport ? ["default"] : []), + ...(importsSpecificNamedExports + ? (importClause!.namedBindings as ts.NamedImports).elements.map(toExportName) + : []), + ...(exportClause !== undefined + ? exportClause.elements.map(toExportName) + : []), + ]; + + for (const importName of namedImportsOrReExports) { + if (bansForModule.has(importName)) { + ctx.addFailureAtNode( + exportClause !== undefined ? exportClause : importClause!, + Rule.MAKE_NAMED_IMPORT_FAILURE_STRING(importName), + ); + } + } + } else { + // If we're here, the user tried to import/re-export the whole module + ctx.addFailure( + name.getStart(ctx.sourceFile) + 1, + name.end - 1, + Rule.IMPLICIT_NAMED_IMPORT_FAILURE_STRING, + ); + } } } } diff --git a/test/rules/import-blacklist/test.ts.lint b/test/rules/import-blacklist/test.ts.lint index 14887486fd6..c66225fdc76 100644 --- a/test/rules/import-blacklist/test.ts.lint +++ b/test/rules/import-blacklist/test.ts.lint @@ -2,9 +2,12 @@ import { Observable } from 'rxjs'; ~~~~ [0] import { Observable } from 'rxjs/Observable'; -import forOwn = require('lodash'); - ~~~~~~ [0] -import forOwn = require('lodash/forOwn'); +import rxjs = require('rxjs'); + ~~~~ [0] +import Observable = require('rxjs/Observable'); + +import * as rxjs from "rxjs"; + ~~~~ [0] // non-static imports cannot be checked import {Observable} from rxjs; @@ -12,7 +15,33 @@ import forOwn = require(lodash); import * as notBlacklisted from "not-blacklisted"; -export * from 'lodash'; - ~~~~~~ [0] +export * from 'rxjs'; + ~~~~ [0] + +export { default } from "dummy"; + ~~~~~~~~~~~ [3] + +export * from "lodash"; + ~~~~~~ [1] + +import * as lodash from "lodash"; + ~~~~~~ [1] +import { difference } from "lodash"; + +import { pull, difference } from "lodash"; + ~~~~~~~~~~~~~~~~~~~~ [2] + +import lodash, { pull } from "lodash"; + ~~~~~~~~~~~~~~~~ [2] + +import { pull as notPull } from "lodash"; + ~~~~~~~~~~~~~~~~~~~ [2] + +import test from "dummy"; + ~~~~ [3] + -[0]: This import is blacklisted, import a submodule instead +[0]: Importing this module is blacklisted. Try importing a submodule instead. +[1]: Some named exports from this module are blacklisted for importing (or re-exporting). Import/re-export only the specific values you want, instead of the whole module. +[2]: The export "pull" is blacklisted. +[3]: Importing (or re-exporting) the default export is blacklisted. diff --git a/test/rules/import-blacklist/tslint.json b/test/rules/import-blacklist/tslint.json index 7abdb4f44a8..3c543ffa93f 100644 --- a/test/rules/import-blacklist/tslint.json +++ b/test/rules/import-blacklist/tslint.json @@ -1,5 +1,5 @@ { "rules": { - "import-blacklist": [true, "lodash", "rxjs"] + "import-blacklist": [true, "rxjs", { "lodash": ["pullAll", "pull"] }, { "dummy": ["default"] }] } } From c9581f72b8fb4121ea601dc88177cf8acc7400d3 Mon Sep 17 00:00:00 2001 From: Ethan Resnick Date: Sun, 16 Sep 2018 17:26:17 -0400 Subject: [PATCH 2/2] import-blacklist: small cleanup --- src/rules/importBlacklistRule.ts | 78 ++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 34 deletions(-) diff --git a/src/rules/importBlacklistRule.ts b/src/rules/importBlacklistRule.ts index d6530fd2e4a..bd8444a22ba 100644 --- a/src/rules/importBlacklistRule.ts +++ b/src/rules/importBlacklistRule.ts @@ -15,7 +15,13 @@ * limitations under the License. */ -import { findImports, ImportKind, isExportDeclaration, isImportDeclaration, isNamedImports } from "tsutils"; +import { + findImports, + ImportKind, + isExportDeclaration, + isImportDeclaration, + isNamedImports +} from "tsutils"; import * as ts from "typescript"; import * as Lint from "../index"; @@ -39,7 +45,7 @@ export class Rule extends Lint.Rules.AbstractRule { oneOf: [ { type: "string", - minLength: 1, + minLength: 1 }, { type: "object", @@ -48,20 +54,20 @@ export class Rule extends Lint.Rules.AbstractRule { minItems: 1, items: { type: "string", - minLength: 1, - }, - }, - }, - ], - }, + minLength: 1 + } + } + } + ] + } }, optionExamples: [ true, [true, "rxjs", "lodash"], - [true, "lodash", { lodash: ["pull", "pullAll"] }], + [true, "lodash", { lodash: ["pull", "pullAll"] }] ], type: "functionality", - typescriptOnly: false, + typescriptOnly: false }; public static WHOLE_MODULE_FAILURE_STRING = @@ -87,23 +93,24 @@ export class Rule extends Lint.Rules.AbstractRule { } } -function walk(ctx: Lint.WalkContext) { - type Options = Array; +type Options = Array; + +function walk(ctx: Lint.WalkContext) { interface BannedImports { [moduleName: string]: true | Set; } // Merge/normalize options. - // E.g., ["a", { "b": ["c"], "d": ["e"] }, "f", { "f": ["g"] }] + // E.g., ["a", { "b": ["c"], "d": ["e", "e"] }, "f", { "f": ["g"] }] // becomes { "a": true, "b": Set(["c"]), "d": Set(["e"]), "f": true }. - const bannedImports = (ctx.options as Options).reduce( + const bannedImports = ctx.options.reduce( (acc, it) => { if (typeof it === "string") { acc[it] = true; } else { - Object.keys(it).forEach((moduleName) => { + Object.keys(it).forEach(moduleName => { if (acc[moduleName] instanceof Set) { - it[moduleName].forEach((bannedImport) => { + it[moduleName].forEach(bannedImport => { (acc[moduleName] as Set).add(bannedImport); }); } else if (acc[moduleName] !== true) { @@ -113,7 +120,7 @@ function walk(ctx: Lint.WalkContext) { } return acc; }, - Object.create(null) as BannedImports, + Object.create(null) as BannedImports ); for (const name of findImports(ctx.sourceFile, ImportKind.All)) { @@ -130,7 +137,7 @@ function walk(ctx: Lint.WalkContext) { ctx.addFailure( name.getStart(ctx.sourceFile) + 1, name.end - 1, - Rule.WHOLE_MODULE_FAILURE_STRING, + Rule.WHOLE_MODULE_FAILURE_STRING ); continue; } @@ -163,28 +170,31 @@ function walk(ctx: Lint.WalkContext) { // checks can help type inference figure out if when don't have undefined. // tslint:disable strict-boolean-expressions - const importClause = parentNode && isImportDeclaration(parentNode) - ? parentNode.importClause - : undefined; + const importClause = + parentNode && isImportDeclaration(parentNode) ? parentNode.importClause : undefined; const importsDefaultExport = importClause && Boolean(importClause.name); // Below, check isNamedImports to rule out the // `import * as ns from "..."` case. - const importsSpecificNamedExports = importClause - && importClause.namedBindings - && isNamedImports(importClause.namedBindings); + const importsSpecificNamedExports = + importClause && + importClause.namedBindings && + isNamedImports(importClause.namedBindings); // If parentNode is an ExportDeclaration, it must represent an // `export from`, as findImports verifies that. Then, if exportClause // is undefined, we're dealing with `export * from ...`. - const reExportsSpecificNamedExports = parentNode - && isExportDeclaration(parentNode) - && Boolean(parentNode.exportClause); + const reExportsSpecificNamedExports = + parentNode && isExportDeclaration(parentNode) && Boolean(parentNode.exportClause); // tslint:enable strict-boolean-expressions - if (importsDefaultExport || importsSpecificNamedExports || reExportsSpecificNamedExports) { + if ( + importsDefaultExport || + importsSpecificNamedExports || + reExportsSpecificNamedExports + ) { // Add an import for the default import and any named bindings. // For the named bindings, we use the name of the export from the // module (i.e., .propertyName) over its alias in the import when @@ -199,18 +209,18 @@ function walk(ctx: Lint.WalkContext) { const namedImportsOrReExports = [ ...(importsDefaultExport ? ["default"] : []), ...(importsSpecificNamedExports - ? (importClause!.namedBindings as ts.NamedImports).elements.map(toExportName) - : []), - ...(exportClause !== undefined - ? exportClause.elements.map(toExportName) + ? (importClause!.namedBindings as ts.NamedImports).elements.map( + toExportName + ) : []), + ...(exportClause !== undefined ? exportClause.elements.map(toExportName) : []) ]; for (const importName of namedImportsOrReExports) { if (bansForModule.has(importName)) { ctx.addFailureAtNode( exportClause !== undefined ? exportClause : importClause!, - Rule.MAKE_NAMED_IMPORT_FAILURE_STRING(importName), + Rule.MAKE_NAMED_IMPORT_FAILURE_STRING(importName) ); } } @@ -219,7 +229,7 @@ function walk(ctx: Lint.WalkContext) { ctx.addFailure( name.getStart(ctx.sourceFile) + 1, name.end - 1, - Rule.IMPLICIT_NAMED_IMPORT_FAILURE_STRING, + Rule.IMPLICIT_NAMED_IMPORT_FAILURE_STRING ); } }