Improve performance of String.concat for seq of type array or list (dotnet#9483)

* Move `String.length` to the top of its module so that the `length` function is in scope

* Improve performance of String.concat for arrays and lists

* Make concatArray local to String.concat

* Testing String.concat, make sure the new three paths are covered

* Remove "foo", "bax", "bar" in tests, making them more readable

Co-authored-by: Phillip Carter <[email protected]>

Author: 2 people

src/fsharp/FSharp.Core/string.fs

@@ -8,6 +8,7 @@ namespace Microsoft.FSharp.Core
     open Microsoft.FSharp.Core.Operators
     open Microsoft.FSharp.Core.Operators.Checked
     open Microsoft.FSharp.Collections
+    open Microsoft.FSharp.Primitives.Basics
 
     [<CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]
     [<RequireQualifiedAccess>]
@@ -22,7 +23,25 @@ namespace Microsoft.FSharp.Core
 
         [<CompiledName("Concat")>]
         let concat sep (strings : seq<string>) =  
-            String.Join(sep, strings)
+
+            let concatArray sep (strings: string []) =
+                match length sep with
+                | 0 -> String.Concat strings
+                // following line should be used when this overload becomes part of .NET Standard (it's only in .NET Core)
+                //| 1 -> String.Join(sep.[0], strings, 0, strings.Length)
+                | _ -> String.Join(sep, strings, 0, strings.Length)
+
+            match strings with
+            | :? array<string> as arr -> 
+                concatArray sep arr
+
+            | :? list<string> as lst -> 
+                lst 
+                |> List.toArray 
+                |> concatArray sep
+
+            | _ ->
+                String.Join(sep, strings)
 
         [<CompiledName("Iterate")>]
         let iter (action : (char -> unit)) (str:string) =

tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs

@@ -23,31 +23,27 @@ type StringModule() =
 
     [<Test>]
     member this.Concat() =
-        let e1 = String.concat null ["foo"]
-        Assert.AreEqual("foo", e1)
-        
-        let e2 = String.concat "" []
-        Assert.AreEqual("", e2)
-        
-        let e3 = String.concat "foo" []
-        Assert.AreEqual("", e3)        
-        
-        let e4 = String.concat "" [null]
-        Assert.AreEqual("", e4)
-        
-        let e5 = String.concat "" [""]
-        Assert.AreEqual("", e5)
-        
-        let e6 = String.concat "foo" ["bar"]
-        Assert.AreEqual("bar", e6)
-        
-        let e7 = String.concat "foo" ["bav";"baz"]
-        Assert.AreEqual("bavfoobaz", e7)
-
-        let e8 = String.concat "foo" [null;"baz";null;"bar"]
-        Assert.AreEqual("foobazfoofoobar", e8)
-        
-        CheckThrowsArgumentNullException(fun () -> String.concat "foo" null |> ignore)
+        /// This tests the three paths of String.concat w.r.t. array, list, seq
+        let execTest f expected arg = 
+            let r1 = f (List.toSeq arg)
+            Assert.AreEqual(expected, r1)
+
+            let r2 = f (List.toArray arg)
+            Assert.AreEqual(expected, r2)
+
+            let r3 = f arg
+            Assert.AreEqual(expected, r3)
+
+        do execTest (String.concat null) "world" ["world"]
+        do execTest (String.concat "") "" []
+        do execTest (String.concat "|||") "" []
+        do execTest (String.concat "") "" [null]
+        do execTest (String.concat "") "" [""]
+        do execTest (String.concat "|||") "apples" ["apples"]
+        do execTest (String.concat " ") "happy together" ["happy"; "together"]
+        do execTest (String.concat "Me! ") "Me! No, you. Me! Me! Oh, them." [null;"No, you. ";null;"Oh, them."]
+
+        CheckThrowsArgumentNullException(fun () -> String.concat "%%%" null |> ignore)
 
     [<Test>]
     member this.Iter() =