Include duplicated inline directory symbols referenced in subsequent sections

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

@@ -42,13 +42,13 @@ public FindEntrySectionAndLoadSymbolsCommand(IMessaging messaging, IEnumerable<I
         /// Gets the collection of redundant symbols that should not be included
         /// in the final output.
         /// </summary>
-        public ISet<IntermediateSymbol> RedundantSymbols { get; private set; }
+        public ISet<IntermediateSymbol> IdenticalDirectorySymbols { get; private set; }
 
         public void Execute()
         {
             var symbolsByName = new Dictionary<string, SymbolWithSection>();
             var possibleConflicts = new HashSet<SymbolWithSection>();
-            var redundantSymbols = new HashSet<IntermediateSymbol>();
+            var identicalDirectorySymbols = new HashSet<IntermediateSymbol>();
 
             if (!Enum.TryParse(this.ExpectedOutputType.ToString(), out SectionType expectedEntrySectionType))
             {
@@ -76,28 +76,27 @@ public void Execute()
                     }
                 }
 
-                // Load all the symbols from the section's tables that create symbols.
+                // Load all the symbols from the section's tables that can be referenced (i.e. have an Id).
                 foreach (var symbol in section.Symbols.Where(t => t.Id != null))
                 {
                     var symbolWithSection = new SymbolWithSection(section, symbol);
+                    var fullName = symbolWithSection.GetFullName();
 
-                    if (!symbolsByName.TryGetValue(symbolWithSection.Name, out var existingSymbol))
+                    if (!symbolsByName.TryGetValue(fullName, out var existingSymbol))
                     {
-                        symbolsByName.Add(symbolWithSection.Name, symbolWithSection);
+                        symbolsByName.Add(fullName, symbolWithSection);
                     }
                     else // uh-oh, duplicate symbols.
                     {
                         // If the duplicate symbols are both private directories, there is a chance that they
-                        // point to identical symbols. Identical directory symbols are redundant and will not cause
-                        // conflicts.
+                        // point to identical symbols. Identical directory symbols should be treated as redundant.
+                        // and not cause conflicts.
                         if (AccessModifier.Section == existingSymbol.Access && AccessModifier.Section == symbolWithSection.Access &&
                             SymbolDefinitionType.Directory == existingSymbol.Symbol.Definition.Type && existingSymbol.Symbol.IsIdentical(symbolWithSection.Symbol))
                         {
-                            // Ensure identical symbol's symbol is marked redundant to ensure (should the symbol be
-                            // referenced into the final output) it will not add duplicate primary keys during
-                            // the .IDT importing.
-                            existingSymbol.AddRedundant(symbolWithSection);
-                            redundantSymbols.Add(symbolWithSection.Symbol);
+                            // Ensure identical symbols are tracked to ensure that only one symbol will end up in linked intermediate.
+                            identicalDirectorySymbols.Add(existingSymbol.Symbol);
+                            identicalDirectorySymbols.Add(symbolWithSection.Symbol);
                         }
                         else
                         {
@@ -111,7 +110,7 @@ public void Execute()
 
             this.SymbolsByName = symbolsByName;
             this.PossibleConflicts = possibleConflicts;
-            this.RedundantSymbols = redundantSymbols;
+            this.IdenticalDirectorySymbols = identicalDirectorySymbols;
         }
     }
 }

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

@@ -40,7 +40,9 @@ public void Execute()
 
                     if (actuallyReferencedDuplicates.Any())
                     {
-                        this.Messaging.Write(ErrorMessages.DuplicateSymbol(referencedDuplicate.Symbol.SourceLineNumbers, referencedDuplicate.Name));
+                        var fullName = referencedDuplicate.GetFullName();
+
+                        this.Messaging.Write(ErrorMessages.DuplicateSymbol(referencedDuplicate.Symbol.SourceLineNumbers, fullName));
 
                         foreach (var duplicate in actuallyReferencedDuplicates)
                         {

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

@@ -91,15 +91,16 @@ private void RecursivelyResolveReferences(IntermediateSection section)
                     else // display errors for the duplicate 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, accessibleSymbol.Name));
+                            this.Messaging.Write(ErrorMessages.DuplicateSymbol(accessibleSymbol.Symbol.SourceLineNumbers, accessibleFullName));
                         }
                         else
                         {
-                            this.Messaging.Write(ErrorMessages.DuplicateSymbol(accessibleSymbol.Symbol.SourceLineNumbers, accessibleSymbol.Name, referencingSourceLineNumber));
+                            this.Messaging.Write(ErrorMessages.DuplicateSymbol(accessibleSymbol.Symbol.SourceLineNumbers, accessibleFullName, referencingSourceLineNumber));
                         }
 
                         foreach (var accessibleDuplicate in accessible.Skip(1))
@@ -146,14 +147,6 @@ private List<SymbolWithSection> DetermineAccessibleSymbols(IntermediateSection r
                 }
             }
 
-            foreach (var dupe in symbolWithSection.Redundants)
-            {
-                if (this.AccessibleSymbol(referencingSection, dupe))
-                {
-                    accessibleSymbols.Add(dupe);
-                }
-            }
-
             return accessibleSymbols;
         }
 

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

@@ -13,7 +13,6 @@ namespace WixToolset.Core.Link
     internal class SymbolWithSection
     {
         private HashSet<SymbolWithSection> possibleConflicts;
-        private HashSet<SymbolWithSection> redundants;
 
         /// <summary>
         /// Creates a symbol for a symbol.
@@ -24,7 +23,6 @@ public SymbolWithSection(IntermediateSection section, IntermediateSymbol symbol)
         {
             this.Symbol = symbol;
             this.Section = section;
-            this.Name = String.Concat(this.Symbol.Definition.Name, ":", this.Symbol.Id.Id);
         }
 
         /// <summary>
@@ -33,12 +31,6 @@ public SymbolWithSection(IntermediateSection section, IntermediateSymbol symbol)
         /// <value>Accessbility of the symbol.</value>
         public AccessModifier Access => this.Symbol.Id.Access;
 
-        /// <summary>
-        /// Gets the name of the symbol.
-        /// </summary>
-        /// <value>Name of the symbol.</value>
-        public string Name { get; }
-
         /// <summary>
         /// Gets the symbol for this symbol.
         /// </summary>
@@ -56,11 +48,6 @@ public SymbolWithSection(IntermediateSection section, IntermediateSymbol symbol)
         /// </summary>
         public IEnumerable<SymbolWithSection> PossiblyConflicts => this.possibleConflicts ?? Enumerable.Empty<SymbolWithSection>();
 
-        /// <summary>
-        /// Gets any duplicates of this symbol with sections that are redundant.
-        /// </summary>
-        public IEnumerable<SymbolWithSection> Redundants => this.redundants ?? Enumerable.Empty<SymbolWithSection>();
-
         /// <summary>
         /// Adds a duplicate symbol with sections that is a possible conflict.
         /// </summary>
@@ -76,17 +63,11 @@ public void AddPossibleConflict(SymbolWithSection symbolWithSection)
         }
 
         /// <summary>
-        /// Adds a duplicate symbol that is redundant.
+        /// Gets the full name of the symbol.
         /// </summary>
-        /// <param name="symbolWithSection">Symbol with section that is redundant of this symbol.</param>
-        public void AddRedundant(SymbolWithSection symbolWithSection)
+        public string GetFullName()
         {
-            if (null == this.redundants)
-            {
-                this.redundants = new HashSet<SymbolWithSection>();
-            }
-
-            this.redundants.Add(symbolWithSection);
+            return String.Concat(this.Symbol.Definition.Name, ":", this.Symbol.Id.Id);
         }
     }
 }

src/wix/WixToolset.Core/Linker.cs

@@ -177,14 +177,20 @@ public Intermediate Link(ILinkContext context)
                 // metadata.
                 var resolvedSection = new IntermediateSection(find.EntrySection.Id, find.EntrySection.Type);
                 var references = new List<WixSimpleReferenceSymbol>();
+                var identicalDirectoryIds = new HashSet<string>(StringComparer.Ordinal);
 
                 foreach (var section in sections)
                 {
                     foreach (var symbol in section.Symbols)
                     {
-                        if (find.RedundantSymbols.Contains(symbol))
+                        // If this symbol is an identical directory, ensure we only visit
+                        // one (and skip the other identicals with the same id).
+                        if (find.IdenticalDirectorySymbols.Contains(symbol))
                         {
-                            continue;
+                            if (!identicalDirectoryIds.Add(symbol.Id.Id))
+                            {
+                                continue;
+                            }
                         }
 
                         var copySymbol = true; // by default, copy symbols.
@@ -351,22 +357,24 @@ private void LoadStandardSymbols(IDictionary<string, SymbolWithSection> symbolsB
             foreach (var actionSymbol in WindowsInstallerStandard.StandardActions())
             {
                 var symbolWithSection = new SymbolWithSection(null, actionSymbol);
+                var fullName = symbolWithSection.GetFullName();
 
                 // If the action's symbol has not already been defined (i.e. overriden by the user), add it now.
-                if (!symbolsByName.ContainsKey(symbolWithSection.Name))
+                if (!symbolsByName.ContainsKey(fullName))
                 {
-                    symbolsByName.Add(symbolWithSection.Name, symbolWithSection);
+                    symbolsByName.Add(fullName, symbolWithSection);
                 }
             }
 
             foreach (var directorySymbol in WindowsInstallerStandard.StandardDirectories())
             {
                 var symbolWithSection = new SymbolWithSection(null, directorySymbol);
+                var fullName = symbolWithSection.GetFullName();
 
                 // If the directory's symbol has not already been defined (i.e. overriden by the user), add it now.
-                if (!symbolsByName.ContainsKey(symbolWithSection.Name))
+                if (!symbolsByName.ContainsKey(fullName))
                 {
-                    symbolsByName.Add(symbolWithSection.Name, symbolWithSection);
+                    symbolsByName.Add(fullName, symbolWithSection);
                 }
             }
         }

src/wix/test/WixToolsetTest.CoreIntegration/DirectoryFixture.cs

@@ -273,5 +273,52 @@ public void CanGetDuplicateTargetSourceName()
                 }, directoryRows.Select(r => String.Join('\t', r.FieldAsString(0), r.FieldAsString(1), r.FieldAsString(2))).ToArray());
             }
         }
+
+        [Fact]
+        public void CanFindRedundantSubdirectoryInSecondSection()
+        {
+            var folder = TestData.Get(@"TestData");
+
+            using (var fs = new DisposableFileSystem())
+            {
+                var baseFolder = fs.GetFolder();
+                var intermediateFolder = Path.Combine(baseFolder, "obj");
+                var msiPath = Path.Combine(baseFolder, "bin", "test.msi");
+                var wixpdbPath = Path.ChangeExtension(msiPath, ".wixpdb");
+
+                var result = WixRunner.Execute(new[]
+                {
+                    "build",
+                    Path.Combine(folder, "Directory", "RedundantSubdirectoryInSecondSection.wxs"),
+                    "-bindpath", Path.Combine(folder, "SingleFile", "data"),
+                    "-intermediateFolder", intermediateFolder,
+                    "-o", msiPath
+                });
+
+                result.AssertSuccess();
+
+                var intermediate = Intermediate.Load(wixpdbPath);
+                var section = intermediate.Sections.Single();
+
+                var dirSymbols = section.Symbols.OfType<WixToolset.Data.Symbols.DirectorySymbol>().ToList();
+                WixAssert.CompareLineByLine(new[]
+                {
+                    @"dKO7wPCF.XLmq6KnqybHHgcBBqtU:ProgramFilesFolder:a\b\c",
+                    @"ProgramFilesFolder:TARGETDIR:PFiles",
+                    @"TARGETDIR::SourceDir"
+                }, dirSymbols.OrderBy(d => d.Id.Id).Select(d => d.Id.Id + ":" + d.ParentDirectoryRef + ":" + d.Name).OrderBy(s => s).ToArray());
+
+                var data = WindowsInstallerData.Load(wixpdbPath);
+                var directoryRows = data.Tables["Directory"].Rows;
+                WixAssert.CompareLineByLine(new[]
+                {
+                    @"d1nVb5_zcCwRCz7i2YXNAofGRmfc:ProgramFilesFolder:a",
+                    @"dijlG.bNicFgvj1_DujiGg9EBGrQ:d1nVb5_zcCwRCz7i2YXNAofGRmfc:b",
+                    @"dKO7wPCF.XLmq6KnqybHHgcBBqtU:dijlG.bNicFgvj1_DujiGg9EBGrQ:c",
+                    "ProgramFilesFolder:TARGETDIR:PFiles",
+                    "TARGETDIR::SourceDir"
+                }, directoryRows.Select(r => r.FieldAsString(0) + ":" + r.FieldAsString(1) + ":" + r.FieldAsString(2)).OrderBy(s => s).ToArray());
+            }
+        }
     }
 }

src/wix/test/WixToolsetTest.CoreIntegration/TestData/Directory/RedundantSubdirectoryInSecondSection.wxs

@@ -0,0 +1,26 @@
+<Wix xmlns="http://wixtoolset.org/schemas/v4/wxs">
+    <Package Name="~RedundantSubdirectories" Version="1.0.0.0" Manufacturer="Example Corporation" UpgradeCode="12E4699F-E774-4D05-8A01-5BDD41BBA127" Compressed="no">
+        <MajorUpgrade DowngradeErrorMessage="A newer version of [ProductName] is already installed." />
+
+        <Feature Id="ProductFeature">
+            <!-- NotIncludeButFirst will define the subdirectory and IncludedButSecond needs the same symbols, this tests linking order -->
+            <ComponentGroupRef Id="IncludedButSecond" />
+        </Feature>
+    </Package>
+
+    <Fragment Id="NotIncludedButFirst">
+        <ComponentGroup Id="NotIncludedButFirst">
+            <Component Directory="ProgramFilesFolder" Subdirectory="a\b\c">
+                <File Name="notincluded.txt" Source="test.txt" />
+            </Component>
+        </ComponentGroup>
+    </Fragment>
+
+    <Fragment Id="IncludedButSecond">
+        <ComponentGroup Id="IncludedButSecond">
+            <Component Directory="ProgramFilesFolder" Subdirectory="a\b\c">
+                <File Name="included.txt" Source="test.txt" />
+            </Component>
+        </ComponentGroup>
+    </Fragment>
+</Wix>