Make IgnoreFile more efficient

paket.dependencies

@@ -7,6 +7,8 @@ nuget FSharp.Core 6.0.1
 nuget FSharp.Compiler.Service 41.0.1
 nuget FsUnit
 nuget FsCheck
+nuget System.IO.Abstractions 16.1.10
+nuget System.IO.Abstractions.TestingHelpers 16.1.10
 nuget Microsoft.NET.Test.Sdk 16.9.1
 nuget nunit 3.13.1
 nuget NUnit3TestAdapter 4.0.0-beta.1
@@ -51,4 +53,4 @@ group client
     nuget FSharp.Core 5
     nuget StreamJsonRpc
     nuget Microsoft.SourceLink.GitHub copy_local: true
-    nuget SemanticVersioning
+    nuget SemanticVersioning

paket.lock

@@ -303,6 +303,11 @@ NUGET
       System.Runtime (>= 4.3)
       System.Text.Encoding (>= 4.3)
       System.Threading.Tasks (>= 4.3)
+    System.IO.Abstractions (16.1.10)
+      System.IO.FileSystem.AccessControl (>= 4.7) - restriction: || (&& (== net5.0) (< netstandard2.1)) (== netcoreapp3.1) (== netstandard2.0)
+      System.IO.FileSystem.AccessControl (>= 5.0) - restriction: || (== net5.0) (&& (== netcoreapp3.1) (>= net5.0)) (&& (== netstandard2.0) (>= net5.0))
+    System.IO.Abstractions.TestingHelpers (16.1.10)
+      System.IO.Abstractions (>= 16.1.10)
     System.IO.FileSystem (4.3)
       Microsoft.NETCore.Platforms (>= 1.1)
       Microsoft.NETCore.Targets (>= 1.1)
@@ -312,6 +317,11 @@ NUGET
       System.Runtime.Handles (>= 4.3)
       System.Text.Encoding (>= 4.3)
       System.Threading.Tasks (>= 4.3)
+    System.IO.FileSystem.AccessControl (5.0)
+      System.Buffers (>= 4.5.1) - restriction: || (&& (== net5.0) (>= monoandroid) (< netstandard1.3)) (&& (== net5.0) (>= monotouch)) (&& (== net5.0) (< netcoreapp2.0)) (&& (== net5.0) (>= xamarinios)) (&& (== net5.0) (>= xamarinmac)) (&& (== net5.0) (>= xamarintvos)) (&& (== net5.0) (>= xamarinwatchos)) (&& (== netcoreapp3.1) (>= monoandroid) (< netstandard1.3)) (&& (== netcoreapp3.1) (>= monotouch)) (&& (== netcoreapp3.1) (< netcoreapp2.0)) (&& (== netcoreapp3.1) (>= xamarinios)) (&& (== netcoreapp3.1) (>= xamarinmac)) (&& (== netcoreapp3.1) (>= xamarintvos)) (&& (== netcoreapp3.1) (>= xamarinwatchos)) (== netstandard2.0)
+      System.Memory (>= 4.5.4) - restriction: || (&& (== net5.0) (< netcoreapp2.0)) (&& (== net5.0) (< netcoreapp2.1)) (&& (== net5.0) (>= uap10.1)) (&& (== netcoreapp3.1) (< netcoreapp2.0)) (&& (== netcoreapp3.1) (< netcoreapp2.1)) (&& (== netcoreapp3.1) (>= uap10.1)) (== netstandard2.0)
+      System.Security.AccessControl (>= 5.0)
+      System.Security.Principal.Windows (>= 5.0)
     System.IO.FileSystem.Primitives (4.3)
       System.Runtime (>= 4.3)
     System.Linq (4.3)

src/Fantomas.Extras/AssemblyInfo.fs

@@ -0,0 +1,7 @@
+namespace Fantomas.Extras.AssemblyInfo
+
+open System.Runtime.CompilerServices
+
+[<assembly: InternalsVisibleTo("Fantomas.Tests")>]
+
+do ()

src/Fantomas.Extras/Fantomas.Extras.fsproj

@@ -16,6 +16,7 @@
     <None Include="paket.references" />
     <None Include="..\..\LICENSE.md" Visible="false" Pack="true" PackagePath="" />
     <None Include="..\..\fantomas_logo.png" Visible="false" Pack="true" PackagePath="" />
+    <Compile Include="AssemblyInfo.fs" />
     <Compile Include="EditorConfig.fs" />
     <Compile Include="IgnoreFile.fs" />
     <Compile Include="FakeHelpers.fs" />
@@ -24,4 +25,4 @@
     <ProjectReference Include="..\Fantomas\Fantomas.fsproj" />
   </ItemGroup>
   <Import Project="..\..\.paket\Paket.Restore.targets" />
-</Project>
+</Project>

src/Fantomas.Extras/IgnoreFile.fs

@@ -3,44 +3,83 @@ namespace Fantomas.Extras
 [<RequireQualifiedAccess>]
 module IgnoreFile =
 
-    open System.IO
+    open System.Collections.Concurrent
+    open System.IO.Abstractions
     open MAB.DotIgnore
 
     [<Literal>]
     let IgnoreFileName = ".fantomasignore"
 
-    let rec private findIgnoreFile (filePath: string) : string option =
-        let allParents =
-            let rec addParent (di: DirectoryInfo) (finalContinuation: string list -> string list) =
-                if isNull di.Parent then
-                    finalContinuation [ di.FullName ]
+    let internal findIgnoreFile<'a>
+        (fs: IFileSystem)
+        (readFile: string -> 'a)
+        (ignoreFiles: ConcurrentDictionary<string, 'a option>)
+        (filePath: string)
+        : 'a option =
+        // Note that this function has side effects: it mutates the state `ignoreFiles`.
+        let rec findIgnoreFileAbove (path: IDirectoryInfo) : 'a option =
+            let potentialFile = fs.Path.Combine(path.FullName, IgnoreFileName)
+
+            match ignoreFiles.TryGetValue path.FullName with
+            | true, Some f -> Some f
+            | true, None ->
+                // A slight inefficiency here, in exchange for making the code a lot shorter:
+                // if we've already computed that there is no .fantomasignore file immediately at this level,
+                // we keep looking upwards in our knowledge of the file hierarchy.
+                // (This is inefficient: we could in theory have stored the actual final result at all levels
+                // once we computed it the first time.)
+                if isNull path.Parent then
+                    None
                 else
-                    addParent di.Parent (fun parents -> di.FullName :: parents |> finalContinuation)
+                    findIgnoreFileAbove path.Parent
+            | _, _ ->
+                let result =
+                    if fs.File.Exists potentialFile then
+                        readFile potentialFile |> Some
+                    else
+                        None
 
-            addParent (Directory.GetParent filePath) id
+                ignoreFiles.TryAdd(path.FullName, result)
+                // We don't care if another thread beat us to this, they'll have
+                // inserted exactly what we wanted to insert anyway
+                |> ignore
 
-        allParents
-        |> List.tryFind (fun p -> Path.Combine(p, IgnoreFileName) |> File.Exists)
-        |> Option.map (fun p -> Path.Combine(p, IgnoreFileName))
+                match result with
+                | None ->
+                    if isNull path.Parent then
+                        None
+                    else
+                        findIgnoreFileAbove path.Parent
+                | Some _ -> result
 
-    let private relativePathPrefix = sprintf ".%c" Path.DirectorySeparatorChar
+        findIgnoreFileAbove (fs.Directory.GetParent filePath)
 
-    let private removeRelativePathPrefix (path: string) =
-        if path.StartsWith(relativePathPrefix) then
+    let private removeRelativePathPrefix (fs: IFileSystem) (path: string) =
+        if
+            path.StartsWith(sprintf ".%c" fs.Path.DirectorySeparatorChar)
+            || path.StartsWith(sprintf ".%c" fs.Path.AltDirectorySeparatorChar)
+        then
             path.Substring(2)
         else
             path
 
+    /// Global static state.
+    /// Store of the IgnoreFiles present in each folder discovered so far.
+    /// This is to save repeatedly hitting the disk for each file, and to save
+    /// loading the IgnoreLists from the disk repeatedly (which is nontrivially expensive!).
+    let private ignoreFiles: ConcurrentDictionary<string, IgnoreList option> =
+        ConcurrentDictionary()
+
     let isIgnoredFile (file: string) =
-        let fullPath = Path.GetFullPath(file)
+        let fs = FileSystem()
+        let fullPath = fs.Path.GetFullPath(file)
 
-        match findIgnoreFile fullPath with
+        match findIgnoreFile fs IgnoreList ignoreFiles fullPath with
         | None -> false
-        | Some ignoreFile ->
-            let ignores = IgnoreList(ignoreFile)
+        | Some ignores ->
 
             try
-                let path = removeRelativePathPrefix file
+                let path = removeRelativePathPrefix fs file
                 ignores.IsIgnored(path, false)
             with
             | ex ->

src/Fantomas.Extras/paket.references

@@ -1,4 +1,5 @@
 FSharp.Core
 editorconfig
 MAB.DotIgnore
-Microsoft.SourceLink.GitHub
+Microsoft.SourceLink.GitHub
+System.IO.Abstractions

src/Fantomas.Tests/Fantomas.Tests.fsproj

@@ -92,6 +92,7 @@
     <Compile Include="IndexSyntaxTests.fs" />
     <Compile Include="InsertFinalNewlineTests.fs" />
     <Compile Include="SingleExprTests.fs" />
+    <Compile Include="IgnoreFileTests.fs" />
   </ItemGroup>
   <ItemGroup>
     <ProjectReference Include="..\Fantomas.Extras\Fantomas.Extras.fsproj" />

src/Fantomas.Tests/IgnoreFileTests.fs

@@ -0,0 +1,140 @@
+module Fantomas.Tests.TestIgnoreFile
+
+open System.Collections.Concurrent
+open System.Collections.Generic
+open NUnit.Framework
+open FsUnitTyped
+open Fantomas.Extras
+open System.IO.Abstractions
+open System.IO.Abstractions.TestingHelpers
+
+let private makeFileHierarchy (fs: IFileSystem) (filePaths: string list) : unit =
+    for path in filePaths do
+        let fileInfo = fs.FileInfo.FromFileName path
+        fileInfo.Directory.Create()
+        fs.File.WriteAllText(fileInfo.FullName, "some text")
+
+let private snapshot (fs: ConcurrentDictionary<'k, 'v>) : ('k * 'v) list =
+    fs
+    |> Seq.map (function
+        | KeyValue (k, v) -> (k, v))
+    |> Seq.toList
+    |> List.sort
+
+/// A helper method to create a `parse` function for injection into IgnoreFile;
+/// this `parse` function will throw if it sees the same key twice.
+/// This allows us to test that we never attempt to read the same .fantomasignore file twice.
+let private oneShotParser () : string -> string =
+    let seen = HashSet()
+
+    fun s ->
+        if seen.Add s then
+            s
+        else
+            failwithf "Seen duplicate key: %s" s
+
+[<TestCase true>]
+[<TestCase false>]
+let ``findIgnoreFile does not crash at the root`` (fantomasIgnorePresent: bool) =
+    let fs = MockFileSystem()
+    let root = fs.Path.GetTempPath() |> fs.Path.GetPathRoot
+
+    let fileAtRoot = fs.Path.Combine(root, "SomeFile.fs")
+
+    let fantomasIgnore =
+        if fantomasIgnorePresent then
+            let target = fs.Path.Combine(root, ".fantomasignore")
+            makeFileHierarchy fs [ target ]
+            Some target
+        else
+            None
+
+    let readFile = oneShotParser ()
+
+    let dict = ConcurrentDictionary()
+
+    IgnoreFile.findIgnoreFile fs readFile dict fileAtRoot
+    |> shouldEqual fantomasIgnore
+
+    snapshot dict
+    |> shouldEqual [ root, fantomasIgnore ]
+
+[<Test>]
+let ``findIgnoreFile preferentially finds the fantomasignore next to the source file`` () =
+    let fs = MockFileSystem()
+    let root = fs.Path.GetTempPath() |> fs.Path.GetPathRoot
+
+    let source = fs.Path.Combine(root, "folder1", "folder2", "SomeSource.fs")
+    let target = fs.Path.Combine(root, "folder1", "folder2", ".fantomasignore")
+
+    [ source
+      target
+      // Another couple, at higher levels of the hierarchy
+      fs.Path.Combine(root, "folder1", ".fantomasignore")
+      fs.Path.Combine(root, ".fantomasignore") ]
+    |> makeFileHierarchy fs
+
+    let readFile = oneShotParser ()
+
+    let dict = ConcurrentDictionary()
+
+    IgnoreFile.findIgnoreFile fs readFile dict source
+    |> shouldEqual (Some target)
+
+    snapshot dict
+    |> shouldEqual [ fs.Directory.GetParent(target).FullName, Some target ]
+
+[<Test>]
+let ``findIgnoreFile can find the fantomasignore one layer up from the source file`` () =
+    let fs = MockFileSystem()
+    let root = fs.Path.GetTempPath() |> fs.Path.GetPathRoot
+
+    let source = fs.Path.Combine(root, "folder1", "folder2", "SomeSource.fs")
+    let target = fs.Path.Combine(root, "folder1", ".fantomasignore")
+
+    [ source
+      target
+      // Another one, at a higher level of the hierarchy
+      fs.Path.Combine(root, ".fantomasignore") ]
+    |> makeFileHierarchy fs
+
+    let readFile = oneShotParser ()
+
+    let dict = ConcurrentDictionary()
+
+    IgnoreFile.findIgnoreFile fs readFile dict source
+    |> shouldEqual (Some target)
+
+    snapshot dict
+    |> shouldEqual [ fs.Directory.GetParent(target).FullName, Some target
+                     fs.Directory.GetParent(source).FullName, None ]
+
+[<Test>]
+let ``findIgnoreFile uses the store`` () =
+    let fs = MockFileSystem()
+    let root = fs.Path.GetTempPath() |> fs.Path.GetPathRoot
+
+    let source1 = fs.Path.Combine(root, "folder1", "folder2", "Source1.fs")
+    let source2 = fs.Path.Combine(root, "folder1", "folder2", "Source2.fs")
+    let target = fs.Path.Combine(root, "folder1", ".fantomasignore")
+
+    [ source1; source2; target ]
+    |> makeFileHierarchy fs
+
+    let readFile = oneShotParser ()
+
+    let dict = ConcurrentDictionary()
+
+    IgnoreFile.findIgnoreFile fs readFile dict source1
+    |> shouldEqual (Some target)
+
+    snapshot dict
+    |> shouldEqual [ fs.Directory.GetParent(target).FullName, Some target
+                     fs.Directory.GetParent(source1).FullName, None ]
+
+    IgnoreFile.findIgnoreFile fs readFile dict source2
+    |> shouldEqual (Some target)
+
+    snapshot dict
+    |> shouldEqual [ fs.Directory.GetParent(target).FullName, Some target
+                     fs.Directory.GetParent(source1).FullName, None ]

src/Fantomas.Tests/TestHelpers.fs

@@ -11,7 +11,6 @@ open FSharp.Compiler.Syntax
 open FSharp.Compiler.Xml
 open Fantomas.FormatConfig
 open Fantomas
-open Fantomas.Extras
 
 [<assembly: Parallelizable(ParallelScope.All)>]
 do ()

src/Fantomas.Tests/paket.references

@@ -4,4 +4,5 @@ FsCheck
 Microsoft.NET.Test.Sdk
 NUnit3TestAdapter
 NunitXml.TestLogger
-FSharp.Core
+FSharp.Core
+System.IO.Abstractions.TestingHelpers