Skip to content

Commit 4016bfc

Browse files
github-actions[bot]CopilotKrzysztof-Cieslak
authored
[Repo Assist] Fix moveFileUp/Down: preserve vertical formatting and fix first-press no-op (#1499)
* Fix moveFileUp/Down breaking vertical formatting and requiring two presses The root cause is that XmlDocument with PreserveWhitespace=true inserts whitespace-only text nodes between XML elements. The original code used node.PreviousSibling / node.NextSibling to find adjacent Compile elements, but those return the intervening whitespace text nodes rather than the actual <Compile> elements. This caused two bugs: 1. The first button press made no visible change to compile order, because InsertBefore(node, whitespaceTextNode) placed the node adjacent to the whitespace—leaving the logical Compile order unchanged. 2. The second press actually moved the element, but produced 'horizontal' formatting (<Compile A /><Compile B /> on one line) because the reinserted element lacked a preceding whitespace text node. Fix: add previousSiblingElement/nextSiblingElement helpers that skip text nodes to find the true adjacent <Compile> elements. Then swap positions using a clone-and-replace strategy: clone both elements, insert the clones in swapped positions, and remove the originals. The whitespace text nodes (indentation) are never touched, so vertical formatting is always preserved. Fixes #1498 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: trigger checks * Address PR review: skip non-Compile siblings, null-guard file lookup, fix temp-file leak - previousSiblingElement/nextSiblingElement now skip non-Compile elements (not just whitespace text nodes), so moveFileUp/moveFileDown won't swap a <Compile> node with <None> or <Content> siblings - moveFileUp/moveFileDown guard against null itemGroup/node (missing file) instead of throwing NullReferenceException - createTempFsproj/createTempFsprojCrlf use Path.GetRandomFileName() to avoid leaking the .tmp file created by Path.GetTempFileName() Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: trigger checks --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Krzysztof Cieślak <krzysztof_cieslak@windowslive.com>
1 parent 621ba02 commit 4016bfc

3 files changed

Lines changed: 318 additions & 14 deletions

File tree

‎src/FsAutoComplete.Core/FsprojEdit.fs‎

Lines changed: 66 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -46,39 +46,91 @@ module FsProjEditor =
4646
node.SetAttribute("Include", includePath)
4747
node
4848

49+
/// Find the previous sibling that is a `<Compile>` Element node, skipping over whitespace-only
50+
/// text nodes and non-`Compile` elements that are present when PreserveWhitespace is true.
51+
let private previousSiblingElement (node: System.Xml.XmlNode) =
52+
let mutable prev = node.PreviousSibling
53+
54+
// Skip over non-element nodes (e.g., whitespace) and elements that are not `<Compile>`.
55+
while not (isNull prev)
56+
&& (prev.NodeType <> System.Xml.XmlNodeType.Element || prev.LocalName <> "Compile") do
57+
prev <- prev.PreviousSibling
58+
59+
prev
60+
61+
/// Find the next sibling that is a `<Compile>` Element node, skipping over whitespace-only
62+
/// text nodes and non-`Compile` elements that are present when PreserveWhitespace is true.
63+
let private nextSiblingElement (node: System.Xml.XmlNode) =
64+
let mutable next = node.NextSibling
65+
66+
// Skip over non-element nodes (e.g., whitespace) and elements that are not `<Compile>`.
67+
while not (isNull next)
68+
&& (next.NodeType <> System.Xml.XmlNodeType.Element || next.LocalName <> "Compile") do
69+
next <- next.NextSibling
70+
71+
next
72+
4973
let moveFileUp (fsprojPath: string) (file: string) =
5074
let xDoc = System.Xml.XmlDocument()
5175
xDoc.PreserveWhitespace <- true // to keep custom formatting if present
5276
xDoc.Load fsprojPath
5377
let xpath = sprintf "//Compile[@Include='%s']/.." file
5478
let itemGroup = xDoc.SelectSingleNode(xpath)
55-
let childXPath = sprintf "//Compile[@Include='%s']" file
56-
let node = itemGroup.SelectSingleNode(childXPath)
57-
let upNode = node.PreviousSibling
5879

59-
if isNull upNode then
80+
if isNull itemGroup then
6081
()
6182
else
62-
itemGroup.RemoveChild node |> ignore
63-
itemGroup.InsertBefore(node, upNode) |> ignore
64-
xDoc.Save fsprojPath
83+
let childXPath = sprintf "//Compile[@Include='%s']" file
84+
let node = itemGroup.SelectSingleNode(childXPath)
85+
86+
if isNull node then
87+
()
88+
else
89+
let prevElement = previousSiblingElement node
90+
91+
if isNull prevElement then
92+
()
93+
else
94+
// Clone both elements, insert clones in swapped positions, then remove the originals.
95+
// Whitespace text nodes (indentation) remain in place so vertical formatting is preserved.
96+
let nodeClone = node.CloneNode(true)
97+
let prevClone = prevElement.CloneNode(true)
98+
itemGroup.InsertBefore(nodeClone, prevElement) |> ignore
99+
itemGroup.InsertBefore(prevClone, node) |> ignore
100+
itemGroup.RemoveChild(prevElement) |> ignore
101+
itemGroup.RemoveChild(node) |> ignore
102+
xDoc.Save fsprojPath
65103

66104
let moveFileDown (fsprojPath: string) (file: string) =
67105
let xDoc = System.Xml.XmlDocument()
68106
xDoc.PreserveWhitespace <- true // to keep custom formatting if present
69107
xDoc.Load fsprojPath
70108
let xpath = sprintf "//Compile[@Include='%s']/.." file
71109
let itemGroup = xDoc.SelectSingleNode(xpath)
72-
let childXPath = sprintf "//Compile[@Include='%s']" file
73-
let node = itemGroup.SelectSingleNode(childXPath)
74-
let downNode = node.NextSibling
75110

76-
if isNull downNode then
111+
if isNull itemGroup then
77112
()
78113
else
79-
itemGroup.RemoveChild node |> ignore
80-
itemGroup.InsertAfter(node, downNode) |> ignore
81-
xDoc.Save fsprojPath
114+
let childXPath = sprintf "//Compile[@Include='%s']" file
115+
let node = itemGroup.SelectSingleNode(childXPath)
116+
117+
if isNull node then
118+
()
119+
else
120+
let nextElement = nextSiblingElement node
121+
122+
if isNull nextElement then
123+
()
124+
else
125+
// Clone both elements, insert clones in swapped positions, then remove the originals.
126+
// Whitespace text nodes (indentation) remain in place so vertical formatting is preserved.
127+
let nodeClone = node.CloneNode(true)
128+
let nextClone = nextElement.CloneNode(true)
129+
itemGroup.InsertBefore(nextClone, node) |> ignore
130+
itemGroup.InsertBefore(nodeClone, nextElement) |> ignore
131+
itemGroup.RemoveChild(node) |> ignore
132+
itemGroup.RemoveChild(nextElement) |> ignore
133+
xDoc.Save fsprojPath
82134

83135
let addFileAbove (fsprojPath: string) (aboveFile: string) (newFileName: string) =
84136
let xDoc = System.Xml.XmlDocument()

‎test/FsAutoComplete.Tests.Lsp/FsProjEditorTests.fs‎

Lines changed: 251 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,251 @@
1+
module FsAutoComplete.Tests.FsProjEditorTests
2+
3+
open Expecto
4+
open FsAutoComplete
5+
open System
6+
open System.IO
7+
open System.Xml
8+
9+
/// Create a temporary .fsproj file whose ItemGroup contains the given files
10+
/// (one <Compile Include="..." /> per line, LF line endings).
11+
/// Returns the path; caller must delete.
12+
let private createTempFsproj (files: string list) =
13+
let path = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName() + ".fsproj")
14+
15+
let filesXml =
16+
files
17+
|> List.map (fun f -> $" <Compile Include=\"{f}\" />")
18+
|> String.concat "\n"
19+
20+
let content =
21+
$"<Project Sdk=\"Microsoft.NET.Sdk\">\n <PropertyGroup>\n <TargetFramework>net8.0</TargetFramework>\n </PropertyGroup>\n <ItemGroup>\n{filesXml}\n </ItemGroup>\n</Project>"
22+
23+
File.WriteAllText(path, content)
24+
path
25+
26+
/// Create a temporary .fsproj file with CRLF line endings.
27+
let private createTempFsprojCrlf (files: string list) =
28+
let path = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName() + ".fsproj")
29+
30+
let filesXml =
31+
files
32+
|> List.map (fun f -> $" <Compile Include=\"{f}\" />")
33+
|> String.concat "\r\n"
34+
35+
let content =
36+
$"<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>"
37+
38+
File.WriteAllText(path, content)
39+
path
40+
41+
/// Return the ordered list of <Compile Include="..." /> values from a .fsproj.
42+
let private getCompileOrder (fsprojPath: string) =
43+
let xDoc = XmlDocument()
44+
xDoc.Load fsprojPath
45+
46+
xDoc.SelectNodes("//Compile[@Include]")
47+
|> Seq.cast<XmlNode>
48+
|> Seq.map (fun n -> n.Attributes.["Include"].InnerText)
49+
|> Seq.toList
50+
51+
/// Return true when every <Compile .../> element appears on its own line
52+
/// (i.e. no two Compile tags share the same line in the saved file).
53+
let private eachCompileOnOwnLine (fsprojPath: string) =
54+
let lines = File.ReadAllLines(fsprojPath)
55+
56+
lines
57+
|> Array.filter (fun l -> l.Contains("<Compile"))
58+
|> Array.forall (fun l ->
59+
// A line that contains exactly one <Compile open tag is fine.
60+
// Count occurrences of "<Compile" – if > 1 they are horizontal.
61+
let occurrences =
62+
let mutable count = 0
63+
let mutable idx = 0
64+
65+
while idx < l.Length do
66+
let pos = l.IndexOf("<Compile", idx, StringComparison.Ordinal)
67+
68+
if pos < 0 then
69+
idx <- l.Length
70+
else
71+
count <- count + 1
72+
idx <- pos + 1
73+
74+
count
75+
76+
occurrences <= 1)
77+
78+
let allTests =
79+
testList
80+
"FsProjEditor"
81+
[ testList
82+
"moveFileUp"
83+
[ testCase "moves second file above first"
84+
<| fun _ ->
85+
let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
86+
87+
try
88+
FsProjEditor.moveFileUp path "B.fs"
89+
Expect.equal (getCompileOrder path) [ "B.fs"; "A.fs"; "C.fs" ] "B.fs should precede A.fs"
90+
finally
91+
File.Delete path
92+
93+
testCase "moves last file up"
94+
<| fun _ ->
95+
let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
96+
97+
try
98+
FsProjEditor.moveFileUp path "C.fs"
99+
Expect.equal (getCompileOrder path) [ "A.fs"; "C.fs"; "B.fs" ] "C.fs should precede B.fs"
100+
finally
101+
File.Delete path
102+
103+
testCase "first file is unchanged"
104+
<| fun _ ->
105+
let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
106+
107+
try
108+
FsProjEditor.moveFileUp path "A.fs"
109+
Expect.equal (getCompileOrder path) [ "A.fs"; "B.fs"; "C.fs" ] "Order should be unchanged"
110+
finally
111+
File.Delete path
112+
113+
testCase "moving up twice reaches top"
114+
<| fun _ ->
115+
let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
116+
117+
try
118+
FsProjEditor.moveFileUp path "C.fs"
119+
FsProjEditor.moveFileUp path "C.fs"
120+
Expect.equal (getCompileOrder path) [ "C.fs"; "A.fs"; "B.fs" ] "C.fs should be first"
121+
finally
122+
File.Delete path
123+
124+
testCase "single press actually changes compile order (regression: required two presses)"
125+
<| fun _ ->
126+
let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
127+
128+
try
129+
FsProjEditor.moveFileUp path "C.fs"
130+
let order = getCompileOrder path
131+
Expect.notEqual order [ "A.fs"; "B.fs"; "C.fs" ] "Compile order must change on first call"
132+
finally
133+
File.Delete path
134+
135+
testCase "preserves vertical formatting with LF line endings"
136+
<| fun _ ->
137+
let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
138+
139+
try
140+
FsProjEditor.moveFileUp path "B.fs"
141+
Expect.isTrue (eachCompileOnOwnLine path) "Each Compile element must remain on its own line"
142+
finally
143+
File.Delete path
144+
145+
testCase "preserves vertical formatting with CRLF line endings"
146+
<| fun _ ->
147+
let path = createTempFsprojCrlf [ "A.fs"; "B.fs"; "C.fs" ]
148+
149+
try
150+
FsProjEditor.moveFileUp path "B.fs"
151+
Expect.isTrue (eachCompileOnOwnLine path) "Each Compile element must remain on its own line (CRLF)"
152+
finally
153+
File.Delete path ]
154+
155+
testList
156+
"moveFileDown"
157+
[ testCase "moves first file below second"
158+
<| fun _ ->
159+
let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
160+
161+
try
162+
FsProjEditor.moveFileDown path "A.fs"
163+
Expect.equal (getCompileOrder path) [ "B.fs"; "A.fs"; "C.fs" ] "A.fs should follow B.fs"
164+
finally
165+
File.Delete path
166+
167+
testCase "moves middle file down"
168+
<| fun _ ->
169+
let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
170+
171+
try
172+
FsProjEditor.moveFileDown path "B.fs"
173+
Expect.equal (getCompileOrder path) [ "A.fs"; "C.fs"; "B.fs" ] "B.fs should follow C.fs"
174+
finally
175+
File.Delete path
176+
177+
testCase "last file is unchanged"
178+
<| fun _ ->
179+
let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
180+
181+
try
182+
FsProjEditor.moveFileDown path "C.fs"
183+
Expect.equal (getCompileOrder path) [ "A.fs"; "B.fs"; "C.fs" ] "Order should be unchanged"
184+
finally
185+
File.Delete path
186+
187+
testCase "moving down twice reaches bottom"
188+
<| fun _ ->
189+
let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
190+
191+
try
192+
FsProjEditor.moveFileDown path "A.fs"
193+
FsProjEditor.moveFileDown path "A.fs"
194+
Expect.equal (getCompileOrder path) [ "B.fs"; "C.fs"; "A.fs" ] "A.fs should be last"
195+
finally
196+
File.Delete path
197+
198+
testCase "single press actually changes compile order (regression: required two presses)"
199+
<| fun _ ->
200+
let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
201+
202+
try
203+
FsProjEditor.moveFileDown path "A.fs"
204+
let order = getCompileOrder path
205+
Expect.notEqual order [ "A.fs"; "B.fs"; "C.fs" ] "Compile order must change on first call"
206+
finally
207+
File.Delete path
208+
209+
testCase "preserves vertical formatting with LF line endings"
210+
<| fun _ ->
211+
let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
212+
213+
try
214+
FsProjEditor.moveFileDown path "B.fs"
215+
Expect.isTrue (eachCompileOnOwnLine path) "Each Compile element must remain on its own line"
216+
finally
217+
File.Delete path
218+
219+
testCase "preserves vertical formatting with CRLF line endings"
220+
<| fun _ ->
221+
let path = createTempFsprojCrlf [ "A.fs"; "B.fs"; "C.fs" ]
222+
223+
try
224+
FsProjEditor.moveFileDown path "B.fs"
225+
Expect.isTrue (eachCompileOnOwnLine path) "Each Compile element must remain on its own line (CRLF)"
226+
finally
227+
File.Delete path ]
228+
229+
testList
230+
"moveFileUp and moveFileDown roundtrip"
231+
[ testCase "move up then down restores original order"
232+
<| fun _ ->
233+
let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
234+
235+
try
236+
FsProjEditor.moveFileUp path "B.fs"
237+
FsProjEditor.moveFileDown path "B.fs"
238+
Expect.equal (getCompileOrder path) [ "A.fs"; "B.fs"; "C.fs" ] "Round-trip should restore original order"
239+
finally
240+
File.Delete path
241+
242+
testCase "move down then up restores original order"
243+
<| fun _ ->
244+
let path = createTempFsproj [ "A.fs"; "B.fs"; "C.fs" ]
245+
246+
try
247+
FsProjEditor.moveFileDown path "B.fs"
248+
FsProjEditor.moveFileUp path "B.fs"
249+
Expect.equal (getCompileOrder path) [ "A.fs"; "B.fs"; "C.fs" ] "Round-trip should restore original order"
250+
finally
251+
File.Delete path ] ]

‎test/FsAutoComplete.Tests.Lsp/Program.fs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,7 @@ let generalTests =
154154
LspHelpersTests.allTests
155155
TipFormatterTests.allTests
156156
FcsInvariantTests.tests
157+
FsProjEditorTests.allTests
157158
decompilerTests ]
158159

159160
[<Tests>]

0 commit comments

Comments
 (0)