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.

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

Uncalled function checks check negation condition #57114

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 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
7 changes: 5 additions & 2 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42919,7 +42919,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function helper(condExpr: Expression, body: Expression | Statement | undefined) {
const location = isLogicalOrCoalescingBinaryExpression(condExpr) ? skipParentheses(condExpr.right) : condExpr;
const location = isLogicalOrCoalescingBinaryExpression(condExpr) ? skipParentheses(condExpr.right)
: isPrefixUnaryExpression(condExpr) ? condExpr.operand
: condExpr;
if (isModuleExportsAccessExpression(location)) {
return;
}
Expand All @@ -42936,7 +42938,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// 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 testedType = isPrefixUnaryExpression(condExpr) ? checkExpression(location) : type;
const callSignatures = getSignaturesOfType(testedType, SignatureKind.Call);
const isPromise = !!getAwaitedTypeOfPromise(type);
if (callSignatures.length === 0 && !isPromise) {
return;
Expand Down
3 changes: 1 addition & 2 deletions src/compiler/moduleSpecifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -996,7 +996,7 @@ function tryGetModuleNameFromRootDirs(rootDirs: readonly string[], moduleFileNam
}

function tryGetModuleNameAsNodeModule({ path, isRedirect }: ModulePath, { getCanonicalFileName, canonicalSourceDirectory }: Info, importingSourceFile: SourceFile, host: ModuleSpecifierResolutionHost, options: CompilerOptions, userPreferences: UserPreferences, packageNameOnly?: boolean, overrideMode?: ResolutionMode): string | undefined {
if (!host.fileExists || !host.readFile) {
if (!host.readFile) {
Copy link
Author

Choose a reason for hiding this comment

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

On the ModuleSpecifierResolutionHost, host.fileExists is always defined, but host.readFile is optionally defined.

Searching the codebase for host.fileExists showed it being used multiple times without a check, so I felt safe removing this. If you think this is wrong or risky, I'm happy to replace this with

if (host.fileExists === undefined || !host.readFile)

or however you think the codebase is best maintained.

Copy link
Member

Choose a reason for hiding this comment

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

This one I believe is correct; this same change was made in #53463.

return undefined;
}
const parts: NodeModulePathParts = getNodeModulePathParts(path)!;
Expand Down Expand Up @@ -1145,7 +1145,6 @@ function tryGetModuleNameAsNodeModule({ path, isRedirect }: ModulePath, { getCan
}

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 = flatten(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 @@ -131,9 +131,6 @@ let pollingChunkSize = createPollingIntervalBasedLevels(defaultChunkLevels);
export let unchangedPollThresholds = createPollingIntervalBasedLevels(defaultChunkLevels);

function setCustomPollingValues(system: System) {
if (!system.getEnvironmentVariable) {
Copy link
Author

Choose a reason for hiding this comment

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

I used VSCode to search for all implementations of System and found only the node System defined in this file. getEnvironmentVariable is always defined as

            getEnvironmentVariable(name: string) {
                return process.env[name] || "";
            },

so I felt safe removing this check -- the condition cannot currently evaluate to true.

Copy link
Member

Choose a reason for hiding this comment

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

There are theoretically external implementations of sys out there, and I'm guessing this guards against them. https://github.com/ionic-team/stencil/blob/fa5ab1b75f19e1117f0cead1caaf6b00ddccadf3/src/compiler/sys/typescript/typescript-sys.ts#L182 (but this code is non-functional post 5.0)

return;
}
const pollingIntervalChanged = setCustomLevels("TSC_WATCH_POLLINGINTERVAL", PollingInterval);
pollingChunkSize = getCustomPollingBasedLevels("TSC_WATCH_POLLINGCHUNKSIZE", defaultChunkLevels) || pollingChunkSize;
unchangedPollThresholds = getCustomPollingBasedLevels("TSC_WATCH_UNCHANGEDPOLLTHRESHOLDS", defaultChunkLevels) || unchangedPollThresholds;
Expand Down
2 changes: 0 additions & 2 deletions src/services/stringCompletions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1182,8 +1182,6 @@ function getCompletionEntriesFromTypings(host: LanguageServiceHost, options: Com
}

function enumerateNodeModulesVisibleToScript(host: LanguageServiceHost, scriptPath: string): readonly string[] {
if (!host.readFile || !host.fileExists) return emptyArray;
Copy link
Author

@AlexMarkUSDS AlexMarkUSDS Jan 21, 2024

Choose a reason for hiding this comment

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

The LanguageServiceHost has a comment

     * Unlike `realpath and `readDirectory`, `readFile` and `fileExists` are now _required_
     * to properly acquire and setup source files under module: node16+ modes.

so I believe these checks have been unnecessary since the compiler was converted to modules in #51387

Copy link
Member

Choose a reason for hiding this comment

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

I would git blame this; node16 is a "recent" addition and requiring those would be new, so this is likely a compatibility shim to ensure that older callers don't crash. We do this sort of thing somewhat often when making "breaking" API changes (but, still detecting older uses and trying to fix them within reason).

Copy link
Author

Choose a reason for hiding this comment

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

It looks like it went in 6 years ago with 70682b7#diff-10538e33f93edc9b510a4cd505ecaa718f6ab878a54b7d044a18ef10b1e36792R383 .

Better understanding the compatibility shim, I'll undo the changes that assume host.readFile and host.fileExists. I wouldn't want an innocent change like this to break someone's module compatibility.


const result: string[] = [];
for (const packageJson of findPackageJsons(scriptPath, host)) {
const contents = readJson(packageJson, host as { readFile: (filename: string) => string | undefined; }); // Cast to assert that readFile is defined
Expand Down
4 changes: 0 additions & 4 deletions src/services/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3670,10 +3670,6 @@ export function findPackageJson(directory: string, host: LanguageServiceHost): s

/** @internal */
export function getPackageJsonsVisibleToFile(fileName: string, host: LanguageServiceHost): readonly ProjectPackageJsonInfo[] {
if (!host.fileExists) {
return [];
}

const packageJsons: ProjectPackageJsonInfo[] = [];
forEachAncestorDirectory(getDirectoryPath(fileName), ancestor => {
const packageJsonFileName = combinePaths(ancestor, "package.json");
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
three.ts(28,14): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?


==== one.ts (0 errors) ====
declare const y: never[] | string[];
export const yThen = y.map(item => item.length);
==== two.ts (0 errors) ====
declare const y: number[][] | string[];
export const yThen = y.map(item => item.length);
==== 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 TS2774: This condition will always return true since this 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
@@ -1,17 +1,22 @@
uncalledFunctionChecksInConditional.ts(5,5): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(9,5): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(9,14): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(13,5): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(32,10): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(36,5): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(40,22): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(44,16): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(48,22): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(6,5): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(10,5): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(10,14): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(14,5): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(33,10): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(37,5): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(41,22): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(45,16): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(49,22): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(58,6): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(62,6): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
uncalledFunctionChecksInConditional.ts(66,6): error TS2801: This condition will always return true since this 'Promise<boolean | undefined>' is always defined.
uncalledFunctionChecksInConditional.ts(74,6): error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?


==== uncalledFunctionChecksInConditional.ts (9 errors) ====
==== uncalledFunctionChecksInConditional.ts (13 errors) ====
declare function isFoo(): boolean;
declare function isBar(): boolean;
declare function isFooPromise(): Promise<boolean | undefined>;
declare const isUndefinedFoo: (() => boolean) | undefined;

if (isFoo) {
Expand Down Expand Up @@ -82,4 +87,33 @@ uncalledFunctionChecksInConditional.ts(48,22): error TS2774: This condition will
if (x && z) {
// no error
z();
}

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

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

if (!isFooPromise()) {
~~~~~~~~~~~~~~
!!! error TS2801: This condition will always return true since this 'Promise<boolean | undefined>' is always defined.
!!! related TS2773 uncalledFunctionChecksInConditional.ts:66:6: Did you forget to use 'await'?
// ts2801 error on isFooPromise
}

if (!isUndefinedFoo) {
// no error
}

if (!isFooPromise) {
~~~~~~~~~~~~
!!! error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?
// error on isFooPromise
}
36 changes: 36 additions & 0 deletions tests/baselines/reference/uncalledFunctionChecksInConditional.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
//// [uncalledFunctionChecksInConditional.ts]
declare function isFoo(): boolean;
declare function isBar(): boolean;
declare function isFooPromise(): Promise<boolean | undefined>;
declare const isUndefinedFoo: (() => boolean) | undefined;

if (isFoo) {
Expand Down Expand Up @@ -55,6 +56,26 @@ if (ux || y || uz || isFoo) {
if (x && z) {
// no error
z();
}

if (!isFoo) {
// error on isFoo
}

if (!isFoo || !isFoo()) {
// error on isFoo
}

if (!isFooPromise()) {
// ts2801 error on isFooPromise
}

if (!isUndefinedFoo) {
// no error
}

if (!isFooPromise) {
// error on isFooPromise
}

//// [uncalledFunctionChecksInConditional.js]
Expand Down Expand Up @@ -92,3 +113,18 @@ if (x && z) {
// no error
z();
}
if (!isFoo) {
// error on isFoo
}
if (!isFoo || !isFoo()) {
// error on isFoo
}
if (!isFooPromise()) {
// ts2801 error on isFooPromise
}
if (!isUndefinedFoo) {
// no error
}
if (!isFooPromise) {
// error on isFooPromise
}
Loading
Loading