Referencing module name using index when resolving module name can be incorrect

[sheetalkamat]

We cannot assume that module name is referenced at index i in containing source file because we could be calling you with only few module names from the source file. https://github.com/microsoft/TypeScript/blob/main/src/compiler/program.ts#L1571 shows that case

Originally posted by @sheetalkamat in #45884 (comment)

[sheetalkamat]

@weswigham

[weswigham]

Hm, the called should just need to get updated to pass in ReferencedFiles then, rather than a raw name list, since the ReferencedFiles include the index in the original name list with which we can lookup the mode of the import. Similar to how getModuleNameStringLiteralAt's callers already use FileReferences.

In any case, do you know how this'd manifest in an issue so we can have a test?

[sheetalkamat]

import {a} from "a"
import { b} from "./b"

I think in this case if b.ts was not present in first pass and then b.ts is created this could be true. Some resolutions can be reused but others need to recalculated in the file.

[weswigham]

So I've spent far, far too long on this - and not on fixing it, no, that's easy. Writing a test that actually proves there's an issue that needs fixing is what's been burning away the hours.

So, your example. It's close to what you'd think would trigger an issue (you'd need to ensure the modes of both imports are different, too, to actually cause a problem, which means using type-only imports and assertions), given the way resolveModulesReusingOldState is written, but no - it's actually impossible for the resolveModuleNames callsite within to get a partial resolution list, at least with our built-in hosts and caches. This is because we only reuse old resolutions at all if the new file is the same as the old file, in which case we'd reuse every resolution - the only missing resolutions would be those which didn't originally resolve, as you suspect. However, when there is a missing resolution, we invalidate all resolutions in the file when any of those missing resolutions appear! (And thus recalculate all resolutions in the file from scratch.) Thus, there's no circumstance where we ever attempt to resolve a partial list of resolutions, as much as the code is evidently written to try to handle such as case.

So it's actually impossible to test my fix for this (which is fairly straightforward), because it's actually impossible to trigger a problem, at least without custom resolution caches and LS hosts that behave very differently from our builtin ones. This is a test you would expect to fail, but it's already passing because of the aforementioned over-invalidation done by our hosts.

So not sure what to do here, tbh.

[sheetalkamat]

Found the test case as part of my module resolution caching work