-
Notifications
You must be signed in to change notification settings - Fork 887
Introduce AbstractWalker for performance #2093
Changes from 6 commits
da3ac7d
252599d
b8ffd67
fbe808e
eacd1a5
0d7ae81
5ec78a4
0389404
4662cf1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,11 +19,11 @@ import * as ts from "typescript"; | |
|
||
import {doesIntersect} from "../utils"; | ||
import {IWalker} from "../walker"; | ||
import {IDisabledInterval, IOptions, IRule, IRuleMetadata, RuleFailure} from "./rule"; | ||
import {IDisabledInterval, IOptions, IRule, IRuleMetadata, RuleFailure, WalkContext} from "./rule"; | ||
|
||
export abstract class AbstractRule implements IRule { | ||
public static metadata: IRuleMetadata; | ||
private options: IOptions; | ||
protected readonly ruleArguments: any[]; | ||
|
||
public static isRuleEnabled(ruleConfigValue: any): boolean { | ||
if (typeof ruleConfigValue === "boolean") { | ||
|
@@ -37,33 +37,39 @@ export abstract class AbstractRule implements IRule { | |
return false; | ||
} | ||
|
||
constructor(ruleName: string, private value: any, private disabledIntervals: IDisabledInterval[]) { | ||
let ruleArguments: any[] = []; | ||
|
||
constructor(protected readonly ruleName: string, private value: any, private disabledIntervals: IDisabledInterval[]) { | ||
if (Array.isArray(value) && value.length > 1) { | ||
ruleArguments = value.slice(1); | ||
this.ruleArguments = value.slice(1); | ||
} else { | ||
this.ruleArguments = []; | ||
} | ||
} | ||
|
||
this.options = { | ||
disabledIntervals, | ||
ruleArguments, | ||
ruleName, | ||
public getOptions(): IOptions { | ||
return { | ||
disabledIntervals: this.disabledIntervals, | ||
ruleArguments: this.ruleArguments, | ||
ruleName: this.ruleName, | ||
}; | ||
} | ||
|
||
public getOptions(): IOptions { | ||
return this.options; | ||
public isEnabled(): boolean { | ||
return AbstractRule.isRuleEnabled(this.value); | ||
} | ||
|
||
public abstract apply(sourceFile: ts.SourceFile, languageService: ts.LanguageService): RuleFailure[]; | ||
|
||
public applyWithWalker(walker: IWalker): RuleFailure[] { | ||
protected applyWithWalker(walker: IWalker): RuleFailure[] { | ||
walker.walk(walker.getSourceFile()); | ||
return this.filterFailures(walker.getFailures()); | ||
} | ||
|
||
public isEnabled(): boolean { | ||
return AbstractRule.isRuleEnabled(this.value); | ||
protected applyWithFunction(sourceFile: ts.SourceFile, walkFn: (ctx: WalkContext<void>) => void): RuleFailure[]; | ||
protected applyWithFunction<T>(sourceFile: ts.SourceFile, walkFn: (ctx: WalkContext<T>) => void, options: T): RuleFailure[]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually, I don't think the generic option helps all that much since it's not constrained and only used by the user |
||
protected applyWithFunction<T>(sourceFile: ts.SourceFile, walkFn: (ctx: WalkContext<T | void>) => void, options?: T): RuleFailure[] { | ||
const ctx = new WalkContext(sourceFile, this.ruleName, options); | ||
walkFn(ctx); | ||
return this.filterFailures(ctx.failures); | ||
} | ||
|
||
protected filterFailures(failures: RuleFailure[]): RuleFailure[] { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,8 +17,6 @@ | |
|
||
import * as ts from "typescript"; | ||
|
||
import {IWalker} from "../walker"; | ||
|
||
export interface IRuleMetadata { | ||
/** | ||
* The kebab-case name of the rule. | ||
|
@@ -101,7 +99,6 @@ export interface IRule { | |
getOptions(): IOptions; | ||
isEnabled(): boolean; | ||
apply(sourceFile: ts.SourceFile, languageService: ts.LanguageService): RuleFailure[]; | ||
applyWithWalker(walker: IWalker): RuleFailure[]; | ||
} | ||
|
||
export class Replacement { | ||
|
@@ -111,6 +108,22 @@ export class Replacement { | |
return replacements.reduce((text, r) => r.apply(text), content); | ||
} | ||
|
||
public static replaceFromTo(start: number, end: number, text: string) { | ||
return new Replacement(start, end - start, text); | ||
} | ||
|
||
public static deleteText(start: number, length: number) { | ||
return new Replacement(start, length, ""); | ||
} | ||
|
||
public static deleteFromTo(start: number, end: number) { | ||
return new Replacement(start, end - start, ""); | ||
} | ||
|
||
public static appendText(start: number, text: string) { | ||
return new Replacement(start, 0, text); | ||
} | ||
|
||
constructor(private innerStart: number, private innerLength: number, private innerText: string) { | ||
} | ||
|
||
|
@@ -265,3 +278,33 @@ export class RuleFailure { | |
return new RuleFailurePosition(position, lineAndCharacter); | ||
} | ||
} | ||
|
||
export class WalkContext<T> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. belongs in separate file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you put this class in a separate file? |
||
public readonly failures: RuleFailure[]; | ||
private limit: number; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rename to |
||
|
||
constructor(public readonly sourceFile: ts.SourceFile, public readonly ruleName: string, public readonly options: T) { | ||
this.failures = []; | ||
this.limit = sourceFile.end; | ||
} | ||
|
||
/** Add a failure with any arbitrary span. Prefer `addFailureAtNode` if possible. */ | ||
public addFailureAt(start: number, width: number, failure: string, fix?: Fix) { | ||
this.addFailure(start, start + width, failure, fix); | ||
} | ||
|
||
public addFailure(start: number, end: number, failure: string, fix?: Fix) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer to leave it as it is now. This new implementation does not need to keep the mistakes made in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assuming all rules will eventually be migrated over to the new style, I'm in agreement. The type system will find incorrect usages though, and the new naming is nicer. If some rules are going to stay as extensions of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, we should try to migrate all existing rules, even I don't know if or when we can eventually deprecate and remove Regarding the method name I don't have a strong opinion. I will change it if you want. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You wrote the code and have thought about it more than me - happy to defer to your preference and leave it as is on this one. |
||
this.failures.push( | ||
new RuleFailure(this.sourceFile, Math.min(start, this.limit), Math.min(end, this.limit), failure, this.ruleName, fix), | ||
); | ||
} | ||
|
||
/** Add a failure using a node's span. */ | ||
public addFailureAtNode(node: ts.Node, failure: string, fix?: Fix) { | ||
this.addFailure(node.getStart(this.sourceFile), node.getEnd(), failure, fix); | ||
} | ||
|
||
public createFix(...replacements: Replacement[]) { | ||
return new Fix(this.ruleName, replacements); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,10 +17,23 @@ | |
|
||
import * as ts from "typescript"; | ||
|
||
import {RuleFailure} from "../rule/rule"; | ||
import {RuleFailure, WalkContext} from "../rule/rule"; | ||
import {IWalker} from "./walker"; | ||
|
||
export interface IWalker { | ||
getSourceFile(): ts.SourceFile; | ||
walk(node: ts.Node): void; | ||
walk(sourceFile: ts.SourceFile): void; | ||
getFailures(): RuleFailure[]; | ||
} | ||
|
||
export abstract class AbstractWalker<T> extends WalkContext<T> implements IWalker { | ||
public abstract walk(sourceFile: ts.SourceFile): void; | ||
|
||
public getSourceFile() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we just expose the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
return this.sourceFile; | ||
} | ||
|
||
public getFailures() { | ||
return this.failures; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,8 +19,6 @@ import * as ts from "typescript"; | |
|
||
import * as Lint from "../index"; | ||
|
||
import { IOptions } from "../language/rule/rule"; | ||
|
||
export class Rule extends Lint.Rules.AbstractRule { | ||
/* tslint:disable:object-literal-sort-keys */ | ||
public static metadata: Lint.IRuleMetadata = { | ||
|
@@ -63,44 +61,29 @@ export class Rule extends Lint.Rules.AbstractRule { | |
public static DEFAULT_ALLOWED = [ -1, 0, 1 ]; | ||
|
||
public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { | ||
return this.applyWithWalker(new NoMagicNumbersWalker(sourceFile, this.getOptions())); | ||
const allowedNumbers = this.ruleArguments.length > 0 ? this.ruleArguments : Rule.DEFAULT_ALLOWED; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this concept of parsing rule options as part of the rule instead of part of the rule implementation - seems like a better separation of concerns to me. |
||
return this.applyWithWalker(new NoMagicNumbersWalker(sourceFile, this.ruleName, new Set(allowedNumbers.map(String)))); | ||
} | ||
} | ||
|
||
class NoMagicNumbersWalker extends Lint.RuleWalker { | ||
// allowed magic numbers | ||
private allowed: Set<string>; | ||
constructor(sourceFile: ts.SourceFile, options: IOptions) { | ||
super(sourceFile, options); | ||
|
||
const configOptions = this.getOptions(); | ||
const allowedNumbers: number[] = configOptions.length > 0 ? configOptions : Rule.DEFAULT_ALLOWED; | ||
this.allowed = new Set(allowedNumbers.map(String)); | ||
} | ||
|
||
public visitNode(node: ts.Node) { | ||
const num = getLiteralNumber(node); | ||
if (num !== undefined) { | ||
if (!Rule.ALLOWED_NODES.has(node.parent!.kind) && !this.allowed.has(num)) { | ||
this.addFailureAtNode(node, Rule.FAILURE_STRING); | ||
class NoMagicNumbersWalker extends Lint.AbstractWalker<Set<string>> { | ||
public walk(sourceFile: ts.SourceFile) { | ||
const cb = (node: ts.Node): void => { | ||
if (node.kind === ts.SyntaxKind.NumericLiteral) { | ||
this.checkNumericLiteral(node, (node as ts.NumericLiteral).text); | ||
} else if (node.kind === ts.SyntaxKind.PrefixUnaryExpression && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unimportant, but I do think this is a nice use case for a type guard function so that the upcoming type assertion can be removed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, but I don't think it's a good idea to scatter the declarations of typeguard functions through the whole codebase. Currently it is done this way which leads to many duplicated small functions. Maybe we should add them to a new module But that's not directly related to this PR and can be considered when migrating other exisiting rules. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, no need to change for this PR. Also saw the discriminated union issue you created, that would be a neat solution as well. |
||
(node as ts.PrefixUnaryExpression).operator === ts.SyntaxKind.MinusToken) { | ||
this.checkNumericLiteral(node, "-" + ((node as ts.PrefixUnaryExpression).operand as ts.NumericLiteral).text); | ||
} else { | ||
ts.forEachChild(node, cb); | ||
} | ||
} else { | ||
super.visitNode(node); | ||
} | ||
}; | ||
return ts.forEachChild(sourceFile, cb); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's why the callback function here has the return type annotation The return is actually there on purpose to serve V8's tail call optimizer. AND once ES6 proper tail calls get shipped (currently only available behind There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is a return value, but why return it from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All return statements in this method return |
||
} | ||
} | ||
|
||
/** If node is a number literal, return a string representation of that number. */ | ||
function getLiteralNumber(node: ts.Node): string | undefined { | ||
if (node.kind === ts.SyntaxKind.NumericLiteral) { | ||
return (node as ts.NumericLiteral).text; | ||
} | ||
if (node.kind !== ts.SyntaxKind.PrefixUnaryExpression) { | ||
return undefined; | ||
} | ||
const { operator, operand } = node as ts.PrefixUnaryExpression; | ||
if (operator === ts.SyntaxKind.MinusToken && operand.kind === ts.SyntaxKind.NumericLiteral) { | ||
return "-" + (operand as ts.NumericLiteral).text; | ||
private checkNumericLiteral(node: ts.Node, num: string) { | ||
if (!Rule.ALLOWED_NODES.has(node.parent!.kind) && !this.options.has(num)) { | ||
this.addFailureAtNode(node, Rule.FAILURE_STRING); | ||
} | ||
} | ||
return undefined; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make this public?