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

Rules that can fix the problems they find #561

Closed
myitcv opened this issue Aug 10, 2015 · 13 comments
Closed

Rules that can fix the problems they find #561

myitcv opened this issue Aug 10, 2015 · 13 comments

Comments

@myitcv
Copy link
Contributor

myitcv commented Aug 10, 2015

Are there any plans to extend tslint in such a way that rules can also be accompanied by transforms that can safely fix the breaks they (the rules) find?

For example, consider noConsecutiveBlankLinesRule. It would be trivial to write a transform to delete lines in files that fell foul of this rule.

Clearly not all breakages can safely be fixed, but most formatting related rules probably could.

Thoughts?

(apologies if there is a task discussing this somewhere, I couldn't find anything based on my searches thus far)


edit by @ADahiya

tags for searchability: auto, --fix, option

@adidahiya
Copy link
Contributor

Yes, this is something I'd definitely like to have, especially since ESLint supports it as well.

@jkillian
Copy link
Contributor

Although this is fairly obvious, there'd be at least two components to doing this:

  1. A part where each rule specified what transforms should be made (as Include suggested fixes along with diagnostics #1048 requests)
  2. A feature that actually runs those transforms on the appropriate files/strings

@jkillian
Copy link
Contributor

microsoft/vscode-tslint#47 contains some work that's a start to this feature

@ScottSWu
Copy link
Contributor

Hi, following up on #1048, we would like for rules to suggest text-replacement fixes for rule failures.

Similar to codelyzer and error-prone, we propose two structs for fixes and replacements (a fix may contain multiple non-contiguous replacements), and that RuleFailure contains an array of suggested fixes. API changes include getters for fixes, and createFix / createReplacement functions for rules to suggest fixes.

This differs from the vscode plugin by allowing rules (including custom rules) to add fixes, rather than matching the diagnostics.

At the moment, this change could be limited to the API and not the command line - applying fixes and cli options might require more discussion.

Some work on this implementation has been done on this fork.

@adidahiya adidahiya added the P1 label Jul 18, 2016
@jkillian
Copy link
Contributor

Excited to see your work on this issue @ScottSWu!

@nojvek
Copy link

nojvek commented Jul 24, 2016

I've just started working on a globally installable npm module that can take output of tslint and automatically fix the errors. I haven't published it yet but will ready by end of month.

https://github.com/nojvek/tslint-fix

Ideally its just nice for tslint to have --fix option. I'm not sure what it takes to get it merged there. I created this because we had a large typescript base that had 1000+ tslint issues and I needed to quickly fix them.

Does this sort of usage make sense?

tslint -c tslint.json src/**/*.ts | tslint-fix

@myitcv
Copy link
Contributor Author

myitcv commented Jul 24, 2016

@nojvek far easier to do any re-writing of the (typed) AST within the rules themselves... else you need to parse, type etc for a second time in your tslint-fix process.

@nojvek
Copy link

nojvek commented Jul 24, 2016

I'm mostly just regex matching the errors and reading source files as
strings. Then just basic manipulation at line:col.

Doing it in tslint could make more rules work. Let me know how I can help.
Would really love to see --fix implemented.

Also tslint has a --check option. Does it so the same typechecking as what
typescript would do?

On Sunday, July 24, 2016, Paul Jolly [email protected] wrote:

@nojvek https://github.com/nojvek far easier to do any re-writing of
the (typed) AST within the rules themselves... else you need to parse, type
etc for a second time in your tslint-fix process.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#561 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA-JVM1UbPUY_m9LvpMOk5vTyji9dLMpks5qY8ikgaJpZM4Fopx3
.

jkillian pushed a commit that referenced this issue Aug 4, 2016
* Created IFix and IReplacement structures for representing fixes
  * programWalker includes createFix and createReplacement
* Added optional parameter to createFailure for suggesting fixes
* Modified semicolon rule to suggest fixes (add / remove semicolon)
* Modified testing to check against a file with fixes applied
  * If there is a ".ts.fix" file, then the test will take the first suggested fix and apply to the test file (without markup) and compare

Addresses #561
@mschnee
Copy link

mschnee commented Aug 19, 2016

The vscode-tslint package (tslint integration for Visual Studio Code) provides a UI to auto fix things in a single file and has a small number of solvers:
image

Looking at the code, it seems somewhat simple: a configuration of functions that takes codeBefore and returns codeAfter:
https://github.com/Microsoft/vscode-tslint/blob/master/tslint-server/src/tslintAutoFix.ts

That said, I make pretty serious use of custom tslint rules which are specific to certain code constructs in my projects. I would expect to be able to write custom fixers, perhaps something that looked like this (a quick-and-dirty example only, I see there are commits above leading towards a likely more robust system)

import * as ts from "typescript";
import * as Lint from "tslint/lib/lint";

/* This rule makes sure that a certain function has the current class name as its first parameter */
export class Rule extends Lint.Rules.AbstractRule {
    public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
        return this.applyWithWalker(new FirstParamClassNameWalker(sourceFile, this.getOptions()));
    }
}

class FirstParamClassNameWalker extends Lint.RuleWalker {
    private currentClass: string;

    public visitClassDeclaration(node: ts.ClassDeclaration) {
        this.currentClass = node.name.getText();
        super.visitClassDeclaration(node);
        this.currentClass = null;
    }

    public visitCallExpression(node: ts.CallExpression) {
        const callName = node.expression.getText();

        // this.firstParamFunction('ShouldBeClassName', otherparam1, otherparam2);
        if (this.currentClass && (callName === 'firstParamFunction')) {
            const argument = node.arguments[0];
            const keyParam = argument.getText().replace(/'/g, '');

            if (keyParam !== this.currentClass) {
                this.addFailure(this.createFailure(argument.getStart(), argument.getWidth(), `${callName} key parameter does not match className: ${keyParam} !==  ${this.currentClass}`));
            }
        }

        super.visitCallExpression(node);
    }

    /* Trivial example */
    public fixCallExpression(failure: RuleFailure) {
        // return this.firstParamFunction('IncorrectName', param1, param2);
        // return this.firstParamFunction('CorrectName', param1, param2)
        // failure.line is the full line, from `return` to `;`
        // failure.range is the failure range from this.createFailure, e.g. `IncorrectName`
        // replace a full line?
        failure.line = failure.line.substring(0, failure.getStart()) + this.className + failure.line.substring(failure.getStart() + failure.getWidth());

        // replace just the range?
        failure.range = this.className;
        super.fixCallExpression(failure);
    }
}

@jkillian
Copy link
Contributor

Fixed by #1423! Only the semicolon rule currently does fixes, but it's a start. @mschnee, you should be able to use this feature in your custom rules.

@mschnee
Copy link

mschnee commented Aug 23, 2016

Thanks @jkillian !

@saulshanabrook
Copy link

@jkillian Has a CLI option been added?

soniro pushed a commit to soniro/tslint that referenced this issue Aug 31, 2016
* Created IFix and IReplacement structures for representing fixes
  * programWalker includes createFix and createReplacement
* Added optional parameter to createFailure for suggesting fixes
* Modified semicolon rule to suggest fixes (add / remove semicolon)
* Modified testing to check against a file with fixes applied
  * If there is a ".ts.fix" file, then the test will take the first suggested fix and apply to the test file (without markup) and compare

Addresses palantir#561
@adidahiya
Copy link
Contributor

adidahiya commented Sep 1, 2016

@saulshanabrook nope, I just filed this issue for that #1534

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants