Skip to content

Fix workspace search always returning no results for first query#82276

Merged
dibarbet merged 4 commits into
dotnet:mainfrom
dibarbet:fix_search_first
Feb 4, 2026
Merged

Fix workspace search always returning no results for first query#82276
dibarbet merged 4 commits into
dotnet:mainfrom
dibarbet:fix_search_first

Conversation

@dibarbet

@dibarbet dibarbet commented Feb 3, 2026

Copy link
Copy Markdown
Member

Resolves #82275

@dibarbet dibarbet requested a review from a team as a code owner February 3, 2026 22:53
{
// If there are no projects in this solution that use OOP, then there's nothing we need to do.
if (_solution.Projects.All(p => !RemoteSupportedLanguages.IsSupported(p.Language)))
if (_solution.Projects.All(p => !RemoteSupportedLanguages.IsSupported(p.Language) || !RemoteHostClient.SupportsRemoteHostClient(_solution.Services, out var _)))

@dibarbet dibarbet Feb 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The caller checks if the s_remoteHostHydrateTask task is completed to see if solution loading is done. Before this change the first call creates a fire and forget task below, causing IsCompleted to always be false on the first call (leading navto to think sln loading is not done and try to use cached values)

OOP is not supported outside of VS, so we can check that upfront instead and return a completed task.

@CyrusNajmabadi

Copy link
Copy Markdown
Contributor

So the general idea here is that the 'host' (the thing calling into nav-to) passes in the right INavigateToSearcherHost. So instead of using the default, just pass one in that says it is fully loaded.

@dibarbet

dibarbet commented Feb 3, 2026

Copy link
Copy Markdown
Member Author

So the general idea here is that the 'host' (the thing calling into nav-to) passes in the right INavigateToSearcherHost. So instead of using the default, just pass one in that says it is fully loaded.

@CyrusNajmabadi should INavigateToSearcherHost maybe be a workspace service instead of being passed in to the NavigateToSearcher (or was there a reason it wasn't done that way)?

The host in this case is LSP, but the handler could either be running inside or outside VS. As a workspace service I could override the default INavigateToSearcherHost in the LSP project which would ensure it picks the right one

@dibarbet

dibarbet commented Feb 3, 2026

Copy link
Copy Markdown
Member Author

So the general idea here is that the 'host' (the thing calling into nav-to) passes in the right INavigateToSearcherHost. So instead of using the default, just pass one in that says it is fully loaded.

@CyrusNajmabadi should INavigateToSearcherHost maybe be a workspace service instead of being passed in to the NavigateToSearcher (or was there a reason it wasn't done that way)?

The host in this case is LSP, but the handler could either be running inside or outside VS. As a workspace service I could override the default INavigateToSearcherHost in the LSP project which would ensure it picks the right one

Nevermind, I can't read. this already exists... 🤦

@jasonmalinowski jasonmalinowski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved, although I'm a little confused why this is implemented the way it is. It seems this interface is a shortcut for the regular implementation of "check if the projects are loaded, and then also check if remote is active". I can imagine we don't want the remote check here, but I'd kinda imagine we'd still want to wait until things have loaded, right?

@dibarbet dibarbet enabled auto-merge February 4, 2026 01:58
@dibarbet

dibarbet commented Feb 4, 2026

Copy link
Copy Markdown
Member Author

I can imagine we don't want the remote check here, but I'd kinda imagine we'd still want to wait until things have loaded, right?

discussed offline, currently the project load check just returns true because there's no operation progress service in the language server (either standalone or even with devkit). Can be a future improvement.

@dibarbet dibarbet merged commit 58efd13 into dotnet:main Feb 4, 2026
24 of 25 checks passed
@dibarbet dibarbet deleted the fix_search_first branch February 4, 2026 02:40
@dotnet-policy-service dotnet-policy-service Bot added this to the Next milestone Feb 4, 2026
@CyrusNajmabadi

Copy link
Copy Markdown
Contributor

Lgtm :-)

@CyrusNajmabadi

Copy link
Copy Markdown
Contributor

It seems this interface is a shortcut for the regular implementation of "check if the projects are loaded, and then also check if remote is active". I can imagine we don't want the remote check here, but I'd kinda imagine we'd still want to wait until things have loaded, right?

Not exactly. The idea behind NavTo (and a lot of tiem was spent on this) was to try to make it a viable system to use practically immediately. As long as we know the solution path (i.e. a solution-id). The goal was that we should effectively be able to make a nav-to request at that point, searching all the old indexed date, recognizing that it may be out of date, but still super useful for someone doing the flow of:

  1. opening VS with a file in mind.
  2. trying to navigate to that result.

So there is an entire flow in nav-to that executes prior to the solution being 'fully loaded' or prior to oop being 'fully hydrated'. In this state we give back 'stale results', and we accommodating of things like files that don't exist, or spans that are incorrect (which otherwise would cause us to report an internal fault for having bad data between oop and VS).

The UI can then state that the results are stale/inaccurate. But the user literally does not have to wait at all.

--

NOte: this work came about as part of a large initiative (probably about 4-5 years ago) to make people able to get into VS and do certain types of operations as quickly as possible. This is when i took a multi-layered approach to perf in the workspace/oop. Including, but not limited to, things like this (allowing times where staleness is fine), as well as the work on 'cone syntax' or improved 'frozen partial' results.

I feel like that initiative got somewhat disbanded. And just replaced with things like "Well... just make the project system load much much faster". But the overall designs are sound, and i still think are useful, especially for very large projects.

Does that make sense @jasonmalinowski

@jasonmalinowski

Copy link
Copy Markdown
Member

The UI can then state that the results are stale/inaccurate.

Thanks for the background -- in the scenario that @dibarbet was working on, there wasn't a UI to show that in. I was also a bit confused just how the implementation was: I would have expected that there was just a nav to "wait until things are ready" method (however that is defined), and all of the logic here was in that service; the default implementation is what's there and instead you can override it. Instead the service is optional and it overrides in one direction but not really the other.

@CyrusNajmabadi

Copy link
Copy Markdown
Contributor

Mentioned offline: Nothing about the current impl is sacrosanct. Feel free to make cleaner.

What matters most simply is that system work quickly, without any waiting for the solution to load, or for oop to hydrate (and remember that oop hydration here would not be project-cone, but solution wide).

We want those requests to kick off without waiting, and to return any stale results they can to VS. We then allow opening those results (as long as the files still exist), and we even are resilient to the spans no longer being valid (we'll clamp them to the current file end). So you can at least still nav to the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

First call to workspace/symbol using roslyn-language-server tool always returns no result

4 participants