Skip to content
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.

Sign up for GitHub

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

Converted to walk function instead of a RuleWalker #22

Merged
merged 1 commit into from
Aug 20, 2018
+90 −84
Merged

Converted to walk function instead of a RuleWalker #22

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
174 changes: 90 additions & 84 deletions noCircularImportsRule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,15 @@
import { relative, sep } from 'path';
import * as Lint from 'tslint';
import { IOptions } from 'tslint/lib/language/rule/rule';
import * as ts from 'typescript';

interface Options {
/** @internal */
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was a little confused how to represent this. It's not common for the interface Options on a rule to be different from the config the rule is allowed to take in.

@internal is recognized by TS and seems to be coming up as part of TSDoc, so I figured it was appropriate here. If you change or remove this I won't be offended at all 😉

compilerOptions: ts.CompilerOptions

/** @internal */
rootDir: string
}

export class Rule extends Lint.Rules.TypedRule {
static FAILURE_STRING = 'circular import detected'

Expand All @@ -22,9 +29,16 @@ export class Rule extends Lint.Rules.TypedRule {
const resolvedFile = sourceFile.fileName
imports.delete(resolvedFile)

const walker = new NoCircularImportsWalker(sourceFile, this.getOptions(), program)
const compilerOptions = program.getCompilerOptions()

return this.applyWithWalker(walker)
return this.applyWithFunction(
sourceFile,
walk,
{
compilerOptions,
rootDir: compilerOptions.rootDir || process.cwd(),
},
program.getTypeChecker())
}
}

Expand All @@ -34,51 +48,44 @@ const imports = new Map<string, Map<string, ts.Node>>()
const found = new Set<string>()
const nodeModulesRe = new RegExp(`\\${sep}node_modules\\${sep}`)

class NoCircularImportsWalker extends Lint.RuleWalker {
constructor(sourceFile: ts.SourceFile, options: IOptions, private program: ts.Program) {
super(sourceFile, options)
}

visitSourceFile(sourceFile: ts.SourceFile) {
// Instead of visiting all children, this is faster. We know imports are statements anyway.
sourceFile.statements.forEach(statement => {
// export declarations seem to be missing from the current SyntaxWalker
if (ts.isExportDeclaration(statement)) {
this.visitImportOrExportDeclaration(statement)
}
else if (ts.isImportDeclaration(statement)) {
this.visitImportOrExportDeclaration(statement)
}
})

const fileName = sourceFile.fileName

// Check for cycles, remove any cycles that have been found already (otherwise we'll report
// false positive on every files that import from the real cycles, and users will be driven
// mad).
// The checkCycle is many order of magnitude faster than getCycle, but does not keep a history
// of the cycle itself. Only get the full cycle if we found one.
if (this.checkCycle(fileName)) {
const allCycles = this.getAllCycles(fileName)

for (const maybeCycle of allCycles) {
// Slice the array so we don't match this file twice.
if (maybeCycle.slice(1, -1).some(fileName => found.has(fileName))) {
continue
}
maybeCycle.forEach(x => found.add(x))
const node = imports.get(fileName) !.get(maybeCycle[1]) !

const compilerOptions = this.program.getCompilerOptions()
this.addFailureAt(node.getStart(), node.getWidth(), Rule.FAILURE_STRING + ": " + maybeCycle
.concat(fileName)
.map(x => relative(compilerOptions.rootDir || process.cwd(), x))
.join(' -> '))
function walk(context: Lint.WalkContext<Options>) {
// Instead of visiting all children, this is faster. We know imports are statements anyway.
context.sourceFile.statements.forEach(statement => {
// export declarations seem to be missing from the current SyntaxWalker
if (ts.isExportDeclaration(statement)) {
visitImportOrExportDeclaration(statement)
}
else if (ts.isImportDeclaration(statement)) {
visitImportOrExportDeclaration(statement)
}
})

const fileName = context.sourceFile.fileName

// Check for cycles, remove any cycles that have been found already (otherwise we'll report
// false positive on every files that import from the real cycles, and users will be driven
// mad).
// The checkCycle is many order of magnitude faster than getCycle, but does not keep a history
// of the cycle itself. Only get the full cycle if we found one.
if (checkCycle(fileName)) {
const allCycles = getAllCycles(fileName)

for (const maybeCycle of allCycles) {
// Slice the array so we don't match this file twice.
if (maybeCycle.slice(1, -1).some(fileName => found.has(fileName))) {
continue
}
maybeCycle.forEach(x => found.add(x))
const node = imports.get(fileName) !.get(maybeCycle[1]) !

context.addFailureAt(node.getStart(), node.getWidth(), Rule.FAILURE_STRING + ": " + maybeCycle
.concat(fileName)
.map(x => relative(context.options.rootDir, x))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

.join(' -> '))
}
}

visitImportOrExportDeclaration(node: ts.ImportDeclaration | ts.ExportDeclaration) {
function visitImportOrExportDeclaration(node: ts.ImportDeclaration | ts.ExportDeclaration) {
if (!node.parent || !ts.isSourceFile(node.parent)) {
return
}
Expand All @@ -91,9 +98,8 @@ class NoCircularImportsWalker extends Lint.RuleWalker {
return
}
const importFileName = node.moduleSpecifier.text
const compilerOptions = this.program.getCompilerOptions()

const resolved = ts.resolveModuleName(importFileName, fileName, compilerOptions, ts.sys)
const resolved = ts.resolveModuleName(importFileName, fileName, context.options.compilerOptions, ts.sys)
if (!resolved || !resolved.resolvedModule) {
return
}
Expand All @@ -105,55 +111,55 @@ class NoCircularImportsWalker extends Lint.RuleWalker {
return
}

this.addToGraph(fileName, resolvedImportFileName, node)
addToGraph(fileName, resolvedImportFileName, node)
}
}

private addToGraph(thisFileName: string, importCanonicalName: string, node: ts.Node) {
let i = imports.get(thisFileName)
if (!i) {
imports.set(thisFileName, i = new Map)
}
i.set(importCanonicalName, node)
function addToGraph(thisFileName: string, importCanonicalName: string, node: ts.Node) {
let i = imports.get(thisFileName)
if (!i) {
imports.set(thisFileName, i = new Map)
}
i.set(importCanonicalName, node)
}

private checkCycle(moduleName: string): boolean {
const accumulator = new Set<string>()

const moduleImport = imports.get(moduleName)
if (!moduleImport)
return false
function checkCycle(moduleName: string): boolean {
const accumulator = new Set<string>()

const toCheck = Array.from(moduleImport.keys())
for (let i = 0; i < toCheck.length; i++) {
const current = toCheck[i]
if (current == moduleName) {
return true
}
accumulator.add(current)
const moduleImport = imports.get(moduleName)
if (!moduleImport)
return false

toCheck.push(
...Array.from((imports.get(current) || new Map).keys())
.filter(i => !accumulator.has(i))
)
const toCheck = Array.from(moduleImport.keys())
for (let i = 0; i < toCheck.length; i++) {
const current = toCheck[i]
if (current == moduleName) {
return true
}
accumulator.add(current)

return false
toCheck.push(
...Array.from((imports.get(current) || new Map).keys())
.filter(i => !accumulator.has(i))
)
}

private getAllCycles(moduleName: string, accumulator: string[] = []): string[][] {
const moduleImport = imports.get(moduleName)
if (!moduleImport) return []
if (accumulator.indexOf(moduleName) !== -1)
return [accumulator]
return false
}

const all: string[][] = []
for (const imp of Array.from(moduleImport.keys())) {
const c = this.getAllCycles(imp, accumulator.concat(moduleName))
function getAllCycles(moduleName: string, accumulator: string[] = []): string[][] {
const moduleImport = imports.get(moduleName)
if (!moduleImport) return []
if (accumulator.indexOf(moduleName) !== -1)
return [accumulator]

if (c.length)
all.push(...c)
}
const all: string[][] = []
for (const imp of Array.from(moduleImport.keys())) {
const c = getAllCycles(imp, accumulator.concat(moduleName))

return all
if (c.length)
all.push(...c)
}

return all
}