Skip to content
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.

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

Unused identifiers compiler code #9200

Merged
merged 44 commits into from
Jun 24, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
617b819
Code changes to update references of the Identifiers
sarangan12 May 24, 2016
a8bc8b4
Added code for handling function, method and coonstructor level local…
sarangan12 May 26, 2016
01966ba
Rebased with origin master
sarangan12 May 26, 2016
1e5ca92
Code changes to handle unused private variables, private methods and …
sarangan12 May 27, 2016
890d178
Code changes to handle namespace level elements
sarangan12 Jun 1, 2016
69dbeea
Code changes to handle unimplemented interfaces
sarangan12 Jun 1, 2016
c78d0e2
Code to optimize the d.ts check
sarangan12 Jun 1, 2016
107b369
Correct Code change to handle the parameters for methods inside inter…
sarangan12 Jun 1, 2016
481baa3
Fix for lint error
sarangan12 Jun 2, 2016
cd1ce0f
Remove Trailing whitespace
sarangan12 Jun 2, 2016
a38149d
Code changes to handle interface implementations
sarangan12 Jun 3, 2016
0571c19
Changes to display the error position correctly
sarangan12 Jun 3, 2016
e17ed58
Compiler Test Cases
sarangan12 Jun 3, 2016
2b7f3a7
Merge remote-tracking branch 'origin' into UpdateReferences
sarangan12 Jun 6, 2016
5a34352
Adding condition to ignore constructor parameters
sarangan12 Jun 15, 2016
93b7490
Removing unnecessary tests
sarangan12 Jun 15, 2016
7aba626
Additional changes for compiler code
sarangan12 Jun 15, 2016
ed5052d
Additional changes to handle constructor scenario
sarangan12 Jun 15, 2016
f5cdc9c
Fixing the consolidated case
sarangan12 Jun 15, 2016
8c3c7b1
Changed logic to search for private instead of public
sarangan12 Jun 15, 2016
dfad7cc
Response to PR Comments
sarangan12 Jun 15, 2016
c325625
Merge remote-tracking branch 'origin' into UnusedIdentifiersCompilerCode
sarangan12 Jun 15, 2016
6a711bc
Changed the error code in test cases as result of merge with master
sarangan12 Jun 15, 2016
d62a43f
Adding the missing file
sarangan12 Jun 16, 2016
043a625
Adding the missing file II
sarangan12 Jun 16, 2016
8f9d4ae
Response to PR comments
sarangan12 Jun 20, 2016
f15448a
Merge remote-tracking branch 'origin' into UnusedIdentifiersCompilerCode
sarangan12 Jun 20, 2016
c82453f
Code changes for checking unused imports
sarangan12 Jun 20, 2016
bcd6fc4
Test Cases for Unused Imports
sarangan12 Jun 20, 2016
49385f4
Response to PR comments
sarangan12 Jun 21, 2016
5993015
Code change specific to position of Import Declaration
sarangan12 Jun 21, 2016
f464f92
Code change for handling the position for unused import
sarangan12 Jun 21, 2016
3b5f8d2
New scenarios for handling parameters in lambda function, type parame…
sarangan12 Jun 22, 2016
ed282d7
Additional scenarios based on PR comments
sarangan12 Jun 22, 2016
e502ba0
Removing a redundant check
sarangan12 Jun 22, 2016
0e2e43d
Added ambient check to imports and typeparatmeter reporting
sarangan12 Jun 23, 2016
d6c2bcd
Merge remote-tracking branch 'origin' into UnusedIdentifiersCompilerCode
sarangan12 Jun 23, 2016
f93c6c8
Added one more scenario to handle type parameters
sarangan12 Jun 24, 2016
4521058
Added new scenario for TypeParameter on Interface
sarangan12 Jun 24, 2016
5eb7153
Refactoring the code
sarangan12 Jun 24, 2016
1401506
Merge remote-tracking branch 'origin' into UnusedIdentifiersCompilerCode
sarangan12 Jun 24, 2016
5361e5f
Added scenario to handle private class elements declared in constructor.
sarangan12 Jun 24, 2016
7fc4616
Minor change to erro reporting
sarangan12 Jun 24, 2016
9753d09
Merge remote-tracking branch 'origin' into UnusedIdentifiersCompilerCode
sarangan12 Jun 24, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
130 changes: 123 additions & 7 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8310,8 +8310,21 @@ namespace ts {
return container === declarationContainer;
}

function updateReferencesForInterfaceHeritiageClauseTargets(node: InterfaceDeclaration): void {
const extendedTypeNode = getClassExtendsHeritageClauseElement(node);
if (extendedTypeNode) {
const t = getTypeFromTypeNode(extendedTypeNode);
if (t !== unknownType && t.symbol && (compilerOptions.noUnusedLocals || compilerOptions.noUnusedParameters) && !isInAmbientContext(node)) {
t.symbol.hasReference = true;
}
}
}

function checkIdentifier(node: Identifier): Type {
const symbol = getResolvedSymbol(node);
if (symbol && (compilerOptions.noUnusedLocals || compilerOptions.noUnusedParameters) && !isInAmbientContext(node)) {
symbol.hasReference = true;
}

// As noted in ECMAScript 6 language spec, arrow functions never have an arguments objects.
// Although in down-level emit of arrow function, we emit it using function expression which means that
Expand Down Expand Up @@ -10202,6 +10215,10 @@ namespace ts {
return unknownType;
}

if ((compilerOptions.noUnusedLocals || compilerOptions.noUnusedParameters) && !isInAmbientContext(node)) {
prop.hasReference = true;
}

getNodeLinks(node).resolvedSymbol = prop;

if (prop.parent && prop.parent.flags & SymbolFlags.Class) {
Expand Down Expand Up @@ -12144,6 +12161,8 @@ namespace ts {
}
}
}
checkUnusedIdentifiers(node);
checkUnusedTypeParameters(node);
}
}

Expand Down Expand Up @@ -13214,6 +13233,9 @@ namespace ts {
checkAsyncFunctionReturnType(<FunctionLikeDeclaration>node);
}
}
if (!(<FunctionDeclaration>node).body) {
checkUnusedTypeParameters(node);
}
}
}

Expand Down Expand Up @@ -13366,6 +13388,8 @@ namespace ts {
checkGrammarConstructorTypeParameters(node) || checkGrammarConstructorTypeAnnotation(node);

checkSourceElement(node.body);
checkUnusedIdentifiers(node);
checkUnusedTypeParameters(node);

const symbol = getSymbolOfNode(node);
const firstDeclaration = getDeclarationOfKind(symbol, node.kind);
Expand Down Expand Up @@ -13558,13 +13582,18 @@ namespace ts {
function checkTypeReferenceNode(node: TypeReferenceNode | ExpressionWithTypeArguments) {
checkGrammarTypeArguments(node, node.typeArguments);
const type = getTypeFromTypeReference(node);
if (type !== unknownType && node.typeArguments) {
// Do type argument local checks only if referenced type is successfully resolved
forEach(node.typeArguments, checkSourceElement);
if (produceDiagnostics) {
const symbol = getNodeLinks(node).resolvedSymbol;
const typeParameters = symbol.flags & SymbolFlags.TypeAlias ? getSymbolLinks(symbol).typeParameters : (<TypeReference>type).target.localTypeParameters;
checkTypeArgumentConstraints(typeParameters, node.typeArguments);
if (type !== unknownType) {
if (type.symbol && (compilerOptions.noUnusedLocals || compilerOptions.noUnusedParameters) && !isInAmbientContext(node)) {
type.symbol.hasReference = true;
}
if (node.typeArguments) {
// Do type argument local checks only if referenced type is successfully resolved
forEach(node.typeArguments, checkSourceElement);
if (produceDiagnostics) {
const symbol = getNodeLinks(node).resolvedSymbol;
const typeParameters = symbol.flags & SymbolFlags.TypeAlias ? getSymbolLinks(symbol).typeParameters : (<TypeReference>type).target.localTypeParameters;
checkTypeArgumentConstraints(typeParameters, node.typeArguments);
}
}
}
}
Expand Down Expand Up @@ -14407,6 +14436,8 @@ namespace ts {
}

Copy link
Contributor

Choose a reason for hiding this comment

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

we should check unsued type parameters in functions as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (And added test cases also)

checkSourceElement(node.body);
checkUnusedIdentifiers(node);
checkUnusedTypeParameters(node);
if (!node.asteriskToken) {
const returnOrPromisedType = node.type && (isAsync ? checkAsyncFunctionReturnType(node) : getTypeFromTypeNode(node.type));
checkAllCodePathsInNonVoidFunctionReturnOrThrow(node, returnOrPromisedType);
Expand All @@ -14428,12 +14459,83 @@ namespace ts {
}
}

function checkUnusedIdentifiers(node: FunctionDeclaration | MethodDeclaration | ConstructorDeclaration | FunctionExpression | ArrowFunction | ForInStatement | Block | CatchClause): void {
if (node.parent.kind !== SyntaxKind.InterfaceDeclaration && (compilerOptions.noUnusedLocals || compilerOptions.noUnusedParameters) && !isInAmbientContext(node)) {
for (const key in node.locals) {
if (hasProperty(node.locals, key)) {
const local = node.locals[key];
if (!local.hasReference && local.valueDeclaration) {
if (local.valueDeclaration.kind !== SyntaxKind.Parameter && compilerOptions.noUnusedLocals) {
error(local.valueDeclaration.name, Diagnostics._0_is_declared_but_never_used, local.name);
}
else if (local.valueDeclaration.kind === SyntaxKind.Parameter && compilerOptions.noUnusedParameters) {
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to check kind in local.valueDeclaration.kind. all nodes have kinds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it

if (local.valueDeclaration.flags === 0) {
error(local.valueDeclaration.name, Diagnostics._0_is_declared_but_never_used, local.name);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need these checks? why not just

forEach(local.declarations, d => error(d, Diagnostics._0_is_declared_but_never_used, key));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have already tried this one. But I feel this might not work. The reason is that ImportEqualsDeclaration is a special case in which we need to use local.declarations[0].name instead of local.declarations[0].

Scenario:
module A {
export class Calculator {
public handelChar() {
}
}
}

module B {
import a = A;
}

Here 'a' must be indicated as error instead of "import a = A". But, I have optimized it a little so the condition does not look long.

Copy link
Contributor

Choose a reason for hiding this comment

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

i would say all errors should be on the declaration.name if it exists.

}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, can we rename this to checkUnusuedModuleLocals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

if (local.declarations) {
    const declaration = local.declarations[0];
    error(declaration.name? declaration.name : declaation, Diagnostics._0_is_declared_but_never_used, key);
}

}
}
}

function checkUnusedClassLocals(node: ClassDeclaration): void {
if (compilerOptions.noUnusedLocals && !isInAmbientContext(node)) {
if (node.members) {
for (const member of node.members) {
if (member.kind === SyntaxKind.MethodDeclaration || member.kind === SyntaxKind.PropertyDeclaration) {
if (isPrivateNode(member) && !member.symbol.hasReference) {
error(member.name, Diagnostics._0_is_declared_but_never_used, member.symbol.name);
}
}
else if (member.kind === SyntaxKind.Constructor) {
for (const parameter of (<ConstructorDeclaration>member).parameters) {
if (isPrivateNode(parameter) && !parameter.symbol.hasReference) {
error(parameter.name, Diagnostics._0_is_declared_but_never_used, parameter.symbol.name);
}
}
}
}
}
}
}

function checkUnusedTypeParameters(node: ClassDeclaration | FunctionDeclaration | MethodDeclaration | FunctionExpression | ArrowFunction | ConstructorDeclaration | SignatureDeclaration | InterfaceDeclaration) {
if (compilerOptions.noUnusedLocals && !isInAmbientContext(node)) {
if (node.typeParameters) {
for (const typeParameter of node.typeParameters) {
if (!typeParameter.symbol.hasReference) {
error(typeParameter.name, Diagnostics._0_is_declared_but_never_used, typeParameter.symbol.name);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

i would extract this to checkUnusedTypeParamters and call it for functions and methods as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
}

function isPrivateNode(node: Node): boolean {
return (node.flags & NodeFlags.Private) !== 0;
}

function checkUnusedModuleLocals(node: ModuleDeclaration | SourceFile): void {
if (compilerOptions.noUnusedLocals && !isInAmbientContext(node)) {
for (const key in node.locals) {
if (hasProperty(node.locals, key)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (declaration.kind === SyntaxKind.ImportSpecifier || declaration.kind === SyntaxKind.ImportClause || declaration.kind === SyntaxKind.NamespaceImport || declaration.kind === SyntaxKind.ImportEqualsDeclaration) {
     error(declaration.name, Diagnostics._0_is_declared_but_never_used, localValue.name);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

i am still not sure i understand why checkUnusedImports is not part of the checkUnusedModuleLocals?

const local = node.locals[key];
if (!local.hasReference && !local.exportSymbol) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i do not think this is correct. you already have the symbol, and the symbol is not exported, so localValue === localValue.declarations[0].symbol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the code.

forEach(local.declarations, d => error(d.name, Diagnostics._0_is_declared_but_never_used, local.name));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to check the isInAmbientContext here. similar to type parameters, there is no reason to have an import that is never used even in ambient context.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we merge this function with the checkUnusedModulePrivates ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the ambient context check. I prefer to keep them separate for sake of clarity,

Copy link
Contributor

Choose a reason for hiding this comment

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

i am not sure i see the point here. just merge them togather, and get rid of all the checks on the kinds.

}
}
}
}

function checkBlock(node: Block) {
// Grammar checking for SyntaxKind.Block
if (node.kind === SyntaxKind.Block) {
checkGrammarStatementInAmbientContext(node);
}
forEach(node.statements, checkSourceElement);
checkUnusedIdentifiers(node);
}

function checkCollisionWithArgumentsInGeneratedCode(node: SignatureDeclaration) {
Expand Down Expand Up @@ -14938,6 +15040,7 @@ namespace ts {
}

checkSourceElement(node.statement);
checkUnusedIdentifiers(node);
}

function checkForInStatement(node: ForInStatement) {
Expand Down Expand Up @@ -14985,6 +15088,7 @@ namespace ts {
}

checkSourceElement(node.statement);
checkUnusedIdentifiers(node);
}

function checkForInOrForOfVariableDeclaration(iterationStatement: ForInStatement | ForOfStatement): void {
Expand Down Expand Up @@ -15424,6 +15528,7 @@ namespace ts {
}

checkBlock(catchClause.block);
checkUnusedIdentifiers(catchClause);
}

if (node.finallyBlock) {
Expand Down Expand Up @@ -15585,6 +15690,8 @@ namespace ts {
}
checkClassLikeDeclaration(node);
forEach(node.members, checkSourceElement);
checkUnusedClassLocals(node);
checkUnusedTypeParameters(node);
}

function checkClassLikeDeclaration(node: ClassLikeDeclaration) {
Expand Down Expand Up @@ -15894,6 +16001,8 @@ namespace ts {

if (produceDiagnostics) {
checkTypeForDuplicateIndexSignatures(node);
updateReferencesForInterfaceHeritiageClauseTargets(node);
checkUnusedTypeParameters(node);
}
}

Expand Down Expand Up @@ -16290,6 +16399,7 @@ namespace ts {

if (node.body) {
checkSourceElement(node.body);
checkUnusedModuleLocals(node);
}
}

Expand Down Expand Up @@ -16470,6 +16580,9 @@ namespace ts {
if (target.flags & SymbolFlags.Type) {
checkTypeNameIsReserved(node.name, Diagnostics.Import_name_cannot_be_0);
}
if ((compilerOptions.noUnusedLocals || compilerOptions.noUnusedParameters) && !isInAmbientContext(node)) {
target.hasReference = true;
}
}
}
else {
Expand Down Expand Up @@ -16812,6 +16925,9 @@ namespace ts {

deferredNodes = [];
forEach(node.statements, checkSourceElement);
if (isExternalModule(node)) {
checkUnusedModuleLocals(node);
}
checkDeferredNodes();
deferredNodes = undefined;

Expand Down
10 changes: 10 additions & 0 deletions src/compiler/commandLineParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,16 @@ namespace ts {
type: "boolean",
description: Diagnostics.Raise_error_on_this_expressions_with_an_implied_any_type,
},
{
name: "noUnusedLocals",
type: "boolean",
description: Diagnostics.Report_Errors_on_Unused_Locals,
},
{
name: "noUnusedParameters",
type: "boolean",
description: Diagnostics.Report_Errors_on_Unused_Parameters
},
{
name: "noLib",
type: "boolean",
Expand Down
12 changes: 12 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -2792,6 +2792,18 @@
"category": "Message",
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be an "Error"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"code": 6132
},
"'{0}' is declared but never used.": {
"category": "Error",
"code": 6133
},
"Report Errors on Unused Locals.": {
"category": "Message",
"code": 6134
},
"Report Errors on Unused Parameters.": {
"category": "Message",
"code": 6135
},

"Variable '{0}' implicitly has an '{1}' type.": {
"category": "Error",
Expand Down
3 changes: 3 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2125,6 +2125,7 @@ namespace ts {
/* @internal */ parent?: Symbol; // Parent symbol
/* @internal */ exportSymbol?: Symbol; // Exported symbol associated with this symbol
/* @internal */ constEnumOnlyModule?: boolean; // True if module contains only const enums or other modules with only const enums
/* @internal */ hasReference?: boolean; // True if the symbol is referenced elsewhere
}

/* @internal */
Expand Down Expand Up @@ -2555,6 +2556,8 @@ namespace ts {
noImplicitAny?: boolean;
noImplicitReturns?: boolean;
noImplicitThis?: boolean;
noUnusedLocals?: boolean;
noUnusedParameters?: boolean;
noImplicitUseStrict?: boolean;
noLib?: boolean;
noResolve?: boolean;
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1702,7 +1702,7 @@ namespace ts {
node.kind === SyntaxKind.ExportAssignment && (<ExportAssignment>node).expression.kind === SyntaxKind.Identifier;
}

export function getClassExtendsHeritageClauseElement(node: ClassLikeDeclaration) {
export function getClassExtendsHeritageClauseElement(node: ClassLikeDeclaration | InterfaceDeclaration) {
const heritageClause = getHeritageClause(node.heritageClauses, SyntaxKind.ExtendsKeyword);
return heritageClause && heritageClause.types.length > 0 ? heritageClause.types[0] : undefined;
}
Expand Down
13 changes: 13 additions & 0 deletions tests/baselines/reference/unusedClassesinModule1.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
tests/cases/compiler/unusedClassesinModule1.ts(3,11): error TS6133: 'Calculator' is declared but never used.


==== tests/cases/compiler/unusedClassesinModule1.ts (1 errors) ====

module A {
class Calculator {
~~~~~~~~~~
!!! error TS6133: 'Calculator' is declared but never used.
public handelChar() {
}
}
}
20 changes: 20 additions & 0 deletions tests/baselines/reference/unusedClassesinModule1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
//// [unusedClassesinModule1.ts]

module A {
class Calculator {
public handelChar() {
}
}
}

//// [unusedClassesinModule1.js]
var A;
(function (A) {
var Calculator = (function () {
function Calculator() {
}
Calculator.prototype.handelChar = function () {
};
return Calculator;
}());
})(A || (A = {}));
12 changes: 12 additions & 0 deletions tests/baselines/reference/unusedClassesinNamespace1.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
tests/cases/compiler/unusedClassesinNamespace1.ts(3,11): error TS6133: 'c1' is declared but never used.


==== tests/cases/compiler/unusedClassesinNamespace1.ts (1 errors) ====

namespace Validation {
class c1 {
~~
!!! error TS6133: 'c1' is declared but never used.

}
}
17 changes: 17 additions & 0 deletions tests/baselines/reference/unusedClassesinNamespace1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//// [unusedClassesinNamespace1.ts]

namespace Validation {
class c1 {

}
}

//// [unusedClassesinNamespace1.js]
var Validation;
(function (Validation) {
var c1 = (function () {
function c1() {
}
return c1;
}());
})(Validation || (Validation = {}));
16 changes: 16 additions & 0 deletions tests/baselines/reference/unusedClassesinNamespace2.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
tests/cases/compiler/unusedClassesinNamespace2.ts(3,11): error TS6133: 'c1' is declared but never used.


==== tests/cases/compiler/unusedClassesinNamespace2.ts (1 errors) ====

namespace Validation {
class c1 {
~~
!!! error TS6133: 'c1' is declared but never used.

}

export class c2 {

}
}
Loading