Handle conflicts between ref and copy local

We need to look at target path of private references as well as
compare that to copy-local items.

netstandard/tools/HandlePackageFileConflicts.cs

@@ -5,11 +5,9 @@
 using Microsoft.Build.Framework;
 using Microsoft.Build.Utilities;
 using System;
-using System.Collections;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.IO;
-using System.Reflection;
 
 namespace Microsoft.DotNet.Build.Tasks
 {
@@ -36,9 +34,22 @@ public partial class HandlePackageFileConflicts : Task
 
         public override bool Execute()
         {
-            ReferencesWithoutConflicts = HandleConflicts(References, GetReferenceKey);
+            // remove References that will conflict at compile
+            var referencesWithoutConflicts = HandleConflicts(References, GetReferenceKey);
 
-            ReferenceCopyLocalPathsWithoutConflicts = HandleConflicts(ReferenceCopyLocalPaths, GetTargetPath);
+            // remove References that will conflict in output
+            Dictionary<string, ITaskItem> referencesByTargetPath;
+            referencesWithoutConflicts = HandleConflicts(referencesWithoutConflicts, GetReferenceTargetPath, out referencesByTargetPath);
+
+            // remove ReferenceCopyLocalPaths that will conflict in output.
+            Dictionary<string, ITaskItem> referenceCopyLocalPathsByTargetPath;
+            var referenceCopyLocalPathsWithoutConflicts = HandleConflicts(ReferenceCopyLocalPaths, GetTargetPath, out referenceCopyLocalPathsByTargetPath);
+
+            // remove ReferenceCopyLocalPaths & set References to Private=false that will conflict in output.
+            referenceCopyLocalPathsWithoutConflicts = HandleConflicts(referenceCopyLocalPathsWithoutConflicts, referenceCopyLocalPathsByTargetPath, referencesByTargetPath);
+
+            ReferencesWithoutConflicts = referencesWithoutConflicts;
+            ReferenceCopyLocalPathsWithoutConflicts = referenceCopyLocalPathsWithoutConflicts;
 
             return !Log.HasLoggedErrors;
         }
@@ -66,28 +77,43 @@ private void EnsurePackageRanks()
         }
 
         /// <summary>
-        /// Examines items for conflicting targetpath and chooses the best item.
+        /// Examines items for conflicting keys and chooses the best item.
         /// </summary>
-        /// <param name="items"></param>
-        /// <returns></returns>
+        /// <param name="items">Items from which to remove conflicts</param>
+        /// <param name="getItemKey">Delegate to calculate key for an item</param>
+        /// <returns>Items without conflicts</returns>
         private ITaskItem[] HandleConflicts(ITaskItem[] items, Func<ITaskItem, string> getItemKey)
         {
-            var winningItems = new Dictionary<string, ITaskItem>(StringComparer.OrdinalIgnoreCase);
+            Dictionary<string, ITaskItem> itemsByKeyUnused;
+
+            return HandleConflicts(items, getItemKey, out itemsByKeyUnused);
+        }
+
+        /// <summary>
+        /// Examines items for conflicting keys and chooses the best item.
+        /// </summary>
+        /// <param name="items">Items from which to remove conflicts</param>
+        /// <param name="getItemKey">Delegate to calculate key for an item</param>
+        /// <param name="winningItemsByKey">Dictionary of items by key without conflicts</param>
+        /// <returns>Items without conflicts</returns>
+        private ITaskItem[] HandleConflicts(ITaskItem[] items, Func<ITaskItem, string> getItemKey, out Dictionary<string, ITaskItem> winningItemsByKey)
+        {
+            winningItemsByKey = new Dictionary<string, ITaskItem>(StringComparer.OrdinalIgnoreCase);
             // ensure we use reference equality and not any overridden equality operators on items
             var conflictsToRemove = new HashSet<ITaskItem>(ReferenceComparer<ITaskItem>.Instance);
 
             foreach (var item in items)
             {
                 var itemKey = getItemKey(item);
 
-                if (itemKey == null)
+                if (String.IsNullOrEmpty(itemKey))
                 {
                     continue;
                 }
 
                 ITaskItem existingItem;
 
-                if (winningItems.TryGetValue(itemKey, out existingItem))
+                if (winningItemsByKey.TryGetValue(itemKey, out existingItem))
                 {
                     // a conflict was found, determine the winner.
                     var winner = HandleConflict(existingItem, item);
@@ -99,10 +125,10 @@ private ITaskItem[] HandleConflicts(ITaskItem[] items, Func<ITaskItem, string> g
                         continue;
                     }
 
-                    if (winner != existingItem)
+                    if (!ReferenceEquals(winner, existingItem))
                     {
                         // replace existing item and add it to conflict list.
-                        winningItems[itemKey] = item;
+                        winningItemsByKey[itemKey] = item;
                         conflictsToRemove.Add(existingItem);
                     }
                     else
@@ -113,13 +139,67 @@ private ITaskItem[] HandleConflicts(ITaskItem[] items, Func<ITaskItem, string> g
                 }
                 else
                 {
-                    winningItems[itemKey] = item;
+                    winningItemsByKey[itemKey] = item;
                 }
             }
 
             return RemoveConflicts(items, conflictsToRemove);
         }
 
+        /// <summary>
+        /// Examines private (copy-local) references amd referenceCopyLocalPaths for inter-conflicts and
+        /// demotes references to non-private or removes items from referenceCopyLocalPaths.
+        /// </summary>
+        /// <param name="referenceCopyLocalPaths">ReferenceCopyLocalPaths item, already filtered for intra-conflicts</param>
+        /// <param name="referenceCopyLocalPathsByTargetPath">ReferenceCopyLocalPaths already filtered for intra-conflicts, indexed by TargetPath</param>
+        /// <param name="referencesByTargetPath">References item for private references, already filtered for compile-time intra-conflicts and copy-local intra-conflicts, indexed by TargetPath.  Items within this collection will be modified as neecessary to handle conflicts with ReferenceCopyLocalPaths by making the reference Private=false.</param>
+        /// <returns>ReferenceCopyLocalPaths item filtered for inter-conflicts with References</returns>
+        private ITaskItem[] HandleConflicts(ITaskItem[] referenceCopyLocalPaths, Dictionary<string, ITaskItem> referenceCopyLocalPathsByTargetPath, Dictionary<string, ITaskItem> referencesByTargetPath)
+        {
+            if (referencesByTargetPath.Count == 0 || referenceCopyLocalPathsByTargetPath.Count == 0)
+            {
+                // nothing to do if either item set doesn't have targetpaths
+                return referenceCopyLocalPaths;
+            }
+
+            // ensure we use reference equality and not any overridden equality operators on items
+            var conflictsToRemove = new HashSet<ITaskItem>(ReferenceComparer<ITaskItem>.Instance);
+
+            // find conflicts between the two, arbitrarily choose ref to iterate over
+            foreach (var referenceByTargetPath in referencesByTargetPath)
+            {
+                var targetPath = referenceByTargetPath.Key;
+                var reference = referenceByTargetPath.Value;
+
+                ITaskItem referenceCopyLocalPath = null;
+
+                if (referenceCopyLocalPathsByTargetPath.TryGetValue(targetPath, out referenceCopyLocalPath))
+                {
+                    // a conflict was found, determine the winner.
+                    var winner = HandleConflict(reference, referenceCopyLocalPath);
+
+                    if (winner == null)
+                    {
+                        // no winner, skip it.
+                        continue;
+                    }
+
+                    if (ReferenceEquals(winner, referenceCopyLocalPath))
+                    {
+                        // ReferenceCopyLocalPath won, make the reference not private, so that it will not copy-local
+                        reference.SetMetadata("Private", "False");
+                    }
+                    else
+                    {
+                        // Reference won, remove the copy-local item.
+                        conflictsToRemove.Add(referenceCopyLocalPath);
+                    }
+                }
+            }
+
+            return RemoveConflicts(referenceCopyLocalPaths, conflictsToRemove);
+        }
+
         private Version GetFileVersion(string sourcePath)
         {
             var fvi = FileVersionInfo.GetVersionInfo(sourcePath);
@@ -173,7 +253,13 @@ private static string GetReferenceKey(ITaskItem item)
 
             if (!String.IsNullOrEmpty(aliases))
             {
-                // skip conflict detection for aliased assemblies
+                // skip compile-time conflict detection for aliased assemblies.
+                // An alias is the way to avoid a conflict
+                //   eg: System, v1.0.0.0 in global will not conflict with System, v2.0.0.0 in `private` alias
+                // We could model each alias scope and try to check for conflicts within that scope,
+                // but this is a ton of complexity for a fringe feature.
+                // Instead, we'll treat an alias as an indication that the developer has opted out of 
+                // conflict resolution.
                 return null;
             }
 
@@ -188,31 +274,35 @@ private static string GetReferenceKey(ITaskItem item)
                 return null;
             }
 
-            // The following is similar to Path.GetFileName but does not throw and returns null
-            // rather than empty for a directory.
-            var length = sourcePath.Length;
-            for (int i = length - 1; --i >= 0;)
+            try
+            {
+                return Path.GetFileName(sourcePath);
+            }
+            catch(ArgumentException)
             {
-                var ch = sourcePath[i];
+                // We won't even try to resolve a conflict if we can't open the file, so ignore invalid paths
+                return null;
+            }
+        }
 
-                if (ch == Path.DirectorySeparatorChar || ch == Path.AltDirectorySeparatorChar || ch == Path.VolumeSeparatorChar)
-                {
-                    // string terminated with a separator: we were given a directory, ignore it.
-                    if (i == length - 1)
-                    {
-                        return null;
-                    }
+        private static string GetReferenceTargetPath(ITaskItem item)
+        {
+            // Determine if the reference will be copied local.  
+            // We're only dealing with primary file references.  For these RAR will 
+            // copy local if Private is true or unset.
 
-                    // we found a separator, return filename portion.
-                    var startOfFileName = i + 1;
-                    return sourcePath.Substring(startOfFileName);
-                }
+            var isPrivate = MSBuildUtilities.ConvertStringToBool(item.GetMetadata("Private"), defaultValue: true);
+
+            if (!isPrivate)
+            {
+                // Private = false means the reference shouldn't be copied.
+                return null;
             }
 
-            // could not find a separator, return source path
-            return sourcePath;
+            return GetTargetPath(item);
         }
 
+
         private static string GetSourcePath(ITaskItem item)
         {
             var sourcePath = item.GetMetadata("HintPath");

netstandard/tools/MSBuildUtilities.cs

@@ -0,0 +1,69 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+using System;
+
+namespace Microsoft.DotNet.Build.Tasks
+{
+    /// <summary>
+    /// Internal utilties copied from microsoft/MSBuild repo.
+    /// </summary>
+    class MSBuildUtilities
+    {
+        /// <summary>
+        /// Converts a string to a bool.  We consider "true/false", "on/off", and 
+        /// "yes/no" to be valid boolean representations in the XML.
+        /// Modified from its original version to not throw, but return a default value.
+        /// </summary>
+        /// <param name="parameterValue">The string to convert.</param>
+        /// <returns>Boolean true or false, corresponding to the string.</returns>
+        internal static bool ConvertStringToBool(string parameterValue, bool defaultValue = false)
+        {
+            if (String.IsNullOrEmpty(parameterValue))
+            {
+                return defaultValue;
+            }
+            else if (ValidBooleanTrue(parameterValue))
+            {
+                return true;
+            }
+            else if (ValidBooleanFalse(parameterValue))
+            {
+                return false;
+            }
+            else
+            {
+                // Unsupported boolean representation.
+                return defaultValue;
+            }
+        }
+
+        /// <summary>
+        /// Returns true if the string represents a valid MSBuild boolean true value,
+        /// such as "on", "!false", "yes"
+        /// </summary>
+        private static bool ValidBooleanTrue(string parameterValue)
+        {
+            return ((String.Compare(parameterValue, "true", StringComparison.OrdinalIgnoreCase) == 0) ||
+                    (String.Compare(parameterValue, "on", StringComparison.OrdinalIgnoreCase) == 0) ||
+                    (String.Compare(parameterValue, "yes", StringComparison.OrdinalIgnoreCase) == 0) ||
+                    (String.Compare(parameterValue, "!false", StringComparison.OrdinalIgnoreCase) == 0) ||
+                    (String.Compare(parameterValue, "!off", StringComparison.OrdinalIgnoreCase) == 0) ||
+                    (String.Compare(parameterValue, "!no", StringComparison.OrdinalIgnoreCase) == 0));
+        }
+
+        /// <summary>
+        /// Returns true if the string represents a valid MSBuild boolean false value,
+        /// such as "!on" "off" "no" "!true"
+        /// </summary>
+        private static bool ValidBooleanFalse(string parameterValue)
+        {
+            return ((String.Compare(parameterValue, "false", StringComparison.OrdinalIgnoreCase) == 0) ||
+                    (String.Compare(parameterValue, "off", StringComparison.OrdinalIgnoreCase) == 0) ||
+                    (String.Compare(parameterValue, "no", StringComparison.OrdinalIgnoreCase) == 0) ||
+                    (String.Compare(parameterValue, "!true", StringComparison.OrdinalIgnoreCase) == 0) ||
+                    (String.Compare(parameterValue, "!on", StringComparison.OrdinalIgnoreCase) == 0) ||
+                    (String.Compare(parameterValue, "!yes", StringComparison.OrdinalIgnoreCase) == 0));
+        }
+    }
+}

netstandard/tools/NETStandard.Tools.csproj

@@ -12,6 +12,7 @@
   <PropertyGroup Condition="'$(Configuration)' == 'net45_Debug'" />
   <ItemGroup>
     <Compile Include="HandlePackageFileConflicts.cs" />
+    <Compile Include="MSBuildUtilities.cs" />
     <Compile Include="ReferenceComparer.cs" />
   </ItemGroup>
   <ItemGroup Condition="'$(TargetGroup)' != 'net45'">