navigateTo: Collect results from all referenced projects. #25283

ghost · 2018-06-27T23:11:29Z

Fixes navigateTo when multiple projects exist.
The old behavior was to just do navigateTo in the current project, and return results in declaration files from other projects.
The new behavior is to iterate over the current project, and all projects it references (and so on), to get all items; and to exclude those from declaration files for other projects (since those would be redundant).
Note this opens projects and doesn't explicitly close them. (It would be bad to close them immediately since the user is probably about to navigate to one of them.) Since they won't have any files open, I think there is code somewhere to detect this and close them eventually? @sheetalkamat

weswigham · 2018-06-28T16:23:12Z

src/services/services.ts

@@ -1623,7 +1626,7 @@ namespace ts {
        );

        function getTargetOfMappedDeclarationFiles(infos: ReadonlyArray<DefinitionInfo> | undefined): DefinitionInfo[] | undefined {
-            return map(infos, d => getTargetOfMappedDeclarationInfo(d));
+            return map(infos, getTargetOfMappedDeclarationInfo);
        }


Map passes a second parameter to the callback, doesn't it? And despite overriding the type to hide it, a second parameter will mangle the initial value of original in getTargetOfMappedDeclarationInfo, no (which is why it wasn't hidden, to point out issues like that)?

weswigham · 2018-06-28T16:26:28Z

src/services/services.ts

-            const sourceFiles = fileName ? [getValidSourceFile(fileName)] : program.getSourceFiles();
+            // Don't include `.d.ts` files that are declarations for other projects,
+            // since in session.ts we will be getting navigateTo items for those projects anyway.
+            const sourceFiles = fileName ? [getValidSourceFile(fileName)] : program.getSourceFiles().filter(f => !isMappedDeclarationFile(f));


Mapped declaration files can exist independently of project references, meaning if you have a mapped file (eg, in a lib that distributes its own source), but no project ref to it, you now lose navTo ability on types from it.

mhegazy · 2018-06-28T23:51:19Z

src/server/session.ts

+
+        const outputs: T[] = [];
+
+        while (true) {


nit. while (projectsToDo.lenght)

mhegazy · 2018-06-28T23:56:05Z

src/services/services.ts

-                originalTextSpan: original.textSpan
-            })
-        );
-
        function getTargetOfMappedDeclarationFiles(infos: ReadonlyArray<DefinitionInfo> | undefined): DefinitionInfo[] | undefined {


I think we should move this out of services too.. it is strange that we do go-to-def here, and nav-to/find all refs in the session.. i think we can do them all in the session, and have the sourcemapper live there as well.

This is hard to do because of the call to sourceMapper.clearCache() in services.ts. @weswigham There's probably a better signal we could use on when to clear the cache?

It's not being evicted correctly right now anyway, since it's evicted on program out of date and not file out of date (because services doesn't track up-to-date ness). Now that it's in server, you should actually be able to just evict the files which are out of date, since the cross-project fs watcher/up-to-date checking stuff should be available. Specifically a file in that cache aughta be invalidated when either the associated file or the associated map updates on disk. Also the cache should likely be shared across all projects, if at all possible - I remarked as much with Ryan when I first implemented it. Honestly it makes way more sense in session because of all that.

mhegazy · 2018-06-28T23:59:45Z

Can we add tests as well?

weswigham · 2018-06-29T00:23:29Z

src/server/session.ts

+
+                const mapsTo = sourceMapper.tryGetMappedLocation(getLocation(output));
+                if (mapsTo) {
+                    projectService.openingProjectForFileIfNecessary(toNormalizedPath(mapsTo.fileName), project => { projectsToDo.push(project); });


The file or location that mapsTo points at may not actually exist on disk (for example, because someone shipped a declaration map but not their original source) - does this/can this still return the .d.ts location if it turns out that the .ts pointed at by the map doesn't exist (AFAIK goto def actually doesn't handle that awfully gracefully right now)?

mhegazy · 2018-06-29T23:24:26Z

+1 for @weswigham's comment. why is fourslash not sufficient?

mhegazy · 2018-07-02T16:49:52Z

src/server/session.ts

+        const toDo: ProjectAndLocation[] = (isProjectsArray(projects) ? projects : projects.projects).map(project => ({ project, location: initialLocation }));
+
+        while (toDo.length) {
+            const { project, location } = Debug.assertDefined(toDo.pop());


i would check the cancelation token here as well.

mhegazy

Looks good to me. @weswigham and @sheetalkamat please take another look as well.

We will be moving the sourceMap caching to the ScriptInfo in a different PR.

sheetalkamat · 2018-07-02T19:37:46Z

src/server/editorServices.ts

+                return { fileJustOpened: false, project: getProject(this.getConfigFileNameForFile(scriptInfo)) };
+            }
+            else {
+                const openResult = this.tryOpenClientFileWithNormalizedPath(fileName, /*fileContent*/ undefined, /*scriptKind*/ undefined, /*hasMixedContent*/ undefined, /*projectRootPath*/ undefined, /*dontOpenIfNotExists*/ true);


Why do you need to open the script info here? This might break asserts if editor opens file after this operation? (we also need to worry about projectRoot when opening a file which is not handled here)

If I change this to:

const getResult = this.getOrCreateScriptInfoOpenedByClientForNormalizedPath(fileName, this.currentDirectory, /*fileContent*/ undefined, /*scriptKind*/ undefined, /*hasMixedContent*/ undefined, /*openIfNotExists*/ false); return getResult && { fileJustOpened: true, project: getProject(this.getConfigFileNameForFile(getResult)) };

There will be an error at Debug.assert(this.openFiles.has(info.path)) in forEachConfigFileLocation. Could you take a look?

I've modified forEachConfigFileLocation to not have that assert in this case. If the file isn't open projectRootPath will be undefined, so anySearchPathOk will be true and it should look in all possible locations.

sheetalkamat · 2018-07-02T19:46:06Z

src/server/session.ts

+            cb({ project, location }, (project, location) => {
+                const mapsTo = project.getSourceMapper().tryGetMappedLocation(location);
+                if (!mapsTo) return false;
+                const info = mapsTo && projectService.tryOpeningProjectForFileIfNecessary(toNormalizedPath(mapsTo.fileName));


I think instead of opening a file, better approach would be to get project if exists otherwise get configFileName and create the project (you dont want inferred project in this scenario) so you need to handle case where project created by configFileName doesnt include the project? You would want to rather use addExternalProjectReference (rename it correctly) instead. We shouldnt open the script info since that has many implications with project Root etc. In this scenario what should be projectRoot? Since we dont want the search to go on to say "/user/username/" or some such?

sheetalkamat · 2018-07-02T19:46:55Z

src/server/session.ts

+                const info = mapsTo && projectService.tryOpeningProjectForFileIfNecessary(toNormalizedPath(mapsTo.fileName));
+                if (!info) return false;
+                if (info.fileJustOpened) filesToClose.push(mapsTo.fileName);
+                if (info.project) toDo.push({ project: info.project, location: mapsTo });


I think it would be better to modify this check as info.project && !seenProject.has(project.projectName) to avoid unnecessary adds.

weswigham · 2018-07-02T21:27:46Z

src/services/sourcemaps.ts

+            return file.sourceMapper = sourcemaps.identitySourceMapper;
+        }
+
+        function tryGetMappedLocation(info: sourcemaps.SourceMappableLocation): sourcemaps.SourceMappableLocation | undefined {


This should probably be named tryGetOriginalLocation, since "MappedLocation" is a bit ambiguous as to weather it refers to the original location of a mapped file or the emitted location of the mapping file (since both directions are possible).

weswigham · 2018-07-02T21:38:07Z

tests/cases/fourslash/server/declarationMapsEnableMapping_NoInline.ts

+//////# sourceMappingURL=index.d.ts.map
+
+goTo.file("/index.ts");
+verify.getEmitOutput(["/dist/index.js", "/dist/index.d.ts.map", "/dist/index.d.ts"]);


Any particular reason we're using this new verifier over verify.baselineGetEmitOutput and @emitThisFile?

Or did this just replace verify.baselineGetEmitOutput, since I see @emitThisFile is still in the test?

insertResultIntoVfs didn't work when the server implementation of LanguageService was used.

weswigham · 2018-07-02T21:45:37Z

src/harness/fourslash.ts

+            }
+        }
+
+        public baselineGetEmitOutput(insertResultsIntoVfs?: boolean): void {


The insertResultsIntoVfs flag only existed to make writing fourslash tests with declaration maps easier. If they're all in the server folder now, and none of them use baselineGetEmitOutput, then this flag (and the associate branch of code) is unused and aught to be removed.

* Support code-fix-all for importFixes * Change description * Update API (#25283)

sheetalkamat · 2018-07-03T22:21:25Z

src/server/editorServices.ts

@@ -1304,10 +1304,8 @@ namespace ts.server {
        private getConfigFileNameForFile(info: ScriptInfo) {


instead of duplicating code, can you please, make infoShouldBeOpen parameter must here?

sheetalkamat · 2018-07-03T22:23:49Z

src/server/editorServices.ts

+        }
+
+        /** @internal */
+        tryOpenClientFileWithNormalizedPath(fileName: NormalizedPath, fileContent?: string, scriptKind?: ScriptKind, hasMixedContent?: boolean, projectRootPath?: NormalizedPath, dontOpenIfNotExists?: boolean): OpenConfiguredProjectResult | undefined {


Can you please revert refactoring here.. No need to separate out tryOpenClientFileWithNormalizedPath

sheetalkamat · 2018-07-03T22:24:30Z

src/server/editorServices.ts

        }

-        private getOrCreateScriptInfoWorker(fileName: NormalizedPath, currentDirectory: string, openedByClient: boolean, fileContent?: string, scriptKind?: ScriptKind, hasMixedContent?: boolean, hostToQueryFileExistsOn?: { fileExists(path: string): boolean; }) {
-            Debug.assert(fileContent === undefined || openedByClient, "ScriptInfo needs to be opened by client to be able to set its user defined content");
+        private getOrCreateScriptInfoWorker(fileName: NormalizedPath, currentDirectory: string, kind: GetScriptInfoKind, fileContent?: string, scriptKind?: ScriptKind, hasMixedContent?: boolean, hostToQueryFileExistsOn?: { fileExists(path: string): boolean; }) {


You shouldnt need refactoring to getOrCreateScriptInfo*. Please revert it.

sheetalkamat · 2018-07-03T22:25:24Z

src/server/editorServices.ts

+        /** @internal */
+        getProjectForFileWithoutOpening(fileName: NormalizedPath): Project | undefined {
+            const scriptInfo = this.filenameToScriptInfo.get(fileName) ||
+                this.getOrCreateScriptInfoOpenedByClientForNormalizedPath(fileName, this.currentDirectory, /*fileContent*/ undefined, /*scriptKind*/ undefined, /*hasMixedContent*/ undefined, /*openIfNotExists*/ false);


You want to use getOrCreateScriptInfoNotOpenedByClientForNormalizedPath here instead

sheetalkamat · 2018-07-03T22:27:13Z

src/server/editorServices.ts

@@ -2085,19 +2099,31 @@ namespace ts.server {
            });
        }

+        private getOrCreateConfiguredProject(configFileName: NormalizedPath): { readonly project: ConfiguredProject, readonly didCreate: boolean } {


I would rather not have this helper (keep it inlined as was for readbility and avoiding unnecessary objects creation)
In getProjectForFileWithoutOpening use this.findConfiguredProjectByProjectName(configFileName) || this.createConfiguredProject(configFileName)

sheetalkamat · 2018-07-03T22:27:51Z

src/server/editorServices.ts

+        }
+
+        /** @internal */
+        fileExists(fileName: NormalizedPath): boolean {


Whats this for?

Called in getMappedLocation in session.ts.

sheetalkamat · 2018-07-03T22:41:34Z

src/server/session.ts

+        cb: (where: ProjectAndLocation, getMappedLocation: (project: Project, location: sourcemaps.SourceMappableLocation) => boolean) => void,
+    ): void {
+        const seenProjects = createMap<true>();
+        const toDo: ProjectAndLocation[] = (isProjectsArray(projects) ? projects : projects.projects).map(project => ({ project, location: initialLocation }));


I would avoid creating this array if there is no sourceMaps to navigate to.. That would save current scenarios to create these arrays unnecessarily and would need arrays only if there are sourceMap. (original project and symLinkedProjects are unique so you can directly add then to seenMap.

That doesn't seem worth it to me... an array with a single element is probably smaller than an Identifier. Possibly smaller than the closure addToTodo.

If you pull out calling of cb to a function you wont need to have closures and will not need the creation of new array if not needed.

eg.

function callbackProjectAndLocation( projectAndLocation: ProjectAndLocation, projectService: ProjectService, seenProjects: Map<true>, toDo: ProjectAndLocation[] | undefined, cb: (where: ProjectAndLocation, getMappedLocation: (project: Project, location: sourcemaps.SourceMappableLocation) => boolean) => void, ) : ProjectAndLocation[] | undefined

In fact you could delay creation of seenProjects as well since that is also needed only when sourceMapped location is found.

sheetalkamat · 2018-07-03T22:42:01Z

src/server/session.ts

+            if (project.getCancellationToken().isCancellationRequested()) continue;
+            cb({ project, location }, (project, location) => {
+                const originalLocation = project.getSourceMapper().tryGetOriginalLocation(location);
+                const originalProject = originalLocation && projectService.getProjectForFileWithoutOpening(toNormalizedPath(originalLocation.fileName));


Don't you need to see if there are symLinkedProjects for this originalLocation ?

ghost · 2018-07-04T00:06:25Z

@sheetalkamat Please re-review

sheetalkamat · 2018-07-05T16:30:30Z

src/server/session.ts

+        const seenProjects = createMap<true>();
+        const toDo: ProjectAndLocation[] = (isProjectsArray(projects) ? projects : projects.projects).map(project => ({ project, location: initialLocation }));
+        if (!isArray(projects) && projects.symLinkedProjects) {
+            projects.symLinkedProjects.forEach((symlinkedProjects, path) => {


You want to add projects and symLinkedProjects to seenProjects as these will be in array to toDo but not in seen so the below check of addToDo will not have these?

sheetalkamat · 2018-07-05T16:35:01Z

src/server/editorServices.ts

+            const scriptInfo = this.filenameToScriptInfo.get(fileName) ||
+                this.getOrCreateScriptInfoNotOpenedByClientForNormalizedPath(fileName, this.currentDirectory, /*fileContent*/ undefined, /*scriptKind*/ undefined, /*hasMixedContent*/ undefined);
+            const configFileName = scriptInfo && this.getConfigFileNameForFile(scriptInfo, /*infoShouldBeOpen*/ false);
+            const project = configFileName === undefined ? undefined : this.findConfiguredProjectByProjectName(configFileName) || this.createConfiguredProject(configFileName);


I think once you create the project you want to check if projectAndLocation.project.containsScriptInfo(scriptInfo) since configured project could not include the script info?

sheetalkamat · 2018-07-05T16:35:34Z

src/server/editorServices.ts

+                this.getOrCreateScriptInfoNotOpenedByClientForNormalizedPath(fileName, this.currentDirectory, /*fileContent*/ undefined, /*scriptKind*/ undefined, /*hasMixedContent*/ undefined);
+            const configFileName = scriptInfo && this.getConfigFileNameForFile(scriptInfo, /*infoShouldBeOpen*/ false);
+            const project = configFileName === undefined ? undefined : this.findConfiguredProjectByProjectName(configFileName) || this.createConfiguredProject(configFileName);
+            return scriptInfo && project && { scriptInfo, project };


Do you need to get scriptInfo.containingProjects instead of just project

ghost · 2018-07-05T21:43:45Z

@sheetalkamat Think we can get this in this week for ts3.0?

sheetalkamat · 2018-07-05T21:48:05Z

src/server/editorServices.ts

+            if (scriptInfo.containingProjects.length) {
+                return { scriptInfo, projects: scriptInfo.containingProjects };
+            }
+            const configFileName = scriptInfo && this.getConfigFileNameForFile(scriptInfo, /*infoShouldBeOpen*/ false);


could be this.getConfigFileNameForFile(scriptInfo, /infoShouldBeOpen/ false);

navigateTo: Collect results from all referenced projects.

a43bfff

ghost requested review from weswigham, RyanCavanaugh and sheetalkamat June 27, 2018 23:11

ghost mentioned this pull request Jun 27, 2018

Add test for goto-definition with project references #24867

Merged

weswigham reviewed Jun 28, 2018

View reviewed changes

Don't use project references, just source maps

39be798

mhegazy reviewed Jun 28, 2018

View reviewed changes

src/server/session.ts Outdated

const outputs: T[] = [];

while (true) {

Copy link

Contributor

mhegazy Jun 28, 2018

Choose a reason for hiding this comment

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

nit. while (projectsToDo.lenght)

Merge branch 'master' into navigateToWithProjects

c522ae9

mhegazy reviewed Jun 28, 2018

View reviewed changes

weswigham reviewed Jun 29, 2018

View reviewed changes

Andy Hanson added 4 commits June 28, 2018 17:36

Move more code to session

e0f099c

Test when implementation file is deleted

c1ef8d9

Use tsserver tests instead of fourslash tests to ensure session is used

4bcd3ec

Support find-all-references

8b7dacf

ghost force-pushed the navigateToWithProjects branch from 4bf47fe to 8cda27f Compare June 30, 2018 02:18

Restore fourslash tests

a56e1d5

ghost force-pushed the navigateToWithProjects branch from 8cda27f to a56e1d5 Compare June 30, 2018 02:19

mhegazy reviewed Jul 2, 2018

View reviewed changes

mhegazy approved these changes Jul 2, 2018

View reviewed changes

Update emit baselines (added missing newline)

3001c8d

sheetalkamat requested changes Jul 2, 2018

View reviewed changes

weswigham reviewed Jul 2, 2018

View reviewed changes

Andy Hanson added 2 commits July 2, 2018 17:41

Support rename

0e62fdc

Merge branch 'master' into navigateToWithProjects

6edfbba

ghost force-pushed the navigateToWithProjects branch from df1162e to 6edfbba Compare July 3, 2018 00:52

Andy Hanson added 3 commits July 3, 2018 11:20

Merge branch 'master' into navigateToWithProjects

62197e1

Don't open/close files

c0b159a

Avoid growing toDo too eagerly

2a99ca9

ghost force-pushed the navigateToWithProjects branch from db7c7aa to 2a99ca9 Compare July 3, 2018 21:31

ghost mentioned this pull request Jul 3, 2018

"rename file" and "move to new file" refactors do not work when using files #24857

Closed

ghost pushed a commit that referenced this pull request Jul 3, 2018

Support code-fix-all for importFixes (#25137)

064ecd4

* Support code-fix-all for importFixes * Change description * Update API (#25283)

sheetalkamat reviewed Jul 3, 2018

View reviewed changes

Andy Hanson added 3 commits July 3, 2018 16:10

@sheetalkamat code review

fa28028

Also get symlinked projects for originalLocation

6898166

Update API (#24966)

9984aeb

sheetalkamat reviewed Jul 5, 2018

View reviewed changes

More @sheetalkamat code review

49f1ad8

sheetalkamat reviewed Jul 5, 2018

View reviewed changes

sheetalkamat approved these changes Jul 5, 2018

View reviewed changes

Andy Hanson added 2 commits July 5, 2018 15:04

Remove unnecessary test

a92bd0a

Update API (#24966)

ae945d9

ghost merged commit 64555aa into master Jul 5, 2018

ghost deleted the navigateToWithProjects branch July 5, 2018 22:39

sandersn mentioned this pull request Jul 6, 2018

Change to references' lineText property breaks find-all-references in emacs #25489

Closed

This pull request was closed.

		@@ -1304,10 +1304,8 @@ namespace ts.server {
		private getConfigFileNameForFile(info: ScriptInfo) {

navigateTo: Collect results from all referenced projects. #25283

navigateTo: Collect results from all referenced projects. #25283

Conversation

ghost commented Jun 27, 2018

weswigham Jun 28, 2018 • edited Loading

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

mhegazy commented Jun 28, 2018

weswigham Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

mhegazy commented Jun 29, 2018

Choose a reason for hiding this comment

mhegazy left a 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

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 Jul 3, 2018 • edited Loading

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

ghost Jul 3, 2018 • edited by ghost Loading

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

ghost commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 5, 2018

Choose a reason for hiding this comment

weswigham Jun 28, 2018 •

edited

Loading

weswigham Jun 29, 2018 •

edited

Loading

sheetalkamat Jul 3, 2018 •

edited

Loading

ghost Jul 3, 2018 •

edited by ghost

Loading