dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.07k stars 4.04k forks source link

VisualStudio FileChangeWatcher doesn't correctly report changes for multiple projects #74716

Closed svick closed 19 minutes ago

svick commented 3 months ago

Version Used: VS 17.10.5, Roslyn main (d939cd62029b)

We have noticed that in Visual Studio our incremental source generator is not regenerating its output when an additional file (that's watched by the incremental generator) is modified by an external action (i.e. the file is changed while not being opened in VS).

I was able to trace this to Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.FileChangeWatcher and specifically to the changes from https://github.com/dotnet/roslyn/pull/70936. Since that PR, it seems FileChangeWatcher is not reporting external changes to the correct project (files from multiple projects form a batch, and changes to these files are then all reported to the same sink, which corresponds to the first project in the batch), which is the cause of our source generator issue.

This can be seen by the following test (place it inside the Microsoft.VisualStudio..LanguageServices.UnitTests project):

Imports System.IO
Imports Microsoft.CodeAnalysis.Shared.TestHooks
Imports Microsoft.CodeAnalysis.Test.Utilities
Imports Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem
Imports Roslyn.Test.Utilities
Imports IVsAsyncFileChangeEx2 = Microsoft.VisualStudio.Shell.IVsAsyncFileChangeEx2

Namespace Microsoft.VisualStudio.LanguageServices.UnitTests.ProjectSystemShim
    <UseExportProvider>
    Public Class FileChangeWatcherTests
        Implements IDisposable

        Private ReadOnly _tempPath As String

        Public Sub New()
            _tempPath = Path.Combine(TempRoot.Root, Path.GetRandomFileName())
            Directory.CreateDirectory(_tempPath)
        End Sub

        Private Sub Dispose() Implements IDisposable.Dispose
            Directory.Delete(_tempPath, recursive:=True)
        End Sub

        <WpfFact>
        Public Async Function WatchingMultipleContexts() As Task
            Using workspace = New EditorTestWorkspace()
                Dim fileChangeService = New MockVsFileChangeEx
                Dim fileChangeWatcher = New FileChangeWatcher(workspace.GetService(Of IAsynchronousOperationListenerProvider)(), Task.FromResult(Of IVsAsyncFileChangeEx2)(fileChangeService))

                Dim context1 = fileChangeWatcher.CreateContext()
                Dim context2 = fileChangeWatcher.CreateContext()

                Dim handler1Called As Boolean = False
                Dim handler2Called As Boolean = False

                AddHandler context1.FileChanged, Sub(sender, args) handler1Called = True
                AddHandler context2.FileChanged, Sub(sender, args) handler2Called = True

                Dim watchedFile1 = context1.EnqueueWatchingFile("file1.txt")
                Dim watchedFile2 = context2.EnqueueWatchingFile("file2.txt")

                Await workspace.GetService(Of AsynchronousOperationListenerProvider)().GetWaiter(FeatureAttribute.Workspace).ExpeditedWaitAsync()

                fileChangeService.FireUpdate("file2.txt")

                Assert.Equal("handler1Called=False, handler2Called=True", $"handler1Called={handler1Called}, handler2Called={handler2Called}")
            End Using
        End Function
    End Class
End Namespace

Expected: handler1Called=False, handler2Called=True
Actual: handler1Called=True, handler2Called=False

svick commented 3 months ago

cc: @ToddGrun, @jasonmalinowski

DoctorKrolic commented 3 months ago

Seems like a duplicate of https://github.com/dotnet/roslyn/issues/74395, which has already been fixed

svick commented 3 months ago

@DoctorKrolic I believe that issue is about additional files that are open and edited inside VS. This issue is about additional files that are modified by something else.

Also, the test above fails on current main.

jasonmalinowski commented 2 weeks ago

@ToddGrun This might also be related to what you're looking at.

jasonmalinowski commented 19 minutes ago

@ToddGrun fixed this in #75815.