From c31ad6fb28e9daed09d585558edfc2f39ad4a3a7 Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Tue, 25 Aug 2015 18:09:32 -0700 Subject: [PATCH 1/6] Add tslint rules for #3994 --- Jakefile.js | 21 ++++++++- scripts/tslint/nextLineRule.ts | 63 +++++++++++++++++++++++++ scripts/tslint/noInferrableTypesRule.ts | 32 +++++++++++++ scripts/tslint/tsconfig.json | 7 +++ tslint.json | 15 ++++-- 5 files changed, 133 insertions(+), 5 deletions(-) create mode 100644 scripts/tslint/nextLineRule.ts create mode 100644 scripts/tslint/noInferrableTypesRule.ts create mode 100644 scripts/tslint/tsconfig.json diff --git a/Jakefile.js b/Jakefile.js index ea13cce6685..b30af95201a 100644 --- a/Jakefile.js +++ b/Jakefile.js @@ -770,17 +770,34 @@ task("update-sublime", ["local", serverFile], function() { jake.cpR(serverFile + ".map", "../TypeScript-Sublime-Plugin/tsserver/"); }); +var tslintRuleDir = "scripts/tslint"; +var tslintRules = ([ + "nextLineRule", + "noInferrableTypesRule" +]); +var tslintRulesFiles = tslintRules.map(function(p) { + return path.join(tslintRuleDir, p + ".ts"); +}); +var tslintRulesOutFiles = tslintRules.map(function(p) { + return path.join(builtLocalDirectory, "tslint", p + ".js"); +}); +desc("Compiles tslint rules to js"); +task("build-rules", tslintRulesOutFiles); +tslintRulesFiles.forEach(function(ruleFile, i) { + compileFile(tslintRulesOutFiles[i], [ruleFile], [ruleFile], [], /*useBuiltCompiler*/ true, /*noOutFile*/ true, /*generateDeclarations*/ false, path.join(builtLocalDirectory, "tslint")); +}); + // if the codebase were free of linter errors we could make jake runtests // run this task automatically desc("Runs tslint on the compiler sources"); -task("lint", [], function() { +task("lint", ["build-rules"], function() { function success(f) { return function() { console.log('SUCCESS: No linter errors in ' + f + '\n'); }}; function failure(f) { return function() { console.log('FAILURE: Please fix linting errors in ' + f + '\n') }}; var lintTargets = compilerSources.concat(harnessCoreSources); for (var i in lintTargets) { var f = lintTargets[i]; - var cmd = 'tslint -c tslint.json ' + f; + var cmd = 'tslint --rules-dir built/local/tslint -c tslint.json ' + f; exec(cmd, success(f), failure(f)); } }, { async: true }); diff --git a/scripts/tslint/nextLineRule.ts b/scripts/tslint/nextLineRule.ts new file mode 100644 index 00000000000..f1288e8e2f0 --- /dev/null +++ b/scripts/tslint/nextLineRule.ts @@ -0,0 +1,63 @@ +/// +/// + +const OPTION_CATCH = "check-catch"; +const OPTION_ELSE = "check-else"; + +export class Rule extends Lint.Rules.AbstractRule { + public static CATCH_FAILURE_STRING = "'catch' should be on the line following the previous block's ending curly brace"; + public static ELSE_FAILURE_STRING = "'else' should be on the line following the previous block's ending curly brace"; + + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + return this.applyWithWalker(new NextLineWalker(sourceFile, this.getOptions())); + } +} + +class NextLineWalker extends Lint.RuleWalker { + public visitIfStatement(node: ts.IfStatement) { + const sourceFile = node.getSourceFile(); + const thenStatement = node.thenStatement; + + const elseStatement = node.elseStatement; + if (!!elseStatement) { + // find the else keyword + const elseKeyword = getFirstChildOfKind(node, ts.SyntaxKind.ElseKeyword); + if (this.hasOption(OPTION_ELSE) && !!elseKeyword) { + const thenStatementEndLoc = sourceFile.getLineAndCharacterOfPosition(thenStatement.getEnd()); + const elseKeywordLoc = sourceFile.getLineAndCharacterOfPosition(elseKeyword.getStart()); + if (thenStatementEndLoc.line !== (elseKeywordLoc.line - 1)) { + const failure = this.createFailure(elseKeyword.getStart(), elseKeyword.getWidth(), Rule.ELSE_FAILURE_STRING); + this.addFailure(failure); + } + } + } + + super.visitIfStatement(node); + } + + public visitTryStatement(node: ts.TryStatement) { + const sourceFile = node.getSourceFile(); + const catchClause = node.catchClause; + + // "visit" try block + const tryKeyword = node.getChildAt(0); + const tryBlock = node.tryBlock; + const tryOpeningBrace = tryBlock.getChildAt(0); + + if (this.hasOption(OPTION_CATCH) && !!catchClause) { + const tryClosingBrace = node.tryBlock.getChildAt(node.tryBlock.getChildCount() - 1); + const catchKeyword = catchClause.getChildAt(0); + const tryClosingBraceLoc = sourceFile.getLineAndCharacterOfPosition(tryClosingBrace.getEnd()); + const catchKeywordLoc = sourceFile.getLineAndCharacterOfPosition(catchKeyword.getStart()); + if (tryClosingBraceLoc.line !== (catchKeywordLoc.line - 1)) { + const failure = this.createFailure(catchKeyword.getStart(), catchKeyword.getWidth(), Rule.CATCH_FAILURE_STRING); + this.addFailure(failure); + } + } + super.visitTryStatement(node); + } +} + +function getFirstChildOfKind(node: ts.Node, kind: ts.SyntaxKind) { + return node.getChildren().filter((child) => child.kind === kind)[0]; +} \ No newline at end of file diff --git a/scripts/tslint/noInferrableTypesRule.ts b/scripts/tslint/noInferrableTypesRule.ts new file mode 100644 index 00000000000..806426dc9f9 --- /dev/null +++ b/scripts/tslint/noInferrableTypesRule.ts @@ -0,0 +1,32 @@ +/// +/// + + +export class Rule extends Lint.Rules.AbstractRule { + public static FAILURE_STRING = "LHS type inferred by RHS expression, remove type annotation"; + + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + const program = ts.createProgram([sourceFile.fileName], Lint.createCompilerOptions()); + return this.applyWithWalker(new InferrableTypeWalker(sourceFile, this.getOptions(), program)); + } +} + +class InferrableTypeWalker extends Lint.RuleWalker { + constructor(file: ts.SourceFile, opts: Lint.IOptions, private program: ts.Program) { + super(program.getSourceFile(file.fileName), opts); + } + + visitVariableStatement(node: ts.VariableStatement) { + node.declarationList.declarations.forEach(e => { + if ( + (!!e.type) && + (!!e.initializer) && + (this.program.getTypeChecker().getTypeAtLocation(e.type) === this.program.getTypeChecker().getContextualType(e.initializer)) + ) { + this.addFailure(this.createFailure(e.type.getStart(), e.type.getWidth(), Rule.FAILURE_STRING)); + } + }); + + super.visitVariableStatement(node); + } +} diff --git a/scripts/tslint/tsconfig.json b/scripts/tslint/tsconfig.json new file mode 100644 index 00000000000..db018ce2776 --- /dev/null +++ b/scripts/tslint/tsconfig.json @@ -0,0 +1,7 @@ +{ + "compilerOptions": { + "noImplicitAny": true, + "module": "commonjs", + "outDir": "../../built/local/tslint" + } +} \ No newline at end of file diff --git a/tslint.json b/tslint.json index 71dc6730de4..2cc6cf80c92 100644 --- a/tslint.json +++ b/tslint.json @@ -8,7 +8,8 @@ "spaces" ], "one-line": [true, - "check-open-brace" + "check-open-brace", + "check-whitespace" ], "no-unreachable": true, "no-use-before-declare": true, @@ -21,7 +22,8 @@ "check-branch", "check-operator", "check-separator", - "check-type" + "check-type", + "check-module" ], "typedef-whitespace": [true, { "call-signature": "nospace", @@ -29,6 +31,13 @@ "parameter": "nospace", "property-declaration": "nospace", "variable-declaration": "nospace" - }] + }], + "next-line": [true, + "check-catch", + "check-else" + ], + "no-internal-module": true, + "no-trailing-whitespace": true, + "no-inferrable-types": true } } From 7813121c4d77e50aad0eed3152ef1f1156c7b574 Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Tue, 25 Aug 2015 18:37:52 -0700 Subject: [PATCH 2/6] compile vs tslints services dts, null check lint --- Jakefile.js | 3 ++- scripts/tslint/nextLineRule.ts | 2 +- scripts/tslint/noInferrableTypesRule.ts | 2 +- scripts/tslint/noNullRule.ts | 20 ++++++++++++++++++++ tslint.json | 3 ++- 5 files changed, 26 insertions(+), 4 deletions(-) create mode 100644 scripts/tslint/noNullRule.ts diff --git a/Jakefile.js b/Jakefile.js index b30af95201a..d8f2355abb7 100644 --- a/Jakefile.js +++ b/Jakefile.js @@ -773,7 +773,8 @@ task("update-sublime", ["local", serverFile], function() { var tslintRuleDir = "scripts/tslint"; var tslintRules = ([ "nextLineRule", - "noInferrableTypesRule" + "noInferrableTypesRule", + "noNullRule" ]); var tslintRulesFiles = tslintRules.map(function(p) { return path.join(tslintRuleDir, p + ".ts"); diff --git a/scripts/tslint/nextLineRule.ts b/scripts/tslint/nextLineRule.ts index f1288e8e2f0..8e24aea45e5 100644 --- a/scripts/tslint/nextLineRule.ts +++ b/scripts/tslint/nextLineRule.ts @@ -1,4 +1,4 @@ -/// +/// /// const OPTION_CATCH = "check-catch"; diff --git a/scripts/tslint/noInferrableTypesRule.ts b/scripts/tslint/noInferrableTypesRule.ts index 806426dc9f9..16e0633c872 100644 --- a/scripts/tslint/noInferrableTypesRule.ts +++ b/scripts/tslint/noInferrableTypesRule.ts @@ -1,4 +1,4 @@ -/// +/// /// diff --git a/scripts/tslint/noNullRule.ts b/scripts/tslint/noNullRule.ts new file mode 100644 index 00000000000..2a2c5bc3717 --- /dev/null +++ b/scripts/tslint/noNullRule.ts @@ -0,0 +1,20 @@ +/// +/// + + +export class Rule extends Lint.Rules.AbstractRule { + public static FAILURE_STRING = "Don't use the 'null' keyword - use 'undefined' for missing values instead"; + + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + return this.applyWithWalker(new NullWalker(sourceFile, this.getOptions())); + } +} + +class NullWalker extends Lint.RuleWalker { + visitNode(node: ts.Node) { + super.visitNode(node); + if (node.kind === ts.SyntaxKind.NullKeyword) { + this.addFailure(this.createFailure(node.getStart(), node.getWidth(), Rule.FAILURE_STRING)); + } + } +} diff --git a/tslint.json b/tslint.json index 2cc6cf80c92..4791e7c937c 100644 --- a/tslint.json +++ b/tslint.json @@ -38,6 +38,7 @@ ], "no-internal-module": true, "no-trailing-whitespace": true, - "no-inferrable-types": true + "no-inferrable-types": true, + "no-null": true } } From 1cd016b2892d923d7da41130adb48d7713d83d14 Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Wed, 26 Aug 2015 11:59:53 -0700 Subject: [PATCH 3/6] Boolean trivia rule --- Jakefile.js | 3 +- scripts/tslint/booleanTriviaRule.ts | 50 +++++++++++++++++++++++++++++ tslint.json | 3 +- 3 files changed, 54 insertions(+), 2 deletions(-) create mode 100644 scripts/tslint/booleanTriviaRule.ts diff --git a/Jakefile.js b/Jakefile.js index d8f2355abb7..3d7c6d93baf 100644 --- a/Jakefile.js +++ b/Jakefile.js @@ -774,7 +774,8 @@ var tslintRuleDir = "scripts/tslint"; var tslintRules = ([ "nextLineRule", "noInferrableTypesRule", - "noNullRule" + "noNullRule", + "booleanTriviaRule" ]); var tslintRulesFiles = tslintRules.map(function(p) { return path.join(tslintRuleDir, p + ".ts"); diff --git a/scripts/tslint/booleanTriviaRule.ts b/scripts/tslint/booleanTriviaRule.ts new file mode 100644 index 00000000000..be32a870ff4 --- /dev/null +++ b/scripts/tslint/booleanTriviaRule.ts @@ -0,0 +1,50 @@ +/// +/// + + +export class Rule extends Lint.Rules.AbstractRule { + public static FAILURE_STRING_FACTORY = (name: string, currently: string) => `Tag boolean argument as '${name}' (currently '${currently}')`; + + public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + const program = ts.createProgram([sourceFile.fileName], Lint.createCompilerOptions()); + const checker = program.getTypeChecker(); + return this.applyWithWalker(new BooleanTriviaWalker(checker, program.getSourceFile(sourceFile.fileName), this.getOptions())); + } +} + +class BooleanTriviaWalker extends Lint.RuleWalker { + constructor(private checker: ts.TypeChecker, file: ts.SourceFile, opts: Lint.IOptions) { + super(file, opts); + } + + visitCallExpression(node: ts.CallExpression) { + super.visitCallExpression(node); + if (node.arguments) { + const targetCallSignature = this.checker.getResolvedSignature(node); + if (!!targetCallSignature) { + const targetParameters = targetCallSignature.getParameters(); + const source = this.getSourceFile(); + for (let index = 0; index < targetParameters.length; index++) { + const param = targetParameters[index]; + const arg = node.arguments[index]; + if (!(arg && param)) continue; + + const argType = this.checker.getContextualType(arg); + if (argType && (argType.getFlags() & ts.TypeFlags.Boolean)) { + if (arg.kind !== ts.SyntaxKind.TrueKeyword && arg.kind !== ts.SyntaxKind.FalseKeyword) { + continue; + } + let triviaContent: string; + const ranges = ts.getLeadingCommentRanges(arg.getFullText(), 0); + if (ranges && ranges.length === 1 && ranges[0].kind === ts.SyntaxKind.MultiLineCommentTrivia) { + triviaContent = arg.getFullText().slice(ranges[0].pos + 2, ranges[0].end - 2); //+/-2 to remove /**/ + } + if (triviaContent !== param.getName()) { + this.addFailure(this.createFailure(arg.getStart(source), arg.getWidth(source), Rule.FAILURE_STRING_FACTORY(param.getName(), triviaContent))); + } + } + } + } + } + } +} diff --git a/tslint.json b/tslint.json index 4791e7c937c..1e83ef90ffe 100644 --- a/tslint.json +++ b/tslint.json @@ -39,6 +39,7 @@ "no-internal-module": true, "no-trailing-whitespace": true, "no-inferrable-types": true, - "no-null": true + "no-null": true, + "boolean-trivia": true } } From dc9dd3e667d9d99a487741f4633c7caad8df9515 Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Wed, 26 Aug 2015 14:47:25 -0700 Subject: [PATCH 4/6] Give up on real typechecking, just check literals --- scripts/tslint/noInferrableTypesRule.ts | 46 ++++++++++++++++++------- 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/scripts/tslint/noInferrableTypesRule.ts b/scripts/tslint/noInferrableTypesRule.ts index 16e0633c872..e1c6614170c 100644 --- a/scripts/tslint/noInferrableTypesRule.ts +++ b/scripts/tslint/noInferrableTypesRule.ts @@ -3,30 +3,52 @@ export class Rule extends Lint.Rules.AbstractRule { - public static FAILURE_STRING = "LHS type inferred by RHS expression, remove type annotation"; + public static FAILURE_STRING_FACTORY = (type: string) => `LHS type (${type}) inferred by RHS expression, remove type annotation`; public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { - const program = ts.createProgram([sourceFile.fileName], Lint.createCompilerOptions()); - return this.applyWithWalker(new InferrableTypeWalker(sourceFile, this.getOptions(), program)); + return this.applyWithWalker(new InferrableTypeWalker(sourceFile, this.getOptions())); } } class InferrableTypeWalker extends Lint.RuleWalker { - constructor(file: ts.SourceFile, opts: Lint.IOptions, private program: ts.Program) { - super(program.getSourceFile(file.fileName), opts); + private originalService: ts.LanguageService; + private fileName: string; + private originalContent: string; + + constructor(file: ts.SourceFile, opts: Lint.IOptions) { + this.fileName = file.fileName; + this.originalContent = file.getFullText(); + this.originalService = ts.createLanguageService(Lint.createLanguageServiceHost(this.fileName, this.originalContent)); + super(this.originalService.getSourceFile(this.fileName), opts); } visitVariableStatement(node: ts.VariableStatement) { node.declarationList.declarations.forEach(e => { - if ( - (!!e.type) && - (!!e.initializer) && - (this.program.getTypeChecker().getTypeAtLocation(e.type) === this.program.getTypeChecker().getContextualType(e.initializer)) - ) { - this.addFailure(this.createFailure(e.type.getStart(), e.type.getWidth(), Rule.FAILURE_STRING)); + if ((!!e.type) && (!!e.initializer)) { + let failure: string; + switch (e.type.kind) { + case ts.SyntaxKind.BooleanKeyword: + if (e.initializer.kind === ts.SyntaxKind.TrueKeyword || e.initializer.kind === ts.SyntaxKind.FalseKeyword) { + failure = 'boolean'; + } + break; + case ts.SyntaxKind.NumberKeyword: + if (e.initializer.kind === ts.SyntaxKind.NumericLiteral) { + failure = 'number'; + } + break; + case ts.SyntaxKind.StringKeyword: + if (e.initializer.kind === ts.SyntaxKind.StringLiteral || e.initializer.kind === ts.SyntaxKind.NoSubstitutionTemplateLiteral) { + failure = 'string'; + } + break; + } + if (failure) { + this.addFailure(this.createFailure(e.type.getStart(), e.type.getWidth(), Rule.FAILURE_STRING_FACTORY(failure))); + } } }); - + super.visitVariableStatement(node); } } From 0d88d8df68dca5704d5234a512fad371463bd920 Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Wed, 26 Aug 2015 14:48:52 -0700 Subject: [PATCH 5/6] Simplify it a bit --- scripts/tslint/noInferrableTypesRule.ts | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/scripts/tslint/noInferrableTypesRule.ts b/scripts/tslint/noInferrableTypesRule.ts index e1c6614170c..1dae47971eb 100644 --- a/scripts/tslint/noInferrableTypesRule.ts +++ b/scripts/tslint/noInferrableTypesRule.ts @@ -11,17 +11,6 @@ export class Rule extends Lint.Rules.AbstractRule { } class InferrableTypeWalker extends Lint.RuleWalker { - private originalService: ts.LanguageService; - private fileName: string; - private originalContent: string; - - constructor(file: ts.SourceFile, opts: Lint.IOptions) { - this.fileName = file.fileName; - this.originalContent = file.getFullText(); - this.originalService = ts.createLanguageService(Lint.createLanguageServiceHost(this.fileName, this.originalContent)); - super(this.originalService.getSourceFile(this.fileName), opts); - } - visitVariableStatement(node: ts.VariableStatement) { node.declarationList.declarations.forEach(e => { if ((!!e.type) && (!!e.initializer)) { From 2793bc2acda93a6d84cf1db19eab98d37a7b8271 Mon Sep 17 00:00:00 2001 From: Wesley Wigham Date: Thu, 17 Sep 2015 14:29:52 -0700 Subject: [PATCH 6/6] Feedback from PR, remove unused identifiers --- scripts/tslint/nextLineRule.ts | 14 ++++++-------- scripts/tslint/noInferrableTypesRule.ts | 10 ++++++++-- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/scripts/tslint/nextLineRule.ts b/scripts/tslint/nextLineRule.ts index 8e24aea45e5..7eec75a1baf 100644 --- a/scripts/tslint/nextLineRule.ts +++ b/scripts/tslint/nextLineRule.ts @@ -24,9 +24,9 @@ class NextLineWalker extends Lint.RuleWalker { const elseKeyword = getFirstChildOfKind(node, ts.SyntaxKind.ElseKeyword); if (this.hasOption(OPTION_ELSE) && !!elseKeyword) { const thenStatementEndLoc = sourceFile.getLineAndCharacterOfPosition(thenStatement.getEnd()); - const elseKeywordLoc = sourceFile.getLineAndCharacterOfPosition(elseKeyword.getStart()); + const elseKeywordLoc = sourceFile.getLineAndCharacterOfPosition(elseKeyword.getStart(sourceFile)); if (thenStatementEndLoc.line !== (elseKeywordLoc.line - 1)) { - const failure = this.createFailure(elseKeyword.getStart(), elseKeyword.getWidth(), Rule.ELSE_FAILURE_STRING); + const failure = this.createFailure(elseKeyword.getStart(sourceFile), elseKeyword.getWidth(sourceFile), Rule.ELSE_FAILURE_STRING); this.addFailure(failure); } } @@ -40,17 +40,15 @@ class NextLineWalker extends Lint.RuleWalker { const catchClause = node.catchClause; // "visit" try block - const tryKeyword = node.getChildAt(0); const tryBlock = node.tryBlock; - const tryOpeningBrace = tryBlock.getChildAt(0); if (this.hasOption(OPTION_CATCH) && !!catchClause) { - const tryClosingBrace = node.tryBlock.getChildAt(node.tryBlock.getChildCount() - 1); - const catchKeyword = catchClause.getChildAt(0); + const tryClosingBrace = tryBlock.getLastToken(sourceFile); + const catchKeyword = catchClause.getFirstToken(sourceFile); const tryClosingBraceLoc = sourceFile.getLineAndCharacterOfPosition(tryClosingBrace.getEnd()); - const catchKeywordLoc = sourceFile.getLineAndCharacterOfPosition(catchKeyword.getStart()); + const catchKeywordLoc = sourceFile.getLineAndCharacterOfPosition(catchKeyword.getStart(sourceFile)); if (tryClosingBraceLoc.line !== (catchKeywordLoc.line - 1)) { - const failure = this.createFailure(catchKeyword.getStart(), catchKeyword.getWidth(), Rule.CATCH_FAILURE_STRING); + const failure = this.createFailure(catchKeyword.getStart(sourceFile), catchKeyword.getWidth(sourceFile), Rule.CATCH_FAILURE_STRING); this.addFailure(failure); } } diff --git a/scripts/tslint/noInferrableTypesRule.ts b/scripts/tslint/noInferrableTypesRule.ts index 1dae47971eb..cbc0162260e 100644 --- a/scripts/tslint/noInferrableTypesRule.ts +++ b/scripts/tslint/noInferrableTypesRule.ts @@ -27,8 +27,14 @@ class InferrableTypeWalker extends Lint.RuleWalker { } break; case ts.SyntaxKind.StringKeyword: - if (e.initializer.kind === ts.SyntaxKind.StringLiteral || e.initializer.kind === ts.SyntaxKind.NoSubstitutionTemplateLiteral) { - failure = 'string'; + switch (e.initializer.kind) { + case ts.SyntaxKind.StringLiteral: + case ts.SyntaxKind.NoSubstitutionTemplateLiteral: + case ts.SyntaxKind.TemplateExpression: + failure = 'string'; + break; + default: + break; } break; }