This is more like a tracking issue. If you are ok with the todos below, I would like to do the refactoring by myself.
__Move funtionality to make it reusable:__
Why? I think RuleWalker is too bloated for the common use case. There are all those unnecessary visitor methods on the call stack and everytime walkChildren is called, there is a new closure created.
Don't get me wrong: this is pretty convenient for beginners and I don't want to rebuild this from scratch. I just want to use my own and do the AST walking myself. But there is some functionality, which I need and which would belong to other modules or classes in my opinion.
isScopeBoundary and isBlockScopeBoundary to utilsaddFailure, failure deduplication (existsFailure) and disabledIntervals-check to RuleOf course RuleWalker should still work the same to be backwards compatible.
__Cleanup:__
skip() and position property, as they seem to be not useful at all__Performance:__
ScopeAwareRuleWalker#getAllScopes() (BlockScopeAwareRuleWalker#getAllBlockScopes() doesn't do this either)sourceFile argument to node.getStart(), getWidth(), getText(), etc. (see #1747)__Convenience:__
Fix and use ruleName from optionsThese changes seem to be going in the right direction. I would also like to suggest the following:
In the RuleWalker make some private properties public readonly. Right now I'm writing my custom rule walker as follows:
class CustomRuleWalker extends SyntaxWalker {
public readonly sourceFile: ts.SourceFile;
public readonly sourceText: string;
public readonly limit: number;
public readonly options: any[];
private failures: RuleFailure[];
private disabledIntervals: IDisabledInterval[];
private ruleName: string;
constructor(sourceFile: ts.SourceFile, options: IOptions) {
super();
this.sourceFile = sourceFile;
this.limit = sourceFile.text.length;
this.sourceText = sourceFile.text.substring(0, this.limit);
this.options = options.ruleArguments;
this.failures = [];
this.disabledIntervals = options.disabledIntervals;
this.ruleName = options.ruleName;
}
public substring(start: number, end: number) {
return this.sourceText.substring(start, end - start);
}
public getLineAndCharacter(node: ts.Node, byEndLocation: boolean = false) {
const index = byEndLocation ? node.getEnd() : node.getStart();
return this.sourceFile.getLineAndCharacterOfPosition(index);
}
public getLine(node: ts.Node, byEndLocation: boolean = false) {
return this.getLineAndCharacter(node, byEndLocation).line;
}
public getLineAndCharacterOfPosition(position: number): ts.LineAndCharacter {
return this.sourceFile.getLineAndCharacterOfPosition(position);
}
public isSingleLineNode(node: ts.Node): boolean {
if (node.kind === ts.SyntaxKind.SyntaxList) {
return node.getFullText(this.sourceFile).indexOf('\n') === -1;
}
return node.getText(this.sourceFile).indexOf('\n') === -1;
}
I would not expect the RuleWalker to define isSingleLineNode or any other getLine... methods but making sourceFile a readonly property would help greatly since I would only need to extend from RuleWalker instead of having to redefine it by extending from the SyntaxWalker.
If the sourceFile is provided then I wouldn't mind having to write node.getText(this.sourceFile).
On a related note, it seems that use of a walker makes AST traversal much slower. (https://gist.github.com/andy-hanson/e9e5c3b33c6514df7825932704603fee)
Many rules deal with only 1 syntax kind and could be changed to use "manual" recursion (via ts.forEachChild).
(Would be nice to have profiling support in the tslint repository. #1131)
Catching up on this issue:
I like the idea of @jmlopez-rod. But instead of extending the existing RuleWalker I'd like to introduce something like MinimalRuleWalker that would require you to implement the walk method yourself and only implement the necessary or useful helper methods of RuleWalker.
```typescript
abstract class MinimalRuleWalker implements IWalker {
public readonly sourceFile: ts.SourceFile;
public readonly options: any[];
public readonly ruleName: string;
public getSourceFile() {} // for compatibility with the interface and RuleWalker
public getFailures() {}
public addFailureAtNode(...) {}
public addFailureAt(...) {} // plus the other addFailure* methods
public createReplacement(...) {}
public createFix(...) {}
public hasOption(...) {}
public abstract walk(sourceFile: ts.SourceFile);
}
``
Implementing awalkmethods that fits the needs of that specific walker would also address @andy-hanson's comment wrt the performance gain of the "manual recursion". I also think this is way better than the current "one walker to rule them all" approach. In factSyntaxWalkergets deoptimized a lot and is dog slow because of the way it uses recursion, the repeated recompilation of thets.forEachChildcallback and the polymorphic nature ofvisitNode`.
If you provide positive feedback on this, I'll submit a PR soon(ish).
We could then migrate existing walkers in an incremental fashion.
@ajafff can remove skip and position now
Most helpful comment
Catching up on this issue:
I like the idea of @jmlopez-rod. But instead of extending the existing
RuleWalkerI'd like to introduce something likeMinimalRuleWalkerthat would require you to implement thewalkmethod yourself and only implement the necessary or useful helper methods of RuleWalker.```typescript
abstract class MinimalRuleWalker implements IWalker {
public readonly sourceFile: ts.SourceFile;
public readonly options: any[];
public readonly ruleName: string;
public getSourceFile() {} // for compatibility with the interface and RuleWalker
public getFailures() {}
public addFailureAtNode(...) {}
public addFailureAt(...) {} // plus the other addFailure* methods
public createReplacement(...) {}
public createFix(...) {}
public hasOption(...) {}
public abstract walk(sourceFile: ts.SourceFile);
}
``
Implementing awalkmethods that fits the needs of that specific walker would also address @andy-hanson's comment wrt the performance gain of the "manual recursion". I also think this is way better than the current "one walker to rule them all" approach. In factSyntaxWalkergets deoptimized a lot and is dog slow because of the way it uses recursion, the repeated recompilation of thets.forEachChildcallback and the polymorphic nature ofvisitNode`.If you provide positive feedback on this, I'll submit a PR soon(ish).
We could then migrate existing walkers in an incremental fashion.