Tslint: Refactor RuleWalker and friends

Created on 18 Dec 2016  路  4Comments  路  Source: palantir/tslint

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.

  • [x] move isScopeBoundary and isBlockScopeBoundary to utils
  • [x] move addFailure, failure deduplication (existsFailure) and disabledIntervals-check to Rule

Of course RuleWalker should still work the same to be backwards compatible.

__Cleanup:__

  • [ ] remove skip() and position property, as they seem to be not useful at all

__Performance:__

  • [x] don't create a new array in ScopeAwareRuleWalker#getAllScopes() (BlockScopeAwareRuleWalker#getAllBlockScopes() doesn't do this either)
  • [x] provide sourceFile argument to node.getStart(), getWidth(), getText(), etc. (see #1747)

__Convenience:__

  • [x] add helper method to create Fix and use ruleName from options
In Discussion Breaking Change Refactor

Most helpful comment

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.

All 4 comments

These 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

Was this page helpful?
0 / 5 - 0 ratings

Related issues

ghost picture ghost  路  3Comments

jacob-robertson picture jacob-robertson  路  3Comments

ypresto picture ypresto  路  3Comments

allbto picture allbto  路  3Comments

DanielKucal picture DanielKucal  路  3Comments