Improve error reporting of duplicate symbols

Virtual symbols provide more interesting ways to have (and avoid) conflicts.
Adding additional messages and cleaning up the existing messages should help
users know what options they have to address conflicts.

This also puts all the conflict resolution in ReportConflictingSymbolsCommand
instead of spreading it across reference resolution as well.

Author: robmen

src/api/wix/WixToolset.Data/ErrorMessages.cs

@@ -323,21 +323,6 @@ public static Message DuplicateSourcesForOutput(string sourceList, string output
             return Message(null, Ids.DuplicateSourcesForOutput, "Multiple source files ({0}) have resulted in the same output file '{1}'. This is likely because the source files only differ in extension or path. Rename the source files to avoid this problem.", sourceList, outputFile);
         }
 
-        public static Message DuplicateSymbol(SourceLineNumber sourceLineNumbers, string symbolName)
-        {
-            return Message(sourceLineNumbers, Ids.DuplicateSymbol, "Duplicate symbol '{0}' found. This typically means that an Id is duplicated. Access modifiers (global, library, file, section) cannot prevent these conflicts. Ensure all your identifiers of a given type (Directory, File, etc.) are unique.", symbolName);
-        }
-
-        public static Message DuplicateSymbol(SourceLineNumber sourceLineNumbers, string symbolName, string referencingSourceLineNumber)
-        {
-            return Message(sourceLineNumbers, Ids.DuplicateSymbol, "Duplicate symbol '{0}' referenced by {1}. This typically means that an Id is duplicated. Ensure all your identifiers of a given type (Directory, File, etc.) are unique or use an access modifier to scope the identfier.", symbolName, referencingSourceLineNumber);
-        }
-
-        public static Message DuplicateSymbol2(SourceLineNumber sourceLineNumbers)
-        {
-            return Message(sourceLineNumbers, Ids.DuplicateSymbol2, "Location of symbol related to previous error.");
-        }
-
         public static Message DuplicateTransform(string transform)
         {
             return Message(null, Ids.DuplicateTransform, "The transform {0} was included twice on the command line. Each transform can be applied to a patch only once.", transform);
@@ -2369,8 +2354,6 @@ public enum Ids
             InvalidDateTimeFormat = 88,
             MultipleEntrySections = 89,
             MultipleEntrySections2 = 90,
-            DuplicateSymbol = 91,
-            DuplicateSymbol2 = 92,
             MissingEntrySection = 93,
             UnresolvedReference = 94,
             MultiplePrimaryReferences = 95,

src/wix/WixToolset.Core/Link/FindEntrySectionAndLoadSymbolsCommand.cs

@@ -36,7 +36,7 @@ public FindEntrySectionAndLoadSymbolsCommand(IMessaging messaging, IEnumerable<I
         /// <summary>
         /// Gets the collection of possibly conflicting symbols.
         /// </summary>
-        public IEnumerable<SymbolWithSection> PossibleConflicts { get; private set; }
+        public IReadOnlyCollection<SymbolWithSection> PossibleConflicts { get; private set; }
 
         /// <summary>
         /// Gets the collection of redundant symbols that should not be included
@@ -140,9 +140,7 @@ public void Execute()
             {
                 if (symbolWithSection.Overrides is null)
                 {
-                    var fullName = symbolWithSection.GetFullName();
-
-                    this.Messaging.Write(LinkerErrors.VirtualSymbolNotFoundForOverride(symbolWithSection.Symbol.SourceLineNumbers, fullName));
+                    this.Messaging.Write(LinkerErrors.VirtualSymbolNotFoundForOverride(symbolWithSection.Symbol));
                 }
             }
 

src/wix/WixToolset.Core/Link/ReportConflictingSymbolsCommand.cs

@@ -9,7 +9,7 @@ namespace WixToolset.Core.Link
 
     internal class ReportConflictingSymbolsCommand
     {
-        public ReportConflictingSymbolsCommand(IMessaging messaging, IEnumerable<SymbolWithSection> possibleConflicts, IEnumerable<IntermediateSection> resolvedSections)
+        public ReportConflictingSymbolsCommand(IMessaging messaging, IReadOnlyCollection<SymbolWithSection> possibleConflicts, ISet<IntermediateSection> resolvedSections)
         {
             this.Messaging = messaging;
             this.PossibleConflicts = possibleConflicts;
@@ -18,35 +18,62 @@ public ReportConflictingSymbolsCommand(IMessaging messaging, IEnumerable<SymbolW
 
         private IMessaging Messaging { get; }
 
-        private  IEnumerable<SymbolWithSection> PossibleConflicts { get; }
+        private IReadOnlyCollection<SymbolWithSection> PossibleConflicts { get; }
 
-        private IEnumerable<IntermediateSection> ResolvedSections { get; }
+        private ISet<IntermediateSection> ResolvedSections { get; }
 
         public void Execute()
         {
-            // Do a quick check if there are any possibly conflicting symbols that don't come from tables that allow
-            // overriding. Hopefully the symbols with possible conflicts list is usually very short list (empty should
-            // be the most common). If we find any matches, we'll do a more costly check to see if the possible conflicting
+            // Do a quick check if there are any possibly conflicting symbols. Hopefully the symbols with possible conflicts
+            // list is a very short list (empty should be the most common).
+            //
+            // If we have conflicts then we'll do a more costly check to see if the possible conflicting
             // symbols are in sections we actually referenced. From the resulting set, show an error for each duplicate
             // (aka: conflicting) symbol.
-            var illegalDuplicates = this.PossibleConflicts.Where(s => s.Symbol.Definition.Type != SymbolDefinitionType.WixAction && s.Symbol.Definition.Type != SymbolDefinitionType.WixVariable).ToList();
-            if (0 < illegalDuplicates.Count)
+            if (0 < this.PossibleConflicts.Count)
             {
-                var referencedSections = new HashSet<IntermediateSection>(this.ResolvedSections);
-
-                foreach (var referencedDuplicate in illegalDuplicates.Where(s => referencedSections.Contains(s.Section)))
+                foreach (var referencedDuplicate in this.PossibleConflicts.Where(s => this.ResolvedSections.Contains(s.Section)))
                 {
-                    var actuallyReferencedDuplicates = referencedDuplicate.PossiblyConflicts.Where(s => referencedSections.Contains(s.Section)).ToList();
+                    var actuallyReferencedDuplicates = referencedDuplicate.PossiblyConflicts.Where(s => this.ResolvedSections.Contains(s.Section)).ToList();
 
-                    if (actuallyReferencedDuplicates.Any())
+                    if (actuallyReferencedDuplicates.Count > 0)
                     {
-                        var fullName = referencedDuplicate.GetFullName();
+                        var conflicts = actuallyReferencedDuplicates.Append(referencedDuplicate).ToList();
+                        var virtualConflicts = conflicts.Where(s => s.Access == AccessModifier.Virtual).ToList();
+                        var overrideConflicts = conflicts.Where(s => s.Access == AccessModifier.Override).ToList();
+                        var otherConflicts = conflicts.Where(s => s.Access != AccessModifier.Virtual && s.Access != AccessModifier.Override).ToList();
+
+                        IEnumerable<SymbolWithSection> reportDuplicates = actuallyReferencedDuplicates;
+
+                        // If multiple symbols are virtual, use the duplicate virtual symbol message.
+                        if (virtualConflicts.Count > 1)
+                        {
+                            var first = virtualConflicts[0];
+                            var referencingSourceLineNumber = first.DirectReferences.FirstOrDefault()?.SourceLineNumbers;
 
-                        this.Messaging.Write(ErrorMessages.DuplicateSymbol(referencedDuplicate.Symbol.SourceLineNumbers, fullName));
+                            reportDuplicates = virtualConflicts.Skip(1);
+
+                            this.Messaging.Write(LinkerErrors.DuplicateVirtualSymbol(first.Symbol, referencingSourceLineNumber));
+                        }
+                        else if (virtualConflicts.Count == 1 && otherConflicts.Count > 0)
+                        {
+                            var first = otherConflicts[0];
+                            var referencingSourceLineNumber = first.DirectReferences.FirstOrDefault()?.SourceLineNumbers;
+
+                            reportDuplicates = virtualConflicts;
+
+                            this.Messaging.Write(LinkerErrors.VirtualSymbolMustBeOverridden(first.Symbol, referencingSourceLineNumber));
+                        }
+                        else
+                        {
+                            var referencingSourceLineNumber = referencedDuplicate.DirectReferences.FirstOrDefault()?.SourceLineNumbers;
+
+                            this.Messaging.Write(LinkerErrors.DuplicateSymbol(referencedDuplicate.Symbol, referencingSourceLineNumber));
+                        }
 
-                        foreach (var duplicate in actuallyReferencedDuplicates)
+                        foreach (var duplicate in reportDuplicates)
                         {
-                            this.Messaging.Write(ErrorMessages.DuplicateSymbol2(duplicate.Symbol.SourceLineNumbers));
+                            this.Messaging.Write(LinkerErrors.DuplicateSymbol2(duplicate.Symbol));
                         }
                     }
                 }

src/wix/WixToolset.Core/Link/ResolveReferencesCommand.cs

@@ -27,9 +27,9 @@ public ResolveReferencesCommand(IMessaging messaging, IntermediateSection entryS
             this.BuildingMergeModule = (SectionType.Module == entrySection.Type);
         }
 
-        public IEnumerable<SymbolWithSection> ReferencedSymbolWithSections => this.referencedSymbols;
+        public IReadOnlyCollection<SymbolWithSection> ReferencedSymbolWithSections => this.referencedSymbols;
 
-        public IEnumerable<IntermediateSection> ResolvedSections => this.resolvedSections;
+        public ISet<IntermediateSection> ResolvedSections => this.resolvedSections;
 
         private bool BuildingMergeModule { get; }
 
@@ -63,55 +63,60 @@ private void RecursivelyResolveReferences(IntermediateSection section)
             // symbols provided.  Then recursively call this method to process the
             // located symbol's section.  All in all this is a very simple depth-first
             // search of the references per-section.
-            foreach (var wixSimpleReferenceRow in section.Symbols.OfType<WixSimpleReferenceSymbol>())
+            foreach (var reference in section.Symbols.OfType<WixSimpleReferenceSymbol>())
             {
                 // If we're building a Merge Module, ignore all references to the Media table
                 // because Merge Modules don't have Media tables.
-                if (this.BuildingMergeModule && wixSimpleReferenceRow.Table == "Media")
+                if (this.BuildingMergeModule && reference.Table == "Media")
                 {
                     continue;
                 }
 
                 // See if the symbol (and any of its duplicates) are appropriately accessible.
-                if (this.symbolsWithSections.TryGetValue(wixSimpleReferenceRow.SymbolicName, out var symbolWithSection))
+                if (this.symbolsWithSections.TryGetValue(reference.SymbolicName, out var symbolWithSection))
                 {
                     var accessible = this.DetermineAccessibleSymbols(section, symbolWithSection);
                     if (accessible.Count == 1)
                     {
                         var accessibleSymbol = accessible[0];
+
+                        accessibleSymbol.AddDirectReference(reference);
+
                         if (this.referencedSymbols.Add(accessibleSymbol) && null != accessibleSymbol.Section)
                         {
                             this.RecursivelyResolveReferences(accessibleSymbol.Section);
                         }
                     }
                     else if (accessible.Count == 0)
                     {
-                        this.Messaging.Write(ErrorMessages.UnresolvedReference(wixSimpleReferenceRow.SourceLineNumbers, wixSimpleReferenceRow.SymbolicName, symbolWithSection.Access));
+                        this.Messaging.Write(ErrorMessages.UnresolvedReference(reference.SourceLineNumbers, reference.SymbolicName, symbolWithSection.Access));
                     }
-                    else // display errors for the duplicate symbols.
+                    else // multiple symbols referenced creates conflicting symbols.
                     {
-                        var accessibleSymbol = accessible[0];
-                        var accessibleFullName = accessibleSymbol.GetFullName();
-                        var referencingSourceLineNumber = wixSimpleReferenceRow.SourceLineNumbers?.ToString();
-
-                        if (String.IsNullOrEmpty(referencingSourceLineNumber))
-                        {
-                            this.Messaging.Write(ErrorMessages.DuplicateSymbol(accessibleSymbol.Symbol.SourceLineNumbers, accessibleFullName));
-                        }
-                        else
+                        // Remember the direct reference to the symbol for the error reporting later,
+                        // but do NOT continue resolving references found in these conflicting symbols.
+                        foreach (var conflict in accessible)
                         {
-                            this.Messaging.Write(ErrorMessages.DuplicateSymbol(accessibleSymbol.Symbol.SourceLineNumbers, accessibleFullName, referencingSourceLineNumber));
-                        }
+                            // This should NEVER happen.
+                            if (!conflict.PossiblyConflicts.Any())
+                            {
+                                throw new InvalidOperationException("If a reference can reference multiple symbols, those symbols MUST have already been recognized as possible conflicts.");
+                            }
 
-                        foreach (var accessibleDuplicate in accessible.Skip(1))
-                        {
-                            this.Messaging.Write(ErrorMessages.DuplicateSymbol2(accessibleDuplicate.Symbol.SourceLineNumbers));
+                            conflict.AddDirectReference(reference);
+
+                            this.referencedSymbols.Add(conflict);
+
+                            if (conflict.Section != null)
+                            {
+                                this.resolvedSections.Add(conflict.Section);
+                            }
                         }
                     }
                 }
                 else
                 {
-                    this.Messaging.Write(ErrorMessages.UnresolvedReference(wixSimpleReferenceRow.SourceLineNumbers, wixSimpleReferenceRow.SymbolicName));
+                    this.Messaging.Write(ErrorMessages.UnresolvedReference(reference.SourceLineNumbers, reference.SymbolicName));
                 }
             }
         }
@@ -133,14 +138,6 @@ private List<SymbolWithSection> DetermineAccessibleSymbols(IntermediateSection r
 
             foreach (var dupe in symbolWithSection.PossiblyConflicts)
             {
-                //// don't count overridable WixActionSymbols
-                //var symbolAction = symbolWithSection.Symbol as WixActionSymbol;
-                //var dupeAction = dupe.Symbol as WixActionSymbol;
-                //if (symbolAction?.Overridable != dupeAction?.Overridable)
-                //{
-                //    continue;
-                //}
-
                 if (this.AccessibleSymbol(referencingSection, dupe))
                 {
                     accessibleSymbols.Add(dupe);

src/wix/WixToolset.Core/Link/SymbolWithSection.cs

@@ -6,12 +6,14 @@ namespace WixToolset.Core.Link
     using System.Collections.Generic;
     using System.Linq;
     using WixToolset.Data;
+    using WixToolset.Data.Symbols;
 
     /// <summary>
     /// Symbol with section representing a single unique symbol.
     /// </summary>
     internal class SymbolWithSection
     {
+        private List<WixSimpleReferenceSymbol> directReferences;
         private HashSet<SymbolWithSection> possibleConflicts;
 
         /// <summary>
@@ -43,6 +45,11 @@ public SymbolWithSection(IntermediateSection section, IntermediateSymbol symbol)
         /// <value>Section for the symbol.</value>
         public IntermediateSection Section { get; }
 
+        /// <summary>
+        /// Gets any duplicates of this symbol with sections that are possible conflicts.
+        /// </summary>
+        public IEnumerable<WixSimpleReferenceSymbol> DirectReferences => this.directReferences ?? Enumerable.Empty<WixSimpleReferenceSymbol>();
+
         /// <summary>
         /// Gets any duplicates of this symbol with sections that are possible conflicts.
         /// </summary>
@@ -67,6 +74,20 @@ public void AddPossibleConflict(SymbolWithSection symbolWithSection)
             this.possibleConflicts.Add(symbolWithSection);
         }
 
+        /// <summary>
+        /// Adds a reference that directly points to this symbol.
+        /// </summary>
+        /// <param name="reference">The direct reference.</param>
+        public void AddDirectReference(WixSimpleReferenceSymbol reference)
+        {
+            if (null == this.directReferences)
+            {
+                this.directReferences = new List<WixSimpleReferenceSymbol>();
+            }
+
+            this.directReferences.Add(reference);
+        }
+
         /// <summary>
         /// Override a virtual symbol.
         /// </summary>