Map workspace to iprojectsite #3425

KevinRansom · 2017-08-08T22:29:32Z

Use the Roslyn workspace as a datasource for source files and referenced projects for the F# Editor language service.

Note: as of right now the source files are not in project file order.

Pilchie · 2017-08-09T21:07:33Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

    override this.Initialize() =
        base.Initialize()

+        let workspaceChanged (args:WorkspaceChangeEventArgs) =
+            match args.Kind with
+            | WorkspaceChangeKind.ProjectAdded -> this.OnProjectAdded(args.ProjectId,   this.Workspace, args.NewSolution)


I don't think Roslyn will fire ProjectAdded events for all the projects in a solution when SolutionAdded is fired, so we probably need to handle the Solution events too.

@Pilchie weirdly we do get project added events. I want to dig into these quite a bit more though.

dsyme

Great work - the approach looks right. Some changes requested

dsyme · 2017-08-18T11:15:10Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

@@ -32,6 +34,12 @@ open Microsoft.VisualStudio.Shell.Interop
 open Microsoft.VisualStudio.FSharp.LanguageService
 open Microsoft.VisualStudio.ComponentModelHost

+
+module LogFile =
+    let log (text:string) =


Please make this either #if DEBUG or use System.Diagnostics.Trace

dsyme · 2017-08-18T11:16:09Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

+        referencePaths |> Seq.iter(fun r -> LogFile.log r)
+        LogFile.log "======================================="
+
+        lock lockObject (fun () -> match sourceFiles.TryGetValue path with


Normal formatting would use 4-space indenting for this, like this:

lock lockObject (fun () -> match sourceFiles.TryGetValue path with | true, _ -> sourceFiles.Remove(path) |> ignore sourceFiles.Add(path, sourcePaths) ... )

dsyme · 2017-08-18T11:17:56Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

    let projectTable = ConcurrentDictionary<ProjectId, Refreshable<ProjectId[] * FSharpProjectOptions>>()

    // A table of information about single-file projects.  Currently we only need the load time of each such file, plus
    // the original options for editing
    let singleFileProjectTable = ConcurrentDictionary<ProjectId, DateTime * FSharpProjectOptions>()

+    // Accumulate sorces and references for each project file
+    let lockObject = new Object()


Normal name for lockObject is syncObj

dsyme · 2017-08-18T11:19:25Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

+        referencePaths |> Seq.iter(fun r -> LogFile.log r)
+        LogFile.log "======================================="
+
+        lock lockObject (fun () -> match sourceFiles.TryGetValue path with


Use sourceFiles.ContainsKey rather than TryGetValue

Actually I believe you can just do sourceFiles.[path] <- sourcePaths - you don't need o explicitly Remove nor check if the collection already contains the value - the indexer will do what you need

dsyme · 2017-08-18T11:20:54Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

+    member __.GetSourceFiles(path:string) = 
+        lock lockObject (fun () -> match sourceFiles.TryGetValue path with
+                                   | true, value -> value
+                                   | _           -> Array.empty<string>)


Normal F# practice is to use [| |] for Array.empty, or to use just Array.empty without the type annotation (which is inferred)

dsyme · 2017-08-18T11:32:41Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs


            projectId
        setup (siteProvider.GetProjectSite()) |> ignore

+    member private __.ProvideProjectSiteProvider(workspace:Workspace, project:Project) =


I think this should be a helper function either outside this class (to prove that it is logically independent of any of the constructs accessible in the class), or as a let function in the class let section, to help show what data it depends in the initialization sequence. Perhaps it can even go in ProjectSitesAndFiles.fs where two other ways of creating IProjectSite are implemented (for scripts and standalone-files).

dsyme · 2017-08-18T11:42:22Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

+    // Accumulate sorces and references for each project file
+    let lockObject = new Object()
+    let sourceFiles = new Dictionary<string, string[]>()
+    let referenceFiles = new Dictionary<string, string[]>()


Why have two tables for the information? Just use

let projectInfoFromCommandLineUpdates = new ConcurrentDictionary<string, string[] * string[]>()

or even

let projectInfoFromCommandLineUpdates = new ConcurrentDictionary<string, ImmutableArray<CommandLineSourceFile> * ImmutableArray<CommandLineReference>()

I think I prefer the second - just snapshot the full information we're given since it may be useful in debugging.

Also, in a recent update I renamed ProjectInfoManager to FSharpProjectOptionsManager to indicate that it was about maintaining a table mapping projects to up-to-date FSharpCompileOptions values. It's now been expanded to contain this orthogonal extra information, so perhaps rename back to ProjectInfoManager or ProjectOptionsManager

dsyme · 2017-08-18T11:42:50Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

+        let sourcePaths = sources |> Seq.map(fun s -> fullPath s.Path) |> Seq.toArray
+        let referencePaths = references |> Seq.map(fun r -> fullPath r.Reference) |> Seq.toArray
+
+        LogFile.log "======================================="


Remove in favour of System.Diagnostics.Trace.

dsyme · 2017-08-18T11:43:41Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

+        let hier = visualStudioWorkspace.GetHierarchy(project.Id)
+        {new IProvideProjectSite with
+             member this.GetProjectSite() =
+                 let compileItems () = projectInfoManager.GetSourceFiles(project.FilePath)


I believe there's no need to define all these little functions, nor is it clearer - just put the implementations in the object expression

dsyme · 2017-08-18T11:46:53Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

+                     member __.ProjectProvider = Some this
+                     member __.AssemblyReferences() = assemblyReferences ()
+                 }
+         interface IVsHierarchy with


Why is an IVsHierarchy implementation is needed here? I do vaguely remember - is it that crappy code in ProjectSitesAndFiles.fs that searches for IVsHierarchy for project references? However if that's the case, then let's document it, and let's also eventually get rid of this in favour of adding an appropriate method into IProjectSite, or even just using the existing AssemblyReferences method

dsyme · 2017-08-18T12:51:26Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

+                              hierarchy, 
+                              null, 
+                              errorReporter) :?> AbstractProject
+                | _ as p -> p


This line can just be | p -> p

dsyme · 2017-08-18T13:59:43Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs


-            if isNull (workspace.ProjectTracker.GetProject projectId) then


Previously we used this line to guard against doing work multiple times for the same project as we traverse the project graph

You've moved the check down a bit, but this means that UpdateProjectInfo may now be called many times per project, along with actions such as creating ProjectExternalErrorReporter, and, most critically, the recursive calls and AddProjectReference calls here:

for referencedSite in ProjectSitesAndFiles.GetReferencedProjectSites (site, this.SystemServiceProvider) do let referencedProjectId = setup referencedSite project.AddProjectReference(ProjectReference referencedProjectId)

I think you will want to somehow want to guard against doing these things many times as we traverse the project graph.

dsyme · 2017-08-18T14:03:05Z

@KevinRansom As part of validating this could you check the before/after times for "devenv.exe CPU usage time at the point we can first perform a file edit" for

devenv tests\projects\stress\big\dense\Dense.sln

devenv tests\projects\stress\huge\dense\Dense.sln

Thanks. Also, we should write out a list of things we need to test manually when changes like this are made, e.g.

Create a .NET Framwork Library + App, add a reference from App to Library, test goto-definition, remove the reference, add it back, test goto-definition etc.
Same for .NET Core Library + App

KevinRansom · 2017-08-18T21:17:02Z

@dsyme .... yes I know ... I have some ideas.

cartermp · 2017-08-21T21:27:53Z

Why is the branch name keys? Is this based on the correct branch?

KevinRansom · 2017-08-21T21:30:38Z

because halfway through I looked at the keyboard fix, then I cherry picked the fix over.

I think because ... reasons .... (none of which are sensible).

cartermp · 2017-08-21T21:44:08Z

I see. Is this PR in a good enough state to validate experience-wise?

KevinRansom · 2017-08-22T08:04:14Z

@cartermp not even close.

KevinRansom · 2017-09-09T08:18:23Z

Closing in favour of: #3564

KevinRansom added 2 commits August 7, 2017 11:57

Add IProvideProjectSite mapping

beb4e32

update language service for sdk files

6cfd52e

msftclas added the cla-already-signed label Aug 8, 2017

Pilchie approved these changes Aug 9, 2017

View reviewed changes

Update language service to use: direct command line notifications.

c37d65b

dsyme requested changes Aug 18, 2017

View reviewed changes

dsyme reviewed Aug 18, 2017

View reviewed changes

cartermp mentioned this pull request Aug 21, 2017

[.NET Core SDK projects] 15.3 preview 3: dependencies are not loaded in VS #3266

Closed

KevinRansom mentioned this pull request Sep 9, 2017

Another go at --- Map workspace to iprojectsite #3564

Merged

KevinRansom closed this Sep 9, 2017

KevinRansom deleted the keys branch September 17, 2017 01:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Map workspace to iprojectsite #3425

Map workspace to iprojectsite #3425

KevinRansom commented Aug 8, 2017

Pilchie Aug 9, 2017

KevinRansom Aug 10, 2017

dsyme left a comment

dsyme Aug 18, 2017

dsyme Aug 18, 2017

dsyme Aug 18, 2017

dsyme Aug 18, 2017

dsyme Aug 18, 2017

dsyme Aug 18, 2017

dsyme Aug 18, 2017

dsyme Aug 18, 2017

dsyme Aug 18, 2017

dsyme Aug 18, 2017

dsyme Aug 18, 2017

dsyme Aug 18, 2017

dsyme Aug 18, 2017

dsyme commented Aug 18, 2017 •

edited

Loading

KevinRansom commented Aug 18, 2017

cartermp commented Aug 21, 2017

KevinRansom commented Aug 21, 2017

cartermp commented Aug 21, 2017

KevinRansom commented Aug 22, 2017

KevinRansom commented Sep 9, 2017


		if isNull (workspace.ProjectTracker.GetProject projectId) then

Map workspace to iprojectsite #3425

Map workspace to iprojectsite #3425

Conversation

KevinRansom commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsyme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsyme commented Aug 18, 2017 • edited Loading

KevinRansom commented Aug 18, 2017

cartermp commented Aug 21, 2017

KevinRansom commented Aug 21, 2017

cartermp commented Aug 21, 2017

KevinRansom commented Aug 22, 2017

KevinRansom commented Sep 9, 2017

dsyme commented Aug 18, 2017 •

edited

Loading