Uncalled function checks check negation condition #57114
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
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. |
|
@microsoft-github-policy-service agree |
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.
| @@ -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) { | |||
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.
There was a problem hiding this comment.
This one I believe is correct; this same change was made in #53463.
| @@ -131,9 +131,6 @@ let pollingChunkSize = createPollingIntervalBasedLevels(defaultChunkLevels); | |||
| 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.
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)
| @@ -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; | |||
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
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).
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.
|
@typescript-bot test top200 @typescript-bot perf test this faster |
|
Heya @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! |
|
Heya @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! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at a9abcb4. You can monitor the build here. |
|
Heya @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! |
|
Heya @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! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
Hey @jakebailey, the results of running the DT tests are ready. Branch only errors:Package: yandex-maps Package: activex-excel Package: js-fixtures Package: morgan Package: xdate |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @jakebailey -- thank you for such a quick review. I've re-added the 3 statements that may be required for back-compatibility with 4bac889. |
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
I'm happy to close this out (or have it closed) based on the discussion of #34815 -- it's a lot of false positives. |
Fixes #34815
Fixes #43096
This PR is partially a revival of #43097, which had gone stale and was closed. Thank you @jonhue for pointing me off in the right direction.