From 217182f85fa737d20fb5846f395cabfa599bf1c6 Mon Sep 17 00:00:00 2001 From: Rob Mensching Date: Tue, 25 Oct 2022 07:01:26 -0700 Subject: Update converted source line numbers when conversion is saved The conversion process has a good chance of changing the shape of .wxs source file. Therefore, the old line numbers may be wrong and should be replaced with the line numbers from the converted file when it is saved. Fixes 6969 --- src/wix/WixToolset.Converters/HashSetExtensions.cs | 17 ++ src/wix/WixToolset.Converters/WixConverter.cs | 263 ++++++++++++++++----- .../WixToolsetTest.Converters/ConverterFixture.cs | 38 ++- .../ConverterIntegrationFixture.cs | 11 +- .../FixDeclarationAndNamespace.wxs | 4 + 5 files changed, 272 insertions(+), 61 deletions(-) create mode 100644 src/wix/WixToolset.Converters/HashSetExtensions.cs create mode 100644 src/wix/test/WixToolsetTest.Converters/TestData/FixDeclarationAndNamespace/FixDeclarationAndNamespace.wxs (limited to 'src') diff --git a/src/wix/WixToolset.Converters/HashSetExtensions.cs b/src/wix/WixToolset.Converters/HashSetExtensions.cs new file mode 100644 index 00000000..49b0d4ac --- /dev/null +++ b/src/wix/WixToolset.Converters/HashSetExtensions.cs @@ -0,0 +1,17 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. Licensed under the Microsoft Reciprocal License. See LICENSE.TXT file in the project root for full license information. + +namespace WixToolset.Converters +{ + using System.Collections.Generic; + + internal static class HashSetExtensions + { + public static void AddRange(this HashSet set, IEnumerable values) + { + foreach (var value in values) + { + set.Add(value); + } + } + } +} diff --git a/src/wix/WixToolset.Converters/WixConverter.cs b/src/wix/WixToolset.Converters/WixConverter.cs index de81c876..cf77681c 100644 --- a/src/wix/WixToolset.Converters/WixConverter.cs +++ b/src/wix/WixToolset.Converters/WixConverter.cs @@ -11,6 +11,7 @@ namespace WixToolset.Converters using System.Text.RegularExpressions; using System.Xml; using System.Xml.Linq; + using System.Xml.XPath; using WixToolset.Data; using WixToolset.Data.WindowsInstaller; using WixToolset.Extensibility.Services; @@ -272,6 +273,8 @@ namespace WixToolset.Converters { WixConverter.WixLocalizationElementWithoutNamespaceName, this.ConvertWixLocalizationElementWithoutNamespace }, }; + this.ConversionMessages = new List(); + this.Messaging = messaging; this.IndentationAmount = indentationAmount; @@ -285,7 +288,7 @@ namespace WixToolset.Converters private CustomTableTarget CustomTableSetting { get; } - private int Messages { get; set; } + private List ConversionMessages { get; } private HashSet ErrorsAsWarnings { get; set; } @@ -308,54 +311,35 @@ namespace WixToolset.Converters /// /// The file to convert. /// Option to save the converted Messages that are found. - /// The number of Messages found. + /// The number of conversions found. public int ConvertFile(string sourceFile, bool saveConvertedFile) { - var document = this.OpenSourceFile(sourceFile); + var savedDocument = false; - if (document is null) + if (this.TryOpenSourceFile(sourceFile, out var document)) { - return 1; - } - - this.ConvertDocument(document); + this.Convert(document); - // Fix Messages if requested and necessary. - if (saveConvertedFile && 0 < this.Messages) - { - this.SaveDocument(document); + // Fix Messages if requested and necessary. + if (saveConvertedFile && 0 < this.ConversionMessages.Count) + { + savedDocument = this.SaveDocument(document); + } } - return this.Messages; + return this.ReportMessages(document, savedDocument); } /// /// Convert a document. /// /// The document to convert. - /// The number of Messages found. + /// The number of conversions found. public int ConvertDocument(XDocument document) { - // Reset the instance info. - this.Messages = 0; - this.SourceVersion = 0; - this.Operation = ConvertOperation.Convert; - - // Remove the declaration. - if (null != document.Declaration - && this.OnInformation(ConverterTestType.DeclarationPresent, null, "This file contains an XML declaration on the first line.")) - { - document.Declaration = null; - TrimLeadingText(document); - } - - this.XRoot = document.Root; - - // Start converting the nodes at the top. - this.ConvertNodes(document.Nodes(), 0); - this.RemoveUnusedNamespaces(document.Root); + this.Convert(document); - return this.Messages; + return this.ReportMessages(document, false); } /// @@ -366,22 +350,20 @@ namespace WixToolset.Converters /// The number of Messages found. public int FormatFile(string sourceFile, bool saveConvertedFile) { - var document = this.OpenSourceFile(sourceFile); + var savedDocument = false; - if (document is null) + if (this.TryOpenSourceFile(sourceFile, out var document)) { - return 1; - } - - this.FormatDocument(document); + this.FormatDocument(document); - // Fix Messages if requested and necessary. - if (saveConvertedFile && 0 < this.Messages) - { - this.SaveDocument(document); + // Fix Messages if requested and necessary. + if (saveConvertedFile && 0 < this.ConversionMessages.Count) + { + savedDocument = this.SaveDocument(document); + } } - return this.Messages; + return this.ReportMessages(document, savedDocument); } /// @@ -390,44 +372,74 @@ namespace WixToolset.Converters /// The document to format. /// The number of Messages found. public int FormatDocument(XDocument document) + { + this.Format(document); + + return this.ReportMessages(document, false); + } + + private void Convert(XDocument document) { // Reset the instance info. - this.Messages = 0; + this.ConversionMessages.Clear(); this.SourceVersion = 0; - this.Operation = ConvertOperation.Format; + this.Operation = ConvertOperation.Convert; // Remove the declaration. if (null != document.Declaration - && this.OnInformation(ConverterTestType.DeclarationPresent, null, "This file contains an XML declaration on the first line.")) + && this.OnInformation(ConverterTestType.DeclarationPresent, document, "This file contains an XML declaration on the first line.")) { document.Declaration = null; TrimLeadingText(document); } + this.XRoot = document.Root; + // Start converting the nodes at the top. this.ConvertNodes(document.Nodes(), 0); this.RemoveUnusedNamespaces(document.Root); + } + + private void Format(XDocument document) + { + // Reset the instance info. + this.ConversionMessages.Clear(); + this.SourceVersion = 0; + this.Operation = ConvertOperation.Format; - return this.Messages; + // Remove the declaration. + if (null != document.Declaration + && this.OnInformation(ConverterTestType.DeclarationPresent, document, "This file contains an XML declaration on the first line.")) + { + document.Declaration = null; + TrimLeadingText(document); + } + + this.XRoot = document.Root; + + // Start converting the nodes at the top. + this.ConvertNodes(document.Nodes(), 0); + this.RemoveUnusedNamespaces(document.Root); } - private XDocument OpenSourceFile(string sourceFile) + private bool TryOpenSourceFile(string sourceFile, out XDocument document) { this.SourceFile = sourceFile; try { - return XDocument.Load(this.SourceFile, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo); + document = XDocument.Load(this.SourceFile, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo); } catch (XmlException e) { this.OnError(ConverterTestType.XmlException, null, "The xml is invalid. Detail: '{0}'", e.Message); + document = null; } - return null; + return document != null; } - private void SaveDocument(XDocument document) + private bool SaveDocument(XDocument document) { var ignoreDeclarationError = this.IgnoreErrors.Contains(ConverterTestType.DeclarationPresent); @@ -437,11 +449,15 @@ namespace WixToolset.Converters { document.Save(writer); } + + return true; } catch (UnauthorizedAccessException) { this.OnError(ConverterTestType.UnauthorizedAccessException, null, "Could not write to file."); } + + return false; } private void ConvertNodes(IEnumerable nodes, int level) @@ -2129,6 +2145,14 @@ namespace WixToolset.Converters convertedAttribute = new XAttribute(ns.GetName(attribute.Name.LocalName), attribute.Value); } + if (convertedAttribute != attribute) + { + foreach (var message in attribute.Annotations()) + { + convertedAttribute.AddAnnotation(message); + } + } + element.Add(convertedAttribute); } } @@ -2214,6 +2238,110 @@ namespace WixToolset.Converters } } + private int ReportMessages(XDocument document, bool saved) + { + var conversionCount = this.ConversionMessages.Count; + + // If the converted/formatted document was saved, update the source line numbers + // in the messages since they will possibly be wrong. + if (saved && conversionCount > 0) + { + var fixedupMessages = new HashSet(); + + // Load the converted document so we can look up new line numbers for + // messages. + var convertedDocument = XDocument.Load(this.SourceFile, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo); + var convertedNavigator = convertedDocument.CreateNavigator(); + + // Look through all nodes in the document and try to fix up their line numbers. + foreach (var elements in document.Descendants()) + { + var fixedup = this.FixupMessageLineNumbers(elements, convertedNavigator); + fixedupMessages.AddRange(fixedup); + + // Attributes are not considered nodes so they must be enumerated independently. + if (elements is XElement element) + { + foreach (var attribute in element.Attributes()) + { + fixedup = this.FixupMessageLineNumbers(attribute, convertedNavigator); + fixedupMessages.AddRange(fixedup); + } + } + } + + // For any messages that couldn't be fixed up, remove their line numbers since they point at lines + // that are possibly wrong after the conversion. + for (var i = 0; i < this.ConversionMessages.Count; ++i) + { + var message = this.ConversionMessages[i]; + + if (!fixedupMessages.Contains(message)) + { + this.ConversionMessages[i] = new Message(new SourceLineNumber(this.SourceFile), message.Level, message.Id, message.ResourceNameOrFormat, message.MessageArgs); + } + } + } + + foreach (var message in this.ConversionMessages) + { + this.Messaging.Write(message); + } + + return conversionCount; + } + + private IReadOnlyCollection FixupMessageLineNumbers(XObject obj, XPathNavigator savedDocument) + { + var messages = obj.Annotations().ToList(); + + if (messages.Count == 0) + { + return Array.Empty(); + } + + // We can't fix up line numbers based on attributes but we can fix up using their parent element, so do so. + if (obj is XAttribute attribute) + { + obj = attribute.Parent; + } + + var fixedupMessages = new List(); + + var modifiedLineNumber = this.GetModifiedNodeLineNumber((XElement)obj, savedDocument); + + foreach (var message in messages) + { + var fixedupMessage = message; + + // If the line number wasn't modified, the existing message's source line nuber is correct + // so skip creating a new one. + if (modifiedLineNumber != null) + { + var index = this.ConversionMessages.IndexOf(message); + + fixedupMessage = new Message(modifiedLineNumber, message.Level, message.Id, message.ResourceNameOrFormat, message.MessageArgs); + + this.ConversionMessages[index] = fixedupMessage; + } + + fixedupMessages.Add(fixedupMessage); + } + + return fixedupMessages; + } + + private SourceLineNumber GetModifiedNodeLineNumber(XElement element, XPathNavigator savedDocument) + { + var xpathToOld = CalculateXPath(element); + + var newNode = savedDocument.SelectSingleNode(xpathToOld); + var newLineNumber = (newNode as IXmlLineInfo).LineNumber; + + var oldLineNumber = (element as IXmlLineInfo)?.LineNumber; + return (oldLineNumber != newLineNumber) ? new SourceLineNumber(this.SourceFile, newLineNumber) : null; + } + /// /// Output an error message to the console. /// @@ -2250,10 +2378,8 @@ namespace WixToolset.Converters return false; } - // Increase the message count. - this.Messages++; + var sourceLine = SourceLineNumberForXmlLineInfo(this.SourceFile, node); - var sourceLine = (null == node) ? new SourceLineNumber(this.SourceFile ?? "wix.exe") : new SourceLineNumber(this.SourceFile, ((IXmlLineInfo)node).LineNumber); var prefix = String.Empty; if (level == MessageLevel.Information) { @@ -2268,11 +2394,34 @@ namespace WixToolset.Converters var msg = new Message(sourceLine, level, (int)converterTestType, format, args); - this.Messaging.Write(msg); + // Add the message as a node annotation so it could be possible to remap source line numbers + // to their new locations after converting/formatting (since lines of code could be moved). + node?.AddAnnotation(msg); + this.ConversionMessages.Add(msg); return true; } + private static string CalculateXPath(XElement element) + { + var builder = new StringBuilder(); + + while (element != null) + { + var index = element.Parent?.Elements().TakeWhile(e => e != element).Count() ?? 0; + builder.Insert(0, $"/*[{index + 1}]"); + + element = element.Parent; + } + + return builder.ToString(); + } + + private static SourceLineNumber SourceLineNumberForXmlLineInfo(string sourceFile, IXmlLineInfo lineInfo) + { + return (lineInfo?.HasLineInfo() == true) ? new SourceLineNumber(sourceFile, lineInfo.LineNumber) : new SourceLineNumber(sourceFile ?? "wix.exe"); + } + /// /// Return an identifier based on passed file/directory name /// @@ -2338,7 +2487,7 @@ namespace WixToolset.Converters comments = initialComments; var nonCommentNodes = new List(); - foreach(var node in nodes) + foreach (var node in nodes) { if (XmlNodeType.Comment == node.NodeType) { diff --git a/src/wix/test/WixToolsetTest.Converters/ConverterFixture.cs b/src/wix/test/WixToolsetTest.Converters/ConverterFixture.cs index d186931b..cd6849d3 100644 --- a/src/wix/test/WixToolsetTest.Converters/ConverterFixture.cs +++ b/src/wix/test/WixToolsetTest.Converters/ConverterFixture.cs @@ -3,6 +3,8 @@ namespace WixToolsetTest.Converters { using System; + using System.IO; + using System.Linq; using System.Xml.Linq; using WixBuildTools.TestSupport; using WixToolset.Converters; @@ -62,7 +64,7 @@ namespace WixToolsetTest.Converters var document = XDocument.Parse(parse, LoadOptions.PreserveWhitespace | LoadOptions.SetLineInfo); var messaging = new MockMessaging(); - var converter = new WixConverter(messaging, 2, ignoreErrors: new[] { "DeclarationPresent" } ); + var converter = new WixConverter(messaging, 2, ignoreErrors: new[] { "DeclarationPresent" }); var errors = converter.ConvertDocument(document); @@ -102,6 +104,40 @@ namespace WixToolsetTest.Converters WixAssert.CompareLineByLine(expected, actual); } + [Fact] + public void CanConvertMainNamespaceFromDisk() + { + var dataFolder = TestData.Get("TestData", "FixDeclarationAndNamespace"); + + var expected = new[] + { + "", + " ", + "", + }; + + using (var fs = new TestDataFolderFileSystem()) + { + fs.Initialize(dataFolder); + var path = Path.Combine(fs.BaseFolder, "FixDeclarationAndNamespace.wxs"); + + var messaging = new MockMessaging(); + var converter = new WixConverter(messaging, 2, null, null); + + var errors = converter.ConvertFile(path, true); + + var messages = messaging.Messages.Select(m => $"{Path.GetFileName(m.SourceLineNumbers.FileName)}({m.SourceLineNumbers.LineNumber}) {m.ToString()}").ToArray(); + WixAssert.CompareLineByLine(new[] + { + "FixDeclarationAndNamespace.wxs() [Converted] This file contains an XML declaration on the first line. (DeclarationPresent)", + "FixDeclarationAndNamespace.wxs(1) [Converted] The namespace 'http://schemas.microsoft.com/wix/2006/wi' is out of date. It must be 'http://wixtoolset.org/schemas/v4/wxs'. (XmlnsValueWrong)" + }, messages); + + var actual = File.ReadAllLines(path); + WixAssert.CompareLineByLine(expected, actual); + } + } + [Fact] public void CanConvertNamedMainNamespace() { diff --git a/src/wix/test/WixToolsetTest.Converters/ConverterIntegrationFixture.cs b/src/wix/test/WixToolsetTest.Converters/ConverterIntegrationFixture.cs index a8908df7..fdebc6b0 100644 --- a/src/wix/test/WixToolsetTest.Converters/ConverterIntegrationFixture.cs +++ b/src/wix/test/WixToolsetTest.Converters/ConverterIntegrationFixture.cs @@ -85,9 +85,14 @@ namespace WixToolsetTest.Converters var messaging = new MockMessaging(); var converter = new WixConverter(messaging, 4); - var errors = converter.ConvertFile(targetFile, true); - - Assert.Single(messaging.Messages.Where(m => m.Id == 5/*WixConverter.ConverterTestType.UnauthorizedAccessException*/)); + var convertedCount = converter.ConvertFile(targetFile, true); + + var errors = messaging.Messages.Where(m => m.Level == WixToolset.Data.MessageLevel.Error).Select(m => m.ToString()).ToArray(); + WixAssert.CompareLineByLine(new[] + { + "Could not write to file. (UnauthorizedAccessException)" + }, errors); + Assert.Equal(10, convertedCount); } } diff --git a/src/wix/test/WixToolsetTest.Converters/TestData/FixDeclarationAndNamespace/FixDeclarationAndNamespace.wxs b/src/wix/test/WixToolsetTest.Converters/TestData/FixDeclarationAndNamespace/FixDeclarationAndNamespace.wxs new file mode 100644 index 00000000..a8923337 --- /dev/null +++ b/src/wix/test/WixToolsetTest.Converters/TestData/FixDeclarationAndNamespace/FixDeclarationAndNamespace.wxs @@ -0,0 +1,4 @@ + + + + -- cgit v1.2.3-55-g6feb