noDts project resolutions verification and updates for incremental behaviour #56016

sheetalkamat · 2023-10-06T22:39:29Z

When resolving module specifier for finding implementation file, we were using module resolution cache which means that it was possible to reuse existing module resolution and add more failed lookup locations to it. Which means that when we release that resolution as part of program update/close, we will try to release more locations than being watched.
Fixed this by marking module resolution cache as read-only after construction of program, which indicates module resolution algorithm to not update the cache or resolution. (instead of freezing internal maps, went ahead with at usage mode approach to avoid having to deal with invalidation logic eg for package json info before program construction) 1b6c9a8

Also updated some of the other noDts project related things:

Project gets updates root files only if the requested file changes 2d40a69
Add isBackgroundProject so we can ensure all the updates where we need to skip things for background project are handled correctly fe7f068
Earlier if the js file was not part of project it would use it only if it was at any point used and cached in services. Instead of that, now it's always added to project to search for implementation from. 0efe4ee

Added coverage for some of the missing cases like

when module specifier resolved from file is not already in the file: d552636
file is added to project after project construction for the first time based on imports present in existing file 43720b9
repeated query shouldn't change project: 5ef80a9
resolution is reused that was done from some sibling folder 8538100
incremental verification for resolveSingleModuleNameWithoutWatching to ensure it does not impact state: a49de0e

… to find the source

…sting methods

…file origination query

// This shows issue with two things: // When resolution is reused from different folder, failed lookups get added so next project update is going to cause ref count problem // package json info is added to cache and thats retained without watching

…o changes to them

jakebailey · 2023-10-09T21:57:24Z

I'm not entirely sure how these types are used in public, but is the readonly flag something that should be exported so that external consumers know to use the API in the right way, or is it unlikely to matter?

sheetalkamat · 2023-10-09T22:21:07Z

I think we only care about resolution cache we created and are using so i am not sure if it matters. But also while investigating i was wondering Project::getModuleResolutionCache should have been internal and i was going to mark it that way and forgot. Will do that.

…been

typescript-bot · 2023-10-09T22:26:05Z

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

jakebailey · 2023-10-09T22:30:17Z

tests/baselines/reference/api/typescript.d.ts

@@ -3354,7 +3354,6 @@ declare namespace ts {
            readFile(fileName: string): string | undefined;
            writeFile(fileName: string, content: string): void;
            fileExists(file: string): boolean;
-            getModuleResolutionCache(): ModuleResolutionCache | undefined;


Hm, https://github.com/marko-js/language-server/blob/6829c580d16eb1c6128c90a73af36fa4efb64d08/packages/language-server/src/ts-plugin/index.ts#L59 uses it, but that may be the only example I found, so maybe it's fine.

They shouldnt have been. Gitlens went in loops but couldnt figure out when and why this was exposed but from looks of it, it should not be exposed at all

It was added in #47388, it seems. @andrewbranch

Wait, but it's internal there. Hm, one sec.

It’s internal on LanguageServiceHost, but not on Project 😕

Ah, yeah, I just discovered that. I guess it should just be dropped?

(To be clear I'm not trying to actually blame anyone, moreso just trying to do the forensics so we know if there was a reason to have it 😅)

andrewbranch · 2023-10-09T22:35:57Z

src/compiler/moduleNameResolver.ts

-        resultFromCache.affectingLocations = updateResolutionField(resultFromCache.affectingLocations, affectingLocations);
-        resultFromCache.resolutionDiagnostics = updateResolutionField(resultFromCache.resolutionDiagnostics, diagnostics);
-        return resultFromCache;
+        if (!cache?.isReadonly) {


My only concern is it seems somewhat easy to miss this check if someone were adding new code that interacts with the cache, and it seems like there still are no compile-time or runtime checks preventing you from mutating the contents at a time when it could cause problems. Would it be worth changing ResolvedModuleWithFailedLookupLocations etc. to have readonly properties that can only be changed by a method on the cache, which could check isReadonly to ensure it’s not being violated?

i can do that, but frankly I dont see point in that since I think we want this method to be that one. Which ensures the resolutionWithFailedLookupLocation is created correctly.

sheetalkamat added 2 commits October 6, 2023 11:54

Add test where different specifier than whats in the file is resolved…

d552636

… to find the source

Refactoring to ensure project files are added and removed through exi…

098e3ef

…sting methods

sheetalkamat force-pushed the sourceDefChanges branch from 13f1ca3 to ed80039 Compare October 6, 2023 22:42

sheetalkamat added 7 commits October 6, 2023 15:52

Some tests where file is added later to the project

43720b9

Fix issue with js file having to be already present in project service

0efe4ee

Add test when repeated goToSource shouldnt change the project

5ef80a9

Fix issue with unnecessary project update even with same source info …

2d40a69

…file origination query

Some more refactoring for handling background projects

fe7f068

Add test for resolution reuse from different folder

8538100

sheetalkamat force-pushed the sourceDefChanges branch from ed80039 to a49de0e Compare October 6, 2023 23:01

Mark the caches as readonly after program construction so there are n…

1b6c9a8

…o changes to them

typescript-bot assigned sheetalkamat Oct 9, 2023

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Oct 9, 2023

sheetalkamat marked this pull request as ready for review October 9, 2023 21:24

sheetalkamat requested review from andrewbranch and jakebailey October 9, 2023 21:24

sheetalkamat changed the title ~~[wip] noDts project resolutions verification and updates for incremental behaviour~~ noDts project resolutions verification and updates for incremental behaviour Oct 9, 2023

Mark project::getModuleResolutionCache as internal as it should have …

e74da81

…been

jakebailey reviewed Oct 9, 2023

View reviewed changes

Merge branch 'main' into sourceDefChanges

d770127

andrewbranch reviewed Oct 9, 2023

View reviewed changes

andrewbranch approved these changes Oct 9, 2023

View reviewed changes

Merge branch 'main' into sourceDefChanges

5ae49dd

sheetalkamat merged commit 9473195 into main Oct 10, 2023

sheetalkamat deleted the sourceDefChanges branch October 10, 2023 17:24

Shinkhal mentioned this pull request Jan 23, 2025

[Bug] Fixing Module Resolution Cache Behavior in noDts Projects #61029

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

noDts project resolutions verification and updates for incremental behaviour #56016

noDts project resolutions verification and updates for incremental behaviour #56016

sheetalkamat commented Oct 6, 2023 •

edited

Loading

jakebailey commented Oct 9, 2023

sheetalkamat commented Oct 9, 2023

typescript-bot commented Oct 9, 2023

jakebailey Oct 9, 2023

sheetalkamat Oct 9, 2023

jakebailey Oct 9, 2023

jakebailey Oct 9, 2023

andrewbranch Oct 9, 2023

jakebailey Oct 9, 2023

jakebailey Oct 9, 2023

andrewbranch Oct 9, 2023

sheetalkamat Oct 9, 2023

noDts project resolutions verification and updates for incremental behaviour #56016

noDts project resolutions verification and updates for incremental behaviour #56016

Conversation

sheetalkamat commented Oct 6, 2023 • edited Loading

jakebailey commented Oct 9, 2023

sheetalkamat commented Oct 9, 2023

typescript-bot commented Oct 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheetalkamat commented Oct 6, 2023 •

edited

Loading