Rewrite enable/disable logic by andy-hanson · Pull Request #2369 · palantir/tslint

andy-hanson · 2017-03-18T19:24:14Z

PR checklist

Addresses an existing issue: #0000
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Move disabledIntervals out of rule options. Rules are not responsible for disabling theirselves. Just filter the failures after the rule returns them.
- While doing this, I removed filterFailures entirely, removing code that looked for duplicate failures. It turns out a few rules were returning duplicate failures, so I changed them directly.
Don't expose disabledIntervals at all. Just have a RuleDisabler interface and a method for getting one from the disable comments in a source file.
- Therefore, we can delete IDisabledInterval.

Is there anything you'd like reviewers to focus on?

There were conflicting tests as to whether explicitly enabling a rule turned off in tslint.json would enable it. I chose that it would. If you don't want that, the isExplicitlyEnabled method could be removed.

CHANGELOG.md entry:

[no-log] change are internal and backwards compatible

andy-hanson · 2017-03-18T19:24:36Z

src/language/rule/rule.ts

    getOptions(): IOptions;
    isEnabled(): boolean;
    apply(sourceFile: ts.SourceFile): RuleFailure[];
-    applyWithWalker(walker: IWalker): RuleFailure[];


This method was never necessary to have here.

should also not be removed for now

andy-hanson · 2017-03-18T19:25:04Z

src/ruleLoader.ts

    for (const ruleOptions of ruleOptionsList) {
        const ruleName = ruleOptions.ruleName;
-        const enableDisableRules = enableDisableRuleMap.get(ruleName);
-        if (ruleOptions.ruleSeverity !== "off" || enableDisableRuleMap) {


It's not necessary to check ruleSeverity here since isEnabled() will do that too.

technically you are right, but this check was added to do less work for disabled rules. With your change you need to find the rule in rulesDirectory and instantiate it before you know if you will ever need it (for every linted file).

Isn't the real problem that we re-load every rule for every file? (Cached, but still.) The only SourceFile-specific things here are ruleDisabler and isJs. Why not eagerly load the rules up-front (including disabled ones and TypeScript-only ones)?

If you use tslint as a library, you can pass different rule configs for every call to Linter.lint.
That's why this needs to be done for every file.
On the other hand, I don't know if that makes sense to keep that "feature".

OK, added the check back.

andy-hanson · 2017-03-18T19:26:22Z

test/rules/_integration/enable-disable/test.ts.lint

 var AAAaA = 'test' // tslint:disable-line:quotemark
 var AAAaA = 'test' // ensure that disable-line rule correctly handles previous `enable rule - disable all` switches

-// tslint:enable:no-var-keyword


This conflicts with an assertion in ruleLoaderTests.ts that it "loads disabled rules if rule in enableDisableRuleMap". (It also now conflicts with enable-via-comment.) Which should we choose?

The behaviour to not load disabled rules was added recently. Even though the rule was loaded, it would have never been executed, because rule.isEnabled() always returned false. I don't know why the test was implemented this way...
I think it makes more sense to keep disabled rules disabled. Otherwise you need to guess the ruleSeverity

What if someone wants a rule disabled by default but enabled in a particular file?

I though about that too. That's a valid use case. But the problem with the missing severity still stands.
If you could enable/disable a rule and set its severity independently, that wouldn't be a problem.
Or just use a default severity (#2279)

I have a test case enable-via-comment where a rule is turned off, but is enabled in the file and gives errors there. This is accomplished by using rule.isEnabled() || ruleDisabler.isExplicitlyEnabled(ruleName) instead of just isEnabled(). What other test cases would need to work?

On that topic: All of the custom implementations of isEnabled() I see are just turning it off in the case of bad options. We should instead throw an error on bad options. I think just like Rules are no longer responsible for tracking their disabled ranges, they shouldn't be responsible for tracking whether they're enabled or not. (As shown here, it especially doesn't make sense if we want to know whether a rule is enabled before loading it.) So we should remove isEnabled() from the Rule interface. Would that help solve this problem?

What I'm actually trying to say: You disable a rule by setting severity: off. Now you turn it on via comment. What severity should this rule have? How do I set a specific severity other than the default?

ajafff · 2017-03-18T20:18:54Z

To summarize both inline discussions:
Linter needs to be refactored. It should get the rule config only once in the constructor instead of every call to Linter.lint. We can then load all rules (even disabled ones to enable them later) upfront to save I/O.
We could / should also validate or parse the options for every rule (related: #1950) instead of doing this partially in some rule's isEnabled method. This needs to be done to ensure a rule is properly configured when enabled via comment.

ajafff · 2017-03-18T20:24:48Z

src/rules/maxFileLineCountRule.ts

-        return ruleFailures;
+
+        return this.applyWithFunction(sourceFile, (ctx) =>
+            ctx.addFailure(0, 1, Rule.FAILURE_STRING(lineCount, lineLimit)));


you could directly return the failure now that applyWithFunction doesn't modify the result anymore

I like this better than constructing new Lint.RuleFailure directly because I don't have to pass in ruleName explicitly. Could have a Rule#createFailure method but I think that would only be used here.
EDIT: nvm, it's actually shorter with new Lint.RuleFailure.

ajafff · 2017-03-18T20:30:28Z

src/rules/maxFileLineCountRule.ts

        const lineCount: number = sourceFile.getLineStarts().length;
-        const disabledIntervals = this.getOptions().disabledIntervals;
-
-        if (lineCount > lineLimit && disabledIntervals.length === 0) {


This changes the behavior of the rule, which should at least be documented.
Before this change the rule would not fail if there were any disabledInvervals. Now you would need to have a disable comment in the first line and first column to disable the rule.

You would only get back the old behavior if the failure would span the whole file. I would not suggest doing that.

Moved error to the bottom, so a tslint:disable anywhere should work. Presumably no one uses tslint:disable-line for this rule.

ajafff · 2017-03-18T21:22:13Z

Related, but no blocker for this PR: when applying fixes, the position of the disabledIntervals may no longer be correct. It needs to be refreshed after each rule that applied fixes.

…ver lint it.

andy-hanson · 2017-03-18T21:25:54Z

In order to keep this PR small, I've removed isExplicitlyEnabled. Changing the Linter API can be done later.

ajafff · 2017-03-18T21:40:17Z

src/rules/eoflineRule.ts

-            new Lint.RuleFailure(sourceFile, length, length, Rule.FAILURE_STRING,
-              this.getOptions().ruleName),
-        ]);
+        return this.applyWithFunction(sourceFile, (ctx) =>


…er object.

nchen63 · 2017-05-04T15:48:19Z

it doesn't look like any unit tests hit enableDisableRules.ts

andy-hanson · 2017-05-05T02:03:24Z

There is a black-box test test/rules/_integration/enable-disable.
It would be nice to have code coverage tests though.

andy-hanson · 2017-05-05T02:08:47Z

Test failure fixed by #2691.

ajafff

I don't know if this is intended to be a breaking change. If it is, just ignore most of my comments below

ajafff · 2017-05-05T10:35:45Z

src/language/rule/abstractRule.ts

-        return this.filterFailures(ctx.failures);
-    }
-
-    protected filterFailures(failures: RuleFailure[]): RuleFailure[] {


breaking change. keep this method, just return the input unchanged and deprecate it

ajafff · 2017-05-05T10:39:05Z

src/language/rule/rule.ts

 import {arrayify, flatMap} from "../../utils";
-import {IWalker} from "../walker";
+
+export interface RuleStatic {


that's a duplicate of RuleConstructor?

ajafff · 2017-05-05T10:41:39Z

src/language/rule/rule.ts

    ruleArguments: any[];
    ruleSeverity: RuleSeverity;
    ruleName: string;
-    disabledIntervals: IDisabledInterval[];


this should be kept and deprecated for backwards compatibility

ajafff · 2017-05-05T10:41:46Z

src/language/rule/rule.ts

-    disabledIntervals: IDisabledInterval[];
-}
-
-export interface IDisabledInterval {


same as above

ajafff · 2017-05-05T10:42:14Z

src/language/rule/rule.ts

    getOptions(): IOptions;
    isEnabled(): boolean;
    apply(sourceFile: ts.SourceFile): RuleFailure[];
-    applyWithWalker(walker: IWalker): RuleFailure[];


should also not be removed for now

ajafff · 2017-05-05T10:42:35Z

src/language/utils.ts

    return ts.createSourceFile(normalizedName, source, ts.ScriptTarget.ES5, /*setParentNodes*/ true);
 }

-export function doesIntersect(failure: RuleFailure, disabledIntervals: IDisabledInterval[]) {


another breaking change

I don't think this is public. (#2446)

andy-hanson · 2017-05-06T00:30:56Z

@nchen This was originally written during tslint 4.0. What is the schedule for new major tslint releases? It seemed to be much slower in the past but the gap from 4.0 to 5.0 was pretty small. If it will be more than a few months before the next major-version release I can add dummy implementations over the breaking changes as @ajaff mentioned. Otherwise I would be fine with waiting a little longer.

nchen63 · 2017-05-06T04:11:09Z

There isn't a major release scheduled. It's usually based on how many breaking changes are queued up

ajafff · 2017-05-06T17:36:43Z

Everything in language/utils.ts is exported. For example as Lint.hasModifier Am 06.05.2017 18:56 schrieb "Andy Hanson" <notifications@github.com>:

…

***@***.**** commented on this pull request. ------------------------------ In src/language/utils.ts <#2369 (comment)>: > export function getSourceFile(fileName: string, source: string): ts.SourceFile { const normalizedName = path.normalize(fileName).replace(/\\/g, "/"); return ts.createSourceFile(normalizedName, source, ts.ScriptTarget.ES5, /*setParentNodes*/ true); } -export function doesIntersect(failure: RuleFailure, disabledIntervals: IDisabledInterval[]) { I don't think this is public. (#2446 <#2446>) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2369 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALaeKLE5PTwR9hNCcIoi_6q16RF9_Mbtks5r3KYjgaJpZM4Mhg9M> .

nchen63 · 2017-05-08T20:04:59Z

src/linter.ts

+    // tslint:disable-next-line member-ordering
+    protected applyFixes(sourceFilePath: string, source: string, fixableFailures: RuleFailure[]): string {
+        if (fixableFailures.some((f) => !f.hasFix())) {
+            throw new Error("!");


needs error text

nchen63 · 2017-05-08T20:11:40Z

src/linter.ts

+        let source: string = sourceFile.text;
+
+        for (const rule of enabledRules) {
+            const hasFixes = fileFailures.some((f) => f.hasFix() && f.getRuleName() === rule.getOptions().ruleName);


not that we should optimize on the --fix code path, but wouldn't it be more efficient to do:

call rule1

fix rule1

call rule2

fix rule2

instead of

call all rules

call rule1 again if there were fixes

fix rule1

call rule2 again if there were fixes

fix rule2

Well, the way the enable/disable logic is done here, it's better to handle disables for all rules at once. Otherwise we would have to parse disables after every rule.

you could reuse the disable map and reload it when the file is overwritten. I'm ok with how it is now, though

I would hold off on that until #1330 is implemented, since that will need to know all the uses of the disable map.

Rewrite enable/disable logic

0a7ddf0

andy-hanson commented Mar 18, 2017

View reviewed changes

Use find helper

f7788e2

ajafff reviewed Mar 18, 2017

View reviewed changes

Remove isExplicitlyEnabled: If a rule isn't turned on in config, ne…

4bc3d4b

…ver lint it.

andy-hanson added 2 commits March 18, 2017 14:30

Move max-file-line-count error to the bottom

c416136

Don't need WalkContext in max-file-line-count

2b61c2d

ajafff reviewed Mar 18, 2017

View reviewed changes

andy-hanson added 2 commits March 18, 2017 14:44

Remove other unnecessary WalkContext

d3da276

Expose a single function 'removeDisabledFailures' instead of a Disabl…

a8459c2

…er object.

adidahiya added the PR: waiting for reviewer label Mar 21, 2017

andy-hanson added 2 commits March 25, 2017 09:15

Merge branch 'master' into enable-disable

c8a31d0

Merge branch 'master' into enable-disable

a0c68b4

This was referenced Apr 12, 2017

DisabledIntervals are not updated after applying fix #2557

Closed

Update dependencies and lint for no-unsafe-any #2544

Merged

Merge branch 'master' into enable-disable

acb22e4

adidahiya requested a review from nchen63 April 17, 2017 13:34

Merge branch 'master' into enable-disable

0d62d89

andy-hanson mentioned this pull request May 4, 2017

Warn on unused disables #2679

Closed

Merge branch 'master' into enable-disable

c3c819e

Merge branch 'master' into enable-disable

34f5df1

ajafff suggested changes May 5, 2017

View reviewed changes

Remove RuleStatic

a364779

nchen63 added the Type: Breaking Change label May 6, 2017

andy-hanson added 2 commits May 6, 2017 10:05

Merge branch 'master' into enable-disable

91990ff

Add back disabledIntervals, and deprecate it

9cdbb18

andy-hanson force-pushed the enable-disable branch from f78dc11 to 9cdbb18 Compare May 6, 2017 17:20

Add back doesIntersect

7036515

adidahiya mentioned this pull request May 8, 2017

Identify lines where rules are unnecessarily disabled #1330

Closed

nchen63 suggested changes May 8, 2017

View reviewed changes

Remove assertion

7551a4b

nchen63 approved these changes May 9, 2017

View reviewed changes

nchen63 merged commit 63fc794 into palantir:master May 9, 2017

andy-hanson deleted the enable-disable branch May 9, 2017 03:09

adidahiya removed the PR: waiting for reviewer label May 22, 2017

Conversation

andy-hanson commented Mar 18, 2017 • edited by nchen63 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-hanson Mar 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-hanson Mar 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-hanson Mar 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajafff commented Mar 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-hanson Mar 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajafff commented Mar 18, 2017

Uh oh!

andy-hanson commented Mar 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchen63 commented May 4, 2017

Uh oh!

andy-hanson commented May 5, 2017

Uh oh!

andy-hanson commented May 5, 2017

Uh oh!

ajafff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

andy-hanson commented Mar 18, 2017 •

edited by nchen63

Loading

andy-hanson Mar 18, 2017 •

edited

Loading

andy-hanson Mar 18, 2017 •

edited

Loading

andy-hanson Mar 18, 2017 •

edited

Loading

andy-hanson Mar 18, 2017 •

edited

Loading

andy-hanson May 9, 2017 •

edited

Loading