Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using ProjectTracker instead of CurrentSolution #5391

Closed
wants to merge 1 commit into from
Closed

Using ProjectTracker instead of CurrentSolution #5391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ open Microsoft.VisualStudio.Shell.Interop
open Microsoft.VisualStudio.ComponentModelHost
open Microsoft.VisualStudio.Text.Outlining
open FSharp.NativeInterop
open FSharp.Editor.SettingsPersistence

#nowarn "9" // NativePtr.toNativeInt

Expand Down Expand Up @@ -118,7 +119,8 @@ type internal FSharpProjectOptionsManager
(
checkerProvider: FSharpCheckerProvider,
[<Import(typeof<VisualStudioWorkspace>)>] workspace: VisualStudioWorkspaceImpl,
[<Import(typeof<SVsServiceProvider>)>] serviceProvider: System.IServiceProvider
[<Import(typeof<SVsServiceProvider>)>] serviceProvider: System.IServiceProvider,
[<Import(typeof<ISettings>)>] _settings: ISettings // This is necessary so that the Settings global doesn't explode.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

We should fix that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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#?

Copy link
Contributor Author

@TIHan TIHan Jul 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

) =

// A table of information about projects, excluding single-file projects.
Expand Down Expand Up @@ -228,15 +230,15 @@ type internal FSharpProjectOptionsManager
| _ -> this.TryGetOptionsForProject(projectId) |> Option.map(fun (parsingOptions, _, projectOptions) -> parsingOptions, projectOptions)

/// get a siteprovider
member this.ProvideProjectSiteProvider(project:Project) = provideProjectSiteProvider(workspace, project, serviceProvider, Some projectOptionsTable)
member this.ProvideProjectSiteProvider(project) = provideProjectSiteProvider(workspace, project, serviceProvider, Some projectOptionsTable)

/// Tell the checker to update the project info for the specified project id
member this.UpdateProjectInfoWithProjectId(projectId:ProjectId, userOpName, invalidateConfig) =
let hier = workspace.GetHierarchy(projectId)
match hier with
| null -> ()
| h when (h.IsCapabilityMatch("CPS")) ->
let project = workspace.CurrentSolution.GetProject(projectId)
let project = workspace.ProjectTracker.GetProject(projectId)
if not (isNull project) then
let siteProvider = this.ProvideProjectSiteProvider(project)
let projectSite = siteProvider.GetProjectSite()
Expand Down
15 changes: 7 additions & 8 deletions vsintegration/src/FSharp.Editor/LanguageService/ProjectSitesAndFiles.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,7 @@ type internal FSharpProjectOptionsTable () =
member __.SetOptionsWithProjectId(projectId:ProjectId, sourcePaths:string[], referencePaths:string[], options:string[]) =
commandLineOptions.[projectId] <- (sourcePaths, referencePaths, options)


let internal provideProjectSiteProvider(workspace:VisualStudioWorkspaceImpl, project:Project, serviceProvider:System.IServiceProvider, projectOptionsTable:FSharpProjectOptionsTable option) =
let internal provideProjectSiteProvider(workspace:VisualStudioWorkspaceImpl, project:AbstractProject, serviceProvider:System.IServiceProvider, projectOptionsTable:FSharpProjectOptionsTable option) =
let hier = workspace.GetHierarchy(project.Id)
let getCommandLineOptionsWithProjectId (projectId) =
match projectOptionsTable with
Expand All @@ -150,14 +149,14 @@ let internal provideProjectSiteProvider(workspace:VisualStudioWorkspaceImpl, pro

{
new IProjectSite with
member __.Description = project.Name
member __.Description = project.DisplayName
member __.CompilationSourceFiles = getCommandLineOptionsWithProjectId(project.Id) |> fst
member __.CompilationOptions =
let _,references,options = getCommandLineOptionsWithProjectId(project.Id)
Array.concat [options; references |> Array.map(fun r -> "-r:" + r)]
member __.CompilationReferences = getCommandLineOptionsWithProjectId(project.Id) |> snd
member site.CompilationBinOutputPath = site.CompilationOptions |> Array.tryPick (fun s -> if s.StartsWith("-o:") then Some s.[3..] else None)
member __.ProjectFileName = project.FilePath
member __.ProjectFileName = project.ProjectFilePath
member __.AdviseProjectSiteChanges(_,_) = ()
member __.AdviseProjectSiteCleaned(_,_) = ()
member __.AdviseProjectSiteClosed(_,_) = ()
Expand Down Expand Up @@ -230,15 +229,15 @@ type internal ProjectSitesAndFiles() =
if not (String.IsNullOrWhiteSpace(path)) then
match projectIdOpt with
| Some(projectId) ->
let project = workspace.CurrentSolution.GetProject(projectId)
let project = workspace.ProjectTracker.GetProject(projectId)
if not (isNull project) then
for reference in project.ProjectReferences do
let project = workspace.CurrentSolution.GetProject(reference.ProjectId)
for reference in project.GetCurrentProjectReferences() do
let project = workspace.ProjectTracker.GetProject(reference.ProjectId)
if not (isNull project) && project.Language = FSharpConstants.FSharpLanguageName then
let siteProvider = provideProjectSiteProvider (workspace, project, serviceProvider, projectOptionsTable)
let referenceProject = workspace.ProjectTracker.GetProject(reference.ProjectId)
let outputPath = referenceProject.BinOutputPath
yield Some project.Id, project.FilePath, outputPath, siteProvider
yield Some project.Id, project.ProjectFilePath, outputPath, siteProvider
| _ -> ()

| (Some references), _ ->
Expand Down