Simplify whitespace handling and fix some long standing bugs in it
Fixes wixtoolset/issues#5883
Rob Mensching committed
Oct 4, 2018 at 13:54 UTC
784208bd46ce05025d8ccaef8542328350038d90
2 files changed
+171
-170
src/test/WixToolsetTest.WixCop/ConverterFixture.cs
+74
@@ -94,8 +94,82 @@ namespace WixToolsetTest.WixCop
94
95
var actual = UnformattedDocumentString(document);
96
97
+ Assert.Equal(expected, actual);
98
Assert.Equal(4, errors);
99
+ }
100
+
101
+ [Fact]
102
+ public void CanPreserveNewLines()
103
+ {
104
+ var parse = String.Join(Environment.NewLine,
105
+ "<?xml version='1.0' encoding='utf-8'?>",
106
+ "<Wix xmlns='http://wixtoolset.org/schemas/v4/wxs'>",
107
+ " <Fragment>",
108
+ "",
109
+ " <Property Id='Prop' Value='Val' />",
110
+ "",
111
+ " </Fragment>",
112
+ "</Wix>");
113
+
114
+ var expected = String.Join(Environment.NewLine,
115
+ "<?xml version=\"1.0\" encoding=\"utf-16\"?>",
116
+ "<Wix xmlns=\"http://wixtoolset.org/schemas/v4/wxs\">",
117
+ " <Fragment>",
118
+ "",
119
+ " <Property Id=\"Prop\" Value=\"Val\" />",
120
+ "",
121
+ " </Fragment>",
122
+ "</Wix>");
123
+
124
+ var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo);
125
+
126
+ var messaging = new DummyMessaging();
127
+ var converter = new Converter(messaging, 4, null, null);
128
+
129
+ var conversions = converter.ConvertDocument(document);
130
+
131
+ var actual = UnformattedDocumentString(document);
132
+
133
+ Assert.Equal(expected, actual);
134
+ Assert.Equal(3, conversions);
135
+ }
136
+
137
+ [Fact]
138
+ public void CanConvertWithNewLineAtEndOfFile()
139
+ {
140
+ var parse = String.Join(Environment.NewLine,
141
+ "<?xml version='1.0' encoding='utf-8'?>",
142
+ "<Wix xmlns='http://wixtoolset.org/schemas/v4/wxs'>",
143
+ " <Fragment>",
144
+ "",
145
+ " <Property Id='Prop' Value='Val' />",
146
+ "",
147
+ " </Fragment>",
148
+ "</Wix>",
149
+ "");
150
+
151
+ var expected = String.Join(Environment.NewLine,
152
+ "<?xml version=\"1.0\" encoding=\"utf-16\"?>",
153
+ "<Wix xmlns=\"http://wixtoolset.org/schemas/v4/wxs\">",
154
+ " <Fragment>",
155
+ "",
156
+ " <Property Id=\"Prop\" Value=\"Val\" />",
157
+ "",
158
+ " </Fragment>",
159
+ "</Wix>",
160
+ "");
161
+
162
+ var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo);
163
+
164
+ var messaging = new DummyMessaging();
165
+ var converter = new Converter(messaging, 4, null, null);
166
+
167
+ var conversions = converter.ConvertDocument(document);
168
+
169
+ var actual = UnformattedDocumentString(document);
170
+
171
Assert.Equal(expected, actual);
172
+ Assert.Equal(3, conversions);
173
}
174
175
[Fact]
src/wixcop/Converter.cs
+97
-170
@@ -7,6 +7,7 @@ namespace WixToolset.Tools.WixCop
7
using System.Globalization;
8
using System.IO;
9
using System.Linq;
10
+ using System.Text;
11
using System.Xml;
12
using System.Xml.Linq;
13
using WixToolset.Data;
@@ -18,7 +19,7 @@ namespace WixToolset.Tools.WixCop
19
/// </summary>
20
public class Converter
21
{
21
- private const string XDocumentNewLine = "\n"; // XDocument normlizes "\r\n" to just "\n".
22
+ private const char XDocumentNewLine = '\n'; // XDocument normalizes "\r\n" to just "\n".
23
private static readonly XNamespace WixNamespace = "http://wixtoolset.org/schemas/v4/wxs";
24
25
private static readonly XName FileElementName = WixNamespace + "File";
@@ -58,7 +59,7 @@ namespace WixToolset.Tools.WixCop
59
{ "http://schemas.microsoft.com/wix/2006/WixUnit", "http://wixtoolset.org/schemas/v4/wixunit" },
60
};
61
61
- private Dictionary<XName, Action<XElement>> ConvertElementMapping;
62
+ private readonly Dictionary<XName, Action<XElement>> ConvertElementMapping;
63
64
/// <summary>
65
/// Instantiate a new Converter class.
@@ -79,14 +80,16 @@ namespace WixToolset.Tools.WixCop
80
{ Converter.PayloadElementName, this.ConvertSuppressSignatureValidation },
81
{ Converter.WixElementWithoutNamespaceName, this.ConvertWixElementWithoutNamespace },
82
};*/
82
- this.ConvertElementMapping = new Dictionary<XName, Action<XElement>>();
83
- this.ConvertElementMapping.Add(Converter.FileElementName, this.ConvertFileElement);
84
- this.ConvertElementMapping.Add(Converter.ExePackageElementName, this.ConvertSuppressSignatureValidation);
85
- this.ConvertElementMapping.Add(Converter.MsiPackageElementName, this.ConvertSuppressSignatureValidation);
86
- this.ConvertElementMapping.Add(Converter.MspPackageElementName, this.ConvertSuppressSignatureValidation);
87
- this.ConvertElementMapping.Add(Converter.MsuPackageElementName, this.ConvertSuppressSignatureValidation);
88
- this.ConvertElementMapping.Add(Converter.PayloadElementName, this.ConvertSuppressSignatureValidation);
89
- this.ConvertElementMapping.Add(Converter.WixElementWithoutNamespaceName, this.ConvertWixElementWithoutNamespace);
83
+ this.ConvertElementMapping = new Dictionary<XName, Action<XElement>>
84
+ {
85
+ { Converter.FileElementName, this.ConvertFileElement },
86
+ { Converter.ExePackageElementName, this.ConvertSuppressSignatureValidation },
87
+ { Converter.MsiPackageElementName, this.ConvertSuppressSignatureValidation },
88
+ { Converter.MspPackageElementName, this.ConvertSuppressSignatureValidation },
89
+ { Converter.MsuPackageElementName, this.ConvertSuppressSignatureValidation },
90
+ { Converter.PayloadElementName, this.ConvertSuppressSignatureValidation },
91
+ { Converter.WixElementWithoutNamespaceName, this.ConvertWixElementWithoutNamespace }
92
+ };
93
94
this.Messaging = messaging;
95
@@ -129,7 +132,7 @@ namespace WixToolset.Tools.WixCop
132
}
133
catch (XmlException e)
134
{
132
- this.OnError(ConverterTestType.XmlException, (XObject)null, "The xml is invalid. Detail: '{0}'", e.Message);
135
+ this.OnError(ConverterTestType.XmlException, null, "The xml is invalid. Detail: '{0}'", e.Message);
136
137
return this.Errors;
138
}
@@ -148,7 +151,7 @@ namespace WixToolset.Tools.WixCop
151
}
152
catch (UnauthorizedAccessException)
153
{
151
- this.OnError(ConverterTestType.UnauthorizedAccessException, (XObject)null, "Could not write to file.");
154
+ this.OnError(ConverterTestType.UnauthorizedAccessException, null, "Could not write to file.");
155
}
156
}
157
@@ -177,51 +180,90 @@ namespace WixToolset.Tools.WixCop
180
}
181
else // missing declaration
182
{
180
- if (this.OnError(ConverterTestType.DeclarationMissing, (XNode)null, "This file is missing an XML declaration on the first line."))
183
+ if (this.OnError(ConverterTestType.DeclarationMissing, null, "This file is missing an XML declaration on the first line."))
184
{
185
document.Declaration = new XDeclaration("1.0", "utf-8", null);
183
- document.Root.AddBeforeSelf(new XText(XDocumentNewLine));
186
+ document.Root.AddBeforeSelf(new XText(XDocumentNewLine.ToString()));
187
}
188
}
189
190
// Start converting the nodes at the top.
188
- this.ConvertNode(document.Root, 0);
191
+ this.ConvertNodes(document.Nodes(), 0);
192
193
return this.Errors;
194
}
195
193
- /// <summary>
194
- /// Convert a single xml node.
195
- /// </summary>
196
- /// <param name="node">The node to convert.</param>
197
- /// <param name="level">The depth level of the node.</param>
198
- /// <returns>The converted node.</returns>
199
- private void ConvertNode(XNode node, int level)
196
+ private void ConvertNodes(IEnumerable<XNode> nodes, int level)
197
{
201
- // Convert this node's whitespace.
202
- if ((XmlNodeType.Comment == node.NodeType && 0 > ((XComment)node).Value.IndexOf(XDocumentNewLine, StringComparison.Ordinal)) ||
203
- XmlNodeType.CDATA == node.NodeType || XmlNodeType.Element == node.NodeType || XmlNodeType.ProcessingInstruction == node.NodeType)
198
+ // Note we operate on a copy of the node list since we may
199
+ // remove some whitespace nodes during this processing.
200
+ foreach (var node in nodes.ToList())
201
{
205
- this.ConvertWhitespace(node, level);
206
- }
202
+ if (node is XText text)
203
+ {
204
+ if (!String.IsNullOrWhiteSpace(text.Value))
205
+ {
206
+ text.Value = text.Value.Trim();
207
+ }
208
+ else if (node.NextNode is XCData cdata)
209
+ {
210
+ this.EnsurePrecedingWhitespaceRemoved(text, node, ConverterTestType.WhitespacePrecedingNodeWrong);
211
+ }
212
+ else if (node.NextNode is XElement element)
213
+ {
214
+ this.EnsurePrecedingWhitespaceCorrect(text, node, level, ConverterTestType.WhitespacePrecedingNodeWrong);
215
+ }
216
+ else if (node.NextNode is null) // this is the space before the close element
217
+ {
218
+ if (node.PreviousNode is null || node.PreviousNode is XCData)
219
+ {
220
+ this.EnsurePrecedingWhitespaceRemoved(text, node.Parent, ConverterTestType.WhitespacePrecedingEndElementWrong);
221
+ }
222
+ else if (level == 0) // root element's close tag
223
+ {
224
+ this.EnsurePrecedingWhitespaceCorrect(text, node, 0, ConverterTestType.WhitespacePrecedingEndElementWrong);
225
+ }
226
+ else
227
+ {
228
+ this.EnsurePrecedingWhitespaceCorrect(text, node, level - 1, ConverterTestType.WhitespacePrecedingEndElementWrong);
229
+ }
230
+ }
231
+ }
232
+ else if (node is XElement element)
233
+ {
234
+ this.ConvertElement(element);
235
208
- // Convert this node if it is an element.
209
- var element = node as XElement;
236
+ this.ConvertNodes(element.Nodes(), level + 1);
237
+ }
238
+ }
239
+ }
240
211
- if (null != element)
241
+ private void EnsurePrecedingWhitespaceCorrect(XText whitespace, XNode node, int level, ConverterTestType testType)
242
+ {
243
+ if (!Converter.LeadingWhitespaceValid(this.IndentationAmount, level, whitespace.Value))
244
{
213
- this.ConvertElement(element);
214
-
215
- // Convert all children of this element.
216
- IEnumerable<XNode> children = element.Nodes().ToList();
245
+ var message = testType == ConverterTestType.WhitespacePrecedingEndElementWrong ? "The whitespace preceding this end element is incorrect." : "The whitespace preceding this node is incorrect.";
246
218
- foreach (var child in children)
247
+ if (this.OnError(testType, node, message))
248
{
220
- this.ConvertNode(child, level + 1);
249
+ Converter.FixupWhitespace(this.IndentationAmount, level, whitespace);
250
}
251
}
252
}
253
254
+ private void EnsurePrecedingWhitespaceRemoved(XText whitespace, XNode node, ConverterTestType testType)
255
+ {
256
+ if (!String.IsNullOrEmpty(whitespace.Value))
257
+ {
258
+ var message = testType == ConverterTestType.WhitespacePrecedingEndElementWrong ? "The whitespace preceding this end element is incorrect." : "The whitespace preceding this node is incorrect.";
259
+
260
+ if (this.OnError(testType, node, message))
261
+ {
262
+ whitespace.Remove();
263
+ }
264
+ }
265
+ }
266
+
267
private void ConvertElement(XElement element)
268
{
269
// Gather any deprecated namespaces, then update this element tree based on those deprecations.
@@ -229,9 +271,7 @@ namespace WixToolset.Tools.WixCop
271
272
foreach (var declaration in element.Attributes().Where(a => a.IsNamespaceDeclaration))
273
{
232
- XNamespace ns;
233
-
234
- if (Converter.OldToNewNamespaceMapping.TryGetValue(declaration.Value, out ns))
274
+ if (Converter.OldToNewNamespaceMapping.TryGetValue(declaration.Value, out var ns))
275
{
276
if (this.OnError(ConverterTestType.XmlnsValueWrong, declaration, "The namespace '{0}' is out of date. It must be '{1}'.", declaration.Value, ns.NamespaceName))
277
{
@@ -245,10 +285,8 @@ namespace WixToolset.Tools.WixCop
285
Converter.UpdateElementsWithDeprecatedNamespaces(element.DescendantsAndSelf(), deprecatedToUpdatedNamespaces);
286
}
287
248
- // Convert the node in much greater detail.
249
- Action<XElement> convert;
250
-
251
- if (this.ConvertElementMapping.TryGetValue(element.Name, out convert))
288
+ // Apply any specialized conversion actions.
289
+ if (this.ConvertElementMapping.TryGetValue(element.Name, out var convert))
290
{
291
convert(element);
292
}
@@ -318,96 +356,20 @@ namespace WixToolset.Tools.WixCop
356
}
357
}
358
321
- /// <summary>
322
- /// Convert the whitespace adjacent to a node.
323
- /// </summary>
324
- /// <param name="node">The node to convert.</param>
325
- /// <param name="level">The depth level of the node.</param>
326
- private void ConvertWhitespace(XNode node, int level)
327
- {
328
- // Fix the whitespace before this node.
329
- if (node.PreviousNode is XText whitespace)
330
- {
331
- if (XmlNodeType.CDATA == node.NodeType)
332
- {
333
- if (this.OnError(ConverterTestType.WhitespacePrecedingCDATAWrong, node, "There should be no whitespace preceding a CDATA node."))
334
- {
335
- whitespace.Remove();
336
- }
337
- }
338
- else
339
- {
340
- // TODO: this code complains about whitespace even after the file has been fixed.
341
- if (!Converter.IsLegalWhitespace(this.IndentationAmount, level, whitespace.Value))
342
- {
343
- if (this.OnError(ConverterTestType.WhitespacePrecedingNodeWrong, node, "The whitespace preceding this node is incorrect."))
344
- {
345
- Converter.FixWhitespace(this.IndentationAmount, level, whitespace);
346
- }
347
- }
348
- }
349
- }
350
-
351
- // Fix the whitespace after CDATA nodes.
352
- if (node is XCData cdata)
353
- {
354
- whitespace = cdata.NextNode as XText;
355
-
356
- if (null != whitespace)
357
- {
358
- if (this.OnError(ConverterTestType.WhitespaceFollowingCDATAWrong, node, "There should be no whitespace following a CDATA node."))
359
- {
360
- whitespace.Remove();
361
- }
362
- }
363
- }
364
- else
365
- {
366
- // Fix the whitespace inside and after this node (except for Error which may contain just whitespace).
367
- if (node is XElement element && "Error" != element.Name.LocalName)
368
- {
369
- if (!element.HasElements && !element.IsEmpty && String.IsNullOrEmpty(element.Value.Trim()))
370
- {
371
- if (this.OnError(ConverterTestType.NotEmptyElement, element, "This should be an empty element since it contains nothing but whitespace."))
372
- {
373
- element.RemoveNodes();
374
- }
375
- }
376
-
377
- whitespace = node.NextNode as XText;
378
-
379
- if (null != whitespace)
380
- {
381
- // TODO: this code crashes when level is 0,
382
- // complains about whitespace even after the file has been fixed,
383
- // and the error text doesn't match the error.
384
- if (!Converter.IsLegalWhitespace(this.IndentationAmount, level - 1, whitespace.Value))
385
- {
386
- if (this.OnError(ConverterTestType.WhitespacePrecedingEndElementWrong, whitespace, "The whitespace preceding this end element is incorrect."))
387
- {
388
- Converter.FixWhitespace(this.IndentationAmount, level - 1, whitespace);
389
- }
390
- }
391
- }
392
- }
393
- }
394
- }
395
-
359
private IEnumerable<ConverterTestType> YieldConverterTypes(IEnumerable<string> types)
360
{
361
if (null != types)
362
{
363
foreach (var type in types)
364
{
402
- ConverterTestType itt;
365
404
- if (Enum.TryParse<ConverterTestType>(type, true, out itt))
366
+ if (Enum.TryParse<ConverterTestType>(type, true, out var itt))
367
{
368
yield return itt;
369
}
370
else // not a known ConverterTestType
371
{
410
- this.OnError(ConverterTestType.ConverterTestTypeUnknown, (XObject)null, "Unknown error type: '{0}'.", type);
372
+ this.OnError(ConverterTestType.ConverterTestTypeUnknown, null, "Unknown error type: '{0}'.", type);
373
}
374
}
375
}
@@ -417,9 +379,8 @@ namespace WixToolset.Tools.WixCop
379
{
380
foreach (var element in elements)
381
{
420
- XNamespace ns;
382
422
- if (deprecatedToUpdatedNamespaces.TryGetValue(element.Name.Namespace, out ns))
383
+ if (deprecatedToUpdatedNamespaces.TryGetValue(element.Name.Namespace, out var ns))
384
{
385
element.Name = ns.GetName(element.Name.LocalName);
386
}
@@ -456,67 +417,33 @@ namespace WixToolset.Tools.WixCop
417
/// <param name="level">The depth level that should match this whitespace.</param>
418
/// <param name="whitespace">The whitespace to validate.</param>
419
/// <returns>true if the whitespace is legal; false otherwise.</returns>
459
- private static bool IsLegalWhitespace(int indentationAmount, int level, string whitespace)
420
+ private static bool LeadingWhitespaceValid(int indentationAmount, int level, string whitespace)
421
{
461
- // strip off leading newlines; there can be an arbitrary number of these
462
- while (whitespace.StartsWith(XDocumentNewLine, StringComparison.Ordinal))
463
- {
464
- whitespace = whitespace.Substring(XDocumentNewLine.Length);
465
- }
422
+ // Strip off leading newlines; there can be an arbitrary number of these.
423
+ whitespace = whitespace.TrimStart(XDocumentNewLine);
424
467
- // check the length
468
- if (whitespace.Length != level * indentationAmount)
469
- {
470
- return false;
471
- }
425
+ var indentation = new string(' ', level * indentationAmount);
426
473
- // check the spaces
474
- foreach (var character in whitespace)
475
- {
476
- if (' ' != character)
477
- {
478
- return false;
479
- }
480
- }
481
-
482
- return true;
427
+ return whitespace == indentation;
428
}
429
430
/// <summary>
486
- /// Fix the whitespace in a Whitespace node.
431
+ /// Fix the whitespace in a whitespace node.
432
/// </summary>
433
/// <param name="indentationAmount">Indentation value to use when validating leading whitespace.</param>
434
/// <param name="level">The depth level of the desired whitespace.</param>
435
/// <param name="whitespace">The whitespace node to fix.</param>
491
- private static void FixWhitespace(int indentationAmount, int level, XText whitespace)
436
+ private static void FixupWhitespace(int indentationAmount, int level, XText whitespace)
437
{
493
- var newLineCount = 0;
494
-
495
- for (var i = 0; i + 1 < whitespace.Value.Length; ++i)
496
- {
497
- if (XDocumentNewLine == whitespace.Value.Substring(i, 2))
498
- {
499
- ++i; // skip an extra character
500
- ++newLineCount;
501
- }
502
- }
503
-
504
- if (0 == newLineCount)
505
- {
506
- newLineCount = 1;
507
- }
438
+ var value = new StringBuilder(whitespace.Value.Length);
439
509
- // reset the whitespace value
510
- whitespace.Value = String.Empty;
440
+ // Keep any previous preceeding new lines.
441
+ var newlines = whitespace.Value.TakeWhile(c => c == XDocumentNewLine).Count();
442
512
- // add the correct number of newlines
513
- for (var i = 0; i < newLineCount; ++i)
514
- {
515
- whitespace.Value = String.Concat(whitespace.Value, XDocumentNewLine);
516
- }
443
+ // Ensure there is always at least one new line before the indentation.
444
+ value.Append(XDocumentNewLine, newlines == 0 ? 1 : newlines);
445
518
- // add the correct number of spaces based on configured indentation amount
519
- whitespace.Value = String.Concat(whitespace.Value, new string(' ', level * indentationAmount));
446
+ whitespace.Value = value.Append(' ', level * indentationAmount).ToString();
447
}
448
449
/// <summary>