Skip to content

Support deleting all unused type parameters in a list, and deleting @template tag #25748

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
2 commits merged into from
Jul 27, 2018
Merged

Support deleting all unused type parameters in a list, and deleting @template tag #25748

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
966aa80
Support deleting all unused type parameters in a list, and deleting @…
Jul 18, 2018
44afd99
Support type parameter in 'infer'
Jul 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 39 additions & 9 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22538,6 +22538,7 @@ namespace ts {
grammarErrorOnNode(node, Diagnostics.infer_declarations_are_only_permitted_in_the_extends_clause_of_a_conditional_type);
}
checkSourceElement(node.typeParameter);
registerForUnusedIdentifiersCheck(node);
}

function checkImportType(node: ImportTypeNode) {
Expand Down Expand Up @@ -23566,7 +23567,8 @@ namespace ts {
type PotentiallyUnusedIdentifier =
| SourceFile | ModuleDeclaration | ClassLikeDeclaration | InterfaceDeclaration
| Block | CaseBlock | ForStatement | ForInStatement | ForOfStatement
| Exclude<SignatureDeclaration, IndexSignatureDeclaration | JSDocFunctionType> | TypeAliasDeclaration;
| Exclude<SignatureDeclaration, IndexSignatureDeclaration | JSDocFunctionType> | TypeAliasDeclaration
| InferTypeNode;

function checkUnusedIdentifiers(potentiallyUnusedIdentifiers: ReadonlyArray<PotentiallyUnusedIdentifier>, addDiagnostic: AddUnusedDiagnostic) {
for (const node of potentiallyUnusedIdentifiers) {
Expand Down Expand Up @@ -23606,6 +23608,7 @@ namespace ts {
case SyntaxKind.FunctionType:
case SyntaxKind.ConstructorType:
case SyntaxKind.TypeAliasDeclaration:
case SyntaxKind.InferType:
checkUnusedTypeParameters(node, addDiagnostic);
break;
default:
Expand Down Expand Up @@ -23659,21 +23662,48 @@ namespace ts {
}
}

function checkUnusedTypeParameters(
node: ClassDeclaration | ClassExpression | FunctionDeclaration | MethodDeclaration | FunctionExpression | ArrowFunction | ConstructorDeclaration | SignatureDeclaration | InterfaceDeclaration | TypeAliasDeclaration,
addDiagnostic: AddUnusedDiagnostic,
): void {
function checkUnusedTypeParameters(node: ClassLikeDeclaration | SignatureDeclaration | InterfaceDeclaration | TypeAliasDeclaration | InferTypeNode, addDiagnostic: AddUnusedDiagnostic): void {
// Only report errors on the last declaration for the type parameter container;
// this ensures that all uses have been accounted for.
const typeParameters = getEffectiveTypeParameterDeclarations(node);
if (!(node.flags & NodeFlags.Ambient) && last(getSymbolOfNode(node).declarations) === node) {
if (node.flags & NodeFlags.Ambient || node.kind !== SyntaxKind.InferType && last(getSymbolOfNode(node).declarations) !== node) return;

if (node.kind === SyntaxKind.InferType) {
const { typeParameter } = node;
if (isTypeParameterUnused(typeParameter)) {
addDiagnostic(node, UnusedKind.Parameter, createDiagnosticForNode(node, Diagnostics._0_is_declared_but_its_value_is_never_read, idText(typeParameter.name)));
}
}
else {
const typeParameters = getEffectiveTypeParameterDeclarations(node);
const seenParentsWithEveryUnused = new NodeSet<DeclarationWithTypeParameterChildren>();

for (const typeParameter of typeParameters) {
if (!(getMergedSymbol(typeParameter.symbol).isReferenced! & SymbolFlags.TypeParameter) && !isIdentifierThatStartsWithUnderscore(typeParameter.name)) {
addDiagnostic(typeParameter, UnusedKind.Parameter, createDiagnosticForNode(typeParameter.name, Diagnostics._0_is_declared_but_its_value_is_never_read, symbolName(typeParameter.symbol)));
if (!isTypeParameterUnused(typeParameter)) continue;

const name = idText(typeParameter.name);
const { parent } = typeParameter;
if (parent.kind !== SyntaxKind.InferType && parent.typeParameters!.every(isTypeParameterUnused)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You dont have to iterate on all parameters here, why not just iterate over the type parameters in the current loop and report error in the end of the loop instead?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to know whether to report an error at each type parameter individually, or once for the whole list.
So when looking at a type parameter, we need to know whether all future type parameters are also unused. That needs at least two loops -- one to check isTypeParameterUnused for everything, and one to report errors individually if necessary. (We could cache the result of isTypeParameterUnused, but that's a simple function so probably not important to do.)

It's possible that we could calculate parent.typeParameters!.every(isTypeParameterUnused) eagerly instead of potentially once per loop, but the common case is that every type parameter is used. So in the common case, we only calculate isTypeParameterUnused once per type parameter, which is as efficient as it gets.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sheetalkamat OK to merge?

if (seenParentsWithEveryUnused.tryAdd(parent)) {
const range = isJSDocTemplateTag(parent)
// Whole @template tag
? rangeOfNode(parent)
// Include the `<>` in the error message
: rangeOfTypeParameters(parent.typeParameters!);
const only = typeParameters.length === 1;
const message = only ? Diagnostics._0_is_declared_but_its_value_is_never_read : Diagnostics.All_type_parameters_are_unused;
const arg0 = only ? name : undefined;
addDiagnostic(typeParameter, UnusedKind.Parameter, createFileDiagnostic(getSourceFileOfNode(parent), range.pos, range.end - range.pos, message, arg0));
}
}
else {
addDiagnostic(typeParameter, UnusedKind.Parameter, createDiagnosticForNode(typeParameter, Diagnostics._0_is_declared_but_its_value_is_never_read, name));
}
}
}
}
function isTypeParameterUnused(typeParameter: TypeParameterDeclaration): boolean {
return !(getMergedSymbol(typeParameter.symbol).isReferenced! & SymbolFlags.TypeParameter) && !isIdentifierThatStartsWithUnderscore(typeParameter.name);
}

function addToGroup<K, V>(map: Map<[K, V[]]>, key: K, value: V, getKey: (key: K) => number | string): void {
const keyString = String(getKey(key));
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2093,7 +2093,7 @@ namespace ts {
return arg => f(arg) || g(arg);
}

export function assertTypeIsNever(_: never): void { } // tslint:disable-line no-empty
export function assertType<T>(_: T): void { } // tslint:disable-line no-empty

export function singleElementArray<T>(t: T | undefined): T[] | undefined {
return t === undefined ? undefined : [t];
Expand Down
20 changes: 20 additions & 0 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3639,6 +3639,10 @@
"category": "Message",
"code": 6204
},
"All type parameters are unused": {
"category": "Error",
"code": 6205
},

"Projects to reference": {
"category": "Message",
Expand Down Expand Up @@ -4188,6 +4192,14 @@
"category": "Message",
"code": 90010
},
"Remove template tag": {
"category": "Message",
"code": 90011
},
"Remove type parameters": {
"category": "Message",
"code": 90012
},
"Import '{0}' from module \"{1}\"": {
"category": "Message",
"code": 90013
Expand Down Expand Up @@ -4256,6 +4268,14 @@
"category": "Message",
"code": 90029
},
"Replace 'infer {0}' with 'unknown'": {
"category": "Message",
"code": 90030
},
"Replace all unused 'infer' with 'unknown'": {
"category": "Message",
"code": 90031
},
"Convert function to an ES2015 class": {
"category": "Message",
"code": 95001
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7019,8 +7019,8 @@ namespace ts {
skipWhitespace();
const typeParameter = <TypeParameterDeclaration>createNode(SyntaxKind.TypeParameter);
typeParameter.name = parseJSDocIdentifierName(Diagnostics.Unexpected_token_A_type_parameter_name_was_expected_without_curly_braces);
skipWhitespace();
finishNode(typeParameter);
skipWhitespace();
typeParameters.push(typeParameter);
} while (parseOptionalJsdoc(SyntaxKind.CommaToken));

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/tsbuild.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1263,7 +1263,7 @@ namespace ts {
// Don't report status on "solution" projects
break;
default:
assertTypeIsNever(status);
assertType<never>(status);
}
}
}
91 changes: 86 additions & 5 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,19 @@ namespace ts {

export function isDeclarationWithTypeParameters(node: Node): node is DeclarationWithTypeParameters;
export function isDeclarationWithTypeParameters(node: DeclarationWithTypeParameters): node is DeclarationWithTypeParameters {
switch (node.kind) {
case SyntaxKind.JSDocCallbackTag:
case SyntaxKind.JSDocTypedefTag:
case SyntaxKind.JSDocSignature:
return true;
default:
assertType<DeclarationWithTypeParameterChildren>(node);
return isDeclarationWithTypeParameterChildren(node);
}
}

export function isDeclarationWithTypeParameterChildren(node: Node): node is DeclarationWithTypeParameterChildren;
export function isDeclarationWithTypeParameterChildren(node: DeclarationWithTypeParameterChildren): node is DeclarationWithTypeParameterChildren {
switch (node.kind) {
case SyntaxKind.CallSignature:
case SyntaxKind.ConstructSignature:
Expand All @@ -686,12 +699,9 @@ namespace ts {
case SyntaxKind.SetAccessor:
case SyntaxKind.FunctionExpression:
case SyntaxKind.ArrowFunction:
case SyntaxKind.JSDocCallbackTag:
case SyntaxKind.JSDocTypedefTag:
case SyntaxKind.JSDocSignature:
return true;
default:
assertTypeIsNever(node);
assertType<never>(node);
return false;
}
}
Expand Down Expand Up @@ -776,7 +786,7 @@ namespace ts {
return createDiagnosticForNodeInSourceFile(sourceFile, node, message, arg0, arg1, arg2, arg3);
}

export function createDiagnosticForNodeArray(sourceFile: SourceFile, nodes: NodeArray<Node>, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number, arg3?: string | number): Diagnostic {
export function createDiagnosticForNodeArray(sourceFile: SourceFile, nodes: NodeArray<Node>, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number, arg3?: string | number): DiagnosticWithLocation {
const start = skipTrivia(sourceFile.text, nodes.pos);
return createFileDiagnostic(sourceFile, start, nodes.end - start, message, arg0, arg1, arg2, arg3);
}
Expand Down Expand Up @@ -8101,4 +8111,75 @@ namespace ts {
}
return { min, max };
}

export interface ReadonlyNodeSet<TNode extends Node> {
has(node: TNode): boolean;
forEach(cb: (node: TNode) => void): void;
some(pred: (node: TNode) => boolean): boolean;
}

export class NodeSet<TNode extends Node> implements ReadonlyNodeSet<TNode> {
private map = createMap<TNode>();

add(node: TNode): void {
this.map.set(String(getNodeId(node)), node);
}
tryAdd(node: TNode): boolean {
if (this.has(node)) return false;
this.add(node);
return true;
}
has(node: TNode): boolean {
return this.map.has(String(getNodeId(node)));
}
forEach(cb: (node: TNode) => void): void {
this.map.forEach(cb);
}
some(pred: (node: TNode) => boolean): boolean {
return forEachEntry(this.map, pred) || false;
}
}

export interface ReadonlyNodeMap<TNode extends Node, TValue> {
get(node: TNode): TValue | undefined;
has(node: TNode): boolean;
}

export class NodeMap<TNode extends Node, TValue> implements ReadonlyNodeMap<TNode, TValue> {
private map = createMap<{ node: TNode, value: TValue }>();

get(node: TNode): TValue | undefined {
const res = this.map.get(String(getNodeId(node)));
return res && res.value;
}

getOrUpdate(node: TNode, setValue: () => TValue): TValue {
const res = this.get(node);
if (res) return res;
const value = setValue();
this.set(node, value);
return value;
}

set(node: TNode, value: TValue): void {
this.map.set(String(getNodeId(node)), { node, value });
}

has(node: TNode): boolean {
return this.map.has(String(getNodeId(node)));
}

forEach(cb: (value: TValue, node: TNode) => void): void {
this.map.forEach(({ node, value }) => cb(value, node));
}
}

export function rangeOfNode(node: Node): TextRange {
return { pos: getTokenPosOfNode(node), end: node.end };
}

export function rangeOfTypeParameters(typeParameters: NodeArray<TypeParameterDeclaration>): TextRange {
// Include the `<>`
return { pos: typeParameters.pos - 1, end: typeParameters.end + 1 };
}
}
4 changes: 2 additions & 2 deletions src/harness/fourslash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1496,8 +1496,8 @@ Actual: ${stringify(fullActual)}`);
const actualTags = selectedItem.tags;
assert.equal(actualTags.length, (options.tags || ts.emptyArray).length, this.assertionMessageAtLastKnownMarker("signature help tags"));
ts.zipWith((options.tags || ts.emptyArray), actualTags, (expectedTag, actualTag) => {
assert.equal(expectedTag.name, actualTag.name);
assert.equal(expectedTag.text, actualTag.text, this.assertionMessageAtLastKnownMarker("signature help tag " + actualTag.name));
assert.equal(actualTag.name, expectedTag.name);
assert.equal(actualTag.text, expectedTag.text, this.assertionMessageAtLastKnownMarker("signature help tag " + actualTag.name));
});

const allKeys: ReadonlyArray<keyof FourSlashInterface.VerifySignatureHelpOptions> = [
Expand Down
Loading