Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Introduce AbstractWalker for performance #2093

Merged
merged 9 commits into from
Feb 3, 2017
Merged

Introduce AbstractWalker for performance #2093

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
da3ac7d
Implement AbstractWalker
ajafff Jan 19, 2017
252599d
Implement WalkContext and Rule.applyWithFunction
ajafff Jan 23, 2017
b8ffd67
Refactor no-null-keyword to walk function
ajafff Jan 23, 2017
fbe808e
Refactor no-magic-numbers to AbstractWalker
ajafff Jan 23, 2017
eacd1a5
PR feedback
ajafff Jan 24, 2017
0d7ae81
Remove languageService property for now
ajafff Jan 25, 2017
5ec78a4
Address review comments
ajafff Feb 1, 2017
0389404
Merge branch 'master' into abstract_walker
ajafff Feb 1, 2017
4662cf1
Restore location of AbstractRule#isEnabled
ajafff Feb 1, 2017
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
10 changes: 8 additions & 2 deletions src/language/rule/abstractRule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ 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;
Expand Down Expand Up @@ -57,11 +57,17 @@ export abstract class AbstractRule implements IRule {

public abstract apply(sourceFile: ts.SourceFile, languageService: ts.LanguageService): RuleFailure[];

public applyWithWalker(walker: IWalker): RuleFailure[] {
protected applyWithWalker(walker: IWalker): RuleFailure[] {
Copy link
Contributor

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

Copy link
Contributor

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?

walker.walk(walker.getSourceFile());
return this.filterFailures(walker.getFailures());
}

protected applyWithFunction<T>(sourceFile: ts.SourceFile, options: T, walkFn: (ctx: WalkContext<T>) => void) {
const ctx = new WalkContext<T>(sourceFile, this.options.ruleName, options);
walkFn(ctx);
return this.filterFailures(ctx.getFailures());
}

public isEnabled(): boolean {
return AbstractRule.isRuleEnabled(this.value);
}
Expand Down
53 changes: 50 additions & 3 deletions src/language/rule/rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@

import * as ts from "typescript";

import {IWalker} from "../walker";

export interface IRuleMetadata {
/**
* The kebab-case name of the rule.
Expand Down Expand Up @@ -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 {
Expand All @@ -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) {
}

Expand Down Expand Up @@ -265,3 +278,37 @@ export class RuleFailure {
return new RuleFailurePosition(position, lineAndCharacter);
}
}

export class WalkContext<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

belongs in separate file

Copy link
Contributor

Choose a reason for hiding this comment

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

can you put this class in a separate file?

private limit: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to sourceLastIndex or similar

private failures: RuleFailure[];

constructor(public readonly sourceFile: ts.SourceFile, public readonly ruleName: string, public readonly options: T) {
this.failures = [];
this.limit = sourceFile.getFullWidth();
}

public getFailures() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of making it private then?

Copy link
Contributor

Choose a reason for hiding this comment

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

The failures array could just be declared as public readonly failures: RuleFailure[] = [] and let the AbstractWalker implement the getFailures method.

return this.failures;
}

/** 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) {
Copy link
Contributor

@jkillian jkillian Jan 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps we should use addFailureFromStartToEnd so it mirrors the methods in RuleWalker? I know it's a bit of an awkward name... I see why you named it as you did though, since this is the new "base" method that all the other methods call

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 RuleWalker.
When migrating an existing rule, you will have some conflicts anyway because of the other missing methods

Copy link
Contributor

Choose a reason for hiding this comment

The 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 RuleWalker though, then it'll get confusing to have the different methods existing for perpetuity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, we should try to migrate all existing rules, even ScopeAwareRuleWalker and subclasses thereof. At least that's my naive plan.

I don't know if or when we can eventually deprecate and remove RuleWalker for implementers of custom rules. In the end it's your decision if you want to keep it for backwards compatibility.

Regarding the method name I don't have a strong opinion. I will change it if you want.

Copy link
Contributor

Choose a reason for hiding this comment

The 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);
}
}
13 changes: 11 additions & 2 deletions src/language/walker/walker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,19 @@

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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just expose the sourceFile property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sourceFile is exposed as public property. This methods only needs to be there to implement the IWalker interface for compatibility with RuleWalker

return this.sourceFile;
}
}
55 changes: 20 additions & 35 deletions src/rules/noMagicNumbersRule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -63,44 +61,31 @@ 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 options = this.getOptions();
const ruleArguments = options.ruleArguments;
const allowedNumbers = ruleArguments.length > 0 ? ruleArguments : Rule.DEFAULT_ALLOWED;
return this.applyWithWalker(new NoMagicNumbersWalker(sourceFile, options.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 &&
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 src/language/typeguard.ts as needed.

But that's not directly related to this PR and can be considered when migrating other exisiting rules.

Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

@andy-hanson andy-hanson Jan 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

ts.forEachChild actually does return a value (assuming the callback does), so this was confusing at first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why the callback function here has the return type annotation : void.

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 --harmony_tailcalls flag) we get the ability to handle arbitrarily deep nested trees for free.

Copy link
Contributor

Choose a reason for hiding this comment

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

there is a return value, but why return it from walk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All return statements in this method return void. As explained above, it is much faster to use return for calls in tail position when it does not return anything.

}
}

/** 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;
}
24 changes: 9 additions & 15 deletions src/rules/noNullKeywordRule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,25 +40,19 @@ export class Rule extends Lint.Rules.AbstractRule {
public static FAILURE_STRING = "Use 'undefined' instead of 'null'";

public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
return this.applyWithWalker(new NullWalker(sourceFile, this.getOptions()));
return this.applyWithFunction(sourceFile, undefined, walk);
}
}

class NullWalker extends Lint.RuleWalker {
public visitNode(node: ts.Node) {
super.visitNode(node);
if (node.kind === ts.SyntaxKind.NullKeyword && !isPartOfType(node)) {
this.addFailureAtNode(node, Rule.FAILURE_STRING);
function walk(ctx: Lint.WalkContext<void>) {
return ts.forEachChild(ctx.sourceFile, cb);
function cb(node: ts.Node): void {
if (node.kind >= ts.SyntaxKind.FirstTypeNode && node.kind <= ts.SyntaxKind.LastTypeNode) {
return; // skip type nodes
}
}
}

function isPartOfType({ parent }: ts.Node) {
while (parent != null) {
if (ts.SyntaxKind.FirstTypeNode <= parent.kind && parent.kind <= ts.SyntaxKind.LastTypeNode) {
return true;
if (node.kind === ts.SyntaxKind.NullKeyword) {
return ctx.addFailureAtNode(node, Rule.FAILURE_STRING);
}
parent = parent.parent;
return ts.forEachChild(node, cb);
}
return false;
}