tslint 5.0.0 contains custom rule breaking changes

[bgold09]

Since tslint v5.0.0, there is a breaking change to the API for custom tslint rules.

The good news is that it looks like changing this is a pretty straightforward refactoring.

- public applyWithProgram(srcFile: ts.SourceFile, langSvc: ts.LanguageService): Lint.RuleFailure[] {
-     return this.applyWithWalker(new Walker(srcFile, this.getOptions(), langSvc.getProgram()));
+ public applyWithProgram(srcFile: ts.SourceFile, program: ts.Program): Lint.RuleFailure[] {
+     return this.applyWithWalker(new Walker(srcFile, this.getOptions(), program));

However changing this would also mean a breaking change to this package/ruleset, since it would make tslint versions >= 5.0.0 the new dependency. Are there any concerns with that? Do we need to ensure that any new updates to these rules are compatible with projects using older version of tslint?

[HamletDRC]

Hi @bgold09
tslint has been introducing a lot of backwards compatibility changes recently. The 4.0.0 upgrade was was quite a bit of work and is not backwards compatible. And now 5.0.0 is released very quickly thereafter. It seems like their approach is to not worry about backwards compatibility and just increase the major version. This is an unfortunate situation for all the rules developers like us.

I suggest you make the change in a non-backwards compatible way and then increase the tslint-microsoft-contrib version to 5.0.0 as well.

Thanks,

[bgold09]

Hmm, this is actually less straightforward than I thought. These rules don't actually use the applyWithProgram() function of TSLint TypedRules, but the language service is used elsewhere.

I'll need to spend some time investigating how to proceed.

[bgold09]

Just a brief update: making pretty good progress and working to get all the tests passing

[loicraux]

Thanks @bgold09 for the update !
Let us know if you are blocked and need any help from our side !

[bgold09]

I have all the tests passing now, with some caveats that I'll explain later. The issue now is that the tslint plugin for grunt does not accept an option to enable type checking (see palantir/grunt-tslint#67); we need that to lint our own project.

However I was able to fix that quite easily, but we'll have to wait for them to accept the PR and publish a new version of the task.

Edit: this actually only results in warnings from tslint that (properly) do not fail the build. As such, I consider the issue to be non-blocking on the grunt-tslint issue. I'm working on completing some cleanup and maintenance things and plan to send the PR soon.

[bgold09]

@loicraux, @HamletDRC: the one remaining thing is to fill in the rule metadata for the new rules that have been introduced. Could you help to determine what we should do for those? They include:

banTypes, matchDefaultExportName, newlineBeforeReturn, noDuplicateSuper, noImportSideEffect, noInvalidTemplateStrings, noNonNullAssertion, noReferenceImport, noSparseArrays, noUnnecessaryCallbackWrapper, preferTemplate, returnUndefined

[loicraux]

Thanks @bgold09 for the update !
For the additional metadata for tslint rules, I would go with the following metadata :

{
    "banTypes": {
        "gitHubIssueOrPR": "https://github.com/palantir/tslint/pull/2175",
        "issueClass": "Ignored",
        "issueType": "Error",
        "severity": "Critical",
        "level": "Opportunity for Excellence",
        "group": "Configurable",
        "commonWeaknessEnumeration": "710"
    },
    "matchDefaultExportName": {
        "gitHubIssueOrPR": "https://github.com/palantir/tslint/pull/2117",
        "issueClass": "Non-SDL",
        "issueType": "Warning",
        "severity": "Important",
        "level": "Opportunity for Excellence",
        "group": "Correctness",
        "commonWeaknessEnumeration": "710"
    },
    "newlineBeforeReturn": {
        "gitHubIssueOrPR": "http://eslint.org/docs/rules/newline-before-return",
        "issueClass": "Non-SDL",
        "issueType": "Warning",
        "severity": "Low",
        "level": "Opportunity for Excellence",
        "group": "Whitespace",
        "commonWeaknessEnumeration": "710"
    },
    "noDuplicateSuper": {
        "gitHubIssueOrPR": "https://github.com/palantir/tslint/issues/1983",
        "issueClass": "Non-SDL",
        "issueType": "Warning",
        "severity": "Important",
        "level": "Opportunity for Excellence",
        "group": "Correctness",
        "commonWeaknessEnumeration": "710"
    },
    "noImportSideEffect": {
        "gitHubIssueOrPR": "https://github.com/palantir/tslint/issues/2116",
        "issueClass": "Non-SDL",
        "issueType": "Warning",
        "severity": "Important",
        "level": "Opportunity for Excellence",
        "group": "Correctness",
        "commonWeaknessEnumeration": "710"
    },
    "noInvalidTemplateStrings": {
        "gitHubIssueOrPR": "https://github.com/palantir/tslint/issues/2283",
        "issueClass": "Non-SDL",
        "issueType": "Warning",
        "severity": "Moderate",
        "level": "Opportunity for Excellence",
        "group": "Correctness",
        "commonWeaknessEnumeration": "710"
    },
    "noNonNullAssertion": {
        "gitHubIssueOrPR": "https://github.com/palantir/tslint/issues/1444",
        "issueClass": "Non-SDL",
        "issueType": "Warning",
        "severity": "Important",
        "level": "Opportunity for Excellence",
        "group": "Correctness",
        "commonWeaknessEnumeration": "710"
    },
    "noReferenceImport": {
        "gitHubIssueOrPR": "https://github.com/palantir/tslint/pull/2273",
        "issueClass": "Non-SDL",
        "issueType": "Warning",
        "severity": "Important",
        "level": "Opportunity for Excellence",
        "group": "Correctness",
        "commonWeaknessEnumeration": "710"
    },
    "noSparseArrays": {
        "issueClass": "Non-SDL",
        "issueType": "Warning",
        "severity": "Important",
        "level": "Opportunity for Excellence",
        "group": "Correctness",
        "commonWeaknessEnumeration": "710"
    },
    "noUnnecessaryCallbackWrapper": {
        "gitHubIssueOrPR": "https://github.com/palantir/tslint/pull/2249",
        "issueClass": "Non-SDL",
        "issueType": "Warning",
        "severity": "Important",
        "level": "Opportunity for Excellence",
        "group": "Correctness",
        "commonWeaknessEnumeration": "710"
    },
    "preferTemplate": {
        "gitHubIssueOrPR": "https://github.com/palantir/tslint/issues/772",
        "issueClass": "Non-SDL",
        "issueType": "Warning",
        "severity": "Important",
        "level": "Opportunity for Excellence",
        "group": "Clarity",
        "commonWeaknessEnumeration": "710"
    },
    "returnUndefined": {
        "gitHubIssueOrPR": "https://github.com/palantir/tslint/pull/2251",
        "issueClass": "Non-SDL",
        "issueType": "Warning",
        "severity": "Important",
        "level": "Opportunity for Excellence",
        "group": "Clarity",
        "commonWeaknessEnumeration": "710",
        "recommendation": "false, // this actually affect the readability of the code"
    }
}

I am not sure for the CWE values, @HamletDRC can you double check please ? ^^
Thanks !
PS : To help, I have added links to the original issues where purpose of the rules are actually described since the tslint documentation is a bit poor :) ... (not to be kept in the metadata json file of course)

[SimonSchick]

Additionally I'm getting false positives with tslint v4 (as required by ms-contrib) and ts 2.3.

[loicraux]

Can you file a separate issue @SimonSchick please ? Thanks !