Allow loading of projects that are missing imports.

baronfel commented 4 months ago

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.

baronfel commented 4 months ago

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 commented 4 months ago

I tested the support-projects-with-missing-imports branch against https://github.com/ionide/ionide-vscode-fsharp/issues/1972.

The branch fixes one of two cases:

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.

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 https://github.com/ionide/proj-info/blob/6fe2b3bf85f20b29a00e8c59700f3f78c87416fb/test/Ionide.ProjInfo.Tests/Tests.fs#L1993-L2016 results in a non-empty parsedProjs.

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

baronfel commented 4 months ago

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 commented 4 months ago

Gonna convert to draft until we have some tests

baronfel commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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.

ionide / proj-info