From 784208bd46ce05025d8ccaef8542328350038d90 Mon Sep 17 00:00:00 2001 From: Rob Mensching Date: Thu, 4 Oct 2018 13:54:59 -0700 Subject: Simplify whitespace handling and fix some long standing bugs in it Fixes wixtoolset/issues#5883 --- src/test/WixToolsetTest.WixCop/ConverterFixture.cs | 74 ++++++ src/wixcop/Converter.cs | 267 ++++++++------------- 2 files changed, 171 insertions(+), 170 deletions(-) (limited to 'src') diff --git a/src/test/WixToolsetTest.WixCop/ConverterFixture.cs b/src/test/WixToolsetTest.WixCop/ConverterFixture.cs index 86931d5a..863a781a 100644 --- a/src/test/WixToolsetTest.WixCop/ConverterFixture.cs +++ b/src/test/WixToolsetTest.WixCop/ConverterFixture.cs @@ -94,8 +94,82 @@ namespace WixToolsetTest.WixCop var actual = UnformattedDocumentString(document); + Assert.Equal(expected, actual); Assert.Equal(4, errors); + } + + [Fact] + public void CanPreserveNewLines() + { + var parse = String.Join(Environment.NewLine, + "", + "", + " ", + "", + " ", + "", + " ", + ""); + + var expected = String.Join(Environment.NewLine, + "", + "", + " ", + "", + " ", + "", + " ", + ""); + + var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo); + + var messaging = new DummyMessaging(); + var converter = new Converter(messaging, 4, null, null); + + var conversions = converter.ConvertDocument(document); + + var actual = UnformattedDocumentString(document); + + Assert.Equal(expected, actual); + Assert.Equal(3, conversions); + } + + [Fact] + public void CanConvertWithNewLineAtEndOfFile() + { + var parse = String.Join(Environment.NewLine, + "", + "", + " ", + "", + " ", + "", + " ", + "", + ""); + + var expected = String.Join(Environment.NewLine, + "", + "", + " ", + "", + " ", + "", + " ", + "", + ""); + + var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo); + + var messaging = new DummyMessaging(); + var converter = new Converter(messaging, 4, null, null); + + var conversions = converter.ConvertDocument(document); + + var actual = UnformattedDocumentString(document); + Assert.Equal(expected, actual); + Assert.Equal(3, conversions); } [Fact] diff --git a/src/wixcop/Converter.cs b/src/wixcop/Converter.cs index 7e8486ab..e125b39c 100644 --- a/src/wixcop/Converter.cs +++ b/src/wixcop/Converter.cs @@ -7,6 +7,7 @@ namespace WixToolset.Tools.WixCop using System.Globalization; using System.IO; using System.Linq; + using System.Text; using System.Xml; using System.Xml.Linq; using WixToolset.Data; @@ -18,7 +19,7 @@ namespace WixToolset.Tools.WixCop /// public class Converter { - private const string XDocumentNewLine = "\n"; // XDocument normlizes "\r\n" to just "\n". + private const char XDocumentNewLine = '\n'; // XDocument normalizes "\r\n" to just "\n". private static readonly XNamespace WixNamespace = "http://wixtoolset.org/schemas/v4/wxs"; private static readonly XName FileElementName = WixNamespace + "File"; @@ -58,7 +59,7 @@ namespace WixToolset.Tools.WixCop { "http://schemas.microsoft.com/wix/2006/WixUnit", "http://wixtoolset.org/schemas/v4/wixunit" }, }; - private Dictionary> ConvertElementMapping; + private readonly Dictionary> ConvertElementMapping; /// /// Instantiate a new Converter class. @@ -79,14 +80,16 @@ namespace WixToolset.Tools.WixCop { Converter.PayloadElementName, this.ConvertSuppressSignatureValidation }, { Converter.WixElementWithoutNamespaceName, this.ConvertWixElementWithoutNamespace }, };*/ - this.ConvertElementMapping = new Dictionary>(); - this.ConvertElementMapping.Add(Converter.FileElementName, this.ConvertFileElement); - this.ConvertElementMapping.Add(Converter.ExePackageElementName, this.ConvertSuppressSignatureValidation); - this.ConvertElementMapping.Add(Converter.MsiPackageElementName, this.ConvertSuppressSignatureValidation); - this.ConvertElementMapping.Add(Converter.MspPackageElementName, this.ConvertSuppressSignatureValidation); - this.ConvertElementMapping.Add(Converter.MsuPackageElementName, this.ConvertSuppressSignatureValidation); - this.ConvertElementMapping.Add(Converter.PayloadElementName, this.ConvertSuppressSignatureValidation); - this.ConvertElementMapping.Add(Converter.WixElementWithoutNamespaceName, this.ConvertWixElementWithoutNamespace); + this.ConvertElementMapping = new Dictionary> + { + { Converter.FileElementName, this.ConvertFileElement }, + { Converter.ExePackageElementName, this.ConvertSuppressSignatureValidation }, + { Converter.MsiPackageElementName, this.ConvertSuppressSignatureValidation }, + { Converter.MspPackageElementName, this.ConvertSuppressSignatureValidation }, + { Converter.MsuPackageElementName, this.ConvertSuppressSignatureValidation }, + { Converter.PayloadElementName, this.ConvertSuppressSignatureValidation }, + { Converter.WixElementWithoutNamespaceName, this.ConvertWixElementWithoutNamespace } + }; this.Messaging = messaging; @@ -129,7 +132,7 @@ namespace WixToolset.Tools.WixCop } catch (XmlException e) { - this.OnError(ConverterTestType.XmlException, (XObject)null, "The xml is invalid. Detail: '{0}'", e.Message); + this.OnError(ConverterTestType.XmlException, null, "The xml is invalid. Detail: '{0}'", e.Message); return this.Errors; } @@ -148,7 +151,7 @@ namespace WixToolset.Tools.WixCop } catch (UnauthorizedAccessException) { - this.OnError(ConverterTestType.UnauthorizedAccessException, (XObject)null, "Could not write to file."); + this.OnError(ConverterTestType.UnauthorizedAccessException, null, "Could not write to file."); } } @@ -177,51 +180,90 @@ namespace WixToolset.Tools.WixCop } else // missing declaration { - if (this.OnError(ConverterTestType.DeclarationMissing, (XNode)null, "This file is missing an XML declaration on the first line.")) + if (this.OnError(ConverterTestType.DeclarationMissing, null, "This file is missing an XML declaration on the first line.")) { document.Declaration = new XDeclaration("1.0", "utf-8", null); - document.Root.AddBeforeSelf(new XText(XDocumentNewLine)); + document.Root.AddBeforeSelf(new XText(XDocumentNewLine.ToString())); } } // Start converting the nodes at the top. - this.ConvertNode(document.Root, 0); + this.ConvertNodes(document.Nodes(), 0); return this.Errors; } - /// - /// Convert a single xml node. - /// - /// The node to convert. - /// The depth level of the node. - /// The converted node. - private void ConvertNode(XNode node, int level) + private void ConvertNodes(IEnumerable nodes, int level) { - // Convert this node's whitespace. - if ((XmlNodeType.Comment == node.NodeType && 0 > ((XComment)node).Value.IndexOf(XDocumentNewLine, StringComparison.Ordinal)) || - XmlNodeType.CDATA == node.NodeType || XmlNodeType.Element == node.NodeType || XmlNodeType.ProcessingInstruction == node.NodeType) + // Note we operate on a copy of the node list since we may + // remove some whitespace nodes during this processing. + foreach (var node in nodes.ToList()) { - this.ConvertWhitespace(node, level); - } + if (node is XText text) + { + if (!String.IsNullOrWhiteSpace(text.Value)) + { + text.Value = text.Value.Trim(); + } + else if (node.NextNode is XCData cdata) + { + this.EnsurePrecedingWhitespaceRemoved(text, node, ConverterTestType.WhitespacePrecedingNodeWrong); + } + else if (node.NextNode is XElement element) + { + this.EnsurePrecedingWhitespaceCorrect(text, node, level, ConverterTestType.WhitespacePrecedingNodeWrong); + } + else if (node.NextNode is null) // this is the space before the close element + { + if (node.PreviousNode is null || node.PreviousNode is XCData) + { + this.EnsurePrecedingWhitespaceRemoved(text, node.Parent, ConverterTestType.WhitespacePrecedingEndElementWrong); + } + else if (level == 0) // root element's close tag + { + this.EnsurePrecedingWhitespaceCorrect(text, node, 0, ConverterTestType.WhitespacePrecedingEndElementWrong); + } + else + { + this.EnsurePrecedingWhitespaceCorrect(text, node, level - 1, ConverterTestType.WhitespacePrecedingEndElementWrong); + } + } + } + else if (node is XElement element) + { + this.ConvertElement(element); - // Convert this node if it is an element. - var element = node as XElement; + this.ConvertNodes(element.Nodes(), level + 1); + } + } + } - if (null != element) + private void EnsurePrecedingWhitespaceCorrect(XText whitespace, XNode node, int level, ConverterTestType testType) + { + if (!Converter.LeadingWhitespaceValid(this.IndentationAmount, level, whitespace.Value)) { - this.ConvertElement(element); - - // Convert all children of this element. - IEnumerable children = element.Nodes().ToList(); + var message = testType == ConverterTestType.WhitespacePrecedingEndElementWrong ? "The whitespace preceding this end element is incorrect." : "The whitespace preceding this node is incorrect."; - foreach (var child in children) + if (this.OnError(testType, node, message)) { - this.ConvertNode(child, level + 1); + Converter.FixupWhitespace(this.IndentationAmount, level, whitespace); } } } + private void EnsurePrecedingWhitespaceRemoved(XText whitespace, XNode node, ConverterTestType testType) + { + if (!String.IsNullOrEmpty(whitespace.Value)) + { + var message = testType == ConverterTestType.WhitespacePrecedingEndElementWrong ? "The whitespace preceding this end element is incorrect." : "The whitespace preceding this node is incorrect."; + + if (this.OnError(testType, node, message)) + { + whitespace.Remove(); + } + } + } + private void ConvertElement(XElement element) { // Gather any deprecated namespaces, then update this element tree based on those deprecations. @@ -229,9 +271,7 @@ namespace WixToolset.Tools.WixCop foreach (var declaration in element.Attributes().Where(a => a.IsNamespaceDeclaration)) { - XNamespace ns; - - if (Converter.OldToNewNamespaceMapping.TryGetValue(declaration.Value, out ns)) + if (Converter.OldToNewNamespaceMapping.TryGetValue(declaration.Value, out var ns)) { if (this.OnError(ConverterTestType.XmlnsValueWrong, declaration, "The namespace '{0}' is out of date. It must be '{1}'.", declaration.Value, ns.NamespaceName)) { @@ -245,10 +285,8 @@ namespace WixToolset.Tools.WixCop Converter.UpdateElementsWithDeprecatedNamespaces(element.DescendantsAndSelf(), deprecatedToUpdatedNamespaces); } - // Convert the node in much greater detail. - Action convert; - - if (this.ConvertElementMapping.TryGetValue(element.Name, out convert)) + // Apply any specialized conversion actions. + if (this.ConvertElementMapping.TryGetValue(element.Name, out var convert)) { convert(element); } @@ -318,96 +356,20 @@ namespace WixToolset.Tools.WixCop } } - /// - /// Convert the whitespace adjacent to a node. - /// - /// The node to convert. - /// The depth level of the node. - private void ConvertWhitespace(XNode node, int level) - { - // Fix the whitespace before this node. - if (node.PreviousNode is XText whitespace) - { - if (XmlNodeType.CDATA == node.NodeType) - { - if (this.OnError(ConverterTestType.WhitespacePrecedingCDATAWrong, node, "There should be no whitespace preceding a CDATA node.")) - { - whitespace.Remove(); - } - } - else - { - // TODO: this code complains about whitespace even after the file has been fixed. - if (!Converter.IsLegalWhitespace(this.IndentationAmount, level, whitespace.Value)) - { - if (this.OnError(ConverterTestType.WhitespacePrecedingNodeWrong, node, "The whitespace preceding this node is incorrect.")) - { - Converter.FixWhitespace(this.IndentationAmount, level, whitespace); - } - } - } - } - - // Fix the whitespace after CDATA nodes. - if (node is XCData cdata) - { - whitespace = cdata.NextNode as XText; - - if (null != whitespace) - { - if (this.OnError(ConverterTestType.WhitespaceFollowingCDATAWrong, node, "There should be no whitespace following a CDATA node.")) - { - whitespace.Remove(); - } - } - } - else - { - // Fix the whitespace inside and after this node (except for Error which may contain just whitespace). - if (node is XElement element && "Error" != element.Name.LocalName) - { - if (!element.HasElements && !element.IsEmpty && String.IsNullOrEmpty(element.Value.Trim())) - { - if (this.OnError(ConverterTestType.NotEmptyElement, element, "This should be an empty element since it contains nothing but whitespace.")) - { - element.RemoveNodes(); - } - } - - whitespace = node.NextNode as XText; - - if (null != whitespace) - { - // TODO: this code crashes when level is 0, - // complains about whitespace even after the file has been fixed, - // and the error text doesn't match the error. - if (!Converter.IsLegalWhitespace(this.IndentationAmount, level - 1, whitespace.Value)) - { - if (this.OnError(ConverterTestType.WhitespacePrecedingEndElementWrong, whitespace, "The whitespace preceding this end element is incorrect.")) - { - Converter.FixWhitespace(this.IndentationAmount, level - 1, whitespace); - } - } - } - } - } - } - private IEnumerable YieldConverterTypes(IEnumerable types) { if (null != types) { foreach (var type in types) { - ConverterTestType itt; - if (Enum.TryParse(type, true, out itt)) + if (Enum.TryParse(type, true, out var itt)) { yield return itt; } else // not a known ConverterTestType { - this.OnError(ConverterTestType.ConverterTestTypeUnknown, (XObject)null, "Unknown error type: '{0}'.", type); + this.OnError(ConverterTestType.ConverterTestTypeUnknown, null, "Unknown error type: '{0}'.", type); } } } @@ -417,9 +379,8 @@ namespace WixToolset.Tools.WixCop { foreach (var element in elements) { - XNamespace ns; - if (deprecatedToUpdatedNamespaces.TryGetValue(element.Name.Namespace, out ns)) + if (deprecatedToUpdatedNamespaces.TryGetValue(element.Name.Namespace, out var ns)) { element.Name = ns.GetName(element.Name.LocalName); } @@ -456,67 +417,33 @@ namespace WixToolset.Tools.WixCop /// The depth level that should match this whitespace. /// The whitespace to validate. /// true if the whitespace is legal; false otherwise. - private static bool IsLegalWhitespace(int indentationAmount, int level, string whitespace) + private static bool LeadingWhitespaceValid(int indentationAmount, int level, string whitespace) { - // strip off leading newlines; there can be an arbitrary number of these - while (whitespace.StartsWith(XDocumentNewLine, StringComparison.Ordinal)) - { - whitespace = whitespace.Substring(XDocumentNewLine.Length); - } + // Strip off leading newlines; there can be an arbitrary number of these. + whitespace = whitespace.TrimStart(XDocumentNewLine); - // check the length - if (whitespace.Length != level * indentationAmount) - { - return false; - } + var indentation = new string(' ', level * indentationAmount); - // check the spaces - foreach (var character in whitespace) - { - if (' ' != character) - { - return false; - } - } - - return true; + return whitespace == indentation; } /// - /// Fix the whitespace in a Whitespace node. + /// Fix the whitespace in a whitespace node. /// /// Indentation value to use when validating leading whitespace. /// The depth level of the desired whitespace. /// The whitespace node to fix. - private static void FixWhitespace(int indentationAmount, int level, XText whitespace) + private static void FixupWhitespace(int indentationAmount, int level, XText whitespace) { - var newLineCount = 0; - - for (var i = 0; i + 1 < whitespace.Value.Length; ++i) - { - if (XDocumentNewLine == whitespace.Value.Substring(i, 2)) - { - ++i; // skip an extra character - ++newLineCount; - } - } - - if (0 == newLineCount) - { - newLineCount = 1; - } + var value = new StringBuilder(whitespace.Value.Length); - // reset the whitespace value - whitespace.Value = String.Empty; + // Keep any previous preceeding new lines. + var newlines = whitespace.Value.TakeWhile(c => c == XDocumentNewLine).Count(); - // add the correct number of newlines - for (var i = 0; i < newLineCount; ++i) - { - whitespace.Value = String.Concat(whitespace.Value, XDocumentNewLine); - } + // Ensure there is always at least one new line before the indentation. + value.Append(XDocumentNewLine, newlines == 0 ? 1 : newlines); - // add the correct number of spaces based on configured indentation amount - whitespace.Value = String.Concat(whitespace.Value, new string(' ', level * indentationAmount)); + whitespace.Value = value.Append(' ', level * indentationAmount).ToString(); } /// -- cgit v1.2.3-55-g6feb