-
Notifications
You must be signed in to change notification settings - Fork 172
Handle temporary projects distinctly from user projects #724
Conversation
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'll post my comments that I accumulated in case we want to come back to this. I'll see what I can come up with and will catch up offline.
Thanks for the review, @bettinaheim! Please let me know how we should move forward with this. |
/// To be used whenever a temporary project file is added. | ||
/// Note that by calling this routine, all processing will be blocked until loading has finished... | ||
/// </summary> | ||
public Task LoadTemporaryProjectAsync( |
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.
@rmshaffer Before we merge this, since we don't want to keep this in the public API:
public Task LoadTemporaryProjectAsync( | |
private Task LoadTemporaryProjectAsync( |
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.
Edit: I saw that the language server calls into this. That's rather unfortunate. It would be good to mark it such that people don't use it - as awkward as it is, I'd suggest marking it with an Obsolete attribute anyway.
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.
Is there a way to correct the public API surface, then, so that we don't have to resort to that? If I understood our discussions correctly, ProjectManager
needs to have some way of identifying these temporary projects so that it can treat them distinctly. And so no matter what we choose, there will be some public contract between LanguageServer
, which creates those temporary projects, and ProjectManager
, which loads them.
Or am I misunderstanding - is your idea that we should ideally treat the temporary projects no differently from ordinary projects?
The language server for VS and VS Code creates a temporary project on disk whenever a
.qs
file is opened that isn't associated with a project.However, there is a race condition when opening a folder with a
.csproj
and quickly opening or creating a.qs
file before the.csproj
has been loaded by the language server. In this case, a temporary project gets created and loaded, and subsequently the on-disk.csproj
is loaded, which results in the file being associated with two compilation units simultaneously. This causes errors in the language server, which does not support this case.The fix in this PR is for
ProjectManager
to keep separate lists of "user projects" and "temporary projects". When looking for a compilation unit manager for a given.qs
file, it always checks the "user projects" list first, then falls back to the "temporary projects" list, and finally falls back to the "default manager" (which provides limited syntax checking only).Other fixes considered:
.csproj
in the folder is then removed, this would leave the temporary project in an inconsistent state..csproj
is loaded that references the same files. This similarly has a bunch of edge cases to consider, and won't work correctly if the.csproj
is then removed.Fixes #613, and fixes the worst instances of #720.
This PR also re-enables the temporary .csproj feature by reverting the changes from PR #733.