From c263f8c3579426ed60c03b8bcdf9a24c7f8a36b3 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Mon, 16 Jan 2017 08:49:00 -0800 Subject: [PATCH] Warn for unnecessarily non-strict `== null` or `== undefined`. --- docs/_data/rules.json | 2 +- docs/rules/strict-type-predicates/index.html | 1 + src/language/utils.ts | 5 - src/rules/strictTypePredicatesRule.ts | 99 ++++++++++++++----- .../rules/strict-type-predicates/test.ts.lint | 30 +++++- 5 files changed, 105 insertions(+), 32 deletions(-) diff --git a/docs/_data/rules.json b/docs/_data/rules.json index 73b495125eb..4e1d6a5061f 100644 --- a/docs/_data/rules.json +++ b/docs/_data/rules.json @@ -1457,7 +1457,7 @@ }, { "ruleName": "strict-type-predicates", - "description": "\nWarns for type predicates that are always true or always false.\nWorks for 'typeof' comparisons to constants (e.g. 'typeof foo === \"string\"'), and equality comparison to 'null'/'undefined'.\nDoes not yet work for 'instanceof'.\nDoes *not* warn for 'if (x.y)' where 'x.y' is always truthy. For that, see strict-boolean-expressions.", + "description": "\nWarns for type predicates that are always true or always false.\nWorks for 'typeof' comparisons to constants (e.g. 'typeof foo === \"string\"'), and equality comparison to 'null'/'undefined'.\n(TypeScript won't let you compare '1 === 2', but it has an exception for '1 === undefined'.)\nDoes not yet work for 'instanceof'.\nDoes *not* warn for 'if (x.y)' where 'x.y' is always truthy. For that, see strict-boolean-expressions.", "optionsDescription": "Not configurable.", "options": null, "optionExamples": [ diff --git a/docs/rules/strict-type-predicates/index.html b/docs/rules/strict-type-predicates/index.html index 289aa8775b8..cf36b958bb0 100644 --- a/docs/rules/strict-type-predicates/index.html +++ b/docs/rules/strict-type-predicates/index.html @@ -4,6 +4,7 @@ Warns for type predicates that are always true or always false. Works for 'typeof' comparisons to constants (e.g. 'typeof foo === "string"'), and equality comparison to 'null'/'undefined'. + (TypeScript won't let you compare '1 === 2', but it has an exception for '1 === undefined'.) Does not yet work for 'instanceof'. Does *not* warn for 'if (x.y)' where 'x.y' is always truthy. For that, see strict-boolean-expressions. optionsDescription: Not configurable. diff --git a/src/language/utils.ts b/src/language/utils.ts index bcba3927019..b79bff5b45f 100644 --- a/src/language/utils.ts +++ b/src/language/utils.ts @@ -178,11 +178,6 @@ export function isTypeFlagSet(type: ts.Type, flagToCheck: ts.TypeFlags): boolean /* tslint:enable:no-bitwise */ } -/** Type predicate to test for a union type. */ -export function isUnionType(type: ts.Type): type is ts.UnionType { - return isTypeFlagSet(type, ts.TypeFlags.Union); -} - /** * Bitwise check for object flags. * Does not work with TypeScript 2.0.x diff --git a/src/rules/strictTypePredicatesRule.ts b/src/rules/strictTypePredicatesRule.ts index 9cd77387488..46c7a89f05d 100644 --- a/src/rules/strictTypePredicatesRule.ts +++ b/src/rules/strictTypePredicatesRule.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2016 Palantir Technologies, Inc. + * Copyright 2017 Palantir Technologies, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -43,6 +43,10 @@ export class Rule extends Lint.Rules.TypedRule { return `Expression is always ${value}.`; } + public static FAILURE_STRICT_PREFER_STRICT_EQUALS(value: "null" | "undefined", isPositive: boolean) { + return `Use '${isPositive ? "===" : "!=="} ${value}' instead.`; + } + public applyWithProgram(srcFile: ts.SourceFile, langSvc: ts.LanguageService): Lint.RuleFailure[] { return this.applyWithWalker(new Walker(srcFile, this.getOptions(), langSvc.getProgram())); } @@ -63,19 +67,37 @@ class Walker extends Lint.ProgramAwareRuleWalker { return; } - const { expression, predicate, isNullOrUndefined } = exprPred; const checker = this.getTypeChecker(); - const exprType = checker.getTypeAtLocation(expression); + const exprType = checker.getTypeAtLocation(exprPred.expression); // TODO: could use checker.getBaseConstraintOfType to help with type parameters, but it's not publicly exposed. if (Lint.isTypeFlagSet(exprType, ts.TypeFlags.Any | ts.TypeFlags.TypeParameter)) { return; } - const value = getConstantBoolean(exprType, predicate); - // 'null'/'undefined' are the only two values *not* assignable to '{}'. - if (value !== undefined && (isNullOrUndefined || !isEmptyType(checker, exprType))) { - this.addFailureAtNode(node, Rule.FAILURE_STRING(value === isPositive)); + const fail = (failure: string) => this.addFailureAtNode(node, failure); + + if (exprPred.isPlain) { + const { predicate, isNullOrUndefined } = exprPred; + const value = getConstantBoolean(exprType, predicate); + // 'null'/'undefined' are the only two values *not* assignable to '{}'. + if (value !== undefined && (isNullOrUndefined || !isEmptyType(checker, exprType))) { + fail(Rule.FAILURE_STRING(value === isPositive)); + } + } else { + const result = testNonStrictNullUndefined(exprType); + switch (typeof result) { + case "boolean": + fail(Rule.FAILURE_STRING(result === isPositive)); + break; + + case "string": + fail(Rule.FAILURE_STRICT_PREFER_STRICT_EQUALS(result as "null" | "undefined", isPositive)); + break; + + default: + } } + } } @@ -94,25 +116,24 @@ function getTypePredicateOneWay(left: ts.Expression, right: ts.Expression, isStr } const expression = (left as ts.TypeOfExpression).expression; const kind = (right as ts.StringLiteral).text; - return { expression, predicate: getTypePredicateForKind(kind), isNullOrUndefined: kind === "undefined" }; + return { isPlain: true, expression, predicate: getTypePredicateForKind(kind), isNullOrUndefined: kind === "undefined" }; case ts.SyntaxKind.NullKeyword: - return nullOrUndefined(false); + return nullOrUndefined(ts.TypeFlags.Null); case ts.SyntaxKind.Identifier: if ((right as ts.Identifier).text === "undefined") { - return nullOrUndefined(true); + return nullOrUndefined(undefinedFlags); } default: return undefined; } - function nullOrUndefined(isUndefined: boolean): TypePredicate { - const flags = isStrictEquals - ? isUndefined ? undefinedFlags : ts.TypeFlags.Null - : (ts.TypeFlags.Null | undefinedFlags); - return { expression: left, predicate: flagPredicate(flags), isNullOrUndefined: true }; + function nullOrUndefined(flags: ts.TypeFlags): TypePredicate { + return isStrictEquals + ? { isPlain: true, expression: left, predicate: flagPredicate(flags), isNullOrUndefined: true } + : { isPlain: false, expression: left }; } } @@ -122,11 +143,18 @@ function isEmptyType(checker: ts.TypeChecker, type: ts.Type) { const undefinedFlags = ts.TypeFlags.Undefined | ts.TypeFlags.Void; -interface TypePredicate { +type TypePredicate = PlainTypePredicate | NonStrictNullUndefinedPredicate; +interface PlainTypePredicate { + isPlain: true; expression: ts.Expression; predicate: Predicate; isNullOrUndefined: boolean; } +/** For `== null` and the like. */ +interface NonStrictNullUndefinedPredicate { + isPlain: false; + expression: ts.Expression; +} type Predicate = (type: ts.Type) => boolean; @@ -168,13 +196,9 @@ function isFunction(t: ts.Type): boolean { /** Returns a boolean value if that should always be the result of a type predicate. */ function getConstantBoolean(type: ts.Type, predicate: (t: ts.Type) => boolean): boolean | undefined { - if (!Lint.isUnionType(type)) { - return predicate(type); - } - let anyTrue = false; let anyFalse = false; - for (const ty of type.types) { + for (const ty of unionParts(type)) { if (predicate(ty)) { anyTrue = true; } else { @@ -189,9 +213,31 @@ function getConstantBoolean(type: ts.Type, predicate: (t: ts.Type) => boolean): return anyTrue; } +/** Returns bool for always/never true, or a string to recommend strict equality. */ +function testNonStrictNullUndefined(type: ts.Type): boolean | string | undefined { + let anyNull = false; + let anyUndefined = false; + let anyOther = false; + for (const ty of unionParts(type)) { + if (Lint.isTypeFlagSet(ty, ts.TypeFlags.Null)) { + anyNull = true; + } else if (Lint.isTypeFlagSet(ty, undefinedFlags)) { + anyUndefined = true; + } else { + anyOther = true; + } + } + + return !anyOther ? true + : anyNull && anyUndefined ? undefined + : anyNull ? "null" + : anyUndefined ? "undefined" + : false; +} + interface Equals { isPositive: boolean; // True for "===" and "==" - isStrict: boolean; + isStrict: boolean; // True for "===" and "!==" } function getEquals(kind: ts.SyntaxKind): Equals | undefined { @@ -208,3 +254,12 @@ function getEquals(kind: ts.SyntaxKind): Equals | undefined { return undefined; } } + +function unionParts(type: ts.Type) { + return isUnionType(type) ? type.types : [type]; +} + +/** Type predicate to test for a union type. */ +function isUnionType(type: ts.Type): type is ts.UnionType { + return Lint.isTypeFlagSet(type, ts.TypeFlags.Union); +} diff --git a/test/rules/strict-type-predicates/test.ts.lint b/test/rules/strict-type-predicates/test.ts.lint index 5b9d9213a16..29e71a836c8 100644 --- a/test/rules/strict-type-predicates/test.ts.lint +++ b/test/rules/strict-type-predicates/test.ts.lint @@ -99,10 +99,6 @@ declare function get(): T; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [F] get() === undefined; - get() == null; - get() == undefined; - get() == null; - get() == undefined; // 'null' and 'undefined' are not assignable to '{}' @@ -111,6 +107,32 @@ declare function get(): T; get<{}>() === undefined; ~~~~~~~~~~~~~~~~~~~~~~~ [F] + + get() == null; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Use '=== undefined' instead.] + + get() == undefined; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Use '=== null' instead.] + + get() == null; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Use '=== null' instead.] + + get() == undefined; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Use '=== undefined' instead.] + + get() != null; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Use '!== undefined' instead.] + + get<{}>() == null; + ~~~~~~~~~~~~~~~~~ [F] + + get() == null; + get() != undefined; + + get() == null; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [T] + get() != undefined; + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [F] } // negation