[Repo Assist] Fix moveFileUp/Down: preserve vertical formatting and fix first-press no-op

src/FsAutoComplete.Core/FsprojEdit.fs

@@ -46,39 +46,91 @@ module FsProjEditor =
     node.SetAttribute("Include", includePath)
     node
 
+  /// Find the previous sibling that is a `<Compile>` Element node, skipping over whitespace-only
+  /// text nodes and non-`Compile` elements that are present when PreserveWhitespace is true.
+  let private previousSiblingElement (node: System.Xml.XmlNode) =
+    let mutable prev = node.PreviousSibling
+
+    // Skip over non-element nodes (e.g., whitespace) and elements that are not `<Compile>`.
+    while not (isNull prev)
+          && (prev.NodeType <> System.Xml.XmlNodeType.Element || prev.LocalName <> "Compile") do
+      prev <- prev.PreviousSibling
+
+    prev
+
+  /// Find the next sibling that is a `<Compile>` Element node, skipping over whitespace-only
+  /// text nodes and non-`Compile` elements that are present when PreserveWhitespace is true.
+  let private nextSiblingElement (node: System.Xml.XmlNode) =
+    let mutable next = node.NextSibling
+
+    // Skip over non-element nodes (e.g., whitespace) and elements that are not `<Compile>`.
+    while not (isNull next)
+          && (next.NodeType <> System.Xml.XmlNodeType.Element || next.LocalName <> "Compile") do
+      next <- next.NextSibling
+
+    next
+
   let moveFileUp (fsprojPath: string) (file: string) =
     let xDoc = System.Xml.XmlDocument()
     xDoc.PreserveWhitespace <- true // to keep custom formatting if present
     xDoc.Load fsprojPath
     let xpath = sprintf "//Compile[@Include='%s']/.." file
     let itemGroup = xDoc.SelectSingleNode(xpath)
-    let childXPath = sprintf "//Compile[@Include='%s']" file
-    let node = itemGroup.SelectSingleNode(childXPath)
-    let upNode = node.PreviousSibling
 
-    if isNull upNode then
+    if isNull itemGroup then
       ()
     else
-      itemGroup.RemoveChild node |> ignore
-      itemGroup.InsertBefore(node, upNode) |> ignore
-      xDoc.Save fsprojPath
+      let childXPath = sprintf "//Compile[@Include='%s']" file
+      let node = itemGroup.SelectSingleNode(childXPath)
+
+      if isNull node then
+        ()
+      else
+        let prevElement = previousSiblingElement node
+
+        if isNull prevElement then
+          ()
+        else
+          // Clone both elements, insert clones in swapped positions, then remove the originals.
+          // Whitespace text nodes (indentation) remain in place so vertical formatting is preserved.
+          let nodeClone = node.CloneNode(true)
+          let prevClone = prevElement.CloneNode(true)
+          itemGroup.InsertBefore(nodeClone, prevElement) |> ignore
+          itemGroup.InsertBefore(prevClone, node) |> ignore
+          itemGroup.RemoveChild(prevElement) |> ignore
+          itemGroup.RemoveChild(node) |> ignore
+          xDoc.Save fsprojPath
 
   let moveFileDown (fsprojPath: string) (file: string) =
     let xDoc = System.Xml.XmlDocument()
     xDoc.PreserveWhitespace <- true // to keep custom formatting if present
     xDoc.Load fsprojPath
     let xpath = sprintf "//Compile[@Include='%s']/.." file
     let itemGroup = xDoc.SelectSingleNode(xpath)
-    let childXPath = sprintf "//Compile[@Include='%s']" file
-    let node = itemGroup.SelectSingleNode(childXPath)
-    let downNode = node.NextSibling
 
-    if isNull downNode then
+    if isNull itemGroup then
       ()
     else
-      itemGroup.RemoveChild node |> ignore
-      itemGroup.InsertAfter(node, downNode) |> ignore
-      xDoc.Save fsprojPath
+      let childXPath = sprintf "//Compile[@Include='%s']" file
+      let node = itemGroup.SelectSingleNode(childXPath)
+
+      if isNull node then
+        ()
+      else
+        let nextElement = nextSiblingElement node
+
+        if isNull nextElement then
+          ()
+        else
+          // Clone both elements, insert clones in swapped positions, then remove the originals.
+          // Whitespace text nodes (indentation) remain in place so vertical formatting is preserved.
+          let nodeClone = node.CloneNode(true)
+          let nextClone = nextElement.CloneNode(true)
+          itemGroup.InsertBefore(nextClone, node) |> ignore
+          itemGroup.InsertBefore(nodeClone, nextElement) |> ignore
+          itemGroup.RemoveChild(node) |> ignore
+          itemGroup.RemoveChild(nextElement) |> ignore
+          xDoc.Save fsprojPath
 
   let addFileAbove (fsprojPath: string) (aboveFile: string) (newFileName: string) =
     let xDoc = System.Xml.XmlDocument()

test/FsAutoComplete.Tests.Lsp/FsProjEditorTests.fs

@@ -0,0 +1,251 @@
+module FsAutoComplete.Tests.FsProjEditorTests
+
+open Expecto
+open FsAutoComplete
+open System
+open System.IO
+open System.Xml
+
+/// Create a temporary .fsproj file whose ItemGroup contains the given files
+/// (one <Compile Include="..." /> per line, LF line endings).
+/// Returns the path; caller must delete.
+let private createTempFsproj (files: string list) =
+  let path = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName() + ".fsproj")
+
+  let filesXml =
+    files
+    |> List.map (fun f -> $"    <Compile Include=\"{f}\" />")
+    |> String.concat "\n"
+
+  let content =
+    $"<Project Sdk=\"Microsoft.NET.Sdk\">\n  <PropertyGroup>\n    <TargetFramework>net8.0</TargetFramework>\n  </PropertyGroup>\n  <ItemGroup>\n{filesXml}\n  </ItemGroup>\n</Project>"
+
+  File.WriteAllText(path, content)
+  path
+
+/// Create a temporary .fsproj file with CRLF line endings.
+let private createTempFsprojCrlf (files: string list) =
+  let path = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName() + ".fsproj")
+
+  let filesXml =
+    files
+    |> List.map (fun f -> $"    <Compile Include=\"{f}\" />")
+    |> String.concat "\r\n"
+
+  let content =
+    $"<Project Sdk=\"Microsoft.NET.Sdk\">\r\n  <PropertyGroup>\r\n    <TargetFramework>net8.0</TargetFramework>\r\n  </PropertyGroup>\r\n  <ItemGroup>\r\n{filesXml}\r\n  </ItemGroup>\r\n</Project>"
+
+  File.WriteAllText(path, content)
+  path
+
+/// Return the ordered list of <Compile Include="..." /> values from a .fsproj.
+let private getCompileOrder (fsprojPath: string) =
+  let xDoc = XmlDocument()
+  xDoc.Load fsprojPath
+
+  xDoc.SelectNodes("//Compile[@Include]")
+  |> Seq.cast<XmlNode>
+  |> Seq.map (fun n -> n.Attributes.["Include"].InnerText)
+  |> Seq.toList
+
+/// Return true when every <Compile .../> element appears on its own line
+/// (i.e. no two Compile tags share the same line in the saved file).
+let private eachCompileOnOwnLine (fsprojPath: string) =
+  let lines = File.ReadAllLines(fsprojPath)
+
+  lines
+  |> Array.filter (fun l -> l.Contains("<Compile"))
+  |> Array.forall (fun l ->
+    // A line that contains exactly one <Compile open tag is fine.
+    // Count occurrences of "<Compile" – if > 1 they are horizontal.
+    let occurrences =
+      let mutable count = 0
+      let mutable idx = 0
+
+      while idx < l.Length do
+        let pos = l.IndexOf("<Compile", idx, StringComparison.Ordinal)
+
+        if pos < 0 then
+          idx <- l.Length
+        else
+          count <- count + 1
+          idx <- pos + 1
+
+      count
+
+    occurrences <= 1)
+
+let allTests =
+  testList
+    "FsProjEditor"
+    [ testList
+        "moveFileUp"
+        [ testCase "moves second file above first"
+          <| fun _ ->
+            let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileUp path "B.fs"
+              Expect.equal (getCompileOrder path) [ "B.fs"; "A.fs"; "C.fs" ] "B.fs should precede A.fs"
+            finally
+              File.Delete path
+
+          testCase "moves last file up"
+          <| fun _ ->
+            let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileUp path "C.fs"
+              Expect.equal (getCompileOrder path) [ "A.fs"; "C.fs"; "B.fs" ] "C.fs should precede B.fs"
+            finally
+              File.Delete path
+
+          testCase "first file is unchanged"
+          <| fun _ ->
+            let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileUp path "A.fs"
+              Expect.equal (getCompileOrder path) [ "A.fs"; "B.fs"; "C.fs" ] "Order should be unchanged"
+            finally
+              File.Delete path
+
+          testCase "moving up twice reaches top"
+          <| fun _ ->
+            let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileUp path "C.fs"
+              FsProjEditor.moveFileUp path "C.fs"
+              Expect.equal (getCompileOrder path) [ "C.fs"; "A.fs"; "B.fs" ] "C.fs should be first"
+            finally
+              File.Delete path
+
+          testCase "single press actually changes compile order (regression: required two presses)"
+          <| fun _ ->
+            let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileUp path "C.fs"
+              let order = getCompileOrder path
+              Expect.notEqual order [ "A.fs"; "B.fs"; "C.fs" ] "Compile order must change on first call"
+            finally
+              File.Delete path
+
+          testCase "preserves vertical formatting with LF line endings"
+          <| fun _ ->
+            let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileUp path "B.fs"
+              Expect.isTrue (eachCompileOnOwnLine path) "Each Compile element must remain on its own line"
+            finally
+              File.Delete path
+
+          testCase "preserves vertical formatting with CRLF line endings"
+          <| fun _ ->
+            let path = createTempFsprojCrlf [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileUp path "B.fs"
+              Expect.isTrue (eachCompileOnOwnLine path) "Each Compile element must remain on its own line (CRLF)"
+            finally
+              File.Delete path ]
+
+      testList
+        "moveFileDown"
+        [ testCase "moves first file below second"
+          <| fun _ ->
+            let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileDown path "A.fs"
+              Expect.equal (getCompileOrder path) [ "B.fs"; "A.fs"; "C.fs" ] "A.fs should follow B.fs"
+            finally
+              File.Delete path
+
+          testCase "moves middle file down"
+          <| fun _ ->
+            let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileDown path "B.fs"
+              Expect.equal (getCompileOrder path) [ "A.fs"; "C.fs"; "B.fs" ] "B.fs should follow C.fs"
+            finally
+              File.Delete path
+
+          testCase "last file is unchanged"
+          <| fun _ ->
+            let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileDown path "C.fs"
+              Expect.equal (getCompileOrder path) [ "A.fs"; "B.fs"; "C.fs" ] "Order should be unchanged"
+            finally
+              File.Delete path
+
+          testCase "moving down twice reaches bottom"
+          <| fun _ ->
+            let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileDown path "A.fs"
+              FsProjEditor.moveFileDown path "A.fs"
+              Expect.equal (getCompileOrder path) [ "B.fs"; "C.fs"; "A.fs" ] "A.fs should be last"
+            finally
+              File.Delete path
+
+          testCase "single press actually changes compile order (regression: required two presses)"
+          <| fun _ ->
+            let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileDown path "A.fs"
+              let order = getCompileOrder path
+              Expect.notEqual order [ "A.fs"; "B.fs"; "C.fs" ] "Compile order must change on first call"
+            finally
+              File.Delete path
+
+          testCase "preserves vertical formatting with LF line endings"
+          <| fun _ ->
+            let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileDown path "B.fs"
+              Expect.isTrue (eachCompileOnOwnLine path) "Each Compile element must remain on its own line"
+            finally
+              File.Delete path
+
+          testCase "preserves vertical formatting with CRLF line endings"
+          <| fun _ ->
+            let path = createTempFsprojCrlf [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileDown path "B.fs"
+              Expect.isTrue (eachCompileOnOwnLine path) "Each Compile element must remain on its own line (CRLF)"
+            finally
+              File.Delete path ]
+
+      testList
+        "moveFileUp and moveFileDown roundtrip"
+        [ testCase "move up then down restores original order"
+          <| fun _ ->
+            let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileUp path "B.fs"
+              FsProjEditor.moveFileDown path "B.fs"
+              Expect.equal (getCompileOrder path) [ "A.fs"; "B.fs"; "C.fs" ] "Round-trip should restore original order"
+            finally
+              File.Delete path
+
+          testCase "move down then up restores original order"
+          <| fun _ ->
+            let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
+
+            try
+              FsProjEditor.moveFileDown path "B.fs"
+              FsProjEditor.moveFileUp path "B.fs"
+              Expect.equal (getCompileOrder path) [ "A.fs"; "B.fs"; "C.fs" ] "Round-trip should restore original order"
+            finally
+              File.Delete path ] ]

test/FsAutoComplete.Tests.Lsp/Program.fs

@@ -154,6 +154,7 @@ let generalTests =
       LspHelpersTests.allTests
       TipFormatterTests.allTests
       FcsInvariantTests.tests
+      FsProjEditorTests.allTests
       decompilerTests ]
 
 [<Tests>]