After using #r on a non-existent .dll, creating that .dll requires document change to update diagnostics

[dsyme]

F# files are not re-checked when changes happen to dependency files such as #load-ed scripts or referenced DLLs.

This greatly affects the usability of the Visual F# Tools as the user has no idea what state the type checking is in, and has to do artificial edits on their source code to clear errors

This is a regression to functionality that has been working since VS2008

For each F# source file (in or out of a project, whether script or otherwise), the compiler service provides a list of files in the DependencyFiles property. These are DLLs and source files. The IDE should install file watchers for these files, and if a change in a watched file is detected then a re-check of the file should be done, resulting in a refresh of the errors

For example,

• if an error is corrected in a file referenced by a #load, and that file is saved to disk, then the red-squiggly on the #load should disappear

• Assume one project references another (e.g. fsc references FSharp.Compiler). If the DLLs for the referenced did not exist on disk, then there will be red-squigglies in the project. When the referenced project gets built, DLLs are created by a build. These DLLs will be in the dependency files. When the files appear on disk, a re-check should occur, and the red squigglies should be cleared.

Repro steps

1. There are many repros. For example, when scripting, create a script1.fsx with this content and save it to an empty directory.

#load "script2.fsx"

System.Console.WriteLine("Hello")

You will see a red-squiggly on script2.fsx

2. Now create an empty script2.fsx using VS or any other tool.

   Expected: the red-squiggly should disappear in script1.fsx when you return to that file (without making any edits to script1.fsx)

3. Now edit script2.fsx using so there is an error in the file. e.g. the contents

   let fail = 1 + 1.0
   

   Save the file and go back to the script1.fsx window.

   Expected: the red-squiggly should re-appear in script1.fsx when you return to that file (without making any edits to script1.fsx)

4. Now edit script2.fsx to clear the error and save the file. Go back to the script1.fsx window.

   Expected: the red-squiggly should disappear from script1.fsx when you return to that file (without making any edits to script1.fsx)

Similarly for cross-project references to DLLs in project builds

Actual behavior

In all the cases above, the red-squigglies are not cleared in script1.fsx until you actually edit script1.fsx (which has the effect of forcing a re-check of the file)

[cartermp]

Thanks for the detailed bug, @dsyme. @brettfo and @Pilchie, I think this one should be high on our list of bugs to fix right now.

[dsyme]

Here are some notes about the old implementation that has been lost

• Assume file B has been checked and has DependencyFiles A1 and A2. (Note A1 and A2 might not yet exist)

• In the old language service code, the DependencyFiles for B were consumed here after checking

• This used the Visual Studio file watching facilities here to install watchers for A1 and A2. (We had to use the VIsual Studio file watching for technical reasons I never understood)

• When a file change to A1 or A2 was detected by the VS file watching facilitieswe called NotifyFileTypeCheckStateIsDirty which stored the file away in dirtyForTypeCheckFiles

• When a file creation to A1 or A2 was detected then we called InvalidateConfiguration to notify the F# compiler service that the whole project should be re-checked. This corresponds to creation of one of the source files of a project, or creation of one of the DLLs in a project.

• IIRC we had to be careful not to listen to DLL deletions since that gives the very annoying behaviour of a "clean" build creating lots of red-squiggly errors

• OnIdle processing then marked any matching files in the RunningDocumentTable as if they had been edited. THis would have the effect of marking file B as "edited" from the point of view of the language service, and it would be rechecked, and the errors cleared.

These fixes are also crucial for correctly updating intellisense information in response to changes in dependent files.

Separately

• NotifyFileTypeCheckStateIsDIrty is also called when a BeforeBackgroundFileCheck event is raised by the background checker This indicates that refreshed background state is available ready to do an updated check of the file. This is crucial to ensure clearing of foreground errors and updates to intellisense information in response to progress in the background build.

• It is also worth reviewing whether CheckProjectInBackground is ever being called in the new language service implementation In the old language service, this was be called each time you switched to a file in a project in Visual Studio (or equally to a script). This would have the effect of kicking off the background project build for the project, causing BeforeBackgroundFileCheck events to be raised file by file as the background build progressed through the files. Without calling CheckProjectInBackground you might not get proper updates in multi-project solutions.

[dsyme]

Note the model described above is not ideal, especially for multi-project solutions (where you shouldn't have to even have referenced DLLs on-disk to check a project). Further, the implementation shouldn't need to do a "foreground" check to refresh errors - they should be pushed from the background build, which should utilize the active, unsaved content of buffers rather than files on disk. The F# Compiler service has various updates which make it possible to do a better job along these lines.

But in VS2017 RC we have no updates in response to file events at all.... So let's first get back to parity and then progress things.

[cartermp]

Agreed.

[dsyme]

I'm having a hell of a hard time understanding how to implement this within the Roslyn framework.

One thing we need is the equivalent of this

    member this.OnIdle() = 
        for file in dirtyForTypeCheckFiles do
            let rdt = this.ServiceProvider.RunningDocumentTable
            match this.ProjectSitesAndFiles.TryGetSourceOfFile(rdt, file) with
            | Some source -> source.RecordChangeToView()
            | None ->  ()
        dirtyForTypeCheckFiles <- Set.empty

That is, when Idle we want to run through a set of documents and "act as if the document has changed" for the purposes of Roslyn semantic analysis.

Another thing we need is a place where we can register and store VS file tracking handlers.

I hacked about with this sort of thing:

        // Add a hook so that when the background builder refreshes the background semantic build for a file, we refresh
        // the editor.
        checker.BeforeBackgroundFileCheck.Add(fun (fileName, extraProjectInfo) ->  
           async {
            try 
                match extraProjectInfo with 
                | Some (:? Workspace as workspace) -> 
                    let solution = workspace.CurrentSolution
                    let documentIds = solution.GetDocumentIdsWithFilePath(fileName)
                    if not documentIds.IsEmpty then 
                        let documentId = documentIds |> Seq.head
                        let document = solution.GetDocument(documentId)
                        let! sourceText = document.GetTextAsync() |> Async.AwaitTask
                        let newSolution = solution.WithDocumentText(documentId, sourceText (* , PreservationMode.PreserveIdentity *))
                        workspace.TryApplyChanges(newSolution) |> ignore
                | _ -> ()
            with ex -> 
                Assert.Exception(ex)
            } |> Async.StartImmediate
        )

I'll definitely need help for this

[vladima]

you can request projects or documents to be re-checked by importing IDiagnosticsAnalyzerService and calling Reanalyze

[dsyme]

@vladima Thanks!!

[dsyme]

@vladima @OmarTawfik @brettfo In the old F# Language Service code (actually FSharp.LanguageService.Base, in C#) we implemented our own ViewFilter type that includes an implementation of IVsTextViewEvents including an action when the window got the focus:

        public virtual void OnSetFocus(IVsTextView view) {
            this.service.OnActiveViewChanged(view);
            ...
        }

Do you know what the equivalent should be in the Roslynized code?

The context is that we basically want to kick off a call to checker.CheckProjectInBackground() when a file in a project gets the focus. This will then make sure all errors eventually get updated.

We don't explicitly implement IVsTextViewEvents in the new code.

thanks
don

[vladima]

I guess something like this might work (browser compiled solution, might contain peanuts errors)

open System.ComponentModel.Composition
open Microsoft.VisualStudio.Editor
open Microsoft.VisualStudio.Utilities
open Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem
open Microsoft.CodeAnalysis.Diagnostics

[<ContentType(FSharpCommonConstants.FSharpContentTypeName)>]
[<Export(typeof<IVsTextViewCreationListener>)>]
type FSharpTextViewCreationListener 
        [<ImportingConstructor>]
        (
            textViewAdapter: IVsEditorAdaptersFactoryService,
            workspace: VisualStudioWorkspaceImpl, 
            analyzerService: IDiagnosticAnalyzerService
        ) =
    interface IVsTextViewCreationListener with
        member this.VsTextViewCreated(textView) =
            let wpfTextView = textViewAdapter.GetWpfTextView(textView)
            match wpfTextView.TextBuffer.Properties.TryGetProperty<ITextDocument>(typeof<ITextDocument>) with
            | true, textDocument ->
                let filePath = textDocument.FilePath
                let onGotFocus = new EventHandler(fun _ _ ->
                    analyzerService.Reanalyze(workspace, projectIds = null, documentIds = workspace.CurrentSolution.GetDocumentIdsWithFilePath(filePath))
                )
                let rec onViewClosed = new EventHandler(fun _ _ -> 
                    wpfTextView.GotAggregateFocus.RemoveHandler(onGotFocus)
                    wpfTextView.Closed.RemoveHandler(onViewClosed)
                )
                wpfTextView.GotAggregateFocus.AddHandler(onGotFocus)
                wpfTextView.Closed.AddHandler(onViewClosed)
            | _ -> ()

[Pilchie]

Roslyn already has a service that tracks the active document, and in theory kicks of re-analysis through the Diagnostic analyzer. @heejaechang - can you point us to that code?

[heejaechang]

does F# do error reporting based on roslyn diagnostic service, then it does all dependency checking and priority queue on changes and etc.

[heejaechang]

these make sure document that has focus get processed first and then opened files and then modified files and etc.

http://source.roslyn.io/#Microsoft.CodeAnalysis.Features/SolutionCrawler/WorkCoordinator.HighPriorityProcessor.cs,70
http://source.roslyn.io/#Microsoft.CodeAnalysis.Features/SolutionCrawler/WorkCoordinator.NormalPriorityProcessor.cs,271

[heejaechang]

it sounds like F# need to abstract this out - http://source.roslyn.io/#Microsoft.CodeAnalysis.Features/SolutionCrawler/WorkCoordinator.SemanticChangeProcessor.cs,32

and make it to enqueue right set of work items to propagate semantic changes for F#.

this doesn't do anything for F# I believe because F# doesn't actually use roslyn's semantic model and current implementation only understand how to propagate semantic changes for ones that provide roslyn's semantic model.

[dsyme]

@vladima Thanks for your code, it worked well, please see PR

[cartermp]

@dsyme Is it safe to close this with your PR?

[dsyme]

No, there is a still a case of propagating changes even when there's been no change in focus to "kick things off" , but files have come/gone from disk.

Basically I believe the F# IncrementalBuilder will correctly notice all dependent file changes if activated to run. But we are relying on regaining the focus to prompt it to run at all.

But this problem is much less severe after the PR

[dsyme]

if moving mouse can cause tagger to change, looks like we don't know the dll has changed (created). looks like file tracker issue.

@heejaechang Yes, but what I'm wondering is whether Roslyn actually includes a file tracker - e.g. does it react to on-disk file change events for referenced DLLs (e.g. not including cross-project references, which let's assume are handled in-memory). I haven't checked the Roslyn source code, it's just a general question

In VS2015 the Visual F# Tools used to install Visual Studio file change event listeners as described here : #1808 (comment). Right now in VS2017
the Visual F# Tools aren't doing any explicit file-change tracking - what I'm wondering is if we should implement it ourselves like we did in VS2015 or if we get some for free from setting up Roslyn metadata references correctly.

[Pilchie]

e.g. does it react to on-disk file change events for referenced DLLs

Yes see VisualStudioMetadataReference, and it's _fileChangeTracker field.

[dsyme]

@Pilchie thanks. A couple of questions

• is the result of a detected change effectively a Reanalzye call?

• We don't currently set up a Roslyn project for F# scripts. We could do that, but what about #load "SomeFile.fs" references in scripts? We should react to changes in "SomeFile.fs" as well. But these are normal F# source code files, rather than .NET DLLs. Does that matter? Should we also use some variation on VisualStudioMetadataReference, or are those only for .NET DLLs?

[Pilchie]

is the result of a detected change effectively a Reanalzye call?

It should (eventually) result in a WorkspaceChanged event with a project updated. I'm not sure which Reanalyze call you mean, so I can't say what the path to that is, and whether it's hooked up for F#.

Note that C# also has #load in csx files, and I remember at least talking about how to deal with it, though I'm not sure if we implemented it. I think they right way handle it is to create a language service project, and add the #loaded files to it. We also hook up file change notifications for closed source files, so the rest should work. Also cc'ing @jasonmalinowski and @tmat to double check my thoughts here.

[Pilchie]

Oh, hey, look at dotnet/roslyn#21964. Maybe we don't handle it properly for C# either.

[dsyme]

I'm closing this very old issue - @TIHan implemented this correctly AFAIU