Fix build (palantir#3853)

* fix non-no-unused-variable tests

* Fix no-unused-variables tests and modify logic in noUnusedVariableRule 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.

* fix lint issue

* hack/fix check-parameters test for 2.9-dev

* add builds for ts 2.7 and 2.8

* fix tests that broke from diagnosis codes changing in 2.9

.circleci/config.yml

@@ -63,6 +63,28 @@ jobs:
           key: dependency-cache-{{ checksum "yarn.lock" }}
       - run: yarn add [email protected]
       - run: yarn test
+  test2.7:
+    docker:
+      - image: circleci/node:6
+    steps:
+      - checkout
+      - attach_workspace:
+          at: '.'
+      - restore_cache:
+          key: dependency-cache-{{ checksum "yarn.lock" }}
+      - run: yarn add [email protected]
+      - run: yarn test
+  test2.8:
+    docker:
+      - image: circleci/node:6
+    steps:
+      - checkout
+      - attach_workspace:
+          at: '.'
+      - restore_cache:
+          key: dependency-cache-{{ checksum "yarn.lock" }}
+      - run: yarn add [email protected]
+      - run: yarn test
   testNext:
     docker:
       - image: circleci/node:latest
@@ -92,6 +114,12 @@ workflows:
       - test2.4:
           requires:
             - build
+      - test2.7:
+          requires:
+            - build
+      - test2.8:
+          requires:
+            - build
       - testNext:
           requires:
             - build

package.json

@@ -51,7 +51,7 @@
     "tsutils": "^2.12.1"
   },
   "peerDependencies": {
-    "typescript": ">=2.1.0 || >=2.1.0-dev || >=2.2.0-dev || >=2.3.0-dev || >=2.4.0-dev || >=2.5.0-dev || >=2.6.0-dev || >=2.7.0-dev || >=2.8.0-dev"
+    "typescript": ">=2.1.0 || >=2.1.0-dev || >=2.2.0-dev || >=2.3.0-dev || >=2.4.0-dev || >=2.5.0-dev || >=2.6.0-dev || >=2.7.0-dev || >=2.8.0-dev || >=2.9.0-dev"
   },
   "devDependencies": {
     "@types/babel-code-frame": "^6.20.0",
@@ -75,7 +75,7 @@
     "ts-node": "^3.3.0",
     "tslint": "^5.8.0",
     "tslint-test-config-non-relative": "file:test/external/tslint-test-config-non-relative",
-    "typescript": "~2.7.2"
+    "typescript": "~2.8.3"
   },
   "license": "Apache-2.0",
   "engines": {

src/rules/noUnusedVariableRule.ts

@@ -126,17 +126,19 @@ function walk(ctx: Lint.WalkContext<Options>, program: ts.Program): void {
             }
         }
 
-        if (kind === UnusedKind.VARIABLE_OR_PARAMETER) {
-            const importName = findImport(diag.start, sourceFile);
-            if (importName !== undefined) {
-                if (declaration && isImportUsed(importName, sourceFile, checker)) {
-                    continue;
-                }
+        if (kind === UnusedKind.VARIABLE_OR_PARAMETER || kind === UnusedKind.DECLARATION) {
+            const importNames = findImports(diag.start, sourceFile, kind);
+            if (importNames.length > 0) {
+                for (const importName of importNames) {
+                    if (declaration && isImportUsed(importName, sourceFile, checker)) {
+                        continue;
+                    }
 
-                if (importSpecifierFailures.has(importName)) {
-                    throw new Error("Should not get 2 errors for the same import.");
+                    if (importSpecifierFailures.has(importName)) {
+                        throw new Error("Should not get 2 errors for the same import.");
+                    }
+                    importSpecifierFailures.set(importName, failure);
                 }
-                importSpecifierFailures.set(importName, failure);
                 continue;
             }
         }
@@ -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 {
-    return forEachImport(sourceFile, (i) => {
+function findImports(pos: number, sourceFile: ts.SourceFile, kind: UnusedKind): ReadonlyArray<ts.Identifier> {
+    const imports = forEachImport(sourceFile, (i) => {
+        if (!isInRange(i, pos)) {
+            return undefined;
+        }
+
         if (i.kind === ts.SyntaxKind.ImportEqualsDeclaration) {
-            if (i.name.getStart() === pos) {
-                return i.name;
-            }
+            return [i.name];
         } else {
             if (i.importClause === undefined) {
                 // Error node
@@ -351,37 +355,74 @@ function findImport(pos: number, sourceFile: ts.SourceFile): ts.Identifier | und
 
             const { name: defaultName, namedBindings } = i.importClause;
             if (namedBindings !== undefined && namedBindings.kind === ts.SyntaxKind.NamespaceImport) {
-                const { name } = namedBindings;
-                if (name.getStart() === pos) {
-                    return name;
-                }
-                return undefined;
+                return [namedBindings.name];
             }
 
-            if (defaultName !== undefined && defaultName.getStart() === pos) {
-                return defaultName;
+            // 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.
+            // From TS2.8, in the case of none of the imports in a line being used,
+            // the single diagnostic TS outputs are different between the 1 import
+            // and 2+ imports cases:
+            // - 1 import in node:
+            //   - diagnostic has kind == 6133 (UnusedKind.VARIABLE_OR_PARAMETER)
+            //   - the text range is the whole node (`import { ... } from "..."`)
+            //     whereas pre-TS2.8, the text range was for the import node. so
+            //     `name.getStart()` won't equal `pos` like in pre-TS2.8
+            // - 2+ imports in node:
+            //   - diagnostic has kind == 6192 (UnusedKind.DECLARATION)
+            //   - we know that all of these are unused
+            if (kind === UnusedKind.DECLARATION) {
+                const imp: ts.Identifier[] = [];
+                if (defaultName !== undefined) {
+                    imp.push(defaultName);
+                }
+                if (namedBindings !== undefined) {
+                    imp.push(...namedBindings.elements.map((el) => el.name));
+                }
+                return imp.length > 0 ? imp : undefined;
+            } else if (defaultName !== undefined && (
+                isInRange(defaultName, pos) || namedBindings === undefined // defaultName is the only option
+            )) {
+                return [defaultName];
             } else if (namedBindings !== undefined) {
-                for (const { name } of namedBindings.elements) {
-                    if (name.getStart() === pos) {
-                        return name;
+                if (namedBindings.elements.length === 1) {
+                    return [namedBindings.elements[0].name];
+                }
+
+                for (const element of namedBindings.elements) {
+                    if (isInRange(element, pos)) {
+                        return [element.name];
                     }
                 }
             }
         }
         return undefined;
     });
+    return imports !== undefined ? imports : [];
+}
+
+function isInRange(range: ts.TextRange, pos: number): boolean {
+    return range.pos <= pos && range.end >= pos;
 }
 
 const enum UnusedKind {
     VARIABLE_OR_PARAMETER,
     PROPERTY,
+    DECLARATION, // Introduced in TS 2.8
 }
 function getUnusedDiagnostic(diag: ts.Diagnostic): UnusedKind | undefined  {
+    // https://github.com/Microsoft/TypeScript/blob/master/src/compiler/diagnosticMessages.json
     switch (diag.code) {
-        case 6133:
-            return UnusedKind.VARIABLE_OR_PARAMETER; // "'{0}' is declared but never used.
+        case 6133: // Pre TS 2.9 "'{0}' is declared but never used.
+                   // TS 2.9+ "'{0}' is declared but its value is never read."
+        case 6196: // TS 2.9+ "'{0}' is declared but never used."
+            return UnusedKind.VARIABLE_OR_PARAMETER;
         case 6138:
             return UnusedKind.PROPERTY; // "Property '{0}' is declared but never used."
+        case 6192:
+            return UnusedKind.DECLARATION; // "All imports in import declaration are unused."
         default:
             return undefined;
     }

src/runner.ts

@@ -269,6 +269,7 @@ async function doLinting(options: Options, files: string[], program: ts.Program
         } else {
             contents = await tryReadFile(file, logger);
         }
+
         if (contents !== undefined) {
             linter.lint(file, contents, configFile);
         }

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

@@ -12,9 +12,12 @@ export class Aaa {
     public set prop(value) { this.bbb = value; }
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~       [Documentation must exist for public properties.]
 
-    // TODO: TypeScript API doesn't give us a symbol for this, so we must ignore it.
+    // TypeScript API doesn't give a symbol for this before version 2.8.0
     // https://github.com/Microsoft/TypeScript/issues/14257
     [Symbol.iterator]() {}
+#if typescript >= 2.8.0
+    ~~~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for methods.]
+#endif
 }
 
 export enum Ddd { }

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

@@ -95,8 +95,6 @@ interface NotATuple {
 declare const notATuple: NotATuple;
 notATuple;
 
-unknownName;
-
 function foo() {
     let xx: 1 | 2 = 1;
     const f = () => xx = 2;

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

@@ -114,9 +114,6 @@ declare const notATuple: NotATuple;
 <NotATuple>notATuple;
 ~~~~~~~~~~~~~~~~~~~~ [0]
 
-unknownName!;
-~~~~~~~~~~~~ [0]
-
 function foo() {
     let xx: 1 | 2 = 1;
     const f = () => xx = 2;

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

@@ -66,7 +66,15 @@ export class DestructuringTests {
 }
 
 abstract class AbstractTest {
+#if typescript >= 2.9.0
+~~~~~~~~~~~~~~~~~~~~~~~~~~~ ['AbstractTest' is declared but never used.]
+#endif
+#if typescript >= 2.8.0 < 2.9.0
+~~~~~~~~~~~~~~~~~~~~~~~~~~~ [err % ('AbstractTest')]
+#endif
+#if typescript < 2.8.0
                ~~~~~~~~~~~~ [err % ('AbstractTest')]
+#endif
     abstract foo(x);
 }
 

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
+~~~~~~~~~~ [err % ('Foo')]
+#else
+       ~~~ [err % ('Foo')]
+#endif
     const path = require("path");
 
     console.log(fs);
@@ -24,7 +28,11 @@ hello.sayHello();
 import Bar = whatever.that.Foo;
 
 module some.module.blah {
-       ~~~~               [err % ('some')]
+#if typescript >= 2.8.0
+~~~~~~~~~~~ [err % ('some')]
+#else
+       ~~~~ [err % ('some')]
+#endif
     export class bar {
         private bar: Bar;
         constructor() {

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

@@ -3,12 +3,16 @@ function func1(x: number, y: number) {
 }
 
 var func2 = () => {
-    ~~~~~           [err % ('func2')]
+    ~~~~~ [err % ('func2')]
     //
 };
 
 function func3() {
-         ~~~~~     [err % ('func3')]
+#if typescript >= 2.8.0
+~~~~~~~~~~~~~~ [err % ('func3')]
+#else
+         ~~~~~ [err % ('func3')]
+#endif
     return func1(1, 2);
 }
 
@@ -17,7 +21,11 @@ export function func4() {
 }
 
 declare function func5(): any;
+#if typescript >= 2.8.0
+~~~~~~~~~~~~~~~~~~~~~~ [err % ('func5')]
+#else
                  ~~~~~ [err % ('func5')]
+#endif
 
 export default function () {
     return 0;

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

@@ -50,3 +50,4 @@ import abc = require('a');
 import def = abc.someVar;
 console.log(def);
 
+

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

@@ -14,9 +14,9 @@ import {a3 as aa3, a4 as aa4} from "a";
 aa3;
 // This import statement is unused and will be deleted along with this comment.
 import {a5, a6} from "a";
-~~~~~~~~~~~~~~~~~~~~~~~~~  [All imports on this line are unused.]
+~~~~~~~~~~~~~~~~~~~~~~~~~  [allErr]
 import {a7} from "a";
-~~~~~~~~~~~~~~~~~~~~~      [All imports on this line are unused.]
+~~~~~~~~~~~~~~~~~~~~~      [allErr]
 import {a8, a9, a10} from "a";
             ~~                [err % ('a9')]
                 ~~~           [err % ('a10')]
@@ -32,7 +32,7 @@ a16;
 
 // Leading comment will not be deleted
 import {unusedFunction} from "legacyDependency"; // Import unusedFunction because ....
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                       [All imports on this line are unused.]
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                       [allErr]
 // Next comment will not be deleted
 
 export import a = require("a");
@@ -42,7 +42,11 @@ $(_.xyz());
 /// <reference path="../externalFormatter.test.ts" />
 
 module S {
+#if typescript >= 2.8.0
+~~~~~~~~ [err % ('S')]
+#else
        ~ [err % ('S')]
+#endif
   var template = "";
       ~~~~~~~~       [err % ('template')]
 }
@@ -54,17 +58,17 @@ import baz from "a";
 import defaultExport, { namedExport } from "a";
        ~~~~~~~~~~~~~                               [err % ('defaultExport')]
 import d1, { d2 } from "a";
-~~~~~~~~~~~~~~~~~~~~~~~~~~~                     [All imports on this line are unused.]
+~~~~~~~~~~~~~~~~~~~~~~~~~~~                     [allErr]
 import d3, { d4 } from "a";
            ~~~~~~                                  [All named bindings are unused.]
 d3;
 
 d3;import d5 from "a";
-   ~~~~~~~~~~~~~~~~~~~                          [All imports on this line are unused.]
+   ~~~~~~~~~~~~~~~~~~~                          [allErr]
 import d6 from "a";d3;
-~~~~~~~~~~~~~~~~~~~                             [All imports on this line are unused.]
+~~~~~~~~~~~~~~~~~~~                             [allErr]
 d3;import d7 from "a";d3;
-   ~~~~~~~~~~~~~~~~~~~                          [All imports on this line are unused.]
+   ~~~~~~~~~~~~~~~~~~~                          [allErr]
 
 bar.someFunc();
 baz();
@@ -81,3 +85,6 @@ console.log(def);
 #else
 [err]: '%s' is declared but never used.
 #endif
+
+[allErr]: All imports on this line are unused.
+[allErrDecl]: All imports in import declaration are unused.