diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 65f550cb664d3..a5420ff4e35a0 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -27874,7 +27874,8 @@ namespace ts { // Grammar checking checkGrammarStatementInAmbientContext(node); - checkTruthinessExpression(node.expression); + const type = checkTruthinessExpression(node.expression); + checkTestingKnownTruthyCallableType(node.expression, type); checkSourceElement(node.thenStatement); if (node.thenStatement.kind === SyntaxKind.EmptyStatement) { @@ -27884,6 +27885,59 @@ namespace ts { checkSourceElement(node.elseStatement); } + function checkTestingKnownTruthyCallableType(testedNode: Expression, type: Type) { + if (!strictNullChecks) { + return; + } + + if (testedNode.kind === SyntaxKind.Identifier || + testedNode.kind === SyntaxKind.PropertyAccessExpression || + testedNode.kind === SyntaxKind.ElementAccessExpression) { + const possiblyFalsy = getFalsyFlags(type); + if (possiblyFalsy) { + return; + } + + // While it technically should be invalid for any known-truthy value + // to be tested, we de-scope to functions that return a boolean as a + // heuristic to identify the most common bugs. There are too many + // false positives for values sourced from type definitions without + // strictNullChecks otherwise. + const callSignatures = getSignaturesOfType(type, SignatureKind.Call); + if (callSignatures.length === 0) { + return; + } + + const alwaysReturnsBoolean = every(callSignatures, signature => { + const returnType = getReturnTypeOfSignature(signature); + const exactlyBoolean = TypeFlags.Boolean | TypeFlags.Union; + if (returnType.flags === exactlyBoolean) { + return true; + } + + if (returnType.flags & TypeFlags.Union) { + const allowedInUnion = TypeFlags.BooleanLike | TypeFlags.Nullable; + let unionContainsBoolean = false; + const unionContainsOnlyAllowedTypes = every((returnType as UnionType).types, innerType => { + if (innerType.flags & TypeFlags.BooleanLike) { + unionContainsBoolean = true; + } + + return (innerType.flags | allowedInUnion) === allowedInUnion; + }); + + return unionContainsBoolean && unionContainsOnlyAllowedTypes; + } + + return false; + }); + + if (alwaysReturnsBoolean) { + error(testedNode, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead); + } + } + } + function checkDoStatement(node: DoStatement) { // Grammar checking checkGrammarStatementInAmbientContext(node); diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index 5cca456dd4905..7ebf7db20c632 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -2689,7 +2689,10 @@ "category": "Error", "code": 2773 }, - + "This condition will always return true since the function is always defined. Did you mean to call it instead?": { + "category": "Error", + "code": 2774 + }, "Import declaration '{0}' is using private name '{1}'.": { "category": "Error", "code": 4000 diff --git a/tests/baselines/reference/truthinessCallExpressionCoercion.errors.txt b/tests/baselines/reference/truthinessCallExpressionCoercion.errors.txt new file mode 100644 index 0000000000000..ec322978f73e9 --- /dev/null +++ b/tests/baselines/reference/truthinessCallExpressionCoercion.errors.txt @@ -0,0 +1,112 @@ +tests/cases/compiler/truthinessCallExpressionCoercion.ts(3,5): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? +tests/cases/compiler/truthinessCallExpressionCoercion.ts(7,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? +tests/cases/compiler/truthinessCallExpressionCoercion.ts(29,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? +tests/cases/compiler/truthinessCallExpressionCoercion.ts(32,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? +tests/cases/compiler/truthinessCallExpressionCoercion.ts(35,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? +tests/cases/compiler/truthinessCallExpressionCoercion.ts(58,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? +tests/cases/compiler/truthinessCallExpressionCoercion.ts(61,9): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? +tests/cases/compiler/truthinessCallExpressionCoercion.ts(78,13): error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? + + +==== tests/cases/compiler/truthinessCallExpressionCoercion.ts (8 errors) ==== + function func() { return Math.random() > 0.5; } + + if (func) { // error + ~~~~ +!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? + } + + function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) { + if (required) { // error + ~~~~~~~~ +!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? + } + + if (optional) { // ok + } + + if (!!required) { // ok + } + + if (required()) { // ok + } + } + + function onlyErrorsWhenReturnsBoolean( + bool: () => boolean, + nullableBool: () => boolean | undefined, + nullableTrue: () => true | undefined, + nullable: () => undefined | null, + notABool: () => string, + unionWithBool: () => boolean | string, + nullableString: () => string | undefined + ) { + if (bool) { // error + ~~~~ +!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? + } + + if (nullableBool) { // error + ~~~~~~~~~~~~ +!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? + } + + if (nullableTrue) { // error + ~~~~~~~~~~~~ +!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? + } + + if (nullable) { // ok + } + + if (notABool) { // ok + } + + if (unionWithBool) { // ok + } + + if (nullableString) { // ok + } + } + + function checksPropertyAndElementAccess() { + const x = { + foo: { + bar() { return true; } + } + } + + if (x.foo.bar) { // error + ~~~~~~~~~ +!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? + } + + if (x.foo['bar']) { // error + ~~~~~~~~~~~~ +!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? + } + } + + function maybeBoolean(param: boolean | (() => boolean)) { + if (param) { // ok + } + } + + class Foo { + maybeIsUser?: () => boolean; + + isUser() { + return true; + } + + test() { + if (this.isUser) { // error + ~~~~~~~~~~~ +!!! error TS2774: This condition will always return true since the function is always defined. Did you mean to call it instead? + } + + if (this.maybeIsUser) { // ok + } + } + } + \ No newline at end of file diff --git a/tests/baselines/reference/truthinessCallExpressionCoercion.js b/tests/baselines/reference/truthinessCallExpressionCoercion.js new file mode 100644 index 0000000000000..7fe35b0f59a26 --- /dev/null +++ b/tests/baselines/reference/truthinessCallExpressionCoercion.js @@ -0,0 +1,146 @@ +//// [truthinessCallExpressionCoercion.ts] +function func() { return Math.random() > 0.5; } + +if (func) { // error +} + +function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) { + if (required) { // error + } + + if (optional) { // ok + } + + if (!!required) { // ok + } + + if (required()) { // ok + } +} + +function onlyErrorsWhenReturnsBoolean( + bool: () => boolean, + nullableBool: () => boolean | undefined, + nullableTrue: () => true | undefined, + nullable: () => undefined | null, + notABool: () => string, + unionWithBool: () => boolean | string, + nullableString: () => string | undefined +) { + if (bool) { // error + } + + if (nullableBool) { // error + } + + if (nullableTrue) { // error + } + + if (nullable) { // ok + } + + if (notABool) { // ok + } + + if (unionWithBool) { // ok + } + + if (nullableString) { // ok + } +} + +function checksPropertyAndElementAccess() { + const x = { + foo: { + bar() { return true; } + } + } + + if (x.foo.bar) { // error + } + + if (x.foo['bar']) { // error + } +} + +function maybeBoolean(param: boolean | (() => boolean)) { + if (param) { // ok + } +} + +class Foo { + maybeIsUser?: () => boolean; + + isUser() { + return true; + } + + test() { + if (this.isUser) { // error + } + + if (this.maybeIsUser) { // ok + } + } +} + + +//// [truthinessCallExpressionCoercion.js] +function func() { return Math.random() > 0.5; } +if (func) { // error +} +function onlyErrorsWhenTestingNonNullableFunctionType(required, optional) { + if (required) { // error + } + if (optional) { // ok + } + if (!!required) { // ok + } + if (required()) { // ok + } +} +function onlyErrorsWhenReturnsBoolean(bool, nullableBool, nullableTrue, nullable, notABool, unionWithBool, nullableString) { + if (bool) { // error + } + if (nullableBool) { // error + } + if (nullableTrue) { // error + } + if (nullable) { // ok + } + if (notABool) { // ok + } + if (unionWithBool) { // ok + } + if (nullableString) { // ok + } +} +function checksPropertyAndElementAccess() { + var x = { + foo: { + bar: function () { return true; } + } + }; + if (x.foo.bar) { // error + } + if (x.foo['bar']) { // error + } +} +function maybeBoolean(param) { + if (param) { // ok + } +} +var Foo = /** @class */ (function () { + function Foo() { + } + Foo.prototype.isUser = function () { + return true; + }; + Foo.prototype.test = function () { + if (this.isUser) { // error + } + if (this.maybeIsUser) { // ok + } + }; + return Foo; +}()); diff --git a/tests/baselines/reference/truthinessCallExpressionCoercion.symbols b/tests/baselines/reference/truthinessCallExpressionCoercion.symbols new file mode 100644 index 0000000000000..3840b0a7a713a --- /dev/null +++ b/tests/baselines/reference/truthinessCallExpressionCoercion.symbols @@ -0,0 +1,155 @@ +=== tests/cases/compiler/truthinessCallExpressionCoercion.ts === +function func() { return Math.random() > 0.5; } +>func : Symbol(func, Decl(truthinessCallExpressionCoercion.ts, 0, 0)) +>Math.random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --)) +>Math : Symbol(Math, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --)) +>random : Symbol(Math.random, Decl(lib.es5.d.ts, --, --)) + +if (func) { // error +>func : Symbol(func, Decl(truthinessCallExpressionCoercion.ts, 0, 0)) +} + +function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) { +>onlyErrorsWhenTestingNonNullableFunctionType : Symbol(onlyErrorsWhenTestingNonNullableFunctionType, Decl(truthinessCallExpressionCoercion.ts, 3, 1)) +>required : Symbol(required, Decl(truthinessCallExpressionCoercion.ts, 5, 54)) +>optional : Symbol(optional, Decl(truthinessCallExpressionCoercion.ts, 5, 78)) + + if (required) { // error +>required : Symbol(required, Decl(truthinessCallExpressionCoercion.ts, 5, 54)) + } + + if (optional) { // ok +>optional : Symbol(optional, Decl(truthinessCallExpressionCoercion.ts, 5, 78)) + } + + if (!!required) { // ok +>required : Symbol(required, Decl(truthinessCallExpressionCoercion.ts, 5, 54)) + } + + if (required()) { // ok +>required : Symbol(required, Decl(truthinessCallExpressionCoercion.ts, 5, 54)) + } +} + +function onlyErrorsWhenReturnsBoolean( +>onlyErrorsWhenReturnsBoolean : Symbol(onlyErrorsWhenReturnsBoolean, Decl(truthinessCallExpressionCoercion.ts, 17, 1)) + + bool: () => boolean, +>bool : Symbol(bool, Decl(truthinessCallExpressionCoercion.ts, 19, 38)) + + nullableBool: () => boolean | undefined, +>nullableBool : Symbol(nullableBool, Decl(truthinessCallExpressionCoercion.ts, 20, 24)) + + nullableTrue: () => true | undefined, +>nullableTrue : Symbol(nullableTrue, Decl(truthinessCallExpressionCoercion.ts, 21, 44)) + + nullable: () => undefined | null, +>nullable : Symbol(nullable, Decl(truthinessCallExpressionCoercion.ts, 22, 41)) + + notABool: () => string, +>notABool : Symbol(notABool, Decl(truthinessCallExpressionCoercion.ts, 23, 37)) + + unionWithBool: () => boolean | string, +>unionWithBool : Symbol(unionWithBool, Decl(truthinessCallExpressionCoercion.ts, 24, 27)) + + nullableString: () => string | undefined +>nullableString : Symbol(nullableString, Decl(truthinessCallExpressionCoercion.ts, 25, 42)) + +) { + if (bool) { // error +>bool : Symbol(bool, Decl(truthinessCallExpressionCoercion.ts, 19, 38)) + } + + if (nullableBool) { // error +>nullableBool : Symbol(nullableBool, Decl(truthinessCallExpressionCoercion.ts, 20, 24)) + } + + if (nullableTrue) { // error +>nullableTrue : Symbol(nullableTrue, Decl(truthinessCallExpressionCoercion.ts, 21, 44)) + } + + if (nullable) { // ok +>nullable : Symbol(nullable, Decl(truthinessCallExpressionCoercion.ts, 22, 41)) + } + + if (notABool) { // ok +>notABool : Symbol(notABool, Decl(truthinessCallExpressionCoercion.ts, 23, 37)) + } + + if (unionWithBool) { // ok +>unionWithBool : Symbol(unionWithBool, Decl(truthinessCallExpressionCoercion.ts, 24, 27)) + } + + if (nullableString) { // ok +>nullableString : Symbol(nullableString, Decl(truthinessCallExpressionCoercion.ts, 25, 42)) + } +} + +function checksPropertyAndElementAccess() { +>checksPropertyAndElementAccess : Symbol(checksPropertyAndElementAccess, Decl(truthinessCallExpressionCoercion.ts, 48, 1)) + + const x = { +>x : Symbol(x, Decl(truthinessCallExpressionCoercion.ts, 51, 9)) + + foo: { +>foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 51, 15)) + + bar() { return true; } +>bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 52, 14)) + } + } + + if (x.foo.bar) { // error +>x.foo.bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 52, 14)) +>x.foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 51, 15)) +>x : Symbol(x, Decl(truthinessCallExpressionCoercion.ts, 51, 9)) +>foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 51, 15)) +>bar : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 52, 14)) + } + + if (x.foo['bar']) { // error +>x.foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 51, 15)) +>x : Symbol(x, Decl(truthinessCallExpressionCoercion.ts, 51, 9)) +>foo : Symbol(foo, Decl(truthinessCallExpressionCoercion.ts, 51, 15)) +>'bar' : Symbol(bar, Decl(truthinessCallExpressionCoercion.ts, 52, 14)) + } +} + +function maybeBoolean(param: boolean | (() => boolean)) { +>maybeBoolean : Symbol(maybeBoolean, Decl(truthinessCallExpressionCoercion.ts, 62, 1)) +>param : Symbol(param, Decl(truthinessCallExpressionCoercion.ts, 64, 22)) + + if (param) { // ok +>param : Symbol(param, Decl(truthinessCallExpressionCoercion.ts, 64, 22)) + } +} + +class Foo { +>Foo : Symbol(Foo, Decl(truthinessCallExpressionCoercion.ts, 67, 1)) + + maybeIsUser?: () => boolean; +>maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion.ts, 69, 11)) + + isUser() { +>isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion.ts, 70, 32)) + + return true; + } + + test() { +>test : Symbol(Foo.test, Decl(truthinessCallExpressionCoercion.ts, 74, 5)) + + if (this.isUser) { // error +>this.isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion.ts, 70, 32)) +>this : Symbol(Foo, Decl(truthinessCallExpressionCoercion.ts, 67, 1)) +>isUser : Symbol(Foo.isUser, Decl(truthinessCallExpressionCoercion.ts, 70, 32)) + } + + if (this.maybeIsUser) { // ok +>this.maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion.ts, 69, 11)) +>this : Symbol(Foo, Decl(truthinessCallExpressionCoercion.ts, 67, 1)) +>maybeIsUser : Symbol(Foo.maybeIsUser, Decl(truthinessCallExpressionCoercion.ts, 69, 11)) + } + } +} + diff --git a/tests/baselines/reference/truthinessCallExpressionCoercion.types b/tests/baselines/reference/truthinessCallExpressionCoercion.types new file mode 100644 index 0000000000000..9ac7bbc25f909 --- /dev/null +++ b/tests/baselines/reference/truthinessCallExpressionCoercion.types @@ -0,0 +1,168 @@ +=== tests/cases/compiler/truthinessCallExpressionCoercion.ts === +function func() { return Math.random() > 0.5; } +>func : () => boolean +>Math.random() > 0.5 : boolean +>Math.random() : number +>Math.random : () => number +>Math : Math +>random : () => number +>0.5 : 0.5 + +if (func) { // error +>func : () => boolean +} + +function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) { +>onlyErrorsWhenTestingNonNullableFunctionType : (required: () => boolean, optional?: (() => boolean) | undefined) => void +>required : () => boolean +>optional : (() => boolean) | undefined + + if (required) { // error +>required : () => boolean + } + + if (optional) { // ok +>optional : (() => boolean) | undefined + } + + if (!!required) { // ok +>!!required : true +>!required : false +>required : () => boolean + } + + if (required()) { // ok +>required() : boolean +>required : () => boolean + } +} + +function onlyErrorsWhenReturnsBoolean( +>onlyErrorsWhenReturnsBoolean : (bool: () => boolean, nullableBool: () => boolean | undefined, nullableTrue: () => true | undefined, nullable: () => null | undefined, notABool: () => string, unionWithBool: () => string | boolean, nullableString: () => string | undefined) => void + + bool: () => boolean, +>bool : () => boolean + + nullableBool: () => boolean | undefined, +>nullableBool : () => boolean | undefined + + nullableTrue: () => true | undefined, +>nullableTrue : () => true | undefined +>true : true + + nullable: () => undefined | null, +>nullable : () => null | undefined +>null : null + + notABool: () => string, +>notABool : () => string + + unionWithBool: () => boolean | string, +>unionWithBool : () => string | boolean + + nullableString: () => string | undefined +>nullableString : () => string | undefined + +) { + if (bool) { // error +>bool : () => boolean + } + + if (nullableBool) { // error +>nullableBool : () => boolean | undefined + } + + if (nullableTrue) { // error +>nullableTrue : () => true | undefined + } + + if (nullable) { // ok +>nullable : () => null | undefined + } + + if (notABool) { // ok +>notABool : () => string + } + + if (unionWithBool) { // ok +>unionWithBool : () => string | boolean + } + + if (nullableString) { // ok +>nullableString : () => string | undefined + } +} + +function checksPropertyAndElementAccess() { +>checksPropertyAndElementAccess : () => void + + const x = { +>x : { foo: { bar(): boolean; }; } +>{ foo: { bar() { return true; } } } : { foo: { bar(): boolean; }; } + + foo: { +>foo : { bar(): boolean; } +>{ bar() { return true; } } : { bar(): boolean; } + + bar() { return true; } +>bar : () => boolean +>true : true + } + } + + if (x.foo.bar) { // error +>x.foo.bar : () => boolean +>x.foo : { bar(): boolean; } +>x : { foo: { bar(): boolean; }; } +>foo : { bar(): boolean; } +>bar : () => boolean + } + + if (x.foo['bar']) { // error +>x.foo['bar'] : () => boolean +>x.foo : { bar(): boolean; } +>x : { foo: { bar(): boolean; }; } +>foo : { bar(): boolean; } +>'bar' : "bar" + } +} + +function maybeBoolean(param: boolean | (() => boolean)) { +>maybeBoolean : (param: boolean | (() => boolean)) => void +>param : boolean | (() => boolean) + + if (param) { // ok +>param : boolean | (() => boolean) + } +} + +class Foo { +>Foo : Foo + + maybeIsUser?: () => boolean; +>maybeIsUser : (() => boolean) | undefined + + isUser() { +>isUser : () => boolean + + return true; +>true : true + } + + test() { +>test : () => void + + if (this.isUser) { // error +>this.isUser : () => boolean +>this : this +>isUser : () => boolean + } + + if (this.maybeIsUser) { // ok +>this.maybeIsUser : (() => boolean) | undefined +>this : this +>maybeIsUser : (() => boolean) | undefined + } + } +} + diff --git a/tests/cases/compiler/truthinessCallExpressionCoercion.ts b/tests/cases/compiler/truthinessCallExpressionCoercion.ts new file mode 100644 index 0000000000000..56ccac4ef0253 --- /dev/null +++ b/tests/cases/compiler/truthinessCallExpressionCoercion.ts @@ -0,0 +1,86 @@ +// @strictNullChecks:true + +function func() { return Math.random() > 0.5; } + +if (func) { // error +} + +function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) { + if (required) { // error + } + + if (optional) { // ok + } + + if (!!required) { // ok + } + + if (required()) { // ok + } +} + +function onlyErrorsWhenReturnsBoolean( + bool: () => boolean, + nullableBool: () => boolean | undefined, + nullableTrue: () => true | undefined, + nullable: () => undefined | null, + notABool: () => string, + unionWithBool: () => boolean | string, + nullableString: () => string | undefined +) { + if (bool) { // error + } + + if (nullableBool) { // error + } + + if (nullableTrue) { // error + } + + if (nullable) { // ok + } + + if (notABool) { // ok + } + + if (unionWithBool) { // ok + } + + if (nullableString) { // ok + } +} + +function checksPropertyAndElementAccess() { + const x = { + foo: { + bar() { return true; } + } + } + + if (x.foo.bar) { // error + } + + if (x.foo['bar']) { // error + } +} + +function maybeBoolean(param: boolean | (() => boolean)) { + if (param) { // ok + } +} + +class Foo { + maybeIsUser?: () => boolean; + + isUser() { + return true; + } + + test() { + if (this.isUser) { // error + } + + if (this.maybeIsUser) { // ok + } + } +}