Using ProjectTracker instead of CurrentSolution

[TIHan]

Fixes the issue reported here: #5395

The fix is to use the AbstractProject type instead of the VS Project type when creating a IProjectSite. The reason why is because we can't rely on CurrentSolution to give us a Project by its ProjectId. However, we can rely on the ProjectTracker to give us an AbstractProject.

[TIHan]

@Pilchie does this look good to you?

[Pilchie]

Presumably this regressed as part of #5284? Why didn't we catch it as part of validation of that change, or in the 3 weeks since with our own dogfooding?

[Pilchie]

It's not clear how this is related to the rest of the change.

[TIHan]

It's a bit complicated. We have a Settings type that relies on the implementation of ISettingss' constructor to be called. We import it here to force the constructor to be called at this point. Unfortunately we do a similar thing already in the FSharpPackage's Initialize method, but since Initialize hasn't been called yet until a document has been opened, we have to do this in order for it to work.

This is absolutely not ideal and a horrific pattern. We need to re-design how all of this flows through our language service.

[Pilchie]

If FSharpPackage hasn't been initialized, I expect lots of horrible things to happen. I would look into fixing that first.

[KevinRansom]

@Pilchie, the updateprojectinfo relies on access to the F# VS Settings. This causes them to be initialized when the CommandLineOptionsEvent arrived. The CommandLine options event may fire before the package is fully initialized.

[Pilchie]

The CommandLine options event may fire before the package is fully initialized.

We should fix that.

[Pilchie]

That's unlikely to be feasible any time soon, since the commandline arguments types are strongly connected to the C#/VB types, and include compiler concepts that don't apply to F#.

Regardless, it wouldn't help this initialization piece. In the short term though, I'm not sure why we couldn't call IVsShell::LoadPackage (or QueryService for a service profferred from the language service) from the project system to ensure the language service is initialized before we start using it.

It's historically been very hard to reason about VS packages that might get loaded by MEF but not have Initialize called on them. If that's happening in F#, my guess is that this is far from the only bug that happens as a result, and it would be better to fix the underlying issue.

Can you look at how the C# Language Service package gets initialized in a similar codepath and explain why that doesn't happen for F#?

[TIHan]

For a long-term solution, I really think we should explore what could possibly be done for the commandline arguments. @jasonmalinowski thinks we might be able to do this. We should all talk about this together.

The ISettings initialization is perfectly fine to happen here; we just have a bad pattern where a global structure relies on its initialization. In a separate PR, we can remove the global structure and just use the ISettings instance when we need it.

[TIHan]

Regardless if we use LoadPackage or not, we should take this PR as using CurrentSolution is not correct here anyways.

[TIHan]

After checking with @jasonmalinowski , calling IVsShell::LoadPackage seems reasonable to do.

[TIHan]

I think I found where to do it: https://github.com/dotnet/roslyn/blob/0959f9d826116a47a6da267cd1420fc803568b28/src/VisualStudio/Core/Impl/ProjectSystem/CPS/CPSProjectFactory.cs#L122
Seems F# is not in that list. :)

[cartermp]

Note for any OSS folks - this may also address a slough of issues related to the F# editor not getting the language service initialized. We'll have to go back and hunt down some of these bugs in the repo and verify if they are fixed/not fixed, but at the very least, this fixes the bug listed in the PR text.

[TIHan]

@Pilchie This didn't regress as part of this: #5284 , because the issue was reported using VS 15.7.3.

I believe I know where this regressed. It regressed when we made the performance improvement for .net sdk projects, and we put it in 15.7.3. We did not have this issue in 15.6 for sure.

Why it regressed and why we missed it is complicated. I will explain:

Why it regressed

Before the perf fix, every time we opened a document, we would update project information. With the perf fix, we stopped doing that as it was a perf problem. This seemed perfectly reasonable to do because HandleCommandLineChanges would be called from the project system to update the project information anyway.

However, there was something that slipped through the cracks. workspace.CurrentSolution.GetProject is not reliable when HandleCommandLineChanges gets called when a solution loads. By not reliable, I mean it will return null even though the project exists in ProjectTracker. When this returns null, we do not propagate any project information. Therefore, when opening a document, we can't retrieve any project options for it to pass to our compiler services; nothing works.

The reason it worked before is because we would update project information when a document opened and calling workspace.CurrentSolution.GetProject seemed to not return null, then the information gets propagated appropriately.

Why we missed it

There are a few reasons why we didn't encounter this earlier:

• FSharpPackage's Initialize method gets called before we fully opened a solution because the F# interactive window had focus on the start page.
• Starting VS and creating a new .NET SDK project always resulted in editor features working, meaning workspace.CurrentSolution.GetProject was successful in that case.
• Everytime we opened an existing solution, we had a F# document automatically open before the entire solution loaded.

Conclusion

This is a specific case for this issue to occur: A solution has to already exist with a F# project and no F# documents are opened automatically when you load the solution from a newly instanced VS.

The fix here is to not use workspace.CurrentSolution.GetProject, and use workspace.ProjectTracker.GetProject. Also, the reason why we need to import ISettings is because Settings is needed when updating project information in HandleCommandLineChanges when the FSharpPackage has not had its Initalize method called.

[Pilchie]

Even after the fix in 15.7.3, until the PR I mentioned above, we used ProjectTracker to get results

[TIHan]

Are you referring to this line? https://github.com/Microsoft/visualfsharp/pull/5284/files#diff-588c38c5c54419143876d9c7ce941753L231

If you look below it it still uses CurrentSolution then we check the result for null afterwards.

I removed the ProjectTracker in that case because it didn't make any sense to try to get a ProjectId by path since I'm passing a ProjectId option as an argument.

[cartermp]

For testing purposes as per discussion with @TIHan :

#5170 - verify if this addresses the repro
#4446 - could see if this addresses the repro

[KevinRansom]

@dotnet-bot test Ubuntu16.04 Release_default Build please

[Pilchie]

@TIHan - I was referring to that line, missed the CurrentSolution after. Thanks for pointing that out.

[TIHan]

Seems the PR here: dotnet/roslyn#28901 will also resolve the issue of editor features not working.

I still think we should merge this PR, only because we should be using ProjectTracker.

[TIHan]

Closing this as per discussion with @jasonmalinowski , we should not be using ProjectTracker as it is going away and not reliable as well. In fact, we shouldn't be doing all of this orchestration in general and needs a serious overhaul.