Skip to content

Uncalled function checks don't work with negation #43097

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.

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 15 additions & 4 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34867,7 +34867,6 @@ namespace ts {

function checkTestingKnownTruthyCallableOrAwaitableType(condExpr: Expression, type: Type, body?: Statement | Expression) {
if (!strictNullChecks) return;
if (getFalsyFlags(type)) return;

if (getAwaitedTypeOfPromise(type)) {
errorAndMaybeSuggestAwait(
Expand All @@ -34878,7 +34877,9 @@ namespace ts {
return;
}

const location = isBinaryExpression(condExpr) ? condExpr.right : condExpr;
const location = isBinaryExpression(condExpr) ? condExpr.right
: isPrefixUnaryExpression(condExpr) ? condExpr.operand
: condExpr;
const testedNode = isIdentifier(location) ? location
: isPropertyAccessExpression(location) ? location.name
: isBinaryExpression(location) && isIdentifier(location.right) ? location.right
Expand All @@ -34889,12 +34890,17 @@ namespace ts {
return;
}

const testedType = isPrefixUnaryExpression(condExpr) ? checkExpression(location) : type;
if (maybeTypeOfKind(testedType, TypeFlags.Undefined)) {
return;
}

// While it technically should be invalid for any known-truthy value
// to be tested, we de-scope to functions unrefenced in the block 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);
const callSignatures = getSignaturesOfType(testedType, SignatureKind.Call);
if (callSignatures.length === 0) {
return;
}
Expand All @@ -34907,7 +34913,12 @@ namespace ts {
const isUsed = isBinaryExpression(condExpr.parent) && isFunctionUsedInBinaryExpressionChain(condExpr.parent, testedSymbol)
|| body && isFunctionUsedInConditionBody(condExpr, body, testedNode, testedSymbol);
if (!isUsed) {
error(location, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
if (getFalsyFlags(type)) {
error(location, Diagnostics.This_condition_will_always_return_false_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
}
else {
error(location, Diagnostics.This_condition_will_always_return_true_since_the_function_is_always_defined_Did_you_mean_to_call_it_instead);
}
}
}

Expand Down
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -3264,6 +3264,10 @@
"category": "Error",
"code": 2802
},
"This condition will always return false since the function is always defined. Did you mean to call it instead?": {
"category": "Error",
"code": 2803
},

"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
Expand Down
4 changes: 0 additions & 4 deletions src/compiler/moduleSpecifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -469,9 +469,6 @@ namespace ts.moduleSpecifiers {
}

function tryGetModuleNameAsNodeModule({ path, isRedirect }: ModulePath, { getCanonicalFileName, sourceDirectory }: Info, host: ModuleSpecifierResolutionHost, options: CompilerOptions, packageNameOnly?: boolean): string | undefined {
if (!host.fileExists || !host.readFile) {
return undefined;
}
const parts: NodeModulePathParts = getNodeModulePathParts(path)!;
if (!parts) {
return undefined;
Expand Down Expand Up @@ -569,7 +566,6 @@ namespace ts.moduleSpecifiers {
}

function tryGetAnyFileFromPath(host: ModuleSpecifierResolutionHost, path: string) {
if (!host.fileExists) return;
// We check all js, `node` and `json` extensions in addition to TS, since node module resolution would also choose those over the directory
const extensions = getSupportedExtensions({ allowJs: true }, [{ extension: "node", isMixedContent: false }, { extension: "json", isMixedContent: false, scriptKind: ScriptKind.JSON }]);
for (const e of extensions) {
Expand Down
3 changes: 0 additions & 3 deletions src/compiler/sys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,6 @@ namespace ts {

/* @internal */
export function setCustomPollingValues(system: System) {
if (!system.getEnvironmentVariable) {
return;
}
const pollingIntervalChanged = setCustomLevels("TSC_WATCH_POLLINGINTERVAL", PollingInterval);
pollingChunkSize = getCustomPollingBasedLevels("TSC_WATCH_POLLINGCHUNKSIZE", defaultChunkLevels) || pollingChunkSize;
unchangedPollThresholds = getCustomPollingBasedLevels("TSC_WATCH_UNCHANGEDPOLLTHRESHOLDS", defaultChunkLevels) || unchangedPollThresholds;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
tests/cases/compiler/three.ts(28,14): error TS2803: This condition will always return false since the function is always defined. Did you mean to call it instead?


==== tests/cases/compiler/one.ts (0 errors) ====
declare const y: never[] | string[];
export const yThen = y.map(item => item.length);
==== tests/cases/compiler/two.ts (0 errors) ====
declare const y: number[][] | string[];
export const yThen = y.map(item => item.length);
==== tests/cases/compiler/three.ts (1 errors) ====
// #42504
interface ResizeObserverCallback {
(entries: ResizeObserverEntry[], observer: ResizeObserver): void;
}
interface ResizeObserverCallback { // duplicate for effect
(entries: ResizeObserverEntry[], observer: ResizeObserver): void;
}

const resizeObserver = new ResizeObserver(([entry]) => {
entry
});
// comment in #35501
interface Callback<T> {
(error: null, result: T): unknown
(error: Error, result: null): unknown
}

interface Task<T> {
(callback: Callback<T>): unknown
}

export function series<T>(tasks: Task<T>[], callback: Callback<T[]>): void {
let index = 0
let results: T[] = []

function next() {
let task = tasks[index]
if (!task) {
~~~~
!!! error TS2803: This condition will always return false since the function is always defined. Did you mean to call it instead?
callback(null, results)
} else {
task((error, result) => {
if (error) {
callback(error, null)
} else {
// must use postfix-!, since `error` and `result` don't have a
// causal relationship when the overloads are combined
results.push(result!)
next()
}
})
}
}
next()
}

series([
cb => setTimeout(() => cb(null, 1), 300),
cb => setTimeout(() => cb(null, 2), 200),
cb => setTimeout(() => cb(null, 3), 100),
], (error, results) => {
if (error) {
console.error(error)
} else {
console.log(results)
}
})

Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
tests/cases/compiler/uncalledFunctionChecksDontWorkWithNegation.ts(4,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/uncalledFunctionChecksDontWorkWithNegation.ts(8,6): error TS2803: This condition will always return false since the function is always defined. Did you mean to call it instead?


==== tests/cases/compiler/uncalledFunctionChecksDontWorkWithNegation.ts (2 errors) ====
declare function isFoo(): boolean;
declare const isUndefinedFoo: (() => boolean) | undefined;

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

if (!isFoo) {
~~~~~
!!! error TS2803: This condition will always return false since the function is always defined. Did you mean to call it instead?
// error
}

if (!isUndefinedFoo) {
// no error
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//// [uncalledFunctionChecksDontWorkWithNegation.ts]
declare function isFoo(): boolean;
declare const isUndefinedFoo: (() => boolean) | undefined;

if (isFoo) {
// error
}

if (!isFoo) {
// error
}

if (!isUndefinedFoo) {
// no error
}


//// [uncalledFunctionChecksDontWorkWithNegation.js]
if (isFoo) {
// error
}
if (!isFoo) {
// error
}
if (!isUndefinedFoo) {
// no error
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
=== tests/cases/compiler/uncalledFunctionChecksDontWorkWithNegation.ts ===
declare function isFoo(): boolean;
>isFoo : Symbol(isFoo, Decl(uncalledFunctionChecksDontWorkWithNegation.ts, 0, 0))

declare const isUndefinedFoo: (() => boolean) | undefined;
>isUndefinedFoo : Symbol(isUndefinedFoo, Decl(uncalledFunctionChecksDontWorkWithNegation.ts, 1, 13))

if (isFoo) {
>isFoo : Symbol(isFoo, Decl(uncalledFunctionChecksDontWorkWithNegation.ts, 0, 0))

// error
}

if (!isFoo) {
>isFoo : Symbol(isFoo, Decl(uncalledFunctionChecksDontWorkWithNegation.ts, 0, 0))

// error
}

if (!isUndefinedFoo) {
>isUndefinedFoo : Symbol(isUndefinedFoo, Decl(uncalledFunctionChecksDontWorkWithNegation.ts, 1, 13))

// no error
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
=== tests/cases/compiler/uncalledFunctionChecksDontWorkWithNegation.ts ===
declare function isFoo(): boolean;
>isFoo : () => boolean

declare const isUndefinedFoo: (() => boolean) | undefined;
>isUndefinedFoo : (() => boolean) | undefined

if (isFoo) {
>isFoo : () => boolean

// error
}

if (!isFoo) {
>!isFoo : false
>isFoo : () => boolean

// error
}

if (!isUndefinedFoo) {
>!isUndefinedFoo : boolean
>isUndefinedFoo : (() => boolean) | undefined

// no error
}

16 changes: 16 additions & 0 deletions tests/cases/compiler/uncalledFunctionChecksDontWorkWithNegation.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// @strictNullChecks: true

declare function isFoo(): boolean;
declare const isUndefinedFoo: (() => boolean) | undefined;

if (isFoo) {
// error
}

if (!isFoo) {
// error
}

if (!isUndefinedFoo) {
// no error
}
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to have tests for when those negated expressions are part of larger logical expressions (e.g. !isFoo || x, etc)

Copy link
Member

Choose a reason for hiding this comment

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

would also like to see what happens when we issue this error for unawaited promises, like so:

declare function foo(): Promise<string | undefined>;
const awaitedValue = foo();
if (awaitedValue) { // Emits ts2801
    // ...
}
if (!awaitedValue) { // What happens here?
    // ...
}