Remove the CanonicalForked state from LanguageServerProjectLoader#82755
Remove the CanonicalForked state from LanguageServerProjectLoader#82755jasonmalinowski wants to merge 2 commits intodotnet:mainfrom
Conversation
This simplifies the approach being used for forked canonical projects and brings them back inline with how we do regular projects. A project where we want a forked canonical project for is initialized to being a primordial project like before, and then we update it to a real project via the usual mechanism. The trick is to treat the build host parameter to TryLoadProjectInMSBuildHostAsync as optional -- once we've built the canonical state we just don't need to use it again.
jasonmalinowski force-pushed the get-rid-of-canonical-state branch from fdf605d to 2eca09a Compare 
| var primordialDoc = AddPrimordialDocument(uri, documentText, languageId); | ||
| Contract.ThrowIfNull(primordialDoc.FilePath); | ||
| | ||
| var doDesignTimeBuild = uri.ParsedUri?.IsFile is true && supportsDesignTimeBuild; |
There was a problem hiding this comment.
I think we now want to do DTBs on documents which are not on disk, so that we can hit the path to use a canonical project in TryLoadProjectInMSBuildHostAsync.
There was a problem hiding this comment.
In the canonical case it's not actually doing a DTB, so the condition could just be updated as needed.
| // For example, loose files which don't look like file-based programs, are put in projects forked from the canonical project loader, only when the setting is enabled, etc. | ||
| // We anticipate that changing this setting will be infrequent, and, the cost of needing to reload will be acceptable given that. | ||
| _logger.LogInformation($"Detected enableFileBasedPrograms changed to '{value}'. Unloading loose file projects."); | ||
| // TODO: just reload now |
There was a problem hiding this comment.
Do we expect TryLoadProjectInMSBuildHostAsync to now also handle the case where "enableFileBasedPrograms": false is set? e.g. should it also be able to cook up a misc project with no references in there, use that, and expect ReloadProjectAsync to do whatever necessary updates to the workspace?
There was a problem hiding this comment.
That was my expectation, especially since the state to create there is pretty small.
| protected override async Task<RemoteProjectLoadResult?> TryLoadProjectInMSBuildHostAsync( | ||
| BuildHostProcessManager buildHostProcessManager, string documentPath, CancellationToken cancellationToken) | ||
| { | ||
| if (!VirtualProjectXmlProvider.IsFileBasedProgram(SourceText.From(File.ReadAllText(documentPath)))) |
There was a problem hiding this comment.
I am expecting this check to be replaced with a call to 'ClassifyDocumentAsync', which will properly handle the case where the file doesn't exist.
That check will need to handle the case where the file doesn't exist and also isn't tracked by LSP. I believe unloading the project is the right thing to do in that case.
Rather than having two separate loaders one for canonical-project- based projects and one for other projects, it's easier if we unify down to one, and just return what the project should look like. This creates a separate BuildHostProcessManager but the simplification there makes that OK.
jasonmalinowski force-pushed the get-rid-of-canonical-state branch from 2eca09a to 6683f84 Compare 
| var virtualProjectXml = $""" | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <TargetFramework>net$(BundledNETCoreAppTargetFrameworkVersion)</TargetFramework> |
There was a problem hiding this comment.
framework is already a property https://github.com/dotnet/sdk/blob/a6af77909c46799080943ee15c028462867c60f5/src/Layout/redist/targets/GenerateBundledVersions.targets#L647C6-L647C38
| <TargetFramework>net$(BundledNETCoreAppTargetFrameworkVersion)</TargetFramework> | |
| <TargetFramework>$(BundledNETCoreAppTargetFramework)</TargetFramework> |
| var forkedInfos = canonicalInfos.SelectAsArray(info => info with | ||
| { | ||
| FilePath = miscDocumentPath, | ||
| Documents = info.Documents.Add(miscDocFileInfo), |
There was a problem hiding this comment.
I think this doesn't work when the document doesn't exist on disk. The downstream handling of this data appears to expect to be able to read the file off disk.
[05:41:50.926] [Error] [LanguageServerProjectLoader] Error while loading untitled:Untitled-1: Exception thrown: System.ArgumentException: Absolute path expected. (Parameter 'path') at Roslyn.Utilities.CompilerPathUtilities.RequireAbsolutePath(String path, String argumentName) in C:\Users\rikki\src\roslyn\src\Workspaces\SharedUtilitiesAndExtensions\Compiler\Core\Utilities\CompilerUtilities\CompilerPathUtilities.cs:line 21 at Microsoft.CodeAnalysis.FileTextLoader..ctor(String path, Encoding defaultEncoding) in C:\Users\rikki\src\roslyn\src\Workspaces\Core\Portable\Workspace\Solution\FileTextLoader.cs:line 49 at Microsoft.CodeAnalysis.WorkspaceFileTextLoader..ctor(SolutionServices services, String path, Encoding defaultEncoding) in C:\Users\rikki\src\roslyn\src\Workspaces\Core\Portable\Workspace\WorkspaceFileTextLoader.cs:line 24 at Microsoft.CodeAnalysis.Workspaces.ProjectSystem.ProjectSystemProjectFactory.CreateFileTextLoader(String fullPath) in C:\Users\rikki\src\roslyn\src\Workspaces\Core\Portable\Workspace\ProjectSystem\ProjectSystemProjectFactory.cs:line 98 at Microsoft.CodeAnalysis.Workspaces.ProjectSystem.ProjectSystemProject.BatchingDocumentCollection.AddFile(String fullPath, SourceCodeKind sourceCodeKind, ImmutableArray`1 folders) in C:\Users\rikki\src\roslyn\src\Workspaces\Core\Portable\Workspace\ProjectSystem\ProjectSystemProject.BatchingDocumentCollection.cs:line 107 at Microsoft.CodeAnalysis.Workspaces.ProjectSystem.ProjectSystemProject.AddSourceFile(String fullPath, SourceCodeKind sourceCodeKind, ImmutableArray`1 folders) in C:\Users\rikki\src\roslyn\src\Workspaces\Core\Portable\Workspace\ProjectSystem\ProjectSystemProject.cs:line 849 at Microsoft.CodeAnalysis.LanguageServer.HostWorkspace.LoadedProject.<>c__DisplayClass28_0.<UpdateWithNewProjectInfoAsync>b__0(DocumentFileInfo document) in C:\Users\rikki\src\roslyn\src\LanguageServer\Microsoft.CodeAnalysis.LanguageServer\HostWorkspace\LoadedProject.cs:line 180 at Microsoft.CodeAnalysis.LanguageServer.HostWorkspace.LoadedProject.<>c__DisplayClass28_1.<UpdateWithNewProjectInfoAsync>g__UpdateProjectSystemProjectCollection|17[T](IEnumerable`1 loadedCollection, IEnumerable`1 oldLoadedCollection, IEqualityComparer`1 comparer, Action`1 addItem, Action`1 removeItem, String logMessage) in C:\Users\rikki\src\roslyn \src\LanguageServer\Microsoft.CodeAnalysis.LanguageServer\HostWorkspace\LoadedProject.cs:line 282 at Microsoft.CodeAnalysis.LanguageServer.HostWorkspace.LoadedProject.UpdateWithNewProjectInfoAsync(ProjectFileInfo newProjectInfo, Boolean isMiscellaneousFile, Boolean hasAllInformation, ILogger logger) in C:\Users\rikki\src\roslyn\src\LanguageServer\Microsoft.CodeAnalysis.LanguageServer\HostWorkspace\LoadedProject.cs:line 176 at Microsoft.CodeAnalysis.LanguageServer.HostWorkspace.LoadedProject.UpdateWithNewProjectInfoAsync(ProjectFileInfo newProjectInfo, Boolean isMiscellaneousFile, Boolean hasAllInformation, ILogger logger) in C:\Users\rikki\src\roslyn\src\LanguageServer\Microsoft.CodeAnalysis.LanguageServer\HostWorkspace\LoadedProject.cs:line 267 at Microsoft.CodeAnalysis.LanguageServer.HostWorkspace.LanguageServerProjectLoader.ReloadProjectAsync(ProjectToLoad projectToLoad, ToastErrorReporter toastErrorReporter, BuildHostProcessManager buildHostProcessManager, CancellationToken cancellationToken) in C:\Users\rikki\src\roslyn\src\LanguageServer\Microsoft.CodeAnalysis.LanguageServer\HostWorks pace\LanguageServerProjectLoader.cs:line 286 I considered whether ProjectFileInfo or DocumentFileInfo could be adjusted to also hold a SourceTextContainer so that we could ultimately pass a possibly-not-on-disk file to ProjectSystemProject.AddSourceTextContainer.
But, those types are declared in a layer which doesn't have a reference to MS.CA. It felt like adding that reference is not part of the intended layering there.
So, I was considering whether to thread the SourceTextContainer through on the side, and ultimately pass it in to LoadedProject.UpdateWithNewProjectInfoAsync, so that it can call AddSourceTextContainer with it.
There was a problem hiding this comment.
Alternatively, I wonder if handling the "file doesn't exist" case, by inserting a "stub" document or something, and letting the GetLspSolutionForWorkspaceAsync replace it with the up-to-date LSP text, would work.
I don't know if that solution would be felt to be too magical.
There was a problem hiding this comment.
But, those types are declared in a layer which doesn't have a reference to MS.CA. It felt like adding that reference is not part of the intended layering there.
Ah, right, we don't put the SourceTexts in that since those are also the types we use to marshal across to the BuildHost, so practically they couldn't fit in there.
by inserting a "stub" document or something, and letting the GetLspSolutionForWorkspaceAsync replace it with the up-to-date LSP text, would work.
That's what I'd expect to happen here -- it doesn't actually matter what text you really put in this document; it'll get replaced by the LSP snapshot anyways. I'd say a simple fix might be that in UpdateWithNewProjectInfoAsync we just say non-absolute paths instead result in a call to AddSourceTextContainer with an empty content.
| } | ||
| finally | ||
| { | ||
| Directory.Delete(tempDirectory, recursive: true); |
There was a problem hiding this comment.
It seems like deleting the directory and then applying project updates based on the contents later breaks things fairly deeply as we can't load the files we were interested in when it comes time to actually load those files in the workspaces project.
I suspect that the right thing to do is change back to IDisposable and delete this on shutdown. Let me know if I missed something.
There was a problem hiding this comment.
as we can't load the files we were interested in when it comes time to actually load those files in the workspaces project.
Can you clarify what files those are? The only files I could imagine might be getting pulled in would be things like the project.assets.json or maybe output paths, but we should likely be nulling those out anyways or it's strange that we have a bunch of projects with the same information.
There was a problem hiding this comment.
For example, Canonical.AssemblyInfo.cs and other files which may be generated during DTB.
There was a problem hiding this comment.
Ah right, I didn't think about those. I'd be OK with either saying:
- We just drop all source files (since we don't really need those files)
- We just leave the files around until shutdown.
There was a problem hiding this comment.
I'm going with (2) as some of the content feels useful, such as default set of global usings.
| This PR was ultimately incorporated into #82509. |
3 participants
This simplifies the approach being used for forked canonical projects and brings them back inline with how we do regular projects. A project where we want a forked canonical project for is initialized to being a primordial project like before, and then we update it to a real project via the usual mechanism. The trick is to treat the build host parameter to TryLoadProjectInMSBuildHostAsync as optional -- once we've built the canonical state we just don't need to use it again.
Microsoft Reviewers: Open in CodeFlow