Addressing code review comments

Author: Omar Tawfik

vsintegration/src/FSharp.Editor/BraceCompletion.fs

@@ -24,7 +24,7 @@ type internal CompletionContext(tokenContext : TokenContext, textManager : IVsTe
         /// Called when user hits Return while inside of a brace completion session.
         member this.OnReturn(session) =
 
-            let lp = [| LANGPREFERENCES(guidLang = Guid(FSharpCommonConstants.languageServiceGuid)) |]
+            let lp = [| LANGPREFERENCES(guidLang = Guid(FSharpCommonConstants.languageServiceGuidString)) |]
             ErrorHandler.ThrowOnFailure(textManager.GetUserPreferences(null, null, lp, null)) |> ignore
 
             // if smart indent is not enabled, or we are in a string, don't do any special formatting

vsintegration/src/FSharp.Editor/FSharpProjectSiteService.fs

@@ -11,7 +11,7 @@ open Microsoft.CodeAnalysis.Host.Mef
 open Microsoft.VisualStudio.FSharp.LanguageService
 open Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem
 
-// Currently the CompilationOptions type in Roslyn in sealed and there's no way to set the compilation options for a project.
+// Currently the CompilationOptions type in Roslyn is sealed and there's no way to set the compilation options for a project.
 // There's no property bag on a project either. So this service is a means to get the host project for a given Roslyn project
 // so that extra F# specific information can be stored on the host project.
 // Note that the FSharpProject is available only through the VS Workspace although we might call this service from projects of 

vsintegration/src/FSharp.Editor/SmartIndent.fs

@@ -49,7 +49,7 @@ type SmartIndent (textView : ITextView, textManager : IVsTextManager) =
         /// or null when no indentation is desired or unable to determine indentation
         member this.GetDesiredIndentation(line : ITextSnapshotLine) =
 
-            let lp = [| LANGPREFERENCES(guidLang = Guid(FSharpCommonConstants.languageServiceGuid)) |]
+            let lp = [| LANGPREFERENCES(guidLang = Guid(FSharpCommonConstants.languageServiceGuidString)) |]
             let indentStyle = if ErrorHandler.Succeeded(textManager.GetUserPreferences(null, null, lp, null)) then lp.[0].IndentStyle else vsIndentStyle.vsIndentStyleDefault
 
             if (indentStyle = vsIndentStyle.vsIndentStyleNone || _textView = null || _textView.IsClosed) then

vsintegration/src/FSharp.Editor/extension.vsixmanifest

@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="utf-8"?>
-<!-- Copyright (c) Microsoft Open Technologies, Inc.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information. -->
+<!-- Copyright (c) Microsoft Corporation.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information. -->
 <PackageManifest Version="2.0.0" xmlns="http://schemas.microsoft.com/developer/vsx-schema/2011" xmlns:d="http://schemas.microsoft.com/developer/vsx-schema-design/2011">
   <Metadata>
     <Identity Id="FSharp.Editor" Version="1.0" Language="en-US" Publisher="srivatsn"/>
@@ -14,7 +14,7 @@
     <SystemComponent>true</SystemComponent>
   </Installation>
   <Dependencies>
-    <Dependency Id="Microsoft.Framework.NDP" DisplayName="Microsoft .NET Framework" d:Source="Manual" Version="[4.5,)" />
+    <Dependency Id="Microsoft.Framework.NDP" DisplayName="Microsoft .NET Framework" d:Source="Manual" Version="[4.6,)" />
   </Dependencies>
   <Assets>
     <Asset Type="Microsoft.VisualStudio.MefComponent" Path="FSharp.Editor.dll"/>

vsintegration/src/FSharp.LanguageService/FSharpCommonConstants.fs

@@ -8,15 +8,15 @@ open System.Diagnostics
 
 module internal FSharpCommonConstants =
     [<Literal>]
-    let packageGuid = "871D2A70-12A2-4e42-9440-425DD92A4116"
+    let packageGuidString = "871D2A70-12A2-4e42-9440-425DD92A4116"
     [<Literal>]
-    let languageServiceGuid = "BC6DD5A5-D4D6-4dab-A00D-A51242DBAF1B"
+    let languageServiceGuidString = "BC6DD5A5-D4D6-4dab-A00D-A51242DBAF1B"
     [<Literal>]
-    let editorFactoryGuid = "4EB7CCB7-4336-4FFD-B12B-396E9FD079A9"
+    let editorFactoryGuidString = "4EB7CCB7-4336-4FFD-B12B-396E9FD079A9"
     [<Literal>]
-    let codePageEditorFactoryGuid = "82A16493-EF43-47E0-B42D-D87BAAB5335D"
+    let codePageEditorFactoryGuidString = "82A16493-EF43-47E0-B42D-D87BAAB5335D"
     [<Literal>]
-    let svsSettingsPersistenceManagerGuid = "9B164E40-C3A2-4363-9BC5-EB4039DEF653"
+    let svsSettingsPersistenceManagerGuidString = "9B164E40-C3A2-4363-9BC5-EB4039DEF653"
     [<Literal>]
     let FSharpLanguageName = "F#"
     [<Literal>]

vsintegration/src/FSharp.LanguageService/FSharpLanguageService.fs

@@ -9,7 +9,6 @@ open System.Runtime.InteropServices
 open Microsoft.FSharp.Compiler.SourceCodeServices
 
 open Microsoft.CodeAnalysis
-open Microsoft.CodeAnalysis.SolutionCrawler
 open Microsoft.VisualStudio
 open Microsoft.VisualStudio.LanguageServices
 open Microsoft.VisualStudio.LanguageServices.Implementation.LanguageService
@@ -21,18 +20,18 @@ open Microsoft.VisualStudio.Shell.Interop
 
 // Workaround to access non-public settings persistence type.
 // GetService( ) with this will work as long as the GUID matches the real type.
-[<Guid(FSharpCommonConstants.svsSettingsPersistenceManagerGuid)>]
+[<Guid(FSharpCommonConstants.svsSettingsPersistenceManagerGuidString)>]
 type internal SVsSettingsPersistenceManager = class end
 
-[<Guid(FSharpCommonConstants.languageServiceGuid)>]
+[<Guid(FSharpCommonConstants.languageServiceGuidString)>]
 type internal FSharpLanguageService(package : FSharpPackage) = 
     inherit AbstractLanguageService<FSharpPackage, FSharpLanguageService, FSharpProjectSite>(package)
 
     override this.ContentTypeName = FSharpCommonConstants.FSharpContentTypeName
     override this.LanguageName = FSharpCommonConstants.FSharpLanguageName
     override this.RoslynLanguageName = FSharpCommonConstants.FSharpLanguageName
 
-    override this.LanguageServiceId = new Guid(FSharpCommonConstants.languageServiceGuid)
+    override this.LanguageServiceId = new Guid(FSharpCommonConstants.languageServiceGuidString)
     override this.DebuggerLanguageId = DebuggerEnvironment.GetLanguageID()
 
     override this.CreateContext(_,_,_,_,_) = raise(System.NotImplementedException())
@@ -51,26 +50,26 @@ type internal FSharpLanguageService(package : FSharpPackage) =
             match hier with
             | :? IProvideProjectSite as siteProvider ->
                 let site = siteProvider.GetProjectSite()
-
-                let projectId = workspace.ProjectTracker.GetOrCreateProjectIdForPath(site.ProjectFileName(), site.ProjectFileName())
+                let projectFileName = site.ProjectFileName()
+                let projectId = workspace.ProjectTracker.GetOrCreateProjectIdForPath(projectFileName, projectFileName)
                 if obj.ReferenceEquals(workspace.ProjectTracker.GetProject(projectId), null) then
-                    let projectSite = new FSharpProjectSite(hier, this.SystemServiceProvider, workspace, site.ProjectFileName());
+                    let projectSite = new FSharpProjectSite(hier, this.SystemServiceProvider, workspace, projectFileName);
                     projectSite.Initialize(hier, site)                    
             | _ -> ()
         | _ -> ()
 
-and [<Guid(FSharpCommonConstants.editorFactoryGuid)>]
+and [<Guid(FSharpCommonConstants.editorFactoryGuidString)>]
     internal FSharpEditorFactory(package : FSharpPackage) =
     inherit AbstractEditorFactory(package)
 
     override this.ContentTypeName = FSharpCommonConstants.FSharpContentTypeName
-    override this.GetFormattedTextChanges(_, _, _, _) = System.Collections.Generic.List<Text.TextChange>() :> System.Collections.Generic.IList<Text.TextChange>
+    override this.GetFormattedTextChanges(_, _, _, _) = upcast Array.empty
 
-and [<Guid(FSharpCommonConstants.codePageEditorFactoryGuid)>]
+and [<Guid(FSharpCommonConstants.codePageEditorFactoryGuidString)>]
     internal FSharpCodePageEditorFactory(editorFactory: FSharpEditorFactory) =
     inherit AbstractCodePageEditorFactory(editorFactory)
 
-and [<Guid(FSharpCommonConstants.packageGuid)>]
+and [<Guid(FSharpCommonConstants.packageGuidString)>]
     internal FSharpPackage() = 
     inherit AbstractPackage<FSharpPackage, FSharpLanguageService, FSharpProjectSite>()
 
@@ -93,7 +92,7 @@ and [<Guid(FSharpCommonConstants.packageGuid)>]
         [|
             // editorFactory :> IVsEditorFactory;
             // codePageEditorFactory :> IVsEditorFactory;
-        |] :> IEnumerable<IVsEditorFactory>
+        |] :> seq<IVsEditorFactory>
 
     override this.RegisterMiscellaneousFilesWorkspaceInformation(_) = ()
 

vsintegration/src/FSharp.LanguageService/FSharpProjectSite.fs

@@ -38,28 +38,18 @@ type internal FSharpProjectSite(hierarchy: IVsHierarchy, serviceProvider: System
         site.AdviseProjectSiteClosed(FSharpCommonConstants.FSharpLanguageServiceCallbackName, 
                                         new AdviseProjectSiteChanges(fun () -> this.Disconnect()))
 
-        // Add files and references.
-        site.SourceFilesOnDisk() |> 
-            Seq.iter(fun file -> this.AddDocument(hier, file))
-        
-        this.GetReferences(site.CompilerFlags()) |> 
-            Seq.iter(fun ref -> this.AddReference(ref) |> ignore)
+        // Add files and references
+        for file in site.SourceFilesOnDisk() do this.AddDocument(hier, file)
+        for ref in this.GetReferences(site.CompilerFlags()) do this.AddReference(ref)
 
         // Capture the F# specific options that we'll pass to the type checker.
         checkOptions <- Some(ProjectSitesAndFiles.GetProjectOptionsForProjectSite(site, site.ProjectFileName()))
 
     member this.GetReferences(flags : string[]) =
-        let (|Reference|_|) (f : string) = if f.StartsWith("-r:") then Some (f.Replace("-r:", "")) else None
-
-        let references = flags |> 
-                         Seq.map(fun flag -> match flag with 
-                                             | Reference ref -> ref
-                                             | _ -> "") |>
-                         Seq.where(fun s -> s <> "")
-        references
+        flags |> Array.choose(fun flag -> if flag.StartsWith("-r:") then Some(flag.Substring(3)) else None)
 
     member this.AddReference(filePath : string) = 
-        this.AddMetadataReferenceAndTryConvertingToProjectReferenceIfPossible(filePath, new MetadataReferenceProperties(), VSConstants.S_FALSE)
+        this.AddMetadataReferenceAndTryConvertingToProjectReferenceIfPossible(filePath, new MetadataReferenceProperties(), VSConstants.S_FALSE) |> ignore
 
     member this.RemoveReference(filePath: string) =
         this.RemoveMetadataReference(filePath)
@@ -75,24 +65,19 @@ type internal FSharpProjectSite(hierarchy: IVsHierarchy, serviceProvider: System
 
     member internal this.OnProjectSettingsChanged(hier: IVsHierarchy, site : IProjectSite) = 
         let sourceFiles = site.SourceFilesOnDisk()
-        // Added files.
-        sourceFiles |> 
-            Seq.where(fun file -> not(this.ContainsFile(file))) |> 
-            Seq.iter(fun file -> this.AddDocument(hier, file))
-        // Removed files.
-        this.GetCurrentDocuments() |> 
-            Seq.where(fun doc -> not(sourceFiles |> Seq.contains(doc.FilePath))) |>
-            Seq.iter(fun doc -> this.RemoveDocument(doc))
 
+        // Added files
+        for file in sourceFiles do if not(this.ContainsFile(file)) then this.AddDocument(hier, file)
+        // Removed files
+        let removedDocuments = this.GetCurrentDocuments() |> Seq.where(fun doc -> not(sourceFiles |> Seq.contains(doc.FilePath))) |> Seq.toList
+        for doc in removedDocuments do this.RemoveDocument(doc)
+        
         let references = this.GetReferences(site.CompilerFlags())
+
         // Added references
-        references |> 
-            Seq.where(fun ref -> not(this.HasMetadataReference(ref))) |>
-            Seq.iter(fun ref -> this.AddReference(ref) |> ignore)
+        for ref in references do if not(this.HasMetadataReference(ref)) then this.AddReference(ref)
         // Removed references
-        this.GetCurrentMetadataReferences() |>
-            Seq.where(fun ref -> not(references |> Seq.contains(ref.FilePath))) |>
-            Seq.iter(fun ref -> this.RemoveReference(ref.FilePath))
+        for ref in this.GetCurrentMetadataReferences() do if not(references |> Seq.contains(ref.FilePath)) then this.RemoveReference(ref.FilePath)
 
         // If the order of files changed, that'll be captured in the checkOptions.
         checkOptions <- Some(ProjectSitesAndFiles.GetProjectOptionsForProjectSite(site, site.ProjectFileName()))

vsintegration/src/FSharp.ProjectSystem.FSharp/Project.fs

@@ -751,7 +751,7 @@ See also ...\SetupAuthoring\FSharp\Registry\FSProjSys_Registration.wxs, e.g.
 
             override x.GetGuidProperty(propid:int, guid:byref<Guid> ) =
                 if (enum propid = __VSHPROPID.VSHPROPID_PreferredLanguageSID) then 
-                    guid <- new Guid(FSharpCommonConstants.languageServiceGuid)
+                    guid <- new Guid(FSharpCommonConstants.languageServiceGuidString)
                     VSConstants.S_OK
                 // below is how VS decide 'which templates' to associate with an 'add new item' call in this project
                 elif (enum propid = __VSHPROPID2.VSHPROPID_AddItemTemplatesGuid) then