Don't try to load file based projects unless we get a .cs file #79844
Conversation
| protected override async Task<RemoteProjectLoadResult?> TryLoadProjectInMSBuildHostAsync( | ||
| BuildHostProcessManager buildHostProcessManager, string documentPath, CancellationToken cancellationToken) | ||
| { | ||
| if (Path.GetExtension(documentPath) != ".cs") |
There was a problem hiding this comment.
This check can/should be up on line 76 where we even consider whether we want to launch this. @RikkiGibson want to make a full fix here?
There was a problem hiding this comment.
And now that my work to get the miscellaneous files unit tests running on this implementation is done, this should be testable. It might be a small fix to get the existing tests catching this problem.
There was a problem hiding this comment.
I got a test okay, just couldn't get it to fail. I'm missing whatever ends up calling try apply changes I think. Maybe a rebuild of the project or something?
@RikkiGibson let know if you want me to push up what I have, in case it help.
There was a problem hiding this comment.
Go ahead, I can pull it down and triage.
|
I'm going to push directly to this PR if that's alright |
|
I tried to adjust the following test to detect the condition we are trying to prevent here, but I was not successful straight away. I suspect we would want to do something like wait for project initialization to finish, then check if the file got moved to the host workspace (we want it to not get moved to the host workspace.) |
|
@RikkiGibson what happens if you added the same "write content" and "wait for workspace" options to that test as I did here? |
|
Thanks for the speedy work @RikkiGibson, I can't approve but your change does look much cleverer than mine! I pushed up my test change to here, so as not to clobber anything in this branch just in case: 94691d9 I was expecting the assert on |
Ahh this might be where my test was going wrong. I was moving it to the host workspace, but making sure it did so as an additional document, not a real document (which is what happens in the wild) |
|
There are now 3 different bug reports this PR will fix, so I'm a strong proponent of merging this without a test, so we can get it into the next pre-release. Test can always be added in a follow up |
|
I do want to add the test and it shouldn't take long once I properly sit down with it again. But I agree it is reasonable to merge for expediency here. |
* upstream/main: (87 commits) Fix ref safety of implicit calls that might capture refs in the receiver (dotnet#76657) Update dependencies from https://github.com/dotnet/dotnet build 278961 Updated Dependencies: System.CommandLine (Version 2.0.0-rc.1.25410.101 -> 2.0.0-rc.1.25411.109) Update checklist for adding new language version (dotnet#79881) Skip ValidateAllOptions flaky test Don't try to load file based projects unless we get a .cs file (dotnet#79844) Update package restore error message. [main] Source code updates from dotnet/dotnet (dotnet#79862) Fix failure to report integration test results when retrying Also fix newly failing tests feedback More semantic update changes (dotnet#79828) Fix flow analysis of extern local functions (dotnet#79741) Allow using `/main` with top-level statements (dotnet#79577) Delete Async JTF.run Delete comment Delete code Cancel in flight work Simplify Fix race condition ...
3 participants
Fixes dotnet/vscode-csharp#8480
Fixes dotnet/vscode-csharp#8512
Fixes dotnet/vscode-csharp#8474
I've run out of time today to get a failing test, but will keep working on it :)