Fix workspace search always returning no results for first query#82276
Conversation
| { | ||
| // 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 _))) |
There was a problem hiding this comment.
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.
|
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 |
…ery" This reverts commit e24fd24.
Nevermind, I can't read. this already exists... 🤦 |
…results on first request
jasonmalinowski
left a comment
There was a problem hiding this comment.
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?
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. |
|
Lgtm :-) |
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:
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 |
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. |
|
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. |
Resolves #82275