Multiproject requests like find all refs, rename and workspace symbols #1991

sheetalkamat · 2025-10-31T16:52:45Z

No description provided.

Also remove unnecessary program update when doing gc on projects

… tree

… with -v so we can diagnose which test is failing

sheetalkamat · 2025-11-04T20:03:04Z

internal/lsp/lspservertests/testserver.go

+	"gotest.tools/v3/assert"
+)
+
+type testServer struct {


Added this to baseline the project state and watching stuff - also allows to not open all files and control which one to open instead of fourslash opening all files. (to control which project is open).

i havent yet ported tests for watching and lifetime of project but i will do that separately

Finer control of open files would be useful in a lot of scenarios, but overall I find fourslash a lot easier to work with. Could we just add some options/functionality to fourslash instead of this?

Copilot

Pull Request Overview

This PR updates test baselines to reflect improved formatting and line numbering consistency in the test output files. The changes normalize how JSON content and TypeScript code are displayed in baseline files, removing inconsistent indentation and adding proper line numbers to all content.

Key Changes

Standardized line numbering for all file content in test baselines
Normalized JSON formatting to use consistent indentation (4 spaces)
Fixed indentation for TypeScript code blocks in test files
Updated hash values in tsbuildinfo files to reflect the normalized content

Reviewed Changes

Copilot reviewed 100 out of 125 changed files in this pull request and generated 62 comments.

Show a summary per file

File	Description
tsc/moduleResolution/alternateResult.js	Added line numbers to JSON package files and normalized their indentation
tsc/declarationEmit/when-using-Windows-paths-and-uppercase-letters.js	Standardized indentation for TypeScript source code and adjusted error column positions
tsbuildWatch/projectsBuilding/*.js	Normalized tsconfig.json indentation across multiple project building tests
tsbuildWatch/programUpdates/*.js	Updated file content formatting and corresponding hash values in tsbuildinfo
tsbuild/resolveJsonModule/*.js	Consistent JSON formatting for all tsconfig.json files
lspservertests/rename/*.js	Added complete baseline content for new rename test cases
lspservertests/findAllRefs/*.js	Added complete baseline content for new find all references test cases

internal/testutil/lsptestutil/baseline.go

...s/reference/lspservertests/declarationMaps/findAllReferences-definition-is-in-mapped-file.js

...selines/reference/lspservertests/declarationMaps/findAllReferences-starting-at-definition.js

...aselines/reference/lspservertests/declarationMaps/findAllReferences-target-does-not-exist.js

...rvertests/findAllRefs/special-handling-of-localness-when-using-method-of-class-expression.js

...ervertests/findAllRefs/special-handling-of-localness-when-using-arrow-function-assignment.js

...pservertests/findAllRefs/special-handling-of-localness-when-using-object-literal-property.js

testdata/baselines/reference/lspservertests/rename/rename-in-common-file-renames-all-project.js

testdata/baselines/reference/lspservertests/rename/ancestor-and-project-ref-management.js

jakebailey

Some comments; have not read the entire thing yet.

internal/lsp/lsproto/_generate/generate.mts

internal/sourcemap/lineinfo.go

internal/lsp/lspservertests/testserver.go

internal/lsp/server.go

internal/testutil/lsptestutil/lsptestutil.go

internal/testutil/lsptestutil/baseline.go

andrewbranch · 2025-11-11T16:09:56Z

internal/compiler/projectreferencefilemapper.go

-		fn(path, config, parent, index)
-		mapper.forEachResolvedReferenceWorker(mapper.referencesInConfigFile[path], fn, config, seenRef)
+		if fn(path, config, parent, index) {
+			return true


This is opposite of the Go .Range convention. I would suggest inverting and renaming everything to use the verb "range" instead of "forEach" so there's no guessing.

andrewbranch · 2025-11-11T16:15:49Z

internal/project/configfileregistry.go

+// For testing
+func (c *ConfigFileRegistry) ForEachTestConfigEntry(cb func(tspath.Path, *TestConfigEntry)) {
+	if c != nil {
+		for path, entry := range c.configs {


Don’t forget that this is a random order iteration—I didn’t see where you’re using this, but it could mess up baselining.

the diff table sorts it before printing

andrewbranch · 2025-11-11T16:30:35Z

internal/project/session.go

+
+func (s *Session) projectContainsFile(project *Project, uri lsproto.DocumentUri) bool {
+	path := s.toPath(uri.FileName())
+	return project.containsFile(path)


I was going to say project.containsFile should just be public, but this Session method is also private, so I'm not sure what this is for.

andrewbranch · 2025-11-11T16:33:43Z

internal/project/session.go

+	return snapshot
+}
+
+func (s *Session) ForEachProjectLocationLoadingProjectTree(


This feels like an extremely specific API not well suited to Session.

andrewbranch · 2025-11-11T16:49:24Z

internal/project/session.go

+func (s *Session) getSnapshot(
+	ctx context.Context,
+	changeRequest snapshotChangeRequest,
+	updateIf func(snapshot *Snapshot, updatedSnapshot bool) UpdateReason,


I like the idea of an abstraction around getting a snapshot with conditional updates, but this feels off as-is. Returning UpdateReasonUnknown to mean “don’t update” is confusing, and ideally the callers of this function shouldn’t care if the snapshot was just updated with file changes. I'm pretty sure the update line I originally had should have been like this:

- if project == nil && !updateSnapshot || project != nil && project.dirty + if !updatesSnapshot && (project == nil || project.dirty)

so that should at least allow removing the updatedSnapshot parameter.

andrewbranch · 2025-11-11T17:04:13Z

internal/project/snapshot.go

+	referencedProjects map[tspath.Path]struct{}
+}
+
+type snapshotChangeRequest struct {


Naming nit: these are resources that must be loaded in preparation for a language service operation. The other fields of SnapshotChange are mostly data that has changed. Maybe something like

type ResourceRequest struct { Documents []lsproto.DocumentUri Projects []tspath.Path ProjectTree *ProjectTreeRequest }

?

andrewbranch · 2025-11-11T17:04:16Z

internal/project/snapshot.go

+	// Ensure default project for these URIs
+	// These are documents that might not be open, but we need default project for them
+	// eg for Find all references when the definition is in a mapped location that is not yet open
+	ensureDefaultProjectForURIs []lsproto.DocumentUri


Does this actually need to be separate from requestedURIs? It sounds like the preparation would be the same, except that for requestedURIs, we assume the default project already exists.

this is where we "create a default project if it doesnt exist".. The other uses (getting LS for request ) already have project and throw if project isnt present. Trying to create project if it does not exist seems like something was intentional so created separate one.

I would have said we could remove the panic and combine them, but we have seen that panic come up in issue reports, so I guess it might be a good idea to keep them separate, if only for diagnostic purposes 🤔

Although, we can easily determine whether the project should already exist by whether the document URI requested is open.

if b.fs.isOpenFile(requestedDocument) { // panic if project doesn't exist } else { // create project if it doesn't exist }

sheetalkamat force-pushed the multiProject branch 5 times, most recently from 46a8abe to f66df9c Compare November 3, 2025 20:32

sheetalkamat added 20 commits November 3, 2025 13:43

Factor out lsp server setup so we can use it for some unit testing

9e6e021

Add some tests that can verify the server multi project things

b7dc8db

Rename tests

28dd671

Declaration map tests

1171089

factor out todos

9516c6f

Handle existing projects

f88b8f2

Framework to queue more locations

4f25a86

Use declaration maps to queue more projects

5d230c1

Actually find and load the original location projects

fcfedab

navigate to source when we can when reporting references

5470a9d

Create ancestor tree

12c0e93

Actually load ancestor project tree

64a2d44

Fix the order of the results and deduplication

812c9bc

Retain ancestor projects and references till open file is present

fee7f97

Also remove unnecessary program update when doing gc on projects

Fix the config lifetime when getting project for non open file

70ca6da

Parallel

23f8f3f

Port special localness checks to determine if we need to load project…

2be521c

… tree

Baseline workspace symbols

0f005e7

Workspace symbols to load more projects

3a76f2a

Show output of panic found when updating fourslash tests and run them…

72e09da

… with -v so we can diagnose which test is failing

sheetalkamat force-pushed the multiProject branch from 97ecb11 to 72e09da Compare November 3, 2025 21:49

sheetalkamat marked this pull request as ready for review November 4, 2025 20:00

Copilot AI review requested due to automatic review settings November 4, 2025 20:00

sheetalkamat commented Nov 4, 2025

View reviewed changes

Merge branch 'main' into multiProject

e2c372c

sheetalkamat requested review from andrewbranch and jakebailey November 4, 2025 20:07

Copilot AI reviewed Nov 4, 2025

View reviewed changes

jakebailey reviewed Nov 5, 2025

View reviewed changes

sheetalkamat added 3 commits November 7, 2025 16:06

Merge branch 'main' into multiProject

e9a71c4

Feedback

7b6c01d

Merge branch 'main' into multiProject

344f432

sheetalkamat requested a review from jakebailey November 10, 2025 19:39

Refactor

6727c37

sheetalkamat mentioned this pull request Nov 11, 2025

Add locks on concurrent alias following checker accesses under incremental mode #2051

Merged

andrewbranch reviewed Nov 11, 2025

View reviewed changes

Multiproject requests like find all refs, rename and workspace symbols #1991

Are you sure you want to change the base?

Multiproject requests like find all refs, rename and workspace symbols #1991

Uh oh!

Conversation

sheetalkamat commented Oct 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jakebailey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants