-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Implement canonical miscellaneous files project loader for non-file-based programs #80748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Made LanguageServerProjectLoader.TransitionPrimordialProjectToLoadedAsync abstract to allow subclasses to customize primordial project transition - Added ExecuteUnderGateAsync helper methods to allow subclasses to safely access loaded projects state - Made ProjectLoadState protected to allow subclass access - Implemented CanonicalMiscFilesProjectLoader that handles non-file-based-program misc files - Integrated canonical loader into FileBasedProgramsProjectSystem with lazy initialization - Updated FileBasedProgramsProjectSystem and LanguageServerProjectSystem to implement new abstract method Co-authored-by: dibarbet <[email protected]>
...r/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/CanonicalMiscFilesProjectLoader.cs
Outdated
Show resolved
Hide resolved
...r/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/CanonicalMiscFilesProjectLoader.cs
Outdated
Show resolved
Hide resolved
...r/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/CanonicalMiscFilesProjectLoader.cs
Outdated
Show resolved
Hide resolved
...r/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/CanonicalMiscFilesProjectLoader.cs
Outdated
Show resolved
Hide resolved
...r/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/CanonicalMiscFilesProjectLoader.cs
Outdated
Show resolved
Hide resolved
...er/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/FileBasedProgramsProjectSystem.cs
Outdated
Show resolved
Hide resolved
...er/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/FileBasedProgramsProjectSystem.cs
Outdated
Show resolved
Hide resolved
...er/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/FileBasedProgramsProjectSystem.cs
Outdated
Show resolved
Hide resolved
...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs
Outdated
Show resolved
Hide resolved
...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs
Outdated
Show resolved
Hide resolved
- Changed _canonicalProjectPath and _canonicalDocumentPath to single Lazy tuple field - Updated AddMiscellaneousDocumentAsync to verify LoadedProject with canonical path and assert single target - Changed method signatures to accept string documentPath instead of DocumentUri - Removed unnecessary null check for canonical paths (handled by Lazy) - Removed all references to host workspace, always use misc factory - Changed warnings to Contract.Fail when project not found - Changed FirstOrDefault to Single and removed null checks - Made helper methods non-nullable, removed null returns - Simplified XML to basic class library template - Removed GetCanonicalArtifactsPath method - Put canonical loader behind EnableFileBasedPrograms option - Inlined isVirtual variable in if statement - Removed comments about "original behavior" - Removed non-async version of ExecuteUnderGateAsync Co-authored-by: dibarbet <[email protected]>
f4f827f to
caa4ae8
Compare
14fe937 to
e82de6a
Compare
e82de6a to
f290d4f
Compare
...r/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/CanonicalMiscFilesProjectLoader.cs
Show resolved
Hide resolved
...ageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/FileBasedProgramsWorkspaceTests.cs
Show resolved
Hide resolved
...ageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/FileBasedProgramsWorkspaceTests.cs
Show resolved
Hide resolved
...r/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/CanonicalMiscFilesProjectLoader.cs
Show resolved
Hide resolved
...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LoadedProject.cs
Outdated
Show resolved
Hide resolved
| _logger = loggerFactory.CreateLogger<FileBasedProgramsProjectSystem>(); | ||
| _projectXmlProvider = projectXmlProvider; | ||
| // Lazily create the canonical misc files loader | ||
| _canonicalMiscFilesLoader = new Lazy<CanonicalMiscFilesProjectLoader>(() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this somewhat suspect, it doesn't seem like one project system should "contain" another project system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another potential path is to create multiple misc files provider. But that is a bit of a larger refactoring as we have to update the rest of the queue and providers to be chained so to speak. Can look into it if you feel strongly about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to take another dive into this area soon (perhaps checking out the change, stepping through some things, observing changes in state over time, etc.), and follow up if I have any ideas to make things simpler and more robust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the last bit I am not quite satisfied with. It's possible it is fine, I just need to put a little more scrutiny on it. I was wondering, is the Lazy partially functioning to break a cycle during initialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No - it can very well not be lazy now actually. Artifact of when the ctor actually did something. I will make it not lazy, but not sure that covers everything you were thinking about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think generally the right path here is to allow multiple misc files providers. The LSPWorkspaceManager would be responsible for iterating them to see which one could handle the document (likely ordered, with FBP first, then canonical).
Then the FBP provider here doesn't need to know about the canonical provider.
It's a little larger change though and I'd prefer to do in a followup if you are OK with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The state management of all the different ways a file can be misc over time, is pretty hard, and severely under-tested. It will be great to be able to put more attention into that area.
I think just getting unstuck on bringing in more test coverage, how to represent more complex scenarios in the test harness, etc, will do us an immense amount of good.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LoadedProject.cs
Outdated
Show resolved
Hide resolved
...ageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/FileBasedProgramsWorkspaceTests.cs
Show resolved
Hide resolved
...ageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/FileBasedProgramsWorkspaceTests.cs
Show resolved
Hide resolved
...ageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/FileBasedProgramsWorkspaceTests.cs
Show resolved
Hide resolved
...r/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/CanonicalMiscFilesProjectLoader.cs
Outdated
Show resolved
Hide resolved
...er/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/FileBasedProgramsProjectSystem.cs
Outdated
Show resolved
Hide resolved
...er/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/FileBasedProgramsProjectSystem.cs
Show resolved
Hide resolved
|
What occurs to me is: we don't have a need to continually update the LoadedProject for the canonical misc file and such. We don't need to do more DTBs of it over time and so on. Therefore I suspect that what we should actually do, is: keep a single field in FBPProjectSystem of type ProjectLoadState, which essentially holds the DTB state of our canonical misc project. Once it transitions to Loaded, we basically never touch it any more. We update the workspace only based on files being added/removed and that is fine and good. We can extract the bits from the base which allow us to respond to the DTB completion and transition the state of the field. Basically I think there are so many steps that should not happen the same way for the canonical, as they do for ordinary projects or FBPs, that we might just want to write a completely new code path which simply doesn't include all the stuff we don't need to do.
Observations from trying to run the language server from the PR branch (some of this is repetition from the previous section)
|
It's definitely possible to do, but we would end up keeping a decent amount of the logic from the abstract base project system (e.g. transitioning the project). imho its kind of a wash either way, the amount of code we can reuse from the base proj system is similarish to the amount that doesn't apply.
I can definitely get behind this.
I don't think we should use it for multiple sessions. What happens if you have two different instances of VSCode trying to access the canonical project? It may work fine, but you seems likely to cause file locking headaches and other issues. Think its completely reasonable to have each instance use its own and clean it up when done.
Not seeing this - a restore of the canonical project?
Unrelated and existing issue dotnet/vscode-csharp#8675 |
There is a need to reload after the restore completes. So I don't think it would cut away as much as I was speculating.
I buy what you are saying here. I think the tooling should be able to tolerate multiple instances being open, especially something like VS and VS Code having the same solution open at the same time. But I get that it tends to cause problems. re: the endless restore, and the misc files not seeing each other--I will try to repro and see what might have been going on with that. |
12e102d to
e3ef2f0
Compare
This requires some additional changes. The popup is managed client side and needs client side changes. However I have a plan to move the popup management entirely server-side (using work done progress), which would allow us to suppress it only on the server. Would prefer to handle that in a separate PR. RIght now the popup for the canonical project should only show up once per session, so isn't too bad of an experience. |
RikkiGibson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change LGTM. Looking forward to more goodness in follow ups.
|
merging so I can work on followups |

Resolves #80743
Resolves dotnet/vscode-csharp#7518
Resolves dotnet/vscode-csharp#7517
Implementation Plan for Canonical Miscellaneous Files Project Loader
Overview
Implementing issue #80743 to create a
CanonicalMiscFilesProjectLoaderthat handles non-file-based-program miscellaneous files using a canonical project approach.Completed ✅
LanguageServerProjectLoaderto support custom primordial project handlingCanonicalMiscFilesProjectLoaderclassCanonicalMiscFilesProjectLoaderwithFileBasedProgramsProjectSystemKey Changes
Implementation Highlights
Improvements from feedback:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.