Merge pull request #659 from vasily-kirichenko/optimize-filename

Fix ProjectCracker, optimize Filename.checkPathForIllegalChars

Author: dsyme

src/fsharp/FSharp.Compiler.Service.ProjectCracker/ProjectCracker.fs

@@ -10,7 +10,6 @@ open System.IO
 open System
 
 type ProjectCracker =
-
     static member GetProjectOptionsFromProjectFileLogged(projectFileName : string, ?properties : (string * string) list, ?loadedTimeStamp, ?enableLogging) =
         let loadedTimeStamp = defaultArg loadedTimeStamp DateTime.MaxValue // Not 'now', we don't want to force reloading
         let properties = defaultArg properties []
@@ -19,10 +18,23 @@ type ProjectCracker =
 
         let rec convert (opts: Microsoft.FSharp.Compiler.SourceCodeServices.ProjectCrackerTool.ProjectOptions) : FSharpProjectOptions =
             let referencedProjects = Array.map (fun (a, b) -> a, convert b) opts.ReferencedProjectOptions
+            
+            let sourceFiles, otherOptions = 
+                opts.Options |> Array.partition (fun x -> Path.GetExtension(x).ToLower() = ".fs")
+            
+            let sepChar = Path.DirectorySeparatorChar
+            
+            let sourceFiles = sourceFiles |> Array.map (fun x -> 
+                match sepChar with
+                | '\\' -> x.Replace('/', '\\')
+                | '/' -> x.Replace('\\', '/')
+                | _ -> x
+            )
+
             logMap := Map.add opts.ProjectFile opts.LogOutput !logMap
             { ProjectFileName = opts.ProjectFile
-              ProjectFileNames = [| |]
-              OtherOptions = opts.Options
+              ProjectFileNames = sourceFiles
+              OtherOptions = otherOptions
               ReferencedProjects = referencedProjects
               IsIncompleteTypeCheckEnvironment = false
               UseScriptResolutionRules = false

src/utils/filename.fs

@@ -15,16 +15,28 @@ let illegalPathChars =
     let chars = Path.GetInvalidPathChars ()
     chars
 
-let checkPathForIllegalChars (path:string) =
-    let len = path.Length
-    for i = 0 to len - 1 do
-        let c = path.[i]
+type private PathState =
+    | Legal
+    | Illegal of path: string * illegalChar: char
+
+let private checkPathForIllegalChars =
+    let cache = System.Collections.Concurrent.ConcurrentDictionary<string, PathState>()
+    fun (path: string) ->
+        match cache.TryGetValue path with
+        | true, Legal -> () 
+        | true, Illegal (path, c) -> raise(IllegalFileNameChar(path, c))
+        | _ ->
+            let len = path.Length
+            for i = 0 to len - 1 do
+                let c = path.[i]
 
-        // Determine if this character is disallowed within a path by
-        // attempting to find it in the array of illegal path characters.
-        for badChar in illegalPathChars do
-            if c = badChar then
-                raise(IllegalFileNameChar(path, c))
+                // Determine if this character is disallowed within a path by
+                // attempting to find it in the array of illegal path characters.
+                for badChar in illegalPathChars do
+                    if c = badChar then
+                        cache.[path] <- Illegal(path, c)
+                        raise(IllegalFileNameChar(path, c))
+            cache.[path] <- Legal
 
 // Case sensitive (original behaviour preserved).
 let checkSuffix (x:string) (y:string) = x.EndsWith(y,System.StringComparison.Ordinal) 

tests/service/ExprTests.fs

@@ -872,7 +872,7 @@ let ``Check use of type provider that provides calls to F# code`` () =
         |> checker.ParseAndCheckProject 
         |> Async.RunSynchronously
 
-    res.Errors.Length |> shouldEqual 0
+    Assert.AreEqual ([||], res.Errors, sprintf "Should not be errors, but: %A" res.Errors)
 
     let results = 
         [ for f in res.AssemblyContents.ImplementationFiles do

tests/service/ProjectOptionsTests.fs

@@ -37,15 +37,22 @@ let checkOptionNotPresent (opts:string[]) s =
 let getReferencedFilenames = Array.choose (fun (o:string) -> if o.StartsWith("-r:") then o.[3..] |> (Path.GetFileName >> Some) else None)
 let getReferencedFilenamesAndContainingFolders = Array.choose (fun (o:string) -> if o.StartsWith("-r:") then o.[3..] |> (fun r -> ((r |> Path.GetFileName), (r |> Path.GetDirectoryName |> Path.GetFileName)) |> Some) else None)
 let getOutputFile = Array.pick (fun (o:string) -> if o.StartsWith("--out:") then o.[6..] |> Some else None)
-let getCompiledFilenames = Array.choose (fun (o:string) -> if o.EndsWith(".fs") then o |> (Path.GetFileName >> Some) else None)
+
+let getCompiledFilenames = 
+    Array.choose (fun (opt: string) -> 
+        if opt.EndsWith ".fs" then 
+            opt |> Path.GetFileName |> Some
+        else None)
+    >> Array.distinct
 
 [<Test>]
 let ``Project file parsing example 1 Default Configuration`` () = 
     let projectFile = __SOURCE_DIRECTORY__ + @"/FSharp.Compiler.Service.Tests.fsproj"
     let options = ProjectCracker.GetProjectOptionsFromProjectFile(projectFile)
 
+    checkOption options.ProjectFileNames "FileSystemTests.fs"
+    
     checkOption options.OtherOptions "FSharp.Compiler.Service.dll"
-    checkOption options.OtherOptions "FileSystemTests.fs"
     checkOption options.OtherOptions "--define:TRACE"
     checkOption options.OtherOptions "--define:DEBUG"
     checkOption options.OtherOptions "--flaterrors"
@@ -58,8 +65,9 @@ let ``Project file parsing example 1 Release Configuration`` () =
     // Check with Configuration = Release
     let options = ProjectCracker.GetProjectOptionsFromProjectFile(projectFile, [("Configuration", "Release")])
 
+    checkOption options.ProjectFileNames "FileSystemTests.fs"
+
     checkOption options.OtherOptions "FSharp.Compiler.Service.dll"
-    checkOption options.OtherOptions "FileSystemTests.fs"
     checkOption options.OtherOptions "--define:TRACE"
     checkOptionNotPresent options.OtherOptions "--define:DEBUG"
     checkOption options.OtherOptions "--debug:pdbonly"
@@ -68,11 +76,11 @@ let ``Project file parsing example 1 Release Configuration`` () =
 let ``Project file parsing example 1 Default configuration relative path`` () = 
     let projectFile = "FSharp.Compiler.Service.Tests.fsproj"
     Directory.SetCurrentDirectory(__SOURCE_DIRECTORY__)
-
     let options = ProjectCracker.GetProjectOptionsFromProjectFile(projectFile)
 
+    checkOption options.ProjectFileNames "FileSystemTests.fs"
+
     checkOption options.OtherOptions "FSharp.Compiler.Service.dll"
-    checkOption options.OtherOptions "FileSystemTests.fs"
     checkOption options.OtherOptions "--define:TRACE"
     checkOption options.OtherOptions "--define:DEBUG"
     checkOption options.OtherOptions "--flaterrors"
@@ -110,21 +118,16 @@ let ``Project file parsing Sample_VS2013_FSharp_Portable_Library_net451_adjusted
 
 [<Test>]
 let ``Project file parsing -- compile files 1``() =
-  let p = ProjectCracker.GetProjectOptionsFromProjectFile(__SOURCE_DIRECTORY__ + @"/data/Test1.fsproj")
-
-  p.OtherOptions
-  |> getCompiledFilenames
-  |> set
-  |> should equal (set [ "Test1File1.fs"; "Test1File2.fs" ])
+  let opts = ProjectCracker.GetProjectOptionsFromProjectFile(__SOURCE_DIRECTORY__ + @"/data/Test1.fsproj")
+  CollectionAssert.AreEqual (["Test1File2.fs"; "Test1File1.fs"], opts.ProjectFileNames |> Array.map Path.GetFileName)
+  CollectionAssert.IsEmpty (getCompiledFilenames opts.OtherOptions)
 
 [<Test>]
 let ``Project file parsing -- compile files 2``() =
-  let p = ProjectCracker.GetProjectOptionsFromProjectFile(__SOURCE_DIRECTORY__ + @"/data/Test2.fsproj")
+  let opts = ProjectCracker.GetProjectOptionsFromProjectFile(__SOURCE_DIRECTORY__ + @"/data/Test2.fsproj")
 
-  p.OtherOptions
-  |> getCompiledFilenames
-  |> set
-  |> should equal (set [ "Test2File1.fs"; "Test2File2.fs" ])
+  CollectionAssert.AreEqual (["Test2File2.fs"; "Test2File1.fs"], opts.ProjectFileNames |> Array.map Path.GetFileName)
+  CollectionAssert.IsEmpty (getCompiledFilenames opts.OtherOptions)
 
 [<Test>]
 let ``Project file parsing -- bad project file``() =
@@ -192,18 +195,16 @@ let ``Project file parsing -- reference project output file``() =
   |> Array.choose (fun (o:string) -> if o.StartsWith("-r:") then o.[3..] |> Some else None)
   |> should contain (normalizePath (__SOURCE_DIRECTORY__ + @"/data/DifferingOutputDir/Dir1/OutputDir1/Test1.dll"))
 
-
 [<Test>]
 let ``Project file parsing -- Tools Version 12``() =
-  let p = ProjectCracker.GetProjectOptionsFromProjectFile(__SOURCE_DIRECTORY__ + @"/data/ToolsVersion12.fsproj")
-
-  checkOption (getReferencedFilenames p.OtherOptions) "System.Core.dll"
+  let opts = ProjectCracker.GetProjectOptionsFromProjectFile(__SOURCE_DIRECTORY__ + @"/data/ToolsVersion12.fsproj")
+  checkOption (getReferencedFilenames opts.OtherOptions) "System.Core.dll"
 
 [<Test>]
 let ``Project file parsing -- Logging``() =
-  let f = normalizePath (__SOURCE_DIRECTORY__ + @"/data/ToolsVersion12.fsproj")
-  let p, logMap = ProjectCracker.GetProjectOptionsFromProjectFileLogged(f, enableLogging=true)
-  let log = logMap.[f]
+  let projectFileName = normalizePath (__SOURCE_DIRECTORY__ + @"/data/ToolsVersion12.fsproj")
+  let _, logMap = ProjectCracker.GetProjectOptionsFromProjectFileLogged(projectFileName, enableLogging=true)
+  let log = logMap.[projectFileName]
 
   if runningOnMono then
     Assert.That(log, Is.StringContaining("Reference System.Core resolved"))
@@ -386,30 +387,26 @@ let ``Project file parsing -- Exe with a PCL reference``() =
 
 [<Test>]
 let ``Project file parsing -- project file contains project reference to out-of-solution project and is used in release mode``() =
-
-    let f = normalizePath(__SOURCE_DIRECTORY__ + @"/data/Test2.fsproj")
-    let p = ProjectCracker.GetProjectOptionsFromProjectFile(f,[("Configuration","Release")])
-    let references = getReferencedFilenamesAndContainingFolders p.OtherOptions |> set
+    let projectFileName = normalizePath(__SOURCE_DIRECTORY__ + @"/data/Test2.fsproj")
+    let opts = ProjectCracker.GetProjectOptionsFromProjectFile(projectFileName,[("Configuration","Release")])
+    let references = getReferencedFilenamesAndContainingFolders opts.OtherOptions |> set
     // Check the reference is to a release DLL
     references |> should contain ("Test1.dll", "Release")
 
 [<Test>]
 let ``Project file parsing -- project file contains project reference to out-of-solution project and is used in debug mode``() =
 
-    let f = normalizePath(__SOURCE_DIRECTORY__ + @"/data/Test2.fsproj")
-    let p = ProjectCracker.GetProjectOptionsFromProjectFile(f,[("Configuration","Debug")])
-    let references = getReferencedFilenamesAndContainingFolders p.OtherOptions |> set
+    let projectFileName = normalizePath(__SOURCE_DIRECTORY__ + @"/data/Test2.fsproj")
+    let opts = ProjectCracker.GetProjectOptionsFromProjectFile(projectFileName,[("Configuration","Debug")])
+    let references = getReferencedFilenamesAndContainingFolders opts.OtherOptions |> set
     // Check the reference is to a debug DLL
     references |> should contain ("Test1.dll", "Debug")
 
 [<Test>]
 let ``Project file parsing -- space in file name``() =
-  let p = ProjectCracker.GetProjectOptionsFromProjectFile(__SOURCE_DIRECTORY__ + @"/data/Space in name.fsproj")
-
-  p.OtherOptions
-  |> getCompiledFilenames
-  |> set
-  |> should equal (set [ "Test2File1.fs"; "Test2File2.fs" ])
+  let opts = ProjectCracker.GetProjectOptionsFromProjectFile(__SOURCE_DIRECTORY__ + @"/data/Space in name.fsproj")
+  CollectionAssert.AreEqual (["Test2File2.fs"; "Test2File1.fs"], opts.ProjectFileNames |> Array.map Path.GetFileName)
+  CollectionAssert.IsEmpty (getCompiledFilenames opts.OtherOptions)
 
 [<Test>]
 let ``Project file parsing -- report files``() =