-
Notifications
You must be signed in to change notification settings - Fork 887
Fix build #3853
Changes from 5 commits
80e2768
f19a629
205e8fa
6452312
9b0cfae
334ae02
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changing to an array since in the case of kind === DECLARATION we want to return all the imports in the node |
||
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,71 @@ 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: might belong in some utils file somewhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm not sure where it would belong though |
||
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 { | ||
switch (diag.code) { | ||
case 6133: | ||
return UnusedKind.VARIABLE_OR_PARAMETER; // "'{0}' is declared but never used. | ||
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; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL this is possible. Is this documented anywhere? If not, should we file a followup issue for another PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
~~~~~~~~~~~~~~~~~~~~~~ [Documentation must exist for methods.] | ||
#endif | ||
} | ||
|
||
export enum Ddd { } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,8 +95,6 @@ interface NotATuple { | |
declare const notATuple: NotATuple; | ||
notATuple; | ||
|
||
unknownName; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did this cause test errors? |
||
|
||
function foo() { | ||
let xx: 1 | 2 = 1; | ||
const f = () => xx = 2; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,8 +65,15 @@ export class DestructuringTests { | |
} | ||
} | ||
|
||
// Could be a bug with 2.9-dev but getPreEmitDiagnostics isn't | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you link to the corresponding TypeScript issue? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 or if there isn't one, file a ticket with them and link it to this PR! |
||
// emitting any diagnostics for classes. | ||
abstract class AbstractTest { | ||
#if typescript >= 2.8.0 < 2.9.0 | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~ [err % ('AbstractTest')] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changes in compiler generated ranges |
||
#endif | ||
#if typescript < 2.8.0 | ||
~~~~~~~~~~~~ [err % ('AbstractTest')] | ||
#endif | ||
abstract foo(x); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,11 @@ | |
const fs = require("fs"); | ||
|
||
module Foo { | ||
~~~ [err % ('Foo')] | ||
#if typescript >= 2.8.0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changes in compiler generated ranges |
||
~~~~~~~~~~ [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() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changes in compiler generated ranges |
||
~~~~~~~~~~~~~~ [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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,3 +50,4 @@ import abc = require('a'); | |
import def = abc.someVar; | ||
console.log(def); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changes in compiler generated ranges |
||
~~~~~~~~ [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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic changes to handle the array return type