Skip to content

Commit

Permalink
Only error when testing functions that return booleans
Browse files Browse the repository at this point in the history
  • Loading branch information
@jwbay
jwbay committed Aug 18, 2019
1 parent 130615a commit 327ff39
Show file tree
Hide file tree
Showing 6 changed files with 379 additions and 93 deletions.
72 changes: 54 additions & 18 deletions src/compiler/checker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27875,24 +27875,7 @@ namespace ts {
checkGrammarStatementInAmbientContext(node);

const type = checkTruthinessExpression(node.expression);

if (strictNullChecks &&
(node.expression.kind === SyntaxKind.Identifier ||
node.expression.kind === SyntaxKind.PropertyAccessExpression ||
node.expression.kind === SyntaxKind.ElementAccessExpression)) {
const possiblyFalsy = !!getFalsyFlags(type);
if (!possiblyFalsy) {
// While it technically should be invalid for any known-truthy value
// to be tested, we de-scope to functions 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) {
error(node.expression, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
}
}
}

checkTestingKnownTruthyCallableType(node.expression, type);
checkSourceElement(node.thenStatement);

if (node.thenStatement.kind === SyntaxKind.EmptyStatement) {
Expand All @@ -27902,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);
Expand Down
59 changes: 51 additions & 8 deletions tests/baselines/reference/truthinessCallExpressionCoercion.errors.txt
View file
Original file line number Diff line number Diff line change
@@ -1,43 +1,86 @@
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(24,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(27,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(44,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(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 (5 errors) ====
==== 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 onlyErrorsWhenNonNullable(required: () => boolean, optional?: () => boolean) {
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
}
}

if (optional) { // 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() { }
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?
Expand Down
66 changes: 59 additions & 7 deletions tests/baselines/reference/truthinessCallExpressionCoercion.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,61 @@ function func() { return Math.random() > 0.5; }
if (func) { // error
}

function onlyErrorsWhenNonNullable(required: () => boolean, optional?: () => boolean) {
function onlyErrorsWhenTestingNonNullableFunctionType(required: () => boolean, optional?: () => boolean) {
if (required) { // error
}

if (optional) { // ok
}

if (!!required) { // ok
}

if (required()) { // ok
}
}

if (optional) { // 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() { }
bar() { return true; }
}
}

if (x.foo.bar) { // error
}

if (x.foo['bar']) { // error
}
}
Expand Down Expand Up @@ -55,18 +89,36 @@ class Foo {
function func() { return Math.random() > 0.5; }
if (func) { // error
}
function onlyErrorsWhenNonNullable(required, optional) {
function onlyErrorsWhenTestingNonNullableFunctionType(required, optional) {
if (required) { // error
}
if (optional) { // ok
}
if (!!required) { // ok
}
if (required()) { // ok
}
if (optional) { // 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 () { }
bar: function () { return true; }
}
};
if (x.foo.bar) { // error
Expand Down
Loading

0 comments on commit 327ff39

Please sign in to comment.