Project References for net sdk projects. #3653

KevinRansom · 2017-09-27T06:38:54Z

Approach

Move ProjectSite wrapping to projectsitesandfiles.fs
Plumb serviceProvider and commandLineOptions deep to allow wrapping
Merge commandline options from disgntime build with FSharpProjectOptimes.

Addresses #3571

KevinRansom · 2017-09-27T22:29:46Z

@dotnet-bot test Windows_NT Release_ci_part2 Build please

KevinRansom · 2017-09-30T00:17:15Z

@dsyme , @cartermp, @brettfo can you guys check this out pls

Pilchie · 2017-09-30T00:42:09Z

I've tried to review this several times, and it's a little dense for me to follow - let's talk about it together on Tuesday.

KevinRansom · 2017-09-30T19:20:59Z

Sure

dsyme

Some changes needed, see comments

dsyme · 2017-10-02T18:05:43Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

@@ -126,6 +122,8 @@ type internal FSharpProjectOptionsManager
    member this.ClearInfoForSingleFileProject(projectId) =
        singleFileProjectTable.TryRemove(projectId) |> ignore

+    member __.CommandLineOptions with get() = commandLineOptions


You can (and should) remove with get() to be idiomatic

Hmm I need to work on my F# programming skillz :-)

dsyme · 2017-10-02T18:06:46Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

+                Some options
+            else
+                Some { ProjectFileName                   = options.ProjectFileName
+                       SourceFiles                       = if sourcePaths.Length = 0 then options.SourceFiles else sourcePaths


You already know from the preceding condition that sourcePaths.Lengths = 0 is false, you can simplify this

The other line is an 'or' || so I don't necessarily know that it is 0.

if sourcePaths.Length = 0 || otherOptions.Length = 0 then

dsyme · 2017-10-02T18:10:18Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

@@ -183,7 +181,23 @@ type internal FSharpProjectOptionsManager
    /// Get the options for a project
    member this.TryGetOptionsForProject(projectId: ProjectId) = 
        match projectTable.TryGetValue(projectId) with
-        | true, ((_referencedProjects, options), _) -> Some options 


I'm concerned by this change. the whole point of TryGetOptionsForProject and projectTable is to maintain the canonical FSharpProjectOptions object for the project so it doesn't need to be recomputed - and FCS can use Stamp to tell if the project options of dependent projects have changed. For example, each time you make a new options object you should bump Stamp.

But with this change, the options stored in projectTable are no longer canonical and up-to-date - instead you have to go through this routine to get the latest options.

It would be better to update options entry in projectTable either here (on-demand) or at some earlier point when we know that the CommandLineOptions/sourcePaths/otherOptions results change

Note that failing to keep the projectTable canonical may have correctness of performance effects on incorrect or recomputation of project options.

dsyme · 2017-10-02T18:12:13Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

@@ -411,13 +367,20 @@ type

    let optionsAssociation = ConditionalWeakTable<IWorkspaceProjectContext, string[]>()

-    member private this.OnProjectAdded(projectId:ProjectId, _newSolution:Solution) = projectInfoManager.UpdateProjectInfoWithProjectId(projectId, "OnProjectAdded")
+    member private this.OnProjectAdded(projectId:ProjectId, _newSolution:Solution)    = projectInfoManager.UpdateProjectInfoWithProjectId(projectId, "OnProjectAdded")


If _newSolution is unused then why not remove it?

I suspect that we will be using eventually. It's just not necessary right now.

dsyme · 2017-10-02T18:12:34Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

+            | WorkspaceChangeKind.ProjectAdded    -> this.OnProjectAdded(args.ProjectId,    args.NewSolution)
+            | WorkspaceChangeKind.ProjectChanged  -> this.OnProjectChanged(args.ProjectId,  args.NewSolution)
+            | WorkspaceChangeKind.ProjectReloaded -> this.OnProjectReloaded(args.ProjectId, args.NewSolution)
+//            | WorkspaceChangeKind.ProjectRemoved  -> this.OnProjectRemoved(args.ProjectId,  args.NewSolution)


What's up with Removed? Please add a comment as to what needs to be done here

dsyme · 2017-10-02T18:13:09Z

vsintegration/src/FSharp.LanguageService/ProjectSitesAndFiles.fs

@@ -101,7 +109,75 @@ type private ProjectSiteOfSingleFile(sourceFile) =
        override this.AssemblyReferences() = [||]

    override x.ToString() = sprintf "ProjectSiteOfSingleFile(%s)" sourceFile
+
+type CommandLineOptions (isEmpty:bool) =


Please add a /// comment explaining what this is. Always...

dsyme · 2017-10-02T18:13:23Z

vsintegration/src/FSharp.LanguageService/ProjectSitesAndFiles.fs

+
+    new () = CommandLineOptions(false)
+
+    member __.GetOptionsWithProjectId(projectId:ProjectId) = 


Please add /// comments on all new members

dsyme · 2017-10-02T18:14:11Z

vsintegration/src/FSharp.LanguageService/ProjectSitesAndFiles.fs

+type CommandLineOptions (isEmpty:bool) =
+
+    let commandLineOptions = new ConcurrentDictionary<ProjectId, string[]*string[]*string[]>()
+    static let emptyCommandLineOptions = lazy new CommandLineOptions(true)


There is no point in making such a trivial object allocation lazy - just remove the ``lazy`

dsyme · 2017-10-02T18:14:35Z

vsintegration/src/FSharp.LanguageService/ProjectSitesAndFiles.fs


+    static member Empty with get () = emptyCommandLineOptions.Value


Remove the with get()

dsyme · 2017-10-02T19:00:18Z

vsintegration/src/FSharp.LanguageService/ProjectSitesAndFiles.fs

+    member __.GetOptionsWithProjectId(projectId:ProjectId) = 
+        match commandLineOptions.TryGetValue projectId with
+        | true, value -> value
+        | _ -> [||], [||], [||]


This should return a None on this branch

KevinRansom · 2017-10-04T18:27:02Z

Hmm seems as if I have broken p2p references :-(

KevinRansom · 2017-10-05T00:48:48Z

@dotnet-bot test Windows_NT Release_ci_part1 Build

cartermp · 2017-10-05T04:42:12Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

@@ -207,8 +207,9 @@ type internal FSharpProjectOptionsManager
        match hier with
        | h when (h.IsCapabilityMatch("CPS")) ->
            let project = workspace.CurrentSolution.GetProject(projectId)
-            let siteProvider = provideProjectSiteProvider(workspace, project, serviceProvider, Some projectOptionsTable)
-            this.UpdateProjectInfo(tryGetOrCreateProjectId, projectId, siteProvider.GetProjectSite(), userOpName)
+            if project <> null then


is x = null or x <> null strictly less efficient than a pattern match or isNull/not isNull call? Not sure if my knowledge here is rusty or not

IIRC 'x = null' can throw NullReferenceException at runtime when x is null and the compiler thinks that it cannot be null.

isNotNull x is probably the way to go.

Hmmm!!! isNotNull is internal: https://github.com/Microsoft/visualfsharp/blob/e6d46b9f56295aa70889ebff59cc0c6f0144a3ce/src/fsharp/FSharp.Core/prim-types.fs#L3324

It came in after we shutdown for 4.1.

Because it changed fsharp.core surface area when I added it. And my changes where rolled back.

In other words: it was public for a week or so

See fsharp/fslang-suggestions#99 (comment)

... we have a design rule not to put "NotXYZ" predicates in the core library

@dsyme ahh well this doen't fall under that rule because it is:
isNotNull :-)

It is a decent api we should make it public for FSharp.Core 4.3

The design rule is clear both for F# and for .NET - we don't put negative "not" predicates in our libraries. It is a slippery slope and we should not go down it. Just use not (isNull x).

If that's not satisfactory we would be much better off putting effort into fsharp/fslang-suggestions#569 to eventually allow !isNull x

KevinRansom · 2017-10-05T17:00:10Z

So this doesn't work unless the project has been pre-built for dotnet sdk projects I still need to figure that out.

cartermp · 2017-10-06T18:46:49Z

This doesn't work for me if the libraries are pre-built either. This is on 15.5 Preview 1 bits. Should I be testing against something else?

dsyme · 2017-10-10T18:57:17Z

vsintegration/src/FSharp.LanguageService/ProjectSitesAndFiles.fs

                    | _ -> ()
-            | None -> ()
+            | None ->


This is dubious - your new code is only being hit when solutionService is None, which should never happen?

Pilchie · 2017-10-13T15:12:31Z

vsintegration/src/FSharp.LanguageService/ProjectSitesAndFiles.fs

+                                // not the intermediate output directory.
+                                // so as a Giant kludge transform obj to bin
+                                // ======================================================================
+                                yield Some projectId, project.FilePath, outputPath.Replace(@"\obj\", @"\bin\"), siteProvider


Yeah, this won't work in general. Both of these paths can (and frequently are) changed by msbuild variables. In Roslyn for non-CPS projects, we get this from the hierarchy with this code. Not sure if that would work for CPS or not. (Tag @davkean)

I know it won't that's why the 'Hack!!!!!!' I think I am pretty sure I need the project System to have set this value. Or I should figure out why FCS needs it ... which is probably more of a winner here.

Pilchie · 2017-10-13T15:13:54Z

vsintegration/src/FSharp.LanguageService/ProjectSitesAndFiles.fs

+                                        // project.OutputFilePath does not have a value at this point ... 
+                                        // so calculate one by peeking at the command line options ... --out:
+                                        // ======================================================================
+                                        match projectOptionsTable with


I don't understand why this works. According to what @davkean was telling me yesterday, at the point where the OutputFilePath is still null, we shouldn't have any options yet, just the source files because it comes from evaluation only...

@kevinpi. Well the command line options from designtimebuild have been received at this point.

KevinRansom force-pushed the projectreferences branch from eaf95ee to 216dca9 Compare September 27, 2017 08:17

KevinRansom changed the title ~~WIP: Project References for net sdk projects.~~ Project References for net sdk projects. Sep 30, 2017

dsyme requested changes Oct 2, 2017

View reviewed changes

dsyme reviewed Oct 2, 2017

View reviewed changes

cartermp mentioned this pull request Oct 2, 2017

[From 15.5 preview test day] - Newly-added file is not recognized #3677

Closed

cartermp reviewed Oct 5, 2017

View reviewed changes

dotnet deleted a comment from forki Oct 10, 2017

dsyme reviewed Oct 10, 2017

View reviewed changes

Pilchie reviewed Oct 13, 2017

View reviewed changes

cartermp mentioned this pull request Oct 16, 2017

Support modern MSBuild 15 like fsproj #2316

Closed

KevinRansom force-pushed the projectreferences branch 2 times, most recently from f6b435c to 22da6d8 Compare October 19, 2017 00:01

KevinRansom closed this Oct 19, 2017

KevinRansom force-pushed the projectreferences branch from 22da6d8 to 68f4187 Compare October 19, 2017 00:07


		new () = CommandLineOptions(false)

		member __.GetOptionsWithProjectId(projectId:ProjectId) =


		static member Empty with get () = emptyCommandLineOptions.Value

Project References for net sdk projects. #3653

Project References for net sdk projects. #3653

Conversation

KevinRansom commented Sep 27, 2017 • edited by cartermp Loading

KevinRansom commented Sep 27, 2017

KevinRansom commented Sep 30, 2017

Pilchie commented Sep 30, 2017

KevinRansom commented Sep 30, 2017

dsyme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinRansom commented Oct 4, 2017

KevinRansom commented Oct 5, 2017

cartermp Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinRansom Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

KevinRansom Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinRansom commented Oct 5, 2017

cartermp commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinRansom commented Sep 27, 2017 •

edited by cartermp

Loading

cartermp Oct 5, 2017 •

edited

Loading

KevinRansom Oct 5, 2017 •

edited

Loading

KevinRansom Oct 5, 2017 •

edited

Loading