diff --git a/src/OmniSharp.Abstractions/FileWatching/IFileSystemWatcher.cs b/src/OmniSharp.Abstractions/FileWatching/IFileSystemWatcher.cs index 200146cea4..c56771b0aa 100644 --- a/src/OmniSharp.Abstractions/FileWatching/IFileSystemWatcher.cs +++ b/src/OmniSharp.Abstractions/FileWatching/IFileSystemWatcher.cs @@ -6,6 +6,8 @@ namespace OmniSharp.FileWatching public interface IFileSystemWatcher { + void Unwatch(string pathOrExtension); + /// /// Call to watch a file or directory path for changes. /// diff --git a/src/OmniSharp.Host/FileWatching/ManualFileSystemWatcher.Callbacks.cs b/src/OmniSharp.Host/FileWatching/ManualFileSystemWatcher.Callbacks.cs deleted file mode 100644 index 05ea8db5c2..0000000000 --- a/src/OmniSharp.Host/FileWatching/ManualFileSystemWatcher.Callbacks.cs +++ /dev/null @@ -1,29 +0,0 @@ -using System.Collections.Generic; - -namespace OmniSharp.FileWatching -{ - internal partial class ManualFileSystemWatcher - { - private class Callbacks - { - private List _callbacks = new List(); - private HashSet _callbackSet = new HashSet(); - - public void Add(FileSystemNotificationCallback callback) - { - if (_callbackSet.Add(callback)) - { - _callbacks.Add(callback); - } - } - - public void Invoke(string filePath, FileChangeType changeType) - { - foreach (var callback in _callbacks) - { - callback(filePath, changeType); - } - } - } - } -} diff --git a/src/OmniSharp.Host/FileWatching/ManualFileSystemWatcher.cs b/src/OmniSharp.Host/FileWatching/ManualFileSystemWatcher.cs index ce7c22ec81..d1095e5cf2 100644 --- a/src/OmniSharp.Host/FileWatching/ManualFileSystemWatcher.cs +++ b/src/OmniSharp.Host/FileWatching/ManualFileSystemWatcher.cs @@ -5,7 +5,7 @@ namespace OmniSharp.FileWatching { - internal partial class ManualFileSystemWatcher : IFileSystemWatcher, IFileSystemNotifier + internal class ManualFileSystemWatcher : IFileSystemWatcher, IFileSystemNotifier { private readonly object _gate = new object(); private readonly Dictionary _callbacksMap; @@ -73,5 +73,43 @@ public void Watch(string pathOrExtension, FileSystemNotificationCallback callbac callbacks.Add(callback); } } + + public void Unwatch(string pathOrExtension) + { + if (pathOrExtension == null) + { + throw new ArgumentNullException(nameof(pathOrExtension)); + } + + lock (_gate) + { + if (_callbacksMap.ContainsKey(pathOrExtension)) + { + _callbacksMap.Remove(pathOrExtension); + } + } + } + + private class Callbacks + { + private readonly List _callbacks = new List(); + private readonly HashSet _callbackSet = new HashSet(); + + public void Add(FileSystemNotificationCallback callback) + { + if (_callbackSet.Add(callback)) + { + _callbacks.Add(callback); + } + } + + public void Invoke(string filePath, FileChangeType changeType) + { + foreach (var callback in _callbacks) + { + callback(filePath, changeType); + } + } + } } } diff --git a/src/OmniSharp.MSBuild/ProjectManager.cs b/src/OmniSharp.MSBuild/ProjectManager.cs index a58c491fb5..4ab29ef422 100644 --- a/src/OmniSharp.MSBuild/ProjectManager.cs +++ b/src/OmniSharp.MSBuild/ProjectManager.cs @@ -10,7 +10,6 @@ using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; using Microsoft.Extensions.Logging; -using Newtonsoft.Json.Linq; using OmniSharp.Eventing; using OmniSharp.FileWatching; using OmniSharp.Models.UpdateBuffer; @@ -18,13 +17,10 @@ using OmniSharp.MSBuild.Models.Events; using OmniSharp.MSBuild.Notification; using OmniSharp.MSBuild.ProjectFile; -using OmniSharp.Roslyn.CSharp.Services.Diagnostics; -using OmniSharp.Roslyn.CSharp.Services.Refactoring.V2; using OmniSharp.Options; using OmniSharp.Roslyn.Utilities; using OmniSharp.Services; using OmniSharp.Utilities; -using System.Reflection; using Microsoft.CodeAnalysis.Diagnostics; using OmniSharp.Roslyn.EditorConfig; @@ -37,11 +33,13 @@ private class ProjectToUpdate public ProjectIdInfo ProjectIdInfo; public string FilePath { get; } public bool AllowAutoRestore { get; set; } + public FileChangeType ChangeType { get; } public ProjectLoadedEventArgs LoadedEventArgs { get; set; } - public ProjectToUpdate(string filePath, bool allowAutoRestore, ProjectIdInfo projectIdInfo) + public ProjectToUpdate(string filePath, bool allowAutoRestore, ProjectIdInfo projectIdInfo, FileChangeType changeType) { ProjectIdInfo = projectIdInfo ?? throw new ArgumentNullException(nameof(projectIdInfo)); + ChangeType = changeType; FilePath = filePath ?? throw new ArgumentNullException(nameof(filePath)); AllowAutoRestore = allowAutoRestore; } @@ -127,7 +125,7 @@ private async Task WaitForProjectModelReadyAsync(string documentPath) if (_projectsRequestedOnDemand.TryAdd(csProjFile, 0 /*unused*/)) { var projectIdInfo = new ProjectIdInfo(ProjectId.CreateNewId(csProjFile), false); - QueueProjectUpdate(csProjFile, allowAutoRestore: true, projectId: projectIdInfo); + QueueProjectUpdate(csProjFile, allowAutoRestore: true, projectId: projectIdInfo, FileChangeType.Unspecified); } } @@ -158,10 +156,10 @@ protected override void DisposeCore(bool disposing) public IEnumerable GetAllProjects() => _projectFiles.GetItems(); public bool TryGetProject(string projectFilePath, out ProjectFileInfo projectFileInfo) => _projectFiles.TryGetValue(projectFilePath, out projectFileInfo); - public void QueueProjectUpdate(string projectFilePath, bool allowAutoRestore, ProjectIdInfo projectId) + public void QueueProjectUpdate(string projectFilePath, bool allowAutoRestore, ProjectIdInfo projectId, FileChangeType changeType = FileChangeType.Unspecified) { _logger.LogInformation($"Queue project update for '{projectFilePath}'"); - _queue.Post(new ProjectToUpdate(projectFilePath, allowAutoRestore, projectId)); + _queue.Post(new ProjectToUpdate(projectFilePath, allowAutoRestore, projectId, changeType)); } public async Task WaitForQueueEmptyAsync() @@ -193,8 +191,8 @@ private void ProcessQueue(CancellationToken cancellationToken) _processingQueue = true; try { - Dictionary projectByFilePathMap = null; - List projectList = null; + var projectByFilePathMap = new Dictionary(StringComparer.OrdinalIgnoreCase); + var projectList = new List(); while (_queue.TryReceive(out var currentProject)) { @@ -203,12 +201,6 @@ private void ProcessQueue(CancellationToken cancellationToken) break; } - if (projectByFilePathMap == null) - { - projectByFilePathMap = new Dictionary(StringComparer.OrdinalIgnoreCase); - projectList = new List(); - } - // Ensure that we don't process the same project twice. However, if a project *does* // appear more than once in the update queue, ensure that AllowAutoRestore is set to true // if any of the updates requires it. @@ -222,7 +214,11 @@ private void ProcessQueue(CancellationToken cancellationToken) continue; } - // TODO: Handle removing project + if(currentProject.ChangeType == FileChangeType.Delete) + { + RemoveProject(currentProject.FilePath); + continue; + } projectByFilePathMap.Add(currentProject.FilePath, currentProject); projectList.Add(currentProject); @@ -258,35 +254,32 @@ private void ProcessQueue(CancellationToken cancellationToken) } } - if (projectByFilePathMap != null) + foreach (var project in projectList) { - foreach (var project in projectList) - { - UpdateProject(project.FilePath); + UpdateProject(project.FilePath); - // Fire loaded events - if (project.LoadedEventArgs != null) + // Fire loaded events + if (project.LoadedEventArgs != null) + { + foreach (var eventSink in _eventSinks) { - foreach (var eventSink in _eventSinks) + try + { + eventSink.ProjectLoaded(project.LoadedEventArgs); + } + catch (Exception ex) { - try - { - eventSink.ProjectLoaded(project.LoadedEventArgs); - } - catch (Exception ex) - { - _logger.LogError(ex, "Exception thrown while calling event sinks"); - } + _logger.LogError(ex, "Exception thrown while calling event sinks"); } } } + } - foreach (var project in projectList) + foreach (var project in projectList) + { + if (_projectFiles.TryGetValue(project.FilePath, out var projectFileInfo)) { - if (_projectFiles.TryGetValue(project.FilePath, out var projectFileInfo)) - { - _packageDependencyChecker.CheckForUnresolvedDependences(projectFileInfo, project.AllowAutoRestore); - } + _packageDependencyChecker.CheckForUnresolvedDependences(projectFileInfo, project.AllowAutoRestore); } } } @@ -348,9 +341,10 @@ private bool RemoveProject(string projectFilePath) if (!_workspace.TryApplyChanges(newSolution)) { _logger.LogError($"Failed to remove project from workspace: '{projectFileInfo.FilePath}'"); + return false; } - // TODO: Stop watching project files + UnwatchProjectFiles(projectFileInfo); return true; } @@ -378,11 +372,9 @@ private void AddProject(ProjectFileInfo projectFileInfo) private void WatchProjectFiles(ProjectFileInfo projectFileInfo) { - // TODO: This needs some improvement. Currently, it tracks both deletions and changes - // as "updates". We should properly remove projects that are deleted. _fileSystemWatcher.Watch(projectFileInfo.FilePath, (file, changeType) => { - QueueProjectUpdate(projectFileInfo.FilePath, allowAutoRestore: true, projectFileInfo.ProjectIdInfo); + QueueProjectUpdate(projectFileInfo.FilePath, allowAutoRestore: true, projectFileInfo.ProjectIdInfo, changeType); }); if (_workspace.EditorConfigEnabled) @@ -413,38 +405,50 @@ private void WatchProjectFiles(ProjectFileInfo projectFileInfo) { _fileSystemWatcher.Watch(projectFileInfo.RuleSet.FilePath, (file, changeType) => { - QueueProjectUpdate(projectFileInfo.FilePath, allowAutoRestore: false, projectFileInfo.ProjectIdInfo); + QueueProjectUpdate(projectFileInfo.FilePath, allowAutoRestore: false, projectFileInfo.ProjectIdInfo, changeType); }); } - if (!string.IsNullOrEmpty(projectFileInfo.ProjectAssetsFile)) + foreach(var projectAssetFile in GetProjectAssetFilePaths(projectFileInfo)) { - _fileSystemWatcher.Watch(projectFileInfo.ProjectAssetsFile, (file, changeType) => + _fileSystemWatcher.Watch(projectAssetFile, (file, changeType) => { - QueueProjectUpdate(projectFileInfo.FilePath, allowAutoRestore: false, projectFileInfo.ProjectIdInfo); + QueueProjectUpdate(projectFileInfo.FilePath, allowAutoRestore: false, projectFileInfo.ProjectIdInfo, changeType); }); + } + } - var restoreDirectory = Path.GetDirectoryName(projectFileInfo.ProjectAssetsFile); - var nugetFileBase = Path.Combine(restoreDirectory, Path.GetFileName(projectFileInfo.FilePath) + ".nuget"); - var nugetCacheFile = nugetFileBase + ".cache"; - var nugetPropsFile = nugetFileBase + ".g.props"; - var nugetTargetsFile = nugetFileBase + ".g.targets"; + private void UnwatchProjectFiles(ProjectFileInfo projectFileInfo) + { + _fileSystemWatcher.Unwatch(projectFileInfo.FilePath); - _fileSystemWatcher.Watch(nugetCacheFile, (file, changeType) => - { - QueueProjectUpdate(projectFileInfo.FilePath, allowAutoRestore: false, projectFileInfo.ProjectIdInfo); - }); + if (projectFileInfo.RuleSet?.FilePath != null) + { + _fileSystemWatcher.Unwatch(projectFileInfo.RuleSet.FilePath); + } - _fileSystemWatcher.Watch(nugetPropsFile, (file, changeType) => - { - QueueProjectUpdate(projectFileInfo.FilePath, allowAutoRestore: false, projectFileInfo.ProjectIdInfo); - }); + foreach(var projectAssetFile in GetProjectAssetFilePaths(projectFileInfo)) + { + _fileSystemWatcher.Unwatch(projectAssetFile); + } + } - _fileSystemWatcher.Watch(nugetTargetsFile, (file, changeType) => - { - QueueProjectUpdate(projectFileInfo.FilePath, allowAutoRestore: false, projectFileInfo.ProjectIdInfo); - }); + private static IEnumerable GetProjectAssetFilePaths(ProjectFileInfo projectFileInfo) + { + if (string.IsNullOrEmpty(projectFileInfo.ProjectAssetsFile)) + { + return new string[] {}; } + + var restoreDirectory = Path.GetDirectoryName(projectFileInfo.ProjectAssetsFile); + var nugetFileBase = Path.Combine(restoreDirectory, Path.GetFileName(projectFileInfo.FilePath) + ".nuget"); + return new[] + { + projectFileInfo.ProjectAssetsFile, + nugetFileBase + ".cache", + nugetFileBase + ".g.props", + nugetFileBase + ".g.targets" + }; } private void UpdateProject(string projectFilePath) @@ -661,7 +665,7 @@ private void UpdateProjectReferences(Project project, ImmutableArray pro referencedProject = ProjectFileInfo.CreateNoBuild(projectReferencePath, _projectLoader, _dotNetInfo); AddProject(referencedProject); - QueueProjectUpdate(projectReferencePath, allowAutoRestore: true, referencedProject.ProjectIdInfo); + QueueProjectUpdate(projectReferencePath, allowAutoRestore: true, referencedProject.ProjectIdInfo, FileChangeType.Unspecified); } } diff --git a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs index 8b35a3c36c..ebb2f039a5 100644 --- a/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs +++ b/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs @@ -168,9 +168,10 @@ private void OnWorkspaceChanged(object sender, WorkspaceChangeEventArgs changeEv QueueForAnalysis(ImmutableArray.Create(changeEvent.DocumentId), AnalyzerWorkType.Foreground); break; case WorkspaceChangeKind.DocumentRemoved: - if (!_currentDiagnosticResultLookup.TryRemove(changeEvent.DocumentId, out _)) + if(_currentDiagnosticResultLookup.TryRemove(changeEvent.DocumentId, out _)) { - _logger.LogDebug($"Tried to remove non existent document from analysis, document: {changeEvent.DocumentId}"); + var existingDocumentsInProject = _workspace.CurrentSolution.GetProject(changeEvent.ProjectId).Documents.Select(x => x.Id).ToImmutableArray(); + QueueForAnalysis(existingDocumentsInProject, AnalyzerWorkType.Background); } break; case WorkspaceChangeKind.ProjectAdded: diff --git a/tests/OmniSharp.Cake.Tests/LineIndexHelperFacts.cs b/tests/OmniSharp.Cake.Tests/LineIndexHelperFacts.cs index 4446050a52..9d90e63da3 100644 --- a/tests/OmniSharp.Cake.Tests/LineIndexHelperFacts.cs +++ b/tests/OmniSharp.Cake.Tests/LineIndexHelperFacts.cs @@ -151,6 +151,10 @@ public async Task TranslateFromGenerated_Should_Translate_To_Negative_If_Outside private class DummyFileSystemWatcher : IFileSystemWatcher { + public void Unwatch(string pathOrExtension) + { + } + public void Watch(string pathOrExtension, FileSystemNotificationCallback callback) { } diff --git a/tests/OmniSharp.MSBuild.Tests/ProjectLoadTestEventEmitter.cs b/tests/OmniSharp.MSBuild.Tests/ProjectLoadTestEventEmitter.cs index 1789d17378..286a4608d5 100644 --- a/tests/OmniSharp.MSBuild.Tests/ProjectLoadTestEventEmitter.cs +++ b/tests/OmniSharp.MSBuild.Tests/ProjectLoadTestEventEmitter.cs @@ -36,9 +36,9 @@ public ExportDescriptorProvider[] AsExportDescriptionProvider(ILoggerFactory log public void Emit(string kind, object args) { - if(args is ProjectConfigurationMessage) + if(args is ProjectConfigurationMessage message) { - ReceivedMessages = ReceivedMessages.Add((ProjectConfigurationMessage)args); + ReceivedMessages = ReceivedMessages.Add(message); _messageEvent.Set(); } } diff --git a/tests/OmniSharp.MSBuild.Tests/ProjectRemovalTests.cs b/tests/OmniSharp.MSBuild.Tests/ProjectRemovalTests.cs new file mode 100644 index 0000000000..6375e24716 --- /dev/null +++ b/tests/OmniSharp.MSBuild.Tests/ProjectRemovalTests.cs @@ -0,0 +1,69 @@ +using System.IO; +using System.Linq; +using System.Threading.Tasks; +using OmniSharp.FileWatching; +using OmniSharp.Models.Diagnostics; +using OmniSharp.Models.FilesChanged; +using TestUtility; +using Xunit; +using Xunit.Abstractions; +using static OmniSharp.MSBuild.Tests.ProjectLoadListenerTests; + +namespace OmniSharp.MSBuild.Tests +{ + public class ProjectRemovalTests : AbstractMSBuildTestFixture + { + public ProjectRemovalTests(ITestOutputHelper output) + : base(output) + { + } + + [Fact] + public async Task WhenProjectIsRemoved_RemoveItFromWorkspace() + { + var emitter = new ProjectLoadTestEventEmitter(); + + using (var testProject = await TestAssets.Instance.GetTestProjectAsync("ProjectWithAnalyzers")) + using (var host = CreateMSBuildTestHost(testProject.Directory, emitter.AsExportDescriptionProvider(LoggerFactory))) + { + var csprojFile = Path.Combine(testProject.Directory, "ProjectWithAnalyzers.csproj"); + await NotifyFileremoved(host, csprojFile); + + emitter.WaitForProjectUpdate(); + + Assert.Empty(host.Workspace.CurrentSolution.Projects); + } + } + + [Fact] + public async Task WhenNewAnalyzerReferenceIsAdded_ThenAutomaticallyUseItWithoutRestart() + { + var emitter = new ProjectLoadTestEventEmitter(); + + using (var testProject = await TestAssets.Instance.GetTestProjectAsync("ProjectWithAnalyzers")) + using (var host = CreateMSBuildTestHost(testProject.Directory, emitter.AsExportDescriptionProvider(LoggerFactory), configurationData: TestHelpers.GetConfigurationDataWithAnalyzerConfig(roslynAnalyzersEnabled: true))) + { + var csprojFile = Path.Combine(testProject.Directory, "ProjectWithAnalyzers.csproj"); + + await NotifyFileremoved(host, csprojFile); + + emitter.WaitForProjectUpdate(); + + var diagnostics = await host.RequestCodeCheckAsync(Path.Combine(testProject.Directory, "Program.cs")); + + Assert.Empty(diagnostics.QuickFixes); + } + } + + private static async Task NotifyFileremoved(OmniSharpTestHost host, string file) + { + await host.GetFilesChangedService().Handle(new[] { + new FilesChangedRequest() { + FileName = file, + ChangeType = FileChangeType.Delete + } + }); + } + + } +}