Refactor error handling for project restoration and update related types and tests

Author: TheAngryByrd

src/Ionide.ProjInfo/Library.fs

@@ -17,11 +17,11 @@ open Patterns
 type ProjectNotRestoredError<'e, 'buildResult> =
     static abstract NotRestored: 'buildResult -> 'e
 
-type ProjectNotRestoredDU<'BuildResult> =
-    | BuildErr of 'BuildResult
+type ParseError<'BuildResult> =
+    | NotRestored of 'BuildResult
 
-    interface ProjectNotRestoredError<ProjectNotRestoredDU<'BuildResult>, 'BuildResult> with
-        static member NotRestored(result) = BuildErr result
+    interface ProjectNotRestoredError<ParseError<'BuildResult>, 'BuildResult> with
+        static member NotRestored(result) = NotRestored result
 
 
 /// functions for .net sdk probing
@@ -966,6 +966,7 @@ module ProjectLoader =
         (analyzers: Analyzer list)
         (allProps: Map<string, Set<string>>)
         (allItems: Map<string, Set<string * Map<string, string>>>)
+        (imports: string list)
         =
         let projDir = Path.GetDirectoryName path
 
@@ -1041,6 +1042,7 @@ module ProjectLoader =
             Analyzers = analyzers
             AllProperties = allProps
             AllItems = allItems
+            Imports = imports
         }
 
 
@@ -1052,7 +1054,6 @@ module ProjectLoader =
         | TraversalProjectInfo of ProjectReference list
         | OtherProjectInfo of ProjectInstance
 
-
     let getLoadedProjectInfo<'e when ProjectNotRestoredError<'e, LoadedProject>> (path: string) customProperties project =
 
         match project with
@@ -1166,12 +1167,16 @@ module ProjectLoader =
                 )
                 |> Seq.toList
 
+            let imports =
+                p.ImportPaths
+                |> Seq.toList
+
 
             if not sdkInfo.RestoreSuccess then
                 Error('e.NotRestored project)
             else
                 let proj =
-                    mapToProject path commandLineArgs p2pRefs compileItems nuGetRefs sdkInfo props customProps analyzers allProperties allItems
+                    mapToProject path commandLineArgs p2pRefs compileItems nuGetRefs sdkInfo props customProps analyzers allProperties allItems imports
 
                 Ok(LoadedProjectInfo.StandardProjectInfo proj)
         | LoadedProject.Other p -> Ok(LoadedProjectInfo.OtherProjectInfo p)
@@ -1432,7 +1437,7 @@ type WorkspaceLoaderViaProjectGraph private (toolsPath, ?globalProperties: (stri
                             | Ok projectOptions ->
 
                                 Some projectOptions
-                            | Error(ProjectNotRestoredDU.BuildErr e) ->
+                            | Error(ParseError.NotRestored e) ->
                                 logger.error (
                                     Log.setMessage "Failed loading projects {error}"
                                     >> Log.addContextDestructured "error" e
@@ -1608,7 +1613,7 @@ type WorkspaceLoader private (toolsPath: ToolsPath, ?globalProperties: (string *
                             | ProjectLoader.LoadedProjectInfo.OtherProjectInfo p -> None
 
                         lst, info
-                    | Error(ProjectNotRestoredDU.BuildErr e) ->
+                    | Error(ParseError.NotRestored e) ->
                         loadingNotification.Trigger(WorkspaceProjectState.Failed(p, GenericError(p, "Not restored")))
                         [], None
 

src/Ionide.ProjInfo/ProjectLoader2.fs

@@ -45,14 +45,14 @@ module SemaphoreSlimExtensions =
             )
 
 
-module Map =
+module internal Map =
 
-    let mapAddSome key value map =
+    let inline mapAddSome key value map =
         match value with
         | Some v -> Map.add key v map
         | None -> map
 
-    let union loses wins =
+    let inline union loses wins =
         Map.fold (fun acc key value -> Map.add key value acc) loses wins
 
     let inline ofDict dictionary =
@@ -320,7 +320,7 @@ module TargetFrameworks =
 // type ProjectProjectMap = ProjectMap<Project>
 // type ProjectGraphMap = ProjectMap<ProjectGraphNode>
 
-module ProjectLoading =
+module internal ProjectLoading =
 
     // let getAllTfms (projectPath: ProjectPath) pc props =
     //     let p = findOrCreateMatchingProject projectPath pc props
@@ -339,7 +339,7 @@ module ProjectLoading =
     //     getAllTfms projectPath
     //     |> Option.bind Array.tryHead
 
-    let defaultProjectInstanceFactory (projectPath: string) (xml: Dictionary<string, string>) (collection: ProjectCollection) =
+    let inline defaultProjectInstanceFactory (projectPath: string) (xml: Dictionary<string, string>) (collection: ProjectCollection) =
         let props = Map.union (Map.ofDict xml) (Map.ofDict collection.GlobalProperties)
         ProjectInstance(projectPath, props, toolsVersion = null, projectCollection = collection)
 
@@ -500,13 +500,13 @@ type ProjectLoader2 =
     /// or TargetFrameworks defined in the project files.</returns>
     /// <remarks>
     /// This method evaluates each project file and checks for the presence of a "TargetFramework"
-    /// property. If it exists, the project is returned as is. If it does not
-    ///     exist, it checks for the "TargetFrameworks"
+    /// property. If it exists, the project is returned as is. If it does not exist, it checks for the "TargetFrameworks"
     /// property and splits it into individual TargetFrameworks. For each TargetFramework, it creates
-    /// a new project
-    /// with the "TargetFramework" global property set to that TargetFramework.
+    /// a new project with the "TargetFramework" global property set to that TargetFramework.
     /// </remarks>
     static member EvaluateAsGraphAllTfms(entryProjectFile: ProjectGraphEntryPoint seq, ?projectCollection: ProjectCollection, ?projectInstanceFactory) =
+        // For some reason, the graph evaluation doesn't handle multiple TFMs well
+        // So first we evaluate the graph to find all projects
         let graph =
             ProjectLoader2.EvaluateAsGraph(entryProjectFile, ?projectCollection = projectCollection, ?projectInstanceFactory = projectInstanceFactory)
 
@@ -528,7 +528,7 @@ type ProjectLoader2 =
                 |> Option.map (fun _ -> ProjectGraphEntryPoint(node.ProjectInstance.FullPath, globalProperties = node.ProjectInstance.GlobalProperties))
             )
 
-
+        // Then, re-evaluate the graph with those projects
         ProjectLoader2.EvaluateAsGraph(projects, ?projectCollection = projectCollection, ?projectInstanceFactory = projectInstanceFactory)
 
     /// <summary>
@@ -585,8 +585,7 @@ type ProjectLoader2 =
     /// <param name="buildParameters">Function to get build parameters for each project.</param
     /// ><param name="targetsToBuild">Optional targets to build. Defaults to design-time build targets.</param>
     /// <param name="flags">Optional flags for the build request. Defaults to ProjectLoader2.DefaultFlags.</param>
-    /// <param name="ct">Optional cancellation token to cancel the
-    /// build.</param>
+    /// <param name="ct">Optional cancellation token to cancel the build.</param>
     /// <returns>A task that returns an array of BuildResult or an error containing the failed build and message.</returns>
     /// <remarks>
     /// This method will visit each project, build it, and then recursively visit its references.

src/Ionide.ProjInfo/Types.fs

@@ -84,6 +84,10 @@ module Types =
         /// Will have Key Value pairs like "PkgIonide_Analyzers" -> "C:\Users\username\.nuget\packages\ionide.analyzers\0.14.7"
         /// The "analyzers/dotnet/fs" subfolder is not included here, just the package root
         Analyzers: Analyzer list
+        /// The full file paths of all the files that during evaluation contributed to this project instance.
+        /// This does not include projects that were never imported because a condition on an Import element was false.
+        /// The outer ProjectRootElement that maps to this project instance itself is not included.
+        Imports: string list
     } with
 
         /// ResolvedTargetPath is the path to the primary reference assembly for this project.

test/Ionide.ProjInfo.Tests/ProjectLoader2Tests.fs

@@ -96,7 +96,7 @@ module ProjectLoader2Tests =
                     |> Seq.choose (fun x ->
                         match x with
                         | Ok(LoadedProjectInfo.StandardProjectInfo x) -> Some x
-                        | Result.Error(ProjectNotRestoredDU.BuildErr _) -> None
+                        | Result.Error(ParseError.NotRestored _) -> None
                         | _ -> None
                     )
                 | Result.Error(BuildErrors.BuildErr(result, errorLogs)) ->
@@ -150,8 +150,11 @@ module ProjectLoader2Tests =
                     function
                     | Ok result ->
                         match ProjectLoader2.Parse result with
-                        | Ok(LoadedProjectInfo.StandardProjectInfo x) -> Some x
-                        | Result.Error(ProjectNotRestoredDU.BuildErr _) -> None
+                        | Ok(LoadedProjectInfo.StandardProjectInfo x) ->
+                            let validProjectRestore = Newtonsoft.Json.JsonConvert.SerializeObject x
+                            File.WriteAllText(Path.Combine(env.TestDir.FullName, "restored-project.json"), validProjectRestore)
+                            Some x
+                        | Result.Error(ParseError.NotRestored _) -> None
                         | _ -> None
                     | _ -> None
                 )
@@ -310,10 +313,12 @@ module ProjectLoader2Tests =
                         | Ok result ->
                             ProjectLoader2.Parse result
                             |> Seq.choose (
-
                                 function
-                                | Ok(LoadedProjectInfo.StandardProjectInfo x) -> Some x
-                                | Result.Error(ProjectNotRestoredDU.BuildErr _) -> None
+                                | Ok(LoadedProjectInfo.StandardProjectInfo x) ->
+                                    let validProjectRestore = Newtonsoft.Json.JsonConvert.SerializeObject x
+                                    File.WriteAllText(Path.Combine(env.TestDir.FullName, "restored-project.json"), validProjectRestore)
+                                    Some x
+                                | Result.Error(ParseError.NotRestored _) -> None
                                 | _ -> None
                             )
                             |> Seq.iter (fun x -> Expect.equal x.SourceFiles expectedSources "")
@@ -323,6 +328,17 @@ module ProjectLoader2Tests =
                     }
                 )
 
+            let contains (s: string) (o: string) = o.Contains(s)
+            let endsWith (s: string) (o: string) = o.EndsWith(s)
+
+            let filesOfInterest = [
+                endsWith "Directory.Build.props"
+                endsWith "Directory.Build.targets"
+                endsWith "Directory.Build.props"
+                endsWith ".sln.targets"
+                endsWith ".Build.rsp"
+            ]
+
 
             testCaseTask
             |> testWithEnv
@@ -359,6 +375,7 @@ module ProjectLoader2Tests =
 
                         let projs = ProjectLoader2.EvaluateAsProjectsAllTfms(entryPoints, projectCollection = pc)
 
+                        let projs = Seq.toList projs
                         // Execution
 
                         let bm = new BuildManagerSession()
@@ -383,11 +400,16 @@ module ProjectLoader2Tests =
                         match result with
                         | Result.Error _ -> failwith "expected success"
                         | Ok result ->
+                            ignore result.ProjectStateAfterBuild.ImportPaths
+
                             match ProjectLoader2.Parse result with
 
+                            | Ok(LoadedProjectInfo.StandardProjectInfo x) ->
+                                let validProjectRestore = Newtonsoft.Json.JsonConvert.SerializeObject x
+                                File.WriteAllText(Path.Combine(env.TestDir.FullName, "restored-project.json"), validProjectRestore)
 
-                            | Ok(LoadedProjectInfo.StandardProjectInfo x) -> Expect.equal x.SourceFiles expectedSources ""
-                            | Result.Error(ProjectNotRestoredDU.BuildErr e) -> failwith "%A" e
+                                Expect.equal x.SourceFiles expectedSources ""
+                            | Result.Error(ParseError.NotRestored e) -> failwithf "%A" e
                             | otherwise -> failwith $"Unexpected result {otherwise}"
 
                     }
@@ -565,7 +587,7 @@ module ProjectLoader2Tests =
 
                         //     File.WriteAllText(Path.Combine(env.TestDir.FullName, "non-restored-project.json"), validProjectRestore)
                         //     Expect.isFalse x.ProjectSdkInfo.RestoreSuccess "expected restore to fail"
-                        | Result.Error(ProjectNotRestoredDU.BuildErr(StandardProject e)) -> ()
+                        | Result.Error(ParseError.NotRestored(StandardProject e)) -> ()
                         | e -> failwithf "%A" e
 
                         return ()

test/Ionide.ProjInfo.Tests/TestUtils.fs

@@ -227,4 +227,3 @@ module Expect =
         match result with
         | Ok v -> v
         | Error _ -> failwith "unreachable"
-

test/Ionide.ProjInfo.Tests/Tests.fs

@@ -1728,7 +1728,7 @@ let testLoadProject toolsPath =
                 match ProjectLoader.getLoadedProjectInfo projPath [] proj with
                 | Ok(ProjectLoader.LoadedProjectInfo.StandardProjectInfo proj) -> Expect.equal proj.ProjectFileName projPath "project file names"
                 | Ok(ProjectLoader.LoadedProjectInfo.TraversalProjectInfo refs) -> failwith "expected standard project, not a traversal project"
-                | Result.Error(ProjectNotRestoredDU.BuildErr exn) -> failwith $"Project Not Restored {exn}"
+                | Result.Error(ParseError.NotRestored exn) -> failwith $"Project Not Restored {exn}"
                 | otherwise -> failwith $"Unexpected result {otherwise}"
         )