Uncalled function checks check negation condition - #57114
Uncalled function checks check negation condition#57114Alex Mark (AlexMarkUSDS) wants to merge 7 commits into
Conversation
Sorry, something went wrong.
|
The TypeScript team hasn't accepted the linked issue #34815. If you can get it accepted, this PR will have a better chance of being reviewed. |
Sorry, something went wrong.
|
@microsoft-github-policy-service agree |
Sorry, something went wrong.
Alex Mark (AlexMarkUSDS)
left a comment
There was a problem hiding this comment.
My final commits show changes to the compiler caused by this change. A number of paths were revealed to be unreachable by this commit, so I've removed that code and left comments as to why I think that was safe to do.
If I was wrong (and I am not familiar with TS internals, so I easily could be), or if that makes the scope of this PR larger than you'd like), I'm happy to go back and replace those with explicit undefined checks.
Sorry, something went wrong.
|
|
||
| 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) { |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
This one I believe is correct; this same change was made in #53463.
Sorry, something went wrong.
| export let unchangedPollThresholds = createPollingIntervalBasedLevels(defaultChunkLevels); | ||
|
|
||
| function setCustomPollingValues(system: System) { | ||
| if (!system.getEnvironmentVariable) { |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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)
Sorry, something went wrong.
| } | ||
|
|
||
| function enumerateNodeModulesVisibleToScript(host: LanguageServiceHost, scriptPath: string): readonly string[] { | ||
| if (!host.readFile || !host.fileExists) return emptyArray; |
There was a problem hiding this comment.
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
Sorry, something went wrong.
There was a problem hiding this comment.
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).
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
TypeScript Bot (@typescript-bot) test top200 TypeScript Bot (@typescript-bot) perf test this faster |
Sorry, something went wrong.
|
Heya Jake Bailey (@jakebailey), I've started to run the faster perf test suite on this PR at a9abcb4. You can monitor the build here. Update: The results are in! |
Sorry, something went wrong.
|
Heya Jake Bailey (@jakebailey), I've started to run the parallelized Definitely Typed test suite on this PR at a9abcb4. You can monitor the build here. Update: The results are in! |
Sorry, something went wrong.
|
Heya Jake Bailey (@jakebailey), I've started to run the tarball bundle task on this PR at a9abcb4. You can monitor the build here. |
Sorry, something went wrong.
|
Heya Jake Bailey (@jakebailey), I've started to run the diff-based user code test suite on this PR at a9abcb4. You can monitor the build here. Update: The results are in! |
Sorry, something went wrong.
|
Heya Jake Bailey (@jakebailey), I've started to run the diff-based top-repos suite on this PR at a9abcb4. You can monitor the build here. Update: The results are in! |
Sorry, something went wrong.
|
Hey Jake Bailey (@jakebailey), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so: {
"devDependencies": {
"typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/159591/artifacts?artifactName=tgz&fileId=0DC22183E915A9D380FB43D26D99F40F4A9A1448C7F8E1164609A1460BD5589402&fileName=/typescript-5.4.0-insiders.20240121.tgz"
}
}
and then running npm install. |
Sorry, something went wrong.
|
Jake Bailey (@jakebailey) Here are the results of running the user test suite comparing main and refs/pull/57114/merge: There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Detailslodash/mnt/ts_downloads/lodash/tsconfig.json
puppeteerpackages/browsers/test/src/tsconfig.json
webpacktsconfig.types.json
|
Sorry, something went wrong.
|
Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready. Package: yandex-maps Error:
/home/vsts/work/1/DefinitelyTyped/types/yandex-maps/yandex-maps-tests.ts
162:10 error TypeScript@local compile error:
This condition will always return true since this function is always defined. Did you mean to call it instead? @definitelytyped/expect
✖ 1 problem (1 error, 0 warnings)
at testTypesVersion (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@definitelytyped+dtslint@0.2.0_typescript@5.4.0-dev.20240121/node_modules/@definitelytyped/dtslint/dist/index.js:166:15)
at async runTests (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@definitelytyped+dtslint@0.2.0_typescript@5.4.0-dev.20240121/node_modules/@definitelytyped/dtslint/dist/index.js:125:9)
Package: activex-excel Error:
/home/vsts/work/1/DefinitelyTyped/types/activex-excel/activex-excel-tests.ts
98:10 error TypeScript@local compile error:
This condition will always return true since this function is always defined. Did you mean to call it instead? @definitelytyped/expect
✖ 1 problem (1 error, 0 warnings)
at testTypesVersion (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@definitelytyped+dtslint@0.2.0_typescript@5.4.0-dev.20240121/node_modules/@definitelytyped/dtslint/dist/index.js:166:15)
at async runTests (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@definitelytyped+dtslint@0.2.0_typescript@5.4.0-dev.20240121/node_modules/@definitelytyped/dtslint/dist/index.js:125:9)
Package: js-fixtures Error:
/home/vsts/work/1/DefinitelyTyped/types/js-fixtures/js-fixtures-tests.ts
16:10 error TypeScript@local compile error:
This condition will always return true since this function is always defined. Did you mean to call it instead? @definitelytyped/expect
✖ 1 problem (1 error, 0 warnings)
at testTypesVersion (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@definitelytyped+dtslint@0.2.0_typescript@5.4.0-dev.20240121/node_modules/@definitelytyped/dtslint/dist/index.js:166:15)
at async runTests (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@definitelytyped+dtslint@0.2.0_typescript@5.4.0-dev.20240121/node_modules/@definitelytyped/dtslint/dist/index.js:125:9)
Package: morgan Error:
/home/vsts/work/1/DefinitelyTyped/types/morgan/morgan-tests.ts
88:10 error TypeScript@local compile error:
This condition will always return true since this function is always defined. Did you mean to call it instead? @definitelytyped/expect
✖ 1 problem (1 error, 0 warnings)
at testTypesVersion (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@definitelytyped+dtslint@0.2.0_typescript@5.4.0-dev.20240121/node_modules/@definitelytyped/dtslint/dist/index.js:166:15)
at async runTests (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@definitelytyped+dtslint@0.2.0_typescript@5.4.0-dev.20240121/node_modules/@definitelytyped/dtslint/dist/index.js:125:9)
Package: xdate Error:
/home/vsts/work/1/DefinitelyTyped/types/xdate/xdate-tests.ts
1177:18 error TypeScript@local compile error:
This condition will always return true since this function is always defined. Did you mean to call it instead? @definitelytyped/expect
✖ 1 problem (1 error, 0 warnings)
at testTypesVersion (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@definitelytyped+dtslint@0.2.0_typescript@5.4.0-dev.20240121/node_modules/@definitelytyped/dtslint/dist/index.js:166:15)
at async runTests (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@definitelytyped+dtslint@0.2.0_typescript@5.4.0-dev.20240121/node_modules/@definitelytyped/dtslint/dist/index.js:125:9)
|
Sorry, something went wrong.
|
Jake Bailey (@jakebailey)
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sorry, something went wrong.
|
Hey Jake Bailey (@jakebailey) -- thank you for such a quick review. I've re-added the 3 statements that may be required for back-compatibility with 4bac889. |
Sorry, something went wrong.
|
Jake Bailey (@jakebailey) Here are the results of running the top-repos suite comparing main and refs/pull/57114/merge: Something interesting changed - please have a look. Detailsapollographql/apollo-client1 of 11 projects failed to build with the old tsc and were ignored src/tsconfig.json
tsconfig.json
calcom/cal.com19 of 22 projects failed to build with the old tsc and were ignored packages/embeds/embed-core/tsconfig.json
packages/embeds/embed-react/tsconfig.json
chakra-ui/chakra-ui4 of 28 projects failed to build with the old tsc and were ignored packages/components/tsconfig.build.json
cheeriojs/cheerio1 of 3 projects failed to build with the old tsc and were ignored tsconfig.json
date-fns/date-fns2 of 4 projects failed to build with the old tsc and were ignored tsconfig.json
tsconfig.lib.json
desktop/desktop1 of 4 projects failed to build with the old tsc and were ignored tsconfig.json
jhipster/generator-jhipster1 of 3 projects failed to build with the old tsc and were ignored tsconfig.json
tsconfig.types.json
jitsi/jitsi-meettsconfig.web.json
lobehub/lobe-chattsconfig.json
microsoft/vscode3 of 54 projects failed to build with the old tsc and were ignored extensions/css-language-features/server/tsconfig.json
extensions/emmet/tsconfig.json
extensions/gulp/tsconfig.json
extensions/html-language-features/server/tsconfig.json
src/tsconfig.json
src/tsconfig.monaco.json
src/tsconfig.tsec.json
outline/outlinetsconfig.json
portainer/portainertsconfig.json
prisma/prisma85 of 106 projects failed to build with the old tsc and were ignored packages/migrate/tsconfig.build.json
pubkey/rxdb10 of 11 projects failed to build with the old tsc and were ignored config/tsconfig.types.json
ReactiveX/rxjs11 of 15 projects failed to build with the old tsc and were ignored packages/rxjs/src/tsconfig.cjs.spec.json
packages/rxjs/src/tsconfig.types.json
redis/node-redis1 of 8 projects failed to build with the old tsc and were ignored packages/test-utils/tsconfig.json
vercel/swr6 of 8 projects failed to build with the old tsc and were ignored tsconfig.json
|
Sorry, something went wrong.
|
I'm happy to close this out (or have it closed) based on the discussion of #34815 -- it's a lot of false positives. |
Sorry, something went wrong.
Fixes #34815
Fixes #43096
This PR is partially a revival of #43097, which had gone stale and was closed. Thank you Jonas Hübotter (@jonhue) for pointing me off in the right direction.