Skip to content

Commit

Permalink
chore: migrate forbidden imports check to ESLint (#6302)
Browse files Browse the repository at this point in the history
  • Loading branch information
@AlCalzone
AlCalzone authored Sep 22, 2023
1 parent 20b1e00 commit 674c32a
Show file tree
Hide file tree
Showing 6 changed files with 186 additions and 421 deletions.
6 changes: 6 additions & 0 deletions .eslintrc.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,5 +188,11 @@ module.exports = {
"@zwave-js/consistent-cc-classes": "error",
},
},
{
files: ["packages/**/*.ts"],
rules: {
"@zwave-js/no-forbidden-imports": "error",
},
},
],
};
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/index.ts
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import { ccAPIValidateArgs } from "./rules/ccapi-validate-args.js";
import { consistentCCClasses } from "./rules/consistent-cc-classes.js";
import { noDebugInTests } from "./rules/no-debug-in-tests.js";
import { noForbiddenImports } from "./rules/no-forbidden-imports.js";

module.exports = {
rules: {
"no-debug-in-tests": noDebugInTests,
"ccapi-validate-args": ccAPIValidateArgs,
"consistent-cc-classes": consistentCCClasses,
"no-forbidden-imports": noForbiddenImports,
},
};
303 changes: 176 additions & 127 deletions ...s/maintenance/src/lintForbiddenImports.ts → ...-plugin/src/rules/no-forbidden-imports.ts
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,7 @@
/*!
* This scripts ensures that files annotated with @forbiddenImports don't import
* anything they are not supposed to.
*/

import { bold, red } from "ansi-colors";
import { ESLintUtils, type TSESTree } from "@typescript-eslint/utils";
import fs from "node:fs";
import path from "node:path";
import ts from "typescript";
import { reportProblem } from "./reportProblem";
import { loadTSConfig, projectRoot } from "./tsAPITools";

// Whitelist some imports that are known not to import forbidden modules
const whitelistedImports = [
Expand Down Expand Up @@ -107,14 +100,43 @@ function getImports(
return output;
}

interface LinterContext {
function resolveImport(
node: ts.Node,
checker: ts.TypeChecker,
): ResolvedImport | undefined {
const sourceFile = node.getSourceFile();

const moduleNameExpr = getExternalModuleName(node);
// if they have a name, that is a string, i.e. not alias definition `import x = y`
if (
moduleNameExpr
&& moduleNameExpr.kind === ts.SyntaxKind.StringLiteral
) {
// Ask the checker about the "symbol: for this module name
// it would be undefined if the module was not found (i.e. error)
const moduleSymbol = checker.getSymbolAtLocation(moduleNameExpr);
const file = moduleSymbol?.getDeclarations()?.[0]?.getSourceFile();
if (file) {
return {
name: moduleNameExpr.getText(sourceFile),
line: ts.getLineAndCharacterOfPosition(
sourceFile,
moduleNameExpr.getStart(),
).line + 1,
sourceFile: file,
};
}
}
}

interface ResolverContext {
program: ts.Program;
resolvedSourceFiles: Map<string, string>;
}

/** Given a definition file, this tries to resolve the original source file */
function resolveSourceFileFromDefinition(
context: LinterContext,
context: ResolverContext,
file: ts.SourceFile,
): ts.SourceFile {
if (context.resolvedSourceFiles.has(file.fileName)) {
Expand Down Expand Up @@ -161,44 +183,94 @@ function resolveSourceFileFromDefinition(
return bail();
}

function relativeToProject(filename: string): string {
return path.relative(projectRoot, filename).replaceAll("\\", "/");
}

const forbiddenImportsRegex =
/^\/\* @forbiddenImports (?<forbidden>.*?) \*\/$/gm;
const forbiddenImportsRegex = /^@forbiddenImports (?<forbidden>.*?)$/;

export function lintForbiddenImports(): Promise<void> {
// Create a Program to represent the project, then pull out the
// source file to parse its AST.

const tsConfig = loadTSConfig(undefined, false);
const program = ts.createProgram(tsConfig.fileNames, {
...tsConfig.options,
preserveSymlinks: false,
});
const checker = program.getTypeChecker();
export const noForbiddenImports = ESLintUtils.RuleCreator.withoutDocs({
create(context) {
// And only those with at least one /* @forbiddenImports ... */ comment
const comments = context.sourceCode.getAllComments()
.filter((c) => c.type === "Block")
.map((c) => c.value.trim());
const forbidden = comments.map((c) => c.match(forbiddenImportsRegex))
.filter((match): match is RegExpMatchArray => !!match)
.map((match) => match.groups?.forbidden?.trim())
.filter((forbidden): forbidden is string => !!forbidden)
.flatMap((forbidden) => forbidden.split(" "));

const context: LinterContext = {
program,
resolvedSourceFiles: new Map(),
};
// No comments, nothing to do
if (!forbidden.length) return {};

let numErrors = 0;
const services = ESLintUtils.getParserServices(context);
const checker = services.program.getTypeChecker();
const projectRoot = services.program.getCurrentDirectory();

// Scan all source files
for (const sourceFile of program.getSourceFiles()) {
const relativePath = relativeToProject(sourceFile.fileName);
function relativeToProject(filename: string): string {
return path.relative(projectRoot, filename).replaceAll(
/[\\\/]/g,
path.sep,
);
}

// Only look at files inside the packages directory
if (!relativePath.startsWith("packages/")) continue;
// Remember which source files we have already visited
const visitedSourceFiles = new Set<string>(context.getFilename());
let todo: { file: ts.SourceFile; importStack: string[] }[] = [];
const resolverContext: ResolverContext = {
program: services.program,
resolvedSourceFiles: new Map(),
};

function addTodo(imp: ResolvedImport, importStack: string[]) {
// try to resolve the original source file for declaration files
const next: ts.SourceFile = imp.sourceFile
.isDeclarationFile
? resolveSourceFileFromDefinition(
resolverContext,
imp.sourceFile,
)
: imp.sourceFile;

if (!visitedSourceFiles.has(next.fileName)) {
todo.push({
file: next,
importStack,
});
}
}

// And only those with at least one @forbiddenImports comment
const forbidden = [...sourceFile.text.matchAll(forbiddenImportsRegex)]
.map((match) => match.groups?.forbidden?.trim())
.filter((forbidden): forbidden is string => !!forbidden)
.flatMap((forbidden) => forbidden.split(" "));
if (!forbidden.length) continue;
function reportForbiddenImport(
node: TSESTree.Node,
imp: ResolvedImport,
importStack: string[],
) {
if (importStack.length === 0) {
context.report({
loc: node.loc,
messageId: "forbidden-import",
data: {
name: imp.name,
},
});
} else {
context.report({
loc: node.loc,
messageId: "forbidden-import-transitive",
data: {
name: imp.name,
stack: [
...importStack,
`❌ ${imp.name}`,
]
.map((file, indent) =>
indent === 0
? file
: `${" ".repeat(indent - 1)}└─ ${file}`
)
.map((line) => `\n${line}`)
.join(""),
},
});
}
}

function checkImport(
imp: ResolvedImport,
Expand All @@ -221,96 +293,73 @@ export function lintForbiddenImports(): Promise<void> {
return forbidden.includes(trimmedImport) ? "forbidden" : "ok";
}

// Resolve the import tree
const visitedSourceFiles = new Set<string>();
const todo: { file: ts.SourceFile; importStack: string[] }[] = [
{ file: sourceFile, importStack: [] },
];
while (todo.length > 0) {
const current = todo.shift()!;
visitedSourceFiles.add(current.file.fileName);
const importStack = [
...current.importStack,
relativeToProject(current.file.fileName),
];

const imports = getImports(current.file, checker);
for (const imp of imports) {
switch (checkImport(imp)) {
case "ignored":
continue;

return {
"ImportDeclaration,ExportNamedDeclaration,ExportAllDeclaration"(
node:
| TSESTree.ImportDeclaration
| TSESTree.ExportAllDeclaration
| TSESTree.ExportNamedDeclaration,
) {
const tsNode = services.esTreeNodeToTSNodeMap.get(node);
const resolvedImport = resolveImport(tsNode, checker);
if (!resolvedImport) return;

// First test the import itself
switch (checkImport(resolvedImport)) {
case "forbidden": {
numErrors++;

const message =
`Found forbidden import of module ${imp.name}:
${
[...importStack, `❌ ${imp.name}`]
.map((file, indent) =>
indent === 0
? file
: `${
" ".repeat(indent - 1)
}└─ ${file}`
)
.join("\n")
}`;

reportProblem({
severity: "error",
// The line number is relative to the declaration file, so we cannot resolve it to the .ts file here
filename: path
.relative(projectRoot, current.file.fileName)
.replaceAll("\\", "/"),
line: imp.line,
message: message,
});
break;
reportForbiddenImport(node, resolvedImport, []);
// No need to report 200 errors on a single import
return;
}
case "ok": {
// try to resolve the original source file for declaration files
const next: ts.SourceFile = imp.sourceFile
.isDeclarationFile
? resolveSourceFileFromDefinition(
context,
imp.sourceFile,
)
: imp.sourceFile;
// Import is okay, add it to the TODO list
addTodo(resolvedImport, []);
}
}

if (!visitedSourceFiles.has(next.fileName)) {
todo.push({
file: next,
importStack,
});
// Continue resolving the import tree until it is empty
todo: while (todo.length > 0) {
const current = todo.shift()!;
visitedSourceFiles.add(current.file.fileName);
const importStack = [
...current.importStack,
relativeToProject(current.file.fileName),
];

const imports = getImports(current.file, checker);
for (const imp of imports) {
switch (checkImport(imp)) {
case "ignored":
continue;

case "forbidden": {
reportForbiddenImport(node, imp, importStack);
// No need to report 200 errors on a single import
break todo;
}
case "ok": {
addTodo(imp, importStack);
}
}
}
}
}
}
}

if (numErrors) {
console.error();
console.error(
red(
`Found ${bold(numErrors.toString())} error${
numErrors !== 1 ? "s" : ""
}!`,
),
);
return Promise.reject(
new Error(
"The forbiddenImports rule had an error! See log output for details.",
),
);
} else {
return Promise.resolve();
}
}

if (require.main === module) {
lintForbiddenImports()
.then(() => process.exit(0))
.catch(() => process.exit(1));
}
todo = [];
},
};
},
meta: {
docs: {
description:
"Ensures that certain files or modules are not transitively imported",
},
type: "problem",
schema: [],
messages: {
"forbidden-import-transitive":
"This import transitively imports forbidden import {{name}}:\nImport stack:{{stack}}",
"forbidden-import": "Found forbidden import of module {{name}}",
},
},
defaultOptions: [],
});
3 changes: 1 addition & 2 deletions packages/maintenance/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,7 @@
"build": "tsc -b tsconfig.build.json --pretty",
"clean": "del-cli build/ \"*.tsbuildinfo\"",
"lint:ts": "eslint --cache --ext .ts \"src/**/*.ts\"",
"lint:ts:fix": "yarn run lint:ts --fix",
"lint:zwave": "yarn ts src/lintForbiddenImports.ts"
"lint:ts:fix": "yarn run lint:ts --fix"
},
"devDependencies": {
"@dprint/formatter": "^0.2.0",
Expand Down
3 changes: 1 addition & 2 deletions packages/maintenance/src/codefind.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
/*!
* This scripts ensures that files annotated with @noExternalImports don't import
* anything from outside the monorepo.
* This scripts helps find certain code patterns via the CLI
*/

import {
Expand Down
Loading

0 comments on commit 674c32a

Please sign in to comment.