Renamed documents should be removed from the workspace and not cached

[eByte23]

This fixes dotnet/vscode-csharp#785

Not sure if its the best way to do it but it fixed my issue

[DustinCampbell]

This should sort of work, though it's a bit weird and pretty inefficient. I have a few concerns with this solution:

1. Simply removing the file without adding it with its new name leaves the workspace in an odd state. The file should get added later if the user performs an action in this file, but it won't appear in any results until that happens. For example, find all references won't return references from the file, diagnostics won't be reported here, etc.
2. Checking to see if a file path "contains" another path does not guarantee they are the same paths.
3. Applying changes to the workspace for every file changed is not efficient. The right thing to do for a scenario like that is to build up a solution with all of the files removed and then apply it once.

The right solution is to expand the /fileschanged end point or add a new end point that properly reports the file change that occurred. So, VS Code (and other editors) should just tell OmniSharp when a file was renamed instead of reporting that "something changed" and rely on OmniSharp to guess what happened.

[david-driscoll]

@DustinCampbell how do File Watchers look like on mono today? (Me not having a mac...)

The real fix would be to have our own file watcher, so that we can respond to these events ourselves, but the reason we did not do that, was simply because the mono file watcher had some weird limits.

[DustinCampbell]

No idea, though even the manual file watcher should have more nuance than just "a change occurred".

[eByte23]

@DustinCampbell it is a bit of a weird way to do it as i said.

Address issue, 1. It is just removing the files when they do not exists any longer and the file change events that already fire put the new document in (hence the reason for the entire issue)
2. Good point it should have been a .StartsWith same as above it
3. Yes I agree, but I wasn't sure on the best way of doing that with out changing all the existing way the file watchers work.

Happy to make changes to get it done, because this issue was a pain point for me

[eByte23]

@DustinCampbell This is still an issue in the current release. Is there anything I can do to get a fix in for this or it is already in current master?

[DustinCampbell]

Sorry for the delay @eByte23. As I mentioned before, this change should "work" most of the time, it leaves the workspace in a temporarily inconsistent state, which I'm really not a fan of. That can cause other strange issues later because the file won't get re-added unless the user actually edits it. I have a branch where I've been working on expanding this endpoint to include information about files deleted/added.

I only did a cursory review before and left some feedback above. I'll go ahead and do a full code review now.

[DustinCampbell]

unnecessary blank line.

[DustinCampbell]

these fields should be readonly

[DustinCampbell]

This is the wrong approach and can possible fail if multiple documents are removed from different projects. The reason for this is because _worksapce.TryApplyChanges(...) will update _workspace.CurrentSolution. However, the foreach loop will have received an enumerator from the original Solution.

I recommend the following approach:

1. Capture _workspace.CurrentSolution into a local variable.
2. Loop through all its projects and documents and make necessary changes to a new Solution.
3. After the loop, call _workpsace.TryApplyChanges with the new Solution if it is different than the old Solution.

Because all of this is done on immutable data structures (except the workspace itself), you shouldn't need the lock at all. Instead, it should be something like this (modulo errors because I'm tying into GitHub):

var directoryName = Path.GetDirectoryName(fileName);

var oldSolution = _workspace.CurrentSolution;
var newSolution = oldSolution;

foreach (var project in oldSolution.Projects)
{
    if (!directoryName.StartsWith(Path.GetDirectoryName(project.FilePath), StringComparison.OrdinalIgnoreCase))
    {
        continue;
    }

    foreach (var document in project.Documents)
    {
        if (fileName.Equals(document.FilePath, StringComparison.OrdinalIgnoreCase) &&
            !File.Exists(fileName))
        {
            newSolution = newSolution.RemoveDocument(document.Id);
        }
    }
}

if (oldSolution != newSolution)
{
   _workspace.TryApplyChanges(newSolution);
}

[DustinCampbell]

StringComparison.OrdinalIgnoreCase

[DustinCampbell]

This should be Equals, not Contains. I don't want to accidentally remove Foo.xaml.cs when Foo.xaml is removed. Also, StringComparison.OrdinalIgnoreCase.

[DustinCampbell]

If you change the loop to stop accessing _workspace.CurrentSolution all of the time, the call to ContainsDocument is unnecessary.

[eByte23]

@DustinCampbell Thanks for the reply and the review, when I get a chance this week a shall make the changes as you have said and do some testing.

Thanks!

[eByte23]

@DustinCampbell Sorry its been so long outstanding. I'll get onto this today and try and make the changes as requested.

Should this target Master or Dev? as I notice there was a branch target change

[willl]

@eByte23 master as dev no longer exists.

[DustinCampbell]

cc @rchande

[rchande]

@eByte23 I also took a stab at this with a slightly different approach--I chose to augment the VS Code file watching to send us notifications of file creations/deletions so we could manipulate the workspace appropriately. Let me know if you have any thoughts for me:
dotnet/vscode-csharp#1805
#987

[eByte23]

@rchande Thanks for the work! It will certainly help but in the circumstance that a fileChange event doesn't fire which sometimes may occur with file watchers as generally they have had a long history of not being 100% reliable (I could be wrong now, just from previous experience).

If we also handle a check to clean "expired" files you could say, the atleast if and event is missed then roslyn side will automatically clean up as well.

Thoughts?

[DustinCampbell]

@eByte23 : I'm going to go ahead and close this now that @rchande's PR is merged. The latest beta of C# for VS Code and the next released version include this change. Thanks for your work here!