Fix build #3853

suchanlee · 2018-04-25T08:47:00Z

PR checklist

Addresses an existing issue: completed-docs erroring on CircleCI:testNext #3813
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Fixes tests that broke due to compiler changes in TS 2.8
Quite a few changes in the logic for no-unused-variables rule due to changes (kind, count, text range) in what the compiler outputs for preEmitDiagnostics

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

CHANGELOG.md entry:

…e for TS2.8 support Logic had to be modified because starting from TS2.8, when all imports in an import node are not used, TS emits only 1 diagnostic object for the whole line as opposed to the previous behavior of outputting a diagnostic with kind == 6192 (UnusedKind.VARIABLE_OR_PARAMETER) for every unused import.

suchanlee · 2018-04-25T08:48:06Z

src/rules/noUnusedVariableRule.ts

@@ -337,12 +339,14 @@ function forEachImport<T>(sourceFile: ts.SourceFile, f: (i: ImportLike) => T | u
    });
 }

-function findImport(pos: number, sourceFile: ts.SourceFile): ts.Identifier | undefined {
+function findImports(pos: number, sourceFile: ts.SourceFile, kind: UnusedKind): ReadonlyArray<ts.Identifier> {


changing to an array since in the case of kind === DECLARATION we want to return all the imports in the node

suchanlee · 2018-04-25T08:48:24Z

src/rules/noUnusedVariableRule.ts

-                }
+        if (kind === UnusedKind.VARIABLE_OR_PARAMETER || kind === UnusedKind.DECLARATION) {
+            const importNames = findImports(diag.start, sourceFile, kind);
+            if (importNames.length > 0) {


logic changes to handle the array return type

suchanlee · 2018-04-25T08:51:11Z

test/rules/no-unused-variable/check-parameters/test.ts.lint

@@ -66,7 +66,11 @@ export class DestructuringTests {
 }

 abstract class AbstractTest {
+#if typescript >= 2.8.0
+~~~~~~~~~~~~~~~~~~~~~~~~~~~ [err % ('AbstractTest')]


changes in compiler generated ranges

suchanlee · 2018-04-25T08:51:18Z

test/rules/no-unused-variable/default/false-positives.ts.lint

@@ -2,7 +2,11 @@
 const fs = require("fs");

 module Foo {
-       ~~~   [err % ('Foo')]
+#if typescript >= 2.8.0


changes in compiler generated ranges

suchanlee · 2018-04-25T08:51:27Z

test/rules/no-unused-variable/default/function.ts.lint

    //
 };

 function func3() {
-         ~~~~~     [err % ('func3')]
+#if typescript >= 2.8.0


changes in compiler generated ranges

suchanlee · 2018-04-25T08:51:36Z

test/rules/no-unused-variable/default/import.ts.lint

@@ -42,7 +42,11 @@ $(_.xyz());
 /// <reference path="../externalFormatter.test.ts" />

 module S {
+#if typescript >= 2.8.0


changes in compiler generated ranges

suchanlee · 2018-04-25T09:34:00Z

Added builds for TS 2.7 and 2.8

suchanlee · 2018-04-25T18:02:44Z

@JoshuaKGoldberg can you take a look at this?

jkillian · 2018-04-25T18:23:06Z

test/rules/no-unused-variable/ignore-pattern/var.ts.lint

@@ -12,8 +12,15 @@ var z;

 export var abcd = 3;

+// Could be a bug with 2.9-dev but getPreEmitDiagnostics isn't


probably should file a ticket to follow-up on this when real 2.9 is released

jkillian · 2018-04-25T18:24:02Z

test/rules/no-unnecessary-type-assertion/test.ts.fix

@@ -95,8 +95,6 @@ interface NotATuple {
 declare const notATuple: NotATuple;
 notATuple;

-unknownName;


did this cause test errors?

JoshuaKGoldberg

LGTM! I'm not very familiar with this particular area, but it all seems reasonable.

JoshuaKGoldberg · 2018-04-25T18:22:17Z

src/rules/noUnusedVariableRule.ts

+    return imports !== undefined ? imports : [];
+}
+
+function isInRange(range: ts.TextRange, pos: number): boolean {


Nit: might belong in some utils file somewhere? isInTextRange?

hmm not sure where it would belong though

JoshuaKGoldberg · 2018-04-25T18:23:09Z

test/rules/completed-docs/defaults/test.ts.lint

    // https://github.com/Microsoft/TypeScript/issues/14257
    [Symbol.iterator]() {}
+#if typescript >= 2.8.0


TIL this is possible. Is this documented anywhere? If not, should we file a followup issue for another PR?

not sure if it has been documented anywhere, I found it while reading the tests. I think documenting it would be useful. Let's file a follow up issue

JoshuaKGoldberg · 2018-04-25T18:23:38Z

test/rules/no-unused-variable/check-parameters/test.ts.lint

@@ -65,8 +65,15 @@ export class DestructuringTests {
    }
 }

+// Could be a bug with 2.9-dev but getPreEmitDiagnostics isn't


Can you link to the corresponding TypeScript issue?

👍 or if there isn't one, file a ticket with them and link it to this PR!

JoshuaKGoldberg · 2018-04-25T18:25:11Z

FYI @amcasey (I think you've been working in this space recently?)

jkillian

LGTM! I don't know all the TS internals super well so I sort of just glanced over the details of the changes for 2.8, but the test cases all look reasonable

johnwiseheart

LGTM so long as we file some tickets for those outstanding issues

suchanlee · 2018-04-26T07:24:50Z

So it wasn't an issue with TS, but the diagnoses were being ignored because the diagnosis code had changed in 2.9. Made changes to the code to handle those and fixed the tests accordingly.

But this solidifies the concern that I had, which is that this rule is complex and hard to maintain with a living and changing compiler. I feel that we should deprecate this rule in the near future for the following reasons:

It is expensive to maintain
The rule is complex
It has many issues: https://github.com/palantir/tslint/issues?utf8=%E2%9C%93&q=is%3Aissue+no-unused-variables
Most importantly, TS supports it very well. What this rule actually does is take what the TS compiler outputs and process them and to construct TSLint failures and fixes which it's surprisingly complex and slow. And I believe until 2.7 the failures that TS/TSLint output were similar, but starting from 2.7, it has deviated quite a bit, both in the error messages and the erroring text range.

Deprecating the rule was discussed: #1481 but the community decided to not deprecate it for two main reasons:
(1) TSLint errors don't affect compilation while TS errors do, and sometimes you just want to fast dev ignoring warnings
(2) TSLint autofixes

(1) is in discussion in microsoft/TypeScript#13408 and is already supported via microsoft/vscode#37616
(2) I don't think is supported yet.

suchanlee · 2018-04-26T18:34:16Z

@jkillian @johnwiseheart @JoshuaKGoldberg ^^^ not that we should take any action now but something to keep track of in the future

JoshuaKGoldberg · 2018-04-26T18:54:13Z

I'm in favor of deprecating it altogether. My team at work just switched from the TSLint rule to TypeScript's flags because the TSLint VS Code extension doesn't support typed rules and we haven't gotten around to using the language service.

Error diagnostics and messages are a big point of investment for TypeScript right now, so this doesn't feel like something worth investing in for TSLint.

Suchan Lee added 2 commits April 25, 2018 01:35

fix non-no-unused-variable tests

80e2768

suchanlee commented Apr 25, 2018

View reviewed changes

Suchan Lee added 2 commits April 25, 2018 01:57

fix lint issue

205e8fa

hack/fix check-parameters test for 2.9-dev

6452312

suchanlee force-pushed the fix-build branch from a215f86 to 6452312 Compare April 25, 2018 09:27

add builds for ts 2.7 and 2.8

9b0cfae

suchanlee requested a review from jkillian April 25, 2018 17:59

jkillian reviewed Apr 25, 2018

View reviewed changes

JoshuaKGoldberg approved these changes Apr 25, 2018

View reviewed changes

jkillian approved these changes Apr 25, 2018

View reviewed changes

jkillian mentioned this pull request Apr 25, 2018

TypeScript 2.8 & 2.9 adaptations (dependencies & tests) #3792

Closed

4 tasks

johnwiseheart approved these changes Apr 25, 2018

View reviewed changes

fix tests that broke from diagnosis codes changing in 2.9

334ae02

suchanlee merged commit a7be726 into master Apr 26, 2018

suchanlee deleted the fix-build branch April 26, 2018 07:25

suchanlee mentioned this pull request Apr 26, 2018

Adding documentation example to deprecationRule #3848

Merged

suchanlee mentioned this pull request May 3, 2018

no-unused-variable finds false positives in imports #3854

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix build #3853

Fix build #3853

suchanlee commented Apr 25, 2018 •

edited

Loading

suchanlee Apr 25, 2018

suchanlee Apr 25, 2018

suchanlee Apr 25, 2018

suchanlee Apr 25, 2018

suchanlee Apr 25, 2018

suchanlee Apr 25, 2018

suchanlee commented Apr 25, 2018

suchanlee commented Apr 25, 2018

jkillian Apr 25, 2018

jkillian Apr 25, 2018

JoshuaKGoldberg left a comment

JoshuaKGoldberg Apr 25, 2018

suchanlee Apr 26, 2018

JoshuaKGoldberg Apr 25, 2018

suchanlee Apr 25, 2018

JoshuaKGoldberg Apr 25, 2018

jkillian Apr 25, 2018

JoshuaKGoldberg commented Apr 25, 2018

jkillian left a comment

johnwiseheart left a comment

suchanlee commented Apr 26, 2018

suchanlee commented Apr 26, 2018

JoshuaKGoldberg commented Apr 26, 2018

		@@ -12,8 +12,15 @@ var z;

		export var abcd = 3;

		// Could be a bug with 2.9-dev but getPreEmitDiagnostics isn't

Fix build #3853

Fix build #3853

Conversation

suchanlee commented Apr 25, 2018 • edited Loading

PR checklist

Overview of change:

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

CHANGELOG.md entry:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suchanlee commented Apr 25, 2018

suchanlee commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Apr 25, 2018

jkillian left a comment

Choose a reason for hiding this comment

johnwiseheart left a comment

Choose a reason for hiding this comment

suchanlee commented Apr 26, 2018

suchanlee commented Apr 26, 2018

JoshuaKGoldberg commented Apr 26, 2018

suchanlee commented Apr 25, 2018 •

edited

Loading