fix up always-truthy check for 0n and support negative numbers #60324
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
the
For Uncommitted Bug
|
@microsoft-github-policy-service agree |
|
@typescript-bot test it |
|
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
@jakebailey should I undraft? I'm unclear since the issue hasn't been accepted |
|
The issue is unlabeled so it's actually just un-triaged; feel free to undraft. |
RyanCavanaugh
requested changes
Oct 31, 2024
|
|
||
| // Not OK; always falsy. | ||
| if (0n) { } |
There was a problem hiding this comment.
Please add // @target: esnext to the top of this file to remove the unnecessary errors in the baseline
| return PredicateSemantics.Always; | ||
| // handle +123, -123, -123n, etc. | ||
| case SyntaxKind.PrefixUnaryExpression: | ||
| const prefixUnaryExpression = node as PrefixUnaryExpression; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
typescript-bot
added
For Backlog Bug
|
@microsoft-github-policy-service agree |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #60320