Skip to content

fix up always-truthy check for 0n and support negative numbers #60324

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix up always-truthy check for 0n and support negative numbers #60324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0fa4c6e
fix up always-truthy check for 0n and support negative numbers
kirkwaiblinger Oct 22, 2024
95934ee
positive numbers too, hehe
kirkwaiblinger Oct 22, 2024
cd55278
test fixes
kirkwaiblinger Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44504,9 +44504,35 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return PredicateSemantics.Sometimes;
}
return PredicateSemantics.Always;
case SyntaxKind.BigIntLiteral:
if ((node as BigIntLiteral).text === "0n") {
// unlike `while(0)`, `while (0n)` is not idiomatic.
return PredicateSemantics.Never;
}
return PredicateSemantics.Always;
// handle +123, -123, -123n, etc.
case SyntaxKind.PrefixUnaryExpression:
const prefixUnaryExpression = node as PrefixUnaryExpression;
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this could just be something like

if (operand.kind === SyntaxKind.NumericLiteral && (operator === SyntaxKind.MinusToken || operator === SyntaxKind.PlusToken)) {
  return getTruthySemantics(operand);
}

? We shouldn't be duplicating all the logic of the numeric case.

Copy link
Author

@kirkwaiblinger kirkwaiblinger Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fair question. I wrote it this way to avoid a few bugs....

if (1) {} // this is intentionally exempted from the check, BUT
if (-1) {} // should report. Would NOT with the recursion.
if (+1) {} // IMO should also report, since this is clearly intended as a number, not as a shorthand for `if (true)`.

// (ditto for the following)
if (0) {} // intentionally exempted from reporting
if (+0) {} // should report IMO. This is clearly intended as a number, and not as a shorthand for `if (false)`.
if (-0) {} // see previous.

Note that the branches are therefore different:

            case SyntaxKind.NumericLiteral:
                // Allow `while(0)` or `while(1)`
                if ((node as NumericLiteral).text === "0" || (node as NumericLiteral).text === "1") {
                    return PredicateSemantics.Sometimes; // <-- special case; while(0) or while(1).
                }
                return PredicateSemantics.Always;

vs

                if (operand.kind === SyntaxKind.NumericLiteral && (operator === SyntaxKind.MinusToken || operator === SyntaxKind.PlusToken)) {
                    if ((operand as NumericLiteral).text === "0") {
                        return PredicateSemantics.Never; // <-- just a numeric zero
                    }
                    else {
                        return PredicateSemantics.Always;
                    }
                }

I think the bigint case is ok to recurse on the unary - operator. However, we just need to be sure it doesn't get hit by recursion from the unary + operator, since if (+0n) isn't falsy. It's an exception!

FWIW, note that ESLint does recurse in the bigint unary + case, and therefore does incur this bug.

So between

a) recurse in these cases, and add context to the recursion to be able to handle the edge cases.
b) recurse in these cases, and accept some IMO faulty edge cases
c) Not recurse at all, get the edge cases right, but ignore things like if (-(-(-3))) {}

I chose c) for simplicity, and added test cases around the while (+0), etc. cases.

But LMK if you'd like a different approach!

Note that within c) there's one opportunity that I see for deduping, which is to extract the identical bigint checks to a function.

        function getBigIntLiteralTruthySemantics (node: BigIntLiteral): PredicateSemantics {
            return node.text === "0n" ? PredicateSemantics.Never : PredicateSemantics.Always;

        }

        switch (node.kind) {
            case SyntaxKind.BigIntLiteral:
                    // unlike `while(0)`, `while (0n)` is not idiomatic.
                return getBigIntLiteralTruthySemantics(node as BigIntLiteral);
            // handle +123, -123, -123n, etc.
            case SyntaxKind.PrefixUnaryExpression:
                const prefixUnaryExpression = node as PrefixUnaryExpression;
                const { operator, operand } = prefixUnaryExpression;
                if (operand.kind === SyntaxKind.BigIntLiteral && operator === SyntaxKind.MinusToken) {
                    return getBigIntLiteralTruthySemantics(operand as BigIntLiteral);
                } else 
                // etc

const { operator, operand } = prefixUnaryExpression;
if (operand.kind === SyntaxKind.NumericLiteral && (operator === SyntaxKind.MinusToken || operator === SyntaxKind.PlusToken)) {
if ((operand as NumericLiteral).text === "0") {
return PredicateSemantics.Never;
}
else {
return PredicateSemantics.Always;
}
}
else if (operand.kind === SyntaxKind.BigIntLiteral && operator === SyntaxKind.MinusToken) {
if ((operand as BigIntLiteral).text === "0n") {
return PredicateSemantics.Never;
}
else {
return PredicateSemantics.Always;
}
}
return PredicateSemantics.Sometimes;
case SyntaxKind.ArrayLiteralExpression:
case SyntaxKind.ArrowFunction:
case SyntaxKind.BigIntLiteral:
case SyntaxKind.ClassExpression:
case SyntaxKind.FunctionExpression:
case SyntaxKind.JsxElement:
Expand Down
8 changes: 7 additions & 1 deletion tests/baselines/reference/conditionalOperatorConditionIsNumberType.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
conditionalOperatorConditionIsNumberType.ts(27,1): error TS2872: This kind of expression is always truthy.
conditionalOperatorConditionIsNumberType.ts(28,1): error TS2872: This kind of expression is always truthy.
conditionalOperatorConditionIsNumberType.ts(29,1): error TS2872: This kind of expression is always truthy.
conditionalOperatorConditionIsNumberType.ts(30,1): error TS2872: This kind of expression is always truthy.
conditionalOperatorConditionIsNumberType.ts(53,23): error TS2872: This kind of expression is always truthy.
conditionalOperatorConditionIsNumberType.ts(54,23): error TS2872: This kind of expression is always truthy.
conditionalOperatorConditionIsNumberType.ts(55,23): error TS2872: This kind of expression is always truthy.
conditionalOperatorConditionIsNumberType.ts(56,32): error TS2872: This kind of expression is always truthy.


==== conditionalOperatorConditionIsNumberType.ts (6 errors) ====
==== conditionalOperatorConditionIsNumberType.ts (8 errors) ====
//Cond ? Expr1 : Expr2, Cond is of number type, Expr1 and Expr2 have the same type
var condNumber: number;

Expand Down Expand Up @@ -37,6 +39,8 @@ conditionalOperatorConditionIsNumberType.ts(56,32): error TS2872: This kind of e
~~~~~~~~~~~
!!! error TS2872: This kind of expression is always truthy.
- 10000000000000 ? exprString1 : exprString2;
~~~~~~~~~~~~~~~~
!!! error TS2872: This kind of expression is always truthy.
1000000000000 ? exprIsObject1 : exprIsObject2;
~~~~~~~~~~~~~
!!! error TS2872: This kind of expression is always truthy.
Expand Down Expand Up @@ -69,6 +73,8 @@ conditionalOperatorConditionIsNumberType.ts(56,32): error TS2872: This kind of e
~~~~~~~~~~~
!!! error TS2872: This kind of expression is always truthy.
var resultIsString2 = - 10000000000000 ? exprString1 : exprString2;
~~~~~~~~~~~~~~~~
!!! error TS2872: This kind of expression is always truthy.
var resultIsObject2 = 1000000000000 ? exprIsObject1 : exprIsObject2;
~~~~~~~~~~~~~
!!! error TS2872: This kind of expression is always truthy.
Expand Down
71 changes: 69 additions & 2 deletions tests/baselines/reference/predicateSemantics.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,21 @@ predicateSemantics.ts(33,8): error TS2872: This kind of expression is always tru
predicateSemantics.ts(34,11): error TS2872: This kind of expression is always truthy.
predicateSemantics.ts(35,8): error TS2872: This kind of expression is always truthy.
predicateSemantics.ts(36,8): error TS2872: This kind of expression is always truthy.
predicateSemantics.ts(47,8): error TS2872: This kind of expression is always truthy.
predicateSemantics.ts(48,8): error TS2873: This kind of expression is always falsy.
predicateSemantics.ts(51,5): error TS2872: This kind of expression is always truthy.
predicateSemantics.ts(54,5): error TS2873: This kind of expression is always falsy.
predicateSemantics.ts(56,5): error TS2873: This kind of expression is always falsy.
predicateSemantics.ts(60,5): error TS2872: This kind of expression is always truthy.
predicateSemantics.ts(62,5): error TS2873: This kind of expression is always falsy.
predicateSemantics.ts(63,5): error TS2873: This kind of expression is always falsy.
predicateSemantics.ts(65,5): error TS2873: This kind of expression is always falsy.
predicateSemantics.ts(67,5): error TS2872: This kind of expression is always truthy.
predicateSemantics.ts(70,5): error TS2872: This kind of expression is always truthy.
predicateSemantics.ts(71,5): error TS2872: This kind of expression is always truthy.


==== predicateSemantics.ts (11 errors) ====
==== predicateSemantics.ts (23 errors) ====
declare let cond: any;

// OK: One or other operand is possibly nullish
Expand Down Expand Up @@ -77,4 +89,59 @@ predicateSemantics.ts(36,8): error TS2872: This kind of expression is always tru
function foo(this: Object | undefined) {
// Should be OK
return this ?? 0;
}
}

// positive numbers
while (+2) {}
~~
!!! error TS2872: This kind of expression is always truthy.
while (+0.000) {}
~~~~~~
!!! error TS2873: This kind of expression is always falsy.

// Not OK; always truthy.
if (1n) { }
~~
!!! error TS2872: This kind of expression is always truthy.

// Not OK; always falsy.
if (0n) { }
~~
!!! error TS2873: This kind of expression is always falsy.
// Not OK; always falsy.
if (-0n) { }
~~~
!!! error TS2873: This kind of expression is always falsy.

// negative numbers
// not OK - truthy
if (-1.2) { }
~~~~
!!! error TS2872: This kind of expression is always truthy.
// not OK - falsy
if (-0.0000){}
~~~~~~~
!!! error TS2873: This kind of expression is always falsy.
if (+0){}
~~
!!! error TS2873: This kind of expression is always falsy.
// not OK - falsy
if (-0n){}
~~~
!!! error TS2873: This kind of expression is always falsy.
// not OK - truthy
if (-13n){}
~~~~
!!! error TS2872: This kind of expression is always truthy.

// not ok - just a truthy number
if (-1){}
~~
!!! error TS2872: This kind of expression is always truthy.
if (+1){}
~~
!!! error TS2872: This kind of expression is always truthy.

declare const identifier: any;
// OK
if (-identifier) {}
88 changes: 68 additions & 20 deletions tests/baselines/reference/predicateSemantics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,18 +44,48 @@ console.log((cond || undefined) && 1 / cond);
function foo(this: Object | undefined) {
// Should be OK
return this ?? 0;
}
}

// positive numbers
while (+2) {}
while (+0.000) {}

// Not OK; always truthy.
if (1n) { }

// Not OK; always falsy.
if (0n) { }
// Not OK; always falsy.
if (-0n) { }

// negative numbers
// not OK - truthy
if (-1.2) { }
// not OK - falsy
if (-0.0000){}
if (+0){}
// not OK - falsy
if (-0n){}
// not OK - truthy
if (-13n){}

// not ok - just a truthy number
if (-1){}
if (+1){}

declare const identifier: any;
// OK
if (-identifier) {}

//// [predicateSemantics.js]
var _a, _b, _c, _d, _e, _f;
// OK: One or other operand is possibly nullish
var test1 = (_a = (cond ? undefined : 32)) !== null && _a !== void 0 ? _a : "possibly reached";
const test1 = (cond ? undefined : 32) ?? "possibly reached";
// Not OK: Both operands nullish
var test2 = (_b = (cond ? undefined : null)) !== null && _b !== void 0 ? _b : "always reached";
const test2 = (cond ? undefined : null) ?? "always reached";
// Not OK: Both operands non-nullish
var test3 = (_c = (cond ? 132 : 17)) !== null && _c !== void 0 ? _c : "unreachable";
const test3 = (cond ? 132 : 17) ?? "unreachable";
// Parens
var test4 = (_d = (cond ? (undefined) : (17))) !== null && _d !== void 0 ? _d : 42;
const test4 = (cond ? (undefined) : (17)) ?? 42;
// Should be OK (special case)
if (!!true) {
}
Expand All @@ -64,19 +94,13 @@ while (0) { }
while (1) { }
while (true) { }
while (false) { }
var p5 = (_e = {}) !== null && _e !== void 0 ? _e : null;
var p6 = (_f = 0 > 1) !== null && _f !== void 0 ? _f : null;
var p7 = null !== null && null !== void 0 ? null : null;
var p8 = (/** @class */ (function () {
function foo() {
}
return foo;
}())) && null;
var p9 = (/** @class */ (function () {
function foo() {
}
return foo;
}())) || null;
const p5 = {} ?? null;
const p6 = 0 > 1 ?? null;
const p7 = null ?? null;
const p8 = (class foo {
}) && null;
const p9 = (class foo {
}) || null;
// Outer expression tests
while ({}) { }
while ({}) { }
Expand All @@ -86,5 +110,29 @@ while ((({}))) { }
console.log((cond || undefined) && 1 / cond);
function foo() {
// Should be OK
return this !== null && this !== void 0 ? this : 0;
return this ?? 0;
}
// positive numbers
while (+2) { }
while (+0.000) { }
// Not OK; always truthy.
if (1n) { }
// Not OK; always falsy.
if (0n) { }
// Not OK; always falsy.
if (-0n) { }
// negative numbers
// not OK - truthy
if (-1.2) { }
// not OK - falsy
if (-0.0000) { }
if (+0) { }
// not OK - falsy
if (-0n) { }
// not OK - truthy
if (-13n) { }
// not ok - just a truthy number
if (-1) { }
if (+1) { }
// OK
if (-identifier) { }
35 changes: 35 additions & 0 deletions tests/baselines/reference/predicateSemantics.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,3 +79,38 @@ function foo(this: Object | undefined) {
return this ?? 0;
>this : Symbol(this, Decl(predicateSemantics.ts, 40, 13))
}

// positive numbers
while (+2) {}
while (+0.000) {}

// Not OK; always truthy.
if (1n) { }

// Not OK; always falsy.
if (0n) { }
// Not OK; always falsy.
if (-0n) { }

// negative numbers
// not OK - truthy
if (-1.2) { }
// not OK - falsy
if (-0.0000){}
if (+0){}
// not OK - falsy
if (-0n){}
// not OK - truthy
if (-13n){}

// not ok - just a truthy number
if (-1){}
if (+1){}

declare const identifier: any;
>identifier : Symbol(identifier, Decl(predicateSemantics.ts, 72, 13))

// OK
if (-identifier) {}
>identifier : Symbol(identifier, Decl(predicateSemantics.ts, 72, 13))

Loading