Allow loading of projects that are missing imports.

[baronfel]

Fixes #205.

This one got away from me a bit, but I think it's for the best.

The Problem

To load project files with missing Imports, we need to supply one or two ProjectLoadSettings flags to everywhere a project file could be parsed. These flags are ProjectLoadSettings.IgnoreMissingImports and ProjectLoadSettings.IgnoreInvalidImports. Ideally we need to find everywhere that a project would be loaded (so anywhere Project(...) or ProjectInstance(...) occur) and make sure these flags are applied.

Phase 1 - protecting the main constructors

For our purposes there are two main places that ProjectLoadSettings can be specified

• when the Project constructor is called in the WorkspaceLoader
• as part of the buildParameters for the WorkspaceLoaderViaProjectGraph

Applying these changes to the existing call-sites was fairly easy - there were something like 6 places that Projects/Project Instances were directly created. Here we come to our first hurdle - the ProjectInstance constructor doesn't surface ProjectLoadSettings in any way. So step one was to turn any sort of ProjectInstance creation into a two-phase operation:

• create the Project with appropriate load settings
• create the ProjectInstance from that Project

This got us most of the way through the tests.

Phase 2 - TFM detection

With one hurdle - the way we detected the TFM for a project involved loading a ProjectInstance directly and reading properties from it, and this turned out to be error prone because of the reasons mentioned above - ProjectInstance doesn't have ProjectLoadSettings. So we needed to create a Project to get the ProjectInstance from, as described above.

Phase 3 - ProjectCollection management

The above fix worked for more tests, but others still failed because the 'same project' was being created (and implicitly assigned to the global ProjectCollection) multiple times - a big no-no for ProjectCollections. So this required two changes:

• create and manage a ProjectCollection as a 'container' for a given call to LoadProjects - this allows us to cache the evaluations and design-time builds over the course of a single call to LoadProjects while also not cluttering/clobbering the global default ProjectCollection
• implement a function that safely retrieves an existing Project from the ProjectCollection if one exists for the same project path + global properties - if not, a new Project is created.

With these two changes, all the tests (including the new tests) became green.

[TheAngryByrd]

[baronfel]

Before this is merged I want to add a test that has a project that would fail without this - to ensure we don't regress.

[ronnieholm]

I tested the support-projects-with-missing-imports branch against ionide/ionide-vscode-fsharp#1972.

The branch fixes one of two cases:

1. Project including reference to Microsoft.Web.WebJobs.Publish NuGet package which indirectly references $(MSBuildExtensionsPath)\Microsoft\VisualStudio\v17.0\WebApplications\Microsoft.WebApplication.targets now is able to load.

2. Project which in project file directly has <Import Project="$(MSBuildExtensionsPath)\Microsoft\VisualStudio\v17.0\WebApplications\Microsoft.WebApplication.targets" /> still fails load with error:

   The imported project "C:\Program Files\dotnet\sdk\6.0.421\Microsoft\VisualStudio\v17.0\WebApplications\Microsoft.WebApplication.targets" was not found. Confirm that the expression in the Import declaration "C:\Program Files\dotnet\sdk\6.0.421\\Microsoft\VisualStudio\v17.0\WebApplications\Microsoft.WebApplication.targets" is correct, and that the file exists on disk.  C:\Users\rh\source\repos\xxx\xxx\xxx.fsproj
   

By "able to load" I mean that

proj-info/test/Ionide.ProjInfo.Tests/Tests.fs

Lines 1993 to 2016 in 6fe2b3b

	let debugTests toolsPath workspaceLoader (workspaceFactory: ToolsPath -> IWorkspaceLoader) =
	ptestCase
	|> withLog
	(sprintf "debug - %s" workspaceLoader)
	(fun logger fs ->
	let loader = workspaceFactory toolsPath
	// let projPath = @"D:\Programowanie\Projekty\Ionide\dotnet-proj-info\src\Ionide.ProjInfo.Sln\Ionide.ProjInfo.Sln.csproj"
	// let parsedProjs = loader.LoadProjects [ projPath ] |> Seq.toList
	// printfn "%A" parsedProjs
	let slnPath =
	@"C:\Users\JimmyByrd\Documents\Repositories\public\TheAngryByrd\FsToolkit.ErrorHandling\FsToolkit.ErrorHandling.sln"
	let parsedProjs =
	loader.LoadSln slnPath
	|> Seq.toList
	// printfn "%A" parsedProjs
	parsedProjs
	|> Seq.iter (fun p -> Expect.isGreaterThan p.SourceFiles.Length 0 $"{p.ProjectFileName} Should have SourceFiles")
	)

results in a non-empty parsedProjs.

Per our chat on Discord yesterday, perhaps VS is employing additional tricks to load case 2.

[baronfel]

1/2 is progress! there are some other project-load-related flags that we might be able to set to address the gap here. I'll make sure to have examples for both of the scenarios you mentioned in the tests I make for this PR so we can make sure we get it right.

[TheAngryByrd]

Gonna convert to draft until we have some tests

[baronfel]

@ronnieholm what version of that webjobs publish package are you using? 1.1.0 is the latest one I can find that has this bad import pattern, and it is 7 years old. There are a few more recent versions that wouldn't have this same problem.

[ronnieholm]

It's a .NET 4.8 project referencing Microsoft.Web.WebJobs.Publish, version 17.1.361.

Commenting out that

<PackageReference Include="Microsoft.Web.WebJobs.Publish" />

line in the .fsproj is the difference between the Ionide extension loading the project or not, the test returning zero parsedProjs or five projects, and dotnet list package --include-transitive failing or not:

$ dotnet list package --include-transitive
error: The imported project "C:\Program Files\dotnet\sdk\8.0.204\Microsoft\VisualStudio\v17.0\WebApplications\Microsoft.WebApplication.targets" was not found.
Confirm that the expression in the Import declaration "C:\Program
Files\dotnet\sdk\8.0.204\Microsoft\VisualStudio\v17.0\WebApplications\Microsoft.WebApplication.targets" is correct, and
that the file exists on disk.  C:\Users\rh\.nuget\packages\microsoft.web.webjobs.publish\17.1.361\build\webjobs.console.targets

The Microsoft.Web.WebJobs.Publish NuGet package has zero dependencies, so the behavior would seem to be caused by it directly. However, the interplay between the project and its transitive dependent projects (the project itself is one of the five parsedProjs projects) may be what's causing the issue.

So I created a new .NET 4.8 console app with only Microsoft.Web.WebJobs.Publish added. It loads fine with the Ionide extension, causes parsedProjs to return the single project, and lists packages with dotnet list package --include-transitive.

That behavior is hard to explain :-/

Today I cannot successfully reproduce case 1 above anymore. I suspect I might have messed up restore/recreation of project.assets.json yesterday.

[baronfel]

Went down a rabbit hold, did a kinda-large refactor that was needed because of some bad usage practices we had, and now we are green. I've left a summary in the PR description, but I'm happy to talk through the changes/add more docs/etc.

[baronfel]

The only reason that I had to do this filtering step is because Configuration is set by default here but some of our tests set Release and expect that to work.

If we removed this default it would be cleaner IMO, plus the SDK already defaults to Debug. Happy to discuss for a different PR.

[baronfel]

a lot of these calls dropped globalProperties in favor of projectCollection because the projectCollection holds the global properties.

[TheAngryByrd]

Looks good, just some minor changes I think we should make sure to do for our future selves.