Skip to content
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

[WIP] Support condition always true in prefix unary expression #60068

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
+1,924 −16
Draft

[WIP] Support condition always true in prefix unary expression #60068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7942777
Support checking that condition will always return `true` when used w…
tjenkinson Sep 13, 2024
41eb788
Update baselines
tjenkinson Sep 13, 2024
72c3c8b
Update to only check for missing `await` for `!` case
tjenkinson Sep 25, 2024
5eb8478
Go back to checking everything for `!` case
tjenkinson Sep 26, 2024
1274b62
Merge remote-tracking branch 'upstream/main' into support-condition-a…
tjenkinson Sep 26, 2024
9630147
Run formatter
tjenkinson Sep 26, 2024
16ae8dc
Switch to checking for assignment when inverted
tjenkinson Dec 29, 2024
8f8b45c
Only check single inversion
tjenkinson Dec 30, 2024
70b70fb
Make it work for the `if (!fnOrPromise)` case
tjenkinson Dec 30, 2024
fcb0c21
Just check if it's referenced later, and if so ignore
tjenkinson Jan 26, 2025
7a14b9d
Add test that currently fails
tjenkinson Jan 26, 2025
4e47c53
Find the nearest block
tjenkinson Jan 26, 2025
5099e88
Fix looping through all children
tjenkinson Jan 26, 2025
75f941d
Use `findAncestor`
tjenkinson Jan 26, 2025
de99724
Resimplify check
tjenkinson Jan 26, 2025
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
48 changes: 32 additions & 16 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44536,31 +44536,36 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
checkSourceElement(node.elseStatement);
}

function checkTestingKnownTruthyCallableOrAwaitableOrEnumMemberType(condExpr: Expression, condType: Type, body?: Statement | Expression) {
function checkTestingKnownTruthyCallableOrAwaitableOrEnumMemberType(inputCondExpr: Expression, condType: Type, body?: Statement | Expression) {
if (!strictNullChecks) return;
bothHelper(condExpr, body);

function bothHelper(condExpr: Expression, body: Expression | Statement | undefined) {
condExpr = skipParentheses(condExpr);

helper(condExpr, body);
bothHelper(inputCondExpr);

function bothHelper(condExpr: Expression) {
condExpr = skipParentheses(condExpr);
helper(condExpr);
while (isBinaryExpression(condExpr) && (condExpr.operatorToken.kind === SyntaxKind.BarBarToken || condExpr.operatorToken.kind === SyntaxKind.QuestionQuestionToken)) {
condExpr = skipParentheses(condExpr.left);
helper(condExpr, body);
helper(condExpr);
}
}

function helper(condExpr: Expression, body: Expression | Statement | undefined) {
const location = isLogicalOrCoalescingBinaryExpression(condExpr) ? skipParentheses(condExpr.right) : condExpr;
function helper(condExpr: Expression) {
let inverted = false;
let location = condExpr;
if (isPrefixUnaryExpression(condExpr) && condExpr.operator === SyntaxKind.ExclamationToken) {
location = skipParentheses(condExpr.operand);
inverted = true;
Copy link
Member

Choose a reason for hiding this comment

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

Another thing to consider (and probably add a test) is that we can have an expression that flips between the negative and positive case, if we have nested negation expressions, e.g.: if (a && !(b || !c)) ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there's really any special handling needed here?

}
location = isLogicalOrCoalescingBinaryExpression(location) ? skipParentheses(location.right) : location;
if (isModuleExportsAccessExpression(location)) {
return;
}
if (isLogicalOrCoalescingBinaryExpression(location)) {
bothHelper(location, body);
bothHelper(location);
return;
}
const type = location === condExpr ? condType : checkExpression(location);
const type = location === inputCondExpr ? condType : checkExpression(location);
if (type.flags & TypeFlags.EnumLiteral && isPropertyAccessExpression(location) && (getNodeLinks(location.expression).resolvedSymbol ?? unknownSymbol).flags & SymbolFlags.Enum) {
// EnumLiteral type at condition with known value is always truthy or always falsy, likely an error
error(location, Diagnostics.This_condition_will_always_return_0, !!(type as LiteralType).value ? "true" : "false");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was a bug before? It's an optimisation to not need to call checkExpression to get the type and use the provided condType, if the location still matched the provided condExpr?

Expand All @@ -44575,8 +44580,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// are too many false positives for values sourced from type
// definitions without strictNullChecks otherwise.
const callSignatures = getSignaturesOfType(type, SignatureKind.Call);
const isFnCall = callSignatures.length > 0;
const isPromise = !!getAwaitedTypeOfPromise(type);
if (callSignatures.length === 0 && !isPromise) {
if (!isFnCall && !isPromise) {
return;
}

Expand All @@ -44588,8 +44594,17 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return;
}

Copy link
Member

Choose a reason for hiding this comment

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

Why do we call isSymbolUsedInConditionBody with location instead of condExpr, in the first case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason without this it wouldn't detect if (!something), but still detect if (!something.else). Didn't dig much into why yet

Copy link
Contributor Author

@tjenkinson tjenkinson Jan 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think it probably should be location and before was a potential bug?

For the inverted case and if (!someVar) we need to pass in the resolved someVar not !someVar so that it passes isIdentifier(expr) and does the

// If the test was a simple identifier, the above check is sufficient

check.

Otherwise it's treating it as a nested object, which will fail

const isUsed = testedSymbol && isBinaryExpression(condExpr.parent) && isSymbolUsedInBinaryExpressionChain(condExpr.parent, testedSymbol)
|| testedSymbol && body && isSymbolUsedInConditionBody(condExpr, body, testedNode, testedSymbol);
let isUsed: boolean;
if (inverted) {
const closestBlock = findAncestor(condExpr.parent, isBlock);
const isUsedLater = !!(testedSymbol && closestBlock && isSymbolUsedInBody(location, closestBlock, testedNode, testedSymbol, condExpr.end + 1));
isUsed = isUsedLater;
}
else {
const isUsedInBody = !!(testedSymbol && body && isSymbolUsedInBody(location, body, testedNode, testedSymbol, 0));
isUsed = isUsedInBody || !!(testedSymbol && isBinaryExpression(condExpr.parent) && isSymbolUsedInBinaryExpressionChain(condExpr.parent, testedSymbol));
}

if (!isUsed) {
if (isPromise) {
errorAndMaybeSuggestAwait(
Expand All @@ -44606,15 +44621,16 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
}

function isSymbolUsedInConditionBody(expr: Expression, body: Statement | Expression, testedNode: Node, testedSymbol: Symbol): boolean {
function isSymbolUsedInBody(expr: Expression, body: Statement | Expression | Node, testedNode: Node, testedSymbol: Symbol, startPos: number): boolean {
return !!forEachChild(body, function check(childNode): boolean | undefined {
if (isIdentifier(childNode)) {
if (childNode.pos >= startPos && isIdentifier(childNode)) {
const childSymbol = getSymbolAtLocation(childNode);
if (childSymbol && childSymbol === testedSymbol) {
// If the test was a simple identifier, the above check is sufficient
if (isIdentifier(expr) || isIdentifier(testedNode) && isBinaryExpression(testedNode.parent)) {
return true;
}

// Otherwise we need to ensure the symbol is called on the same target
let testedExpression = testedNode.parent;
let childExpression = childNode.parent;
Expand Down
140 changes: 140 additions & 0 deletions tests/baselines/reference/falsinessCallExpressionCoercion.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
falsinessCallExpressionCoercion.ts(4,10): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
falsinessCallExpressionCoercion.ts(25,10): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
falsinessCallExpressionCoercion.ts(41,10): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
falsinessCallExpressionCoercion.ts(66,14): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
falsinessCallExpressionCoercion.ts(91,10): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
falsinessCallExpressionCoercion.ts(107,10): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?


==== falsinessCallExpressionCoercion.ts (6 errors) ====
function test1() {
function canAccess() { return false; }

if (!canAccess) { // error
~~~~~~~~~
!!! error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
}
}

function test2() {
function canAccess() { return false; }

if (!canAccess) { // ok
}

canAccess();
}

function test3() {
function canAccess() { return false; }

if (!!!canAccess) { // ok
}
}

function test4(canAccess: () => boolean) {
if (!canAccess) { // error
~~~~~~~~~
!!! error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
}
}

function test5(canAccess?: () => boolean) {
if (!canAccess) { // ok
}
}

function test6() {
const x = {
foo: {
bar() { return true; }
}
};

if (!x.foo.bar) { // error
~~~~~~~~~
!!! error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
}
}

function test7() {
const x = {
foo: {
bar() { return true; }
}
};

if (!x.foo.bar) { // ok
}

x.foo.bar();
}

class Test8 {
maybeIsUser?: () => boolean;

isUser() {
return true;
}

test() {
if (!this.isUser) { // error
~~~~~~~~~~~
!!! error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
}

if (!this.maybeIsUser) { // ok
}
}
}

class Test9 {
isUser() {
return true;
}

test() {
if (!this.isUser) { // ok
}

this.isUser();
}
}

function test10() {
function canAccess() { return false; }

const res = canAccess
if (!res) { // error
~~~
!!! error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
}
}

function test11() {
function canAccess() { return false; }

if (!canAccess) { // ok
} else {
canAccess()
}
}

function test12() {
function canAccess() { return false; }

if (!canAccess || Math.random()) { // error
~~~~~~~~~
!!! error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
}
}

function test13() {
function canAccess() { return false; }

if (!canAccess || Math.random()) { // ok
}

canAccess()
}

Loading
Loading