From c8cf0d7984164f8a8a327397d55beaaa472ce5e1 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Thu, 30 Dec 2021 12:03:33 +1000 Subject: [PATCH 1/8] CA1823 Avoid unused private fields --- eng/CodeAnalysis.ruleset | 2 +- .../RetrievableEntryHashSet/HashSet.cs | 399 ------------------ src/MSBuild.UnitTests/PerfLog_Tests.cs | 3 + src/Shared/UnitTests/EngineTestEnvironment.cs | 2 + 4 files changed, 6 insertions(+), 400 deletions(-) diff --git a/eng/CodeAnalysis.ruleset b/eng/CodeAnalysis.ruleset index 2078c42fe6c..29e3d4ac733 100644 --- a/eng/CodeAnalysis.ruleset +++ b/eng/CodeAnalysis.ruleset @@ -94,7 +94,7 @@ - + diff --git a/src/Build/Collections/RetrievableEntryHashSet/HashSet.cs b/src/Build/Collections/RetrievableEntryHashSet/HashSet.cs index 001c7b87f69..8498844acbd 100644 --- a/src/Build/Collections/RetrievableEntryHashSet/HashSet.cs +++ b/src/Build/Collections/RetrievableEntryHashSet/HashSet.cs @@ -91,8 +91,6 @@ internal class RetrievableEntryHashSet : ICollection, { // store lower 31 bits of hash code private const int Lower31BitMask = 0x7FFFFFFF; - // cutoff point, above which we won't do stackallocs. This corresponds to 100 integers. - private const int StackAllocThreshold = 100; // when constructing a hashset from an existing collection, it may contain duplicates, // so this is used as the max acceptable excess ratio of capacity to count. Note that // this is only used on the ctor and not to automatically shrink if the hashset has, e.g, @@ -1290,338 +1288,6 @@ internal bool EntriesAreReferenceEquals(RetrievableEntryHashSet other) return true; } -#if NEVER - /// - /// Checks if this contains of other's elements. Iterates over other's elements and - /// returns false as soon as it finds an element in other that's not in this. - /// Used by SupersetOf, ProperSupersetOf, and SetEquals. - /// - /// - /// - private bool ContainsAllElements(IEnumerable other) { - foreach (T element in other) { - if (!Contains(element)) { - return false; - } - } - return true; - } - - /// - /// Implementation Notes: - /// If other is a hashset and is using same equality comparer, then checking subset is - /// faster. Simply check that each element in this is in other. - /// - /// Note: if other doesn't use same equality comparer, then Contains check is invalid, - /// which is why callers must take are of this. - /// - /// If callers are concerned about whether this is a proper subset, they take care of that. - /// - /// - /// - /// - private bool IsSubsetOfHashSetWithSameEC(RetrievableEntryHashSet other) { - - foreach (T item in this) { - if (!other.Contains(item)) { - return false; - } - } - return true; - } - - /// - /// If other is a hashset that uses same equality comparer, intersect is much faster - /// because we can use other's Contains - /// - /// - private void IntersectWithHashSetWithSameEC(RetrievableEntryHashSet other) { - for (int i = 0; i < m_lastIndex; i++) { - if (m_slots[i].hashCode >= 0) { - T item = m_slots[i].value; - if (!other.Contains(item)) { - Remove(item); - } - } - } - } - - /// - /// Iterate over other. If contained in this, mark an element in bit array corresponding to - /// its position in m_slots. If anything is unmarked (in bit array), remove it. - /// - /// This attempts to allocate on the stack, if below StackAllocThreshold. - /// - /// - [System.Security.SecuritySafeCritical] - private unsafe void IntersectWithEnumerable(IEnumerable other) { - Debug.Assert(m_buckets != null, "m_buckets shouldn't be null; callers should check first"); - - // keep track of current last index; don't want to move past the end of our bit array - // (could happen if another thread is modifying the collection) - int originalLastIndex = m_lastIndex; - int intArrayLength = BitHelper.ToIntArrayLength(originalLastIndex); - - BitHelper bitHelper; - if (intArrayLength <= StackAllocThreshold) { - int* bitArrayPtr = stackalloc int[intArrayLength]; - bitHelper = new BitHelper(bitArrayPtr, intArrayLength); - } - else { - int[] bitArray = new int[intArrayLength]; - bitHelper = new BitHelper(bitArray, intArrayLength); - } - - // mark if contains: find index of in slots array and mark corresponding element in bit array - foreach (T item in other) { - int index = InternalIndexOf(item); - if (index >= 0) { - bitHelper.MarkBit(index); - } - } - - // if anything unmarked, remove it. Perf can be optimized here if BitHelper had a - // FindFirstUnmarked method. - for (int i = 0; i < originalLastIndex; i++) { - if (m_slots[i].hashCode >= 0 && !bitHelper.IsMarked(i)) { - Remove(m_slots[i].value); - } - } - } - - /// - /// Used internally by set operations which have to rely on bit array marking. This is like - /// Contains but returns index in slots array. - /// - /// - /// - private int InternalIndexOf(T item) { - Debug.Assert(m_buckets != null, "m_buckets was null; callers should check first"); - - int hashCode = InternalGetHashCode(item); - for (int i = m_buckets[hashCode % m_buckets.Length] - 1; i >= 0; i = m_slots[i].next) { - if ((m_slots[i].hashCode) == hashCode && m_comparer.Equals(m_slots[i].value, item)) { - return i; - } - } - // wasn't found - return -1; - } - - /// - /// if other is a set, we can assume it doesn't have duplicate elements, so use this - /// technique: if can't remove, then it wasn't present in this set, so add. - /// - /// As with other methods, callers take care of ensuring that other is a hashset using the - /// same equality comparer. - /// - /// - private void SymmetricExceptWithUniqueHashSet(RetrievableEntryHashSet other) { - foreach (T item in other) { - if (!Remove(item)) { - AddEvenIfPresent(item); - } - } - } - - /// - /// Implementation notes: - /// - /// Used for symmetric except when other isn't a HashSet. This is more tedious because - /// other may contain duplicates. HashSet technique could fail in these situations: - /// 1. Other has a duplicate that's not in this: HashSet technique would add then - /// remove it. - /// 2. Other has a duplicate that's in this: HashSet technique would remove then add it - /// back. - /// In general, its presence would be toggled each time it appears in other. - /// - /// This technique uses bit marking to indicate whether to add/remove the item. If already - /// present in collection, it will get marked for deletion. If added from other, it will - /// get marked as something not to remove. - /// - /// - /// - [System.Security.SecuritySafeCritical] - private unsafe void SymmetricExceptWithEnumerable(IEnumerable other) { - int originalLastIndex = m_lastIndex; - int intArrayLength = BitHelper.ToIntArrayLength(originalLastIndex); - - BitHelper itemsToRemove; - BitHelper itemsAddedFromOther; - if (intArrayLength <= StackAllocThreshold / 2) { - int* itemsToRemovePtr = stackalloc int[intArrayLength]; - itemsToRemove = new BitHelper(itemsToRemovePtr, intArrayLength); - - int* itemsAddedFromOtherPtr = stackalloc int[intArrayLength]; - itemsAddedFromOther = new BitHelper(itemsAddedFromOtherPtr, intArrayLength); - } - else { - int[] itemsToRemoveArray = new int[intArrayLength]; - itemsToRemove = new BitHelper(itemsToRemoveArray, intArrayLength); - - int[] itemsAddedFromOtherArray = new int[intArrayLength]; - itemsAddedFromOther = new BitHelper(itemsAddedFromOtherArray, intArrayLength); - } - - foreach (T item in other) { - int location = 0; - bool added = AddOrGetLocation(item, out location); - if (added) { - // wasn't already present in collection; flag it as something not to remove - // *NOTE* if location is out of range, we should ignore. BitHelper will - // detect that it's out of bounds and not try to mark it. But it's - // expected that location could be out of bounds because adding the item - // will increase m_lastIndex as soon as all the free spots are filled. - itemsAddedFromOther.MarkBit(location); - } - else { - // already there...if not added from other, mark for remove. - // *NOTE* Even though BitHelper will check that location is in range, we want - // to check here. There's no point in checking items beyond originalLastIndex - // because they could not have been in the original collection - if (location < originalLastIndex && !itemsAddedFromOther.IsMarked(location)) { - itemsToRemove.MarkBit(location); - } - } - } - - // if anything marked, remove it - for (int i = 0; i < originalLastIndex; i++) { - if (itemsToRemove.IsMarked(i)) { - Remove(m_slots[i].value); - } - } - } - - /// - /// Add if not already in hashset. Returns an out param indicating index where added. This - /// is used by SymmetricExcept because it needs to know the following things: - /// - whether the item was already present in the collection or added from other - /// - where it's located (if already present, it will get marked for removal, otherwise - /// marked for keeping) - /// - /// - /// - /// - private bool AddOrGetLocation(T value, out int location) { - Debug.Assert(m_buckets != null, "m_buckets is null, callers should have checked"); - - int hashCode = InternalGetHashCode(value); - int bucket = hashCode % m_buckets.Length; - for (int i = m_buckets[hashCode % m_buckets.Length] - 1; i >= 0; i = m_slots[i].next) { - if (m_slots[i].hashCode == hashCode && m_comparer.Equals(m_slots[i].value, value)) { - location = i; - return false; //already present - } - } - int index; - if (m_freeList >= 0) { - index = m_freeList; - m_freeList = m_slots[index].next; - } - else { - if (m_lastIndex == m_slots.Length) { - IncreaseCapacity(); - // this will change during resize - bucket = hashCode % m_buckets.Length; - } - index = m_lastIndex; - m_lastIndex++; - } - m_slots[index].hashCode = hashCode; - m_slots[index].value = value; - m_slots[index].next = m_buckets[bucket] - 1; - m_buckets[bucket] = index + 1; - m_count++; - m_version++; - location = index; - return true; - } - - /// - /// Determines counts that can be used to determine equality, subset, and superset. This - /// is only used when other is an IEnumerable and not a HashSet. If other is a HashSet - /// these properties can be checked faster without use of marking because we can assume - /// other has no duplicates. - /// - /// The following count checks are performed by callers: - /// 1. Equals: checks if unfoundCount = 0 and uniqueFoundCount = m_count; i.e. everything - /// in other is in this and everything in this is in other - /// 2. Subset: checks if unfoundCount >= 0 and uniqueFoundCount = m_count; i.e. other may - /// have elements not in this and everything in this is in other - /// 3. Proper subset: checks if unfoundCount > 0 and uniqueFoundCount = m_count; i.e - /// other must have at least one element not in this and everything in this is in other - /// 4. Proper superset: checks if unfound count = 0 and uniqueFoundCount strictly less - /// than m_count; i.e. everything in other was in this and this had at least one element - /// not contained in other. - /// - /// An earlier implementation used delegates to perform these checks rather than returning - /// an ElementCount struct; however this was changed due to the perf overhead of delegates. - /// - /// - /// Allows us to finish faster for equals and proper superset - /// because unfoundCount must be 0. - /// - [System.Security.SecuritySafeCritical] - private unsafe ElementCount CheckUniqueAndUnfoundElements(IEnumerable other, bool returnIfUnfound) { - ElementCount result; - - // need special case in case this has no elements. - if (m_count == 0) { - int numElementsInOther = 0; - foreach (T item in other) { - numElementsInOther++; - // break right away, all we want to know is whether other has 0 or 1 elements - break; - } - result.uniqueCount = 0; - result.unfoundCount = numElementsInOther; - return result; - } - - - Debug.Assert((m_buckets != null) && (m_count > 0), "m_buckets was null but count greater than 0"); - - int originalLastIndex = m_lastIndex; - int intArrayLength = BitHelper.ToIntArrayLength(originalLastIndex); - - BitHelper bitHelper; - if (intArrayLength <= StackAllocThreshold) { - int* bitArrayPtr = stackalloc int[intArrayLength]; - bitHelper = new BitHelper(bitArrayPtr, intArrayLength); - } - else { - int[] bitArray = new int[intArrayLength]; - bitHelper = new BitHelper(bitArray, intArrayLength); - } - - // count of items in other not found in this - int unfoundCount = 0; - // count of unique items in other found in this - int uniqueFoundCount = 0; - - foreach (T item in other) { - int index = InternalIndexOf(item); - if (index >= 0) { - if (!bitHelper.IsMarked(index)) { - // item hasn't been seen yet - bitHelper.MarkBit(index); - uniqueFoundCount++; - } - } - else { - unfoundCount++; - if (returnIfUnfound) { - break; - } - } - } - - result.uniqueCount = uniqueFoundCount; - result.unfoundCount = unfoundCount; - return result; - } -#endif /// /// Copies this to an array. Used for DebugView /// @@ -1632,71 +1298,6 @@ internal T[] ToArray() CopyTo(newArray); return newArray; } - -#if NEVER - /// - /// Internal method used for HashSetEqualityComparer. Compares set1 and set2 according - /// to specified comparer. - /// - /// Because items are hashed according to a specific equality comparer, we have to resort - /// to n^2 search if they're using different equality comparers. - /// - /// - /// - /// - /// - internal static bool HashSetEquals(RetrievableEntryHashSet set1, RetrievableEntryHashSet set2, IEqualityComparer comparer) { - // handle null cases first - if (set1 == null) { - return (set2 == null); - } - else if (set2 == null) { - // set1 != null - return false; - } - - // all comparers are the same; this is faster - if (AreEqualityComparersEqual(set1, set2)) { - if (set1.Count != set2.Count) { - return false; - } - // suffices to check subset - foreach (T item in set2) { - if (!set1.Contains(item)) { - return false; - } - } - return true; - } - else { // n^2 search because items are hashed according to their respective ECs - foreach (T set2Item in set2) { - bool found = false; - foreach (T set1Item in set1) { - if (comparer.Equals(set2Item, set1Item)) { - found = true; - break; - } - } - if (!found) { - return false; - } - } - return true; - } - } - - /// - /// Checks if equality comparers are equal. This is used for algorithms that can - /// speed up if it knows the other item has unique elements. I.e. if they're using - /// different equality comparers, then uniqueness assumption between sets break. - /// - /// - /// - /// - private static bool AreEqualityComparersEqual(RetrievableEntryHashSet set1, RetrievableEntryHashSet set2) { - return set1.Comparer.Equals(set2.Comparer); - } -#endif private int InternalGetHashCode(string item, int index, int length) { diff --git a/src/MSBuild.UnitTests/PerfLog_Tests.cs b/src/MSBuild.UnitTests/PerfLog_Tests.cs index be18d7b6a47..7b1dd116c38 100644 --- a/src/MSBuild.UnitTests/PerfLog_Tests.cs +++ b/src/MSBuild.UnitTests/PerfLog_Tests.cs @@ -24,11 +24,14 @@ namespace Microsoft.Build.UnitTests { public class PerfLogTests { +#pragma warning disable CA1823 // Avoid unused private fields #if USE_MSBUILD_DLL_EXTN + private const string MSBuildExeName = "MSBuild.dll"; #else private const string MSBuildExeName = "MSBuild.exe"; #endif +#pragma warning restore CA1823 // Avoid unused private fields private readonly ITestOutputHelper _output; diff --git a/src/Shared/UnitTests/EngineTestEnvironment.cs b/src/Shared/UnitTests/EngineTestEnvironment.cs index 4641dc5e2b3..525572496b8 100644 --- a/src/Shared/UnitTests/EngineTestEnvironment.cs +++ b/src/Shared/UnitTests/EngineTestEnvironment.cs @@ -20,7 +20,9 @@ namespace Microsoft.Build.UnitTests public partial class TestEnvironment { // reset the default build manager and the state it might have accumulated from other tests +#pragma warning disable CA1823 // Avoid unused private fields private object _resetBuildManager = new ResetDefaultBuildManager(); +#pragma warning restore CA1823 // Avoid unused private fields private class ResetDefaultBuildManager { From 6d78c96c374949784cf5ee24a2aeabde2076daa7 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Fri, 31 Dec 2021 07:32:08 +1000 Subject: [PATCH 2/8] CA1823 Remove MSBuildExeName from PerfLogTests due to analyzer --- src/MSBuild.UnitTests/PerfLog_Tests.cs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/MSBuild.UnitTests/PerfLog_Tests.cs b/src/MSBuild.UnitTests/PerfLog_Tests.cs index 7b1dd116c38..ba5688af4fd 100644 --- a/src/MSBuild.UnitTests/PerfLog_Tests.cs +++ b/src/MSBuild.UnitTests/PerfLog_Tests.cs @@ -24,15 +24,6 @@ namespace Microsoft.Build.UnitTests { public class PerfLogTests { -#pragma warning disable CA1823 // Avoid unused private fields -#if USE_MSBUILD_DLL_EXTN - - private const string MSBuildExeName = "MSBuild.dll"; -#else - private const string MSBuildExeName = "MSBuild.exe"; -#endif -#pragma warning restore CA1823 // Avoid unused private fields - private readonly ITestOutputHelper _output; public PerfLogTests(ITestOutputHelper output) From 024a6d755eecdac93437f2ad27332477a2031036 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Fri, 31 Dec 2021 08:51:15 +1000 Subject: [PATCH 3/8] StackAllocThreshold is only used in the other #if NEVER blocks. So add it to its own block. This way we avoid the Analyzer error and keeps the code logically grouped. --- .../RetrievableEntryHashSet/HashSet.cs | 431 +++++++++++++++++- 1 file changed, 416 insertions(+), 15 deletions(-) diff --git a/src/Build/Collections/RetrievableEntryHashSet/HashSet.cs b/src/Build/Collections/RetrievableEntryHashSet/HashSet.cs index 8498844acbd..1d3756d4adc 100644 --- a/src/Build/Collections/RetrievableEntryHashSet/HashSet.cs +++ b/src/Build/Collections/RetrievableEntryHashSet/HashSet.cs @@ -91,6 +91,10 @@ internal class RetrievableEntryHashSet : ICollection, { // store lower 31 bits of hash code private const int Lower31BitMask = 0x7FFFFFFF; +#if NEVER + // cutoff point, above which we won't do stackallocs. This corresponds to 100 integers. + private const int StackAllocThreshold = 100; +#endif // when constructing a hashset from an existing collection, it may contain duplicates, // so this is used as the max acceptable excess ratio of capacity to count. Note that // this is only used on the ctor and not to automatically shrink if the hashset has, e.g, @@ -117,7 +121,7 @@ internal class RetrievableEntryHashSet : ICollection, // temporary variable needed during deserialization private SerializationInfo _siInfo; - #region Constructors +#region Constructors public RetrievableEntryHashSet(IEqualityComparer comparer) { @@ -200,7 +204,7 @@ protected RetrievableEntryHashSet(SerializationInfo info, StreamingContext conte _siInfo = info; } - #endregion +#endregion // Convenience to minimise change to callers used to dictionaries public ICollection Keys @@ -226,7 +230,7 @@ public ICollection Values get { return this; } } - #region ICollection methods +#region ICollection methods // Convenience to minimise change to callers used to dictionaries internal T this[string name] @@ -477,9 +481,9 @@ internal void MakeReadOnly() _readOnly = true; } - #endregion +#endregion - #region IEnumerable methods +#region IEnumerable methods public Enumerator GetEnumerator() { @@ -504,9 +508,9 @@ IEnumerator IEnumerable.GetEnumerator() return new Enumerator(this); } - #endregion +#endregion - #region ISerializable methods +#region ISerializable methods // [SecurityPermissionAttribute(SecurityAction.LinkDemand, Flags = SecurityPermissionFlag.SerializationFormatter)] [SecurityCritical] @@ -529,9 +533,9 @@ public virtual void GetObjectData(SerializationInfo info, StreamingContext conte } } - #endregion +#endregion - #region IDeserializationCallback methods +#region IDeserializationCallback methods public virtual void OnDeserialization(Object sender) { @@ -576,9 +580,9 @@ public virtual void OnDeserialization(Object sender) _siInfo = null; } - #endregion +#endregion - #region HashSet methods +#region HashSet methods /// /// Add item to this HashSet. @@ -626,7 +630,7 @@ public void UnionWith(IEnumerable other) } } -#if NEVER +#if NEVER /// /// Takes the intersection of this set with other. Modifies this set. /// @@ -1148,9 +1152,9 @@ public static IEqualityComparer> CreateSetComparer() #endif #endif - #endregion +#endregion - #region Helper methods +#region Helper methods /// /// Initializes buckets and slots arrays. Uses suggested capacity by finding next prime @@ -1288,6 +1292,338 @@ internal bool EntriesAreReferenceEquals(RetrievableEntryHashSet other) return true; } +#if NEVER + /// + /// Checks if this contains of other's elements. Iterates over other's elements and + /// returns false as soon as it finds an element in other that's not in this. + /// Used by SupersetOf, ProperSupersetOf, and SetEquals. + /// + /// + /// + private bool ContainsAllElements(IEnumerable other) { + foreach (T element in other) { + if (!Contains(element)) { + return false; + } + } + return true; + } + + /// + /// Implementation Notes: + /// If other is a hashset and is using same equality comparer, then checking subset is + /// faster. Simply check that each element in this is in other. + /// + /// Note: if other doesn't use same equality comparer, then Contains check is invalid, + /// which is why callers must take are of this. + /// + /// If callers are concerned about whether this is a proper subset, they take care of that. + /// + /// + /// + /// + private bool IsSubsetOfHashSetWithSameEC(RetrievableEntryHashSet other) { + + foreach (T item in this) { + if (!other.Contains(item)) { + return false; + } + } + return true; + } + + /// + /// If other is a hashset that uses same equality comparer, intersect is much faster + /// because we can use other's Contains + /// + /// + private void IntersectWithHashSetWithSameEC(RetrievableEntryHashSet other) { + for (int i = 0; i < m_lastIndex; i++) { + if (m_slots[i].hashCode >= 0) { + T item = m_slots[i].value; + if (!other.Contains(item)) { + Remove(item); + } + } + } + } + + /// + /// Iterate over other. If contained in this, mark an element in bit array corresponding to + /// its position in m_slots. If anything is unmarked (in bit array), remove it. + /// + /// This attempts to allocate on the stack, if below StackAllocThreshold. + /// + /// + [System.Security.SecuritySafeCritical] + private unsafe void IntersectWithEnumerable(IEnumerable other) { + Debug.Assert(m_buckets != null, "m_buckets shouldn't be null; callers should check first"); + + // keep track of current last index; don't want to move past the end of our bit array + // (could happen if another thread is modifying the collection) + int originalLastIndex = m_lastIndex; + int intArrayLength = BitHelper.ToIntArrayLength(originalLastIndex); + + BitHelper bitHelper; + if (intArrayLength <= StackAllocThreshold) { + int* bitArrayPtr = stackalloc int[intArrayLength]; + bitHelper = new BitHelper(bitArrayPtr, intArrayLength); + } + else { + int[] bitArray = new int[intArrayLength]; + bitHelper = new BitHelper(bitArray, intArrayLength); + } + + // mark if contains: find index of in slots array and mark corresponding element in bit array + foreach (T item in other) { + int index = InternalIndexOf(item); + if (index >= 0) { + bitHelper.MarkBit(index); + } + } + + // if anything unmarked, remove it. Perf can be optimized here if BitHelper had a + // FindFirstUnmarked method. + for (int i = 0; i < originalLastIndex; i++) { + if (m_slots[i].hashCode >= 0 && !bitHelper.IsMarked(i)) { + Remove(m_slots[i].value); + } + } + } + + /// + /// Used internally by set operations which have to rely on bit array marking. This is like + /// Contains but returns index in slots array. + /// + /// + /// + private int InternalIndexOf(T item) { + Debug.Assert(m_buckets != null, "m_buckets was null; callers should check first"); + + int hashCode = InternalGetHashCode(item); + for (int i = m_buckets[hashCode % m_buckets.Length] - 1; i >= 0; i = m_slots[i].next) { + if ((m_slots[i].hashCode) == hashCode && m_comparer.Equals(m_slots[i].value, item)) { + return i; + } + } + // wasn't found + return -1; + } + + /// + /// if other is a set, we can assume it doesn't have duplicate elements, so use this + /// technique: if can't remove, then it wasn't present in this set, so add. + /// + /// As with other methods, callers take care of ensuring that other is a hashset using the + /// same equality comparer. + /// + /// + private void SymmetricExceptWithUniqueHashSet(RetrievableEntryHashSet other) { + foreach (T item in other) { + if (!Remove(item)) { + AddEvenIfPresent(item); + } + } + } + + /// + /// Implementation notes: + /// + /// Used for symmetric except when other isn't a HashSet. This is more tedious because + /// other may contain duplicates. HashSet technique could fail in these situations: + /// 1. Other has a duplicate that's not in this: HashSet technique would add then + /// remove it. + /// 2. Other has a duplicate that's in this: HashSet technique would remove then add it + /// back. + /// In general, its presence would be toggled each time it appears in other. + /// + /// This technique uses bit marking to indicate whether to add/remove the item. If already + /// present in collection, it will get marked for deletion. If added from other, it will + /// get marked as something not to remove. + /// + /// + /// + [System.Security.SecuritySafeCritical] + private unsafe void SymmetricExceptWithEnumerable(IEnumerable other) { + int originalLastIndex = m_lastIndex; + int intArrayLength = BitHelper.ToIntArrayLength(originalLastIndex); + + BitHelper itemsToRemove; + BitHelper itemsAddedFromOther; + if (intArrayLength <= StackAllocThreshold / 2) { + int* itemsToRemovePtr = stackalloc int[intArrayLength]; + itemsToRemove = new BitHelper(itemsToRemovePtr, intArrayLength); + + int* itemsAddedFromOtherPtr = stackalloc int[intArrayLength]; + itemsAddedFromOther = new BitHelper(itemsAddedFromOtherPtr, intArrayLength); + } + else { + int[] itemsToRemoveArray = new int[intArrayLength]; + itemsToRemove = new BitHelper(itemsToRemoveArray, intArrayLength); + + int[] itemsAddedFromOtherArray = new int[intArrayLength]; + itemsAddedFromOther = new BitHelper(itemsAddedFromOtherArray, intArrayLength); + } + + foreach (T item in other) { + int location = 0; + bool added = AddOrGetLocation(item, out location); + if (added) { + // wasn't already present in collection; flag it as something not to remove + // *NOTE* if location is out of range, we should ignore. BitHelper will + // detect that it's out of bounds and not try to mark it. But it's + // expected that location could be out of bounds because adding the item + // will increase m_lastIndex as soon as all the free spots are filled. + itemsAddedFromOther.MarkBit(location); + } + else { + // already there...if not added from other, mark for remove. + // *NOTE* Even though BitHelper will check that location is in range, we want + // to check here. There's no point in checking items beyond originalLastIndex + // because they could not have been in the original collection + if (location < originalLastIndex && !itemsAddedFromOther.IsMarked(location)) { + itemsToRemove.MarkBit(location); + } + } + } + + // if anything marked, remove it + for (int i = 0; i < originalLastIndex; i++) { + if (itemsToRemove.IsMarked(i)) { + Remove(m_slots[i].value); + } + } + } + + /// + /// Add if not already in hashset. Returns an out param indicating index where added. This + /// is used by SymmetricExcept because it needs to know the following things: + /// - whether the item was already present in the collection or added from other + /// - where it's located (if already present, it will get marked for removal, otherwise + /// marked for keeping) + /// + /// + /// + /// + private bool AddOrGetLocation(T value, out int location) { + Debug.Assert(m_buckets != null, "m_buckets is null, callers should have checked"); + + int hashCode = InternalGetHashCode(value); + int bucket = hashCode % m_buckets.Length; + for (int i = m_buckets[hashCode % m_buckets.Length] - 1; i >= 0; i = m_slots[i].next) { + if (m_slots[i].hashCode == hashCode && m_comparer.Equals(m_slots[i].value, value)) { + location = i; + return false; //already present + } + } + int index; + if (m_freeList >= 0) { + index = m_freeList; + m_freeList = m_slots[index].next; + } + else { + if (m_lastIndex == m_slots.Length) { + IncreaseCapacity(); + // this will change during resize + bucket = hashCode % m_buckets.Length; + } + index = m_lastIndex; + m_lastIndex++; + } + m_slots[index].hashCode = hashCode; + m_slots[index].value = value; + m_slots[index].next = m_buckets[bucket] - 1; + m_buckets[bucket] = index + 1; + m_count++; + m_version++; + location = index; + return true; + } + + /// + /// Determines counts that can be used to determine equality, subset, and superset. This + /// is only used when other is an IEnumerable and not a HashSet. If other is a HashSet + /// these properties can be checked faster without use of marking because we can assume + /// other has no duplicates. + /// + /// The following count checks are performed by callers: + /// 1. Equals: checks if unfoundCount = 0 and uniqueFoundCount = m_count; i.e. everything + /// in other is in this and everything in this is in other + /// 2. Subset: checks if unfoundCount >= 0 and uniqueFoundCount = m_count; i.e. other may + /// have elements not in this and everything in this is in other + /// 3. Proper subset: checks if unfoundCount > 0 and uniqueFoundCount = m_count; i.e + /// other must have at least one element not in this and everything in this is in other + /// 4. Proper superset: checks if unfound count = 0 and uniqueFoundCount strictly less + /// than m_count; i.e. everything in other was in this and this had at least one element + /// not contained in other. + /// + /// An earlier implementation used delegates to perform these checks rather than returning + /// an ElementCount struct; however this was changed due to the perf overhead of delegates. + /// + /// + /// Allows us to finish faster for equals and proper superset + /// because unfoundCount must be 0. + /// + [System.Security.SecuritySafeCritical] + private unsafe ElementCount CheckUniqueAndUnfoundElements(IEnumerable other, bool returnIfUnfound) { + ElementCount result; + + // need special case in case this has no elements. + if (m_count == 0) { + int numElementsInOther = 0; + foreach (T item in other) { + numElementsInOther++; + // break right away, all we want to know is whether other has 0 or 1 elements + break; + } + result.uniqueCount = 0; + result.unfoundCount = numElementsInOther; + return result; + } + + + Debug.Assert((m_buckets != null) && (m_count > 0), "m_buckets was null but count greater than 0"); + + int originalLastIndex = m_lastIndex; + int intArrayLength = BitHelper.ToIntArrayLength(originalLastIndex); + + BitHelper bitHelper; + if (intArrayLength <= StackAllocThreshold) { + int* bitArrayPtr = stackalloc int[intArrayLength]; + bitHelper = new BitHelper(bitArrayPtr, intArrayLength); + } + else { + int[] bitArray = new int[intArrayLength]; + bitHelper = new BitHelper(bitArray, intArrayLength); + } + + // count of items in other not found in this + int unfoundCount = 0; + // count of unique items in other found in this + int uniqueFoundCount = 0; + + foreach (T item in other) { + int index = InternalIndexOf(item); + if (index >= 0) { + if (!bitHelper.IsMarked(index)) { + // item hasn't been seen yet + bitHelper.MarkBit(index); + uniqueFoundCount++; + } + } + else { + unfoundCount++; + if (returnIfUnfound) { + break; + } + } + } + + result.uniqueCount = uniqueFoundCount; + result.unfoundCount = unfoundCount; + return result; + } +#endif /// /// Copies this to an array. Used for DebugView /// @@ -1298,6 +1634,71 @@ internal T[] ToArray() CopyTo(newArray); return newArray; } + +#if NEVER + /// + /// Internal method used for HashSetEqualityComparer. Compares set1 and set2 according + /// to specified comparer. + /// + /// Because items are hashed according to a specific equality comparer, we have to resort + /// to n^2 search if they're using different equality comparers. + /// + /// + /// + /// + /// + internal static bool HashSetEquals(RetrievableEntryHashSet set1, RetrievableEntryHashSet set2, IEqualityComparer comparer) { + // handle null cases first + if (set1 == null) { + return (set2 == null); + } + else if (set2 == null) { + // set1 != null + return false; + } + + // all comparers are the same; this is faster + if (AreEqualityComparersEqual(set1, set2)) { + if (set1.Count != set2.Count) { + return false; + } + // suffices to check subset + foreach (T item in set2) { + if (!set1.Contains(item)) { + return false; + } + } + return true; + } + else { // n^2 search because items are hashed according to their respective ECs + foreach (T set2Item in set2) { + bool found = false; + foreach (T set1Item in set1) { + if (comparer.Equals(set2Item, set1Item)) { + found = true; + break; + } + } + if (!found) { + return false; + } + } + return true; + } + } + + /// + /// Checks if equality comparers are equal. This is used for algorithms that can + /// speed up if it knows the other item has unique elements. I.e. if they're using + /// different equality comparers, then uniqueness assumption between sets break. + /// + /// + /// + /// + private static bool AreEqualityComparersEqual(RetrievableEntryHashSet set1, RetrievableEntryHashSet set2) { + return set1.Comparer.Equals(set2.Comparer); + } +#endif private int InternalGetHashCode(string item, int index, int length) { @@ -1322,7 +1723,7 @@ private int InternalGetHashCode(string item) return _comparer.GetHashCode(item) & Lower31BitMask; } - #endregion +#endregion // used for set checking operations (using enumerables) that rely on counting internal struct ElementCount From 3d29bf91d41ad8ff46ad37d413ce70e5802916b0 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Sat, 8 Jan 2022 10:28:32 +1000 Subject: [PATCH 4/8] Revert CodeAnalysis.ruleset --- eng/CodeAnalysis.ruleset | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eng/CodeAnalysis.ruleset b/eng/CodeAnalysis.ruleset index 29e3d4ac733..2078c42fe6c 100644 --- a/eng/CodeAnalysis.ruleset +++ b/eng/CodeAnalysis.ruleset @@ -94,7 +94,7 @@ - + From 8b42f675ac0d015dcdbb2ab9153b8be8bbe489cf Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Sat, 8 Jan 2022 10:29:10 +1000 Subject: [PATCH 5/8] enable warning on CA1823 --- eng/Common.globalconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eng/Common.globalconfig b/eng/Common.globalconfig index fd878420d57..a8b55e10e68 100644 --- a/eng/Common.globalconfig +++ b/eng/Common.globalconfig @@ -277,7 +277,7 @@ dotnet_diagnostic.CA1821.severity = warning dotnet_diagnostic.CA1822.severity = none # Avoid unused private fields -dotnet_diagnostic.CA1823.severity = suggestion +dotnet_diagnostic.CA1823.severity = warning # Mark assemblies with NeutralResourcesLanguageAttribute dotnet_diagnostic.CA1824.severity = warning From 956e47c567e024259636da1c034c1c14972e40e2 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Sat, 8 Jan 2022 10:59:27 +1000 Subject: [PATCH 6/8] Fix remaining occurrences of CA1823 violations --- src/Shared/FileUtilities.cs | 21 ------------------- src/Tasks/GenerateResource.cs | 10 --------- src/Tasks/LockCheck.cs | 2 -- src/Tasks/ManifestUtil/mansign2.cs | 17 --------------- src/Tasks/ResolveSDKReference.cs | 10 --------- .../ResourceHandling/MSBuildResXReader.cs | 5 ----- 6 files changed, 65 deletions(-) diff --git a/src/Shared/FileUtilities.cs b/src/Shared/FileUtilities.cs index 238d8d66792..e936cd1fed0 100644 --- a/src/Shared/FileUtilities.cs +++ b/src/Shared/FileUtilities.cs @@ -1089,27 +1089,6 @@ internal static string MakeRelative(string basePath, string path) return StringBuilderCache.GetStringAndRelease(sb); } - /// - /// Helper function to create an Uri object from path. - /// - /// path string - /// uri object - private static Uri CreateUriFromPath(string path) - { - ErrorUtilities.VerifyThrowArgumentLength(path, nameof(path)); - - Uri pathUri; - - // Try absolute first, then fall back on relative, otherwise it - // makes some absolute UNC paths like (\\foo\bar) relative ... - if (!Uri.TryCreate(path, UriKind.Absolute, out pathUri)) - { - pathUri = new Uri(path, UriKind.Relative); - } - - return pathUri; - } - /// /// Normalizes the path if and only if it is longer than max path, /// or would be if rooted by the current directory. diff --git a/src/Tasks/GenerateResource.cs b/src/Tasks/GenerateResource.cs index 4dabaed7f16..1cb9808bd88 100644 --- a/src/Tasks/GenerateResource.cs +++ b/src/Tasks/GenerateResource.cs @@ -926,17 +926,7 @@ public override bool Execute() private static bool allowMOTW; private const string CLSID_InternetSecurityManager = "7b8a2d94-0ac9-11d1-896c-00c04fb6bfc4"; - - private const uint ZoneLocalMachine = 0; - - private const uint ZoneIntranet = 1; - - private const uint ZoneTrusted = 2; - private const uint ZoneInternet = 3; - - private const uint ZoneUntrusted = 4; - private static IInternetSecurityManager internetSecurityManager = null; // Resources can have arbitrarily serialized objects in them which can execute arbitrary code diff --git a/src/Tasks/LockCheck.cs b/src/Tasks/LockCheck.cs index 566553eff41..fcd8a41a300 100644 --- a/src/Tasks/LockCheck.cs +++ b/src/Tasks/LockCheck.cs @@ -83,8 +83,6 @@ internal struct RM_UNIQUE_PROCESS public FILETIME ProcessStartTime; } - const int RM_INVALID_SESSION = -1; - const int RM_INVALID_PROCESS = -1; const int CCH_RM_MAX_APP_NAME = 255; const int CCH_RM_MAX_SVC_NAME = 63; const int ERROR_SEM_TIMEOUT = 121; diff --git a/src/Tasks/ManifestUtil/mansign2.cs b/src/Tasks/ManifestUtil/mansign2.cs index 2923d63cddb..5faa4ccb921 100644 --- a/src/Tasks/ManifestUtil/mansign2.cs +++ b/src/Tasks/ManifestUtil/mansign2.cs @@ -281,20 +281,6 @@ private void init() #endif } - private static XmlElement FindIdElement(XmlElement context, string idValue) - { - if (context == null) - return null; - - XmlElement idReference = context.SelectSingleNode("//*[@Id=\"" + idValue + "\"]") as XmlElement; - if (idReference != null) - return idReference; - idReference = context.SelectSingleNode("//*[@id=\"" + idValue + "\"]") as XmlElement; - if (idReference != null) - return idReference; - return context.SelectSingleNode("//*[@ID=\"" + idValue + "\"]") as XmlElement; - } - public override XmlElement GetIdElement(XmlDocument document, string idValue) { // We only care about Id references inside of the KeyInfo section @@ -320,9 +306,6 @@ internal class SignedCmiManifest2 private const string Sha1SignatureMethodUri = @"http://www.w3.org/2000/09/xmldsig#rsa-sha1"; private const string Sha1DigestMethod = @"http://www.w3.org/2000/09/xmldsig#sha1"; - private const string wintrustPolicyFlagsRegPath = "Software\\Microsoft\\Windows\\CurrentVersion\\WinTrust\\Trust Providers\\Software Publishing"; - private const string wintrustPolicyFlagsRegName = "State"; - private SignedCmiManifest2() { } internal SignedCmiManifest2(XmlDocument manifestDom, bool useSha256) diff --git a/src/Tasks/ResolveSDKReference.cs b/src/Tasks/ResolveSDKReference.cs index 1a489c2c402..972b81c1d3c 100644 --- a/src/Tasks/ResolveSDKReference.cs +++ b/src/Tasks/ResolveSDKReference.cs @@ -661,16 +661,6 @@ internal class SDKReference : IEquatable /// private const string X64Arch = "X64"; - /// - /// X86 architecture name - /// - private const string X86Arch = "X86"; - - /// - /// ARM architecture name - /// - private const string ARMArch = "ARM"; - /// /// ANY CPU architecture name /// diff --git a/src/Tasks/ResourceHandling/MSBuildResXReader.cs b/src/Tasks/ResourceHandling/MSBuildResXReader.cs index f3f3d5db1ff..86dc5d50311 100644 --- a/src/Tasks/ResourceHandling/MSBuildResXReader.cs +++ b/src/Tasks/ResourceHandling/MSBuildResXReader.cs @@ -72,13 +72,8 @@ private static void ParseAssemblyAlias(Dictionary aliases, XEleme // Consts from https://github.com/dotnet/winforms/blob/16b192389b377c647ab3d280130781ab1a9d3385/src/System.Windows.Forms/src/System/Resources/ResXResourceWriter.cs#L46-L63 private const string Beta2CompatSerializedObjectMimeType = "text/microsoft-urt/psuedoml-serialized/base64"; private const string CompatBinSerializedObjectMimeType = "text/microsoft-urt/binary-serialized/base64"; - private const string CompatSoapSerializedObjectMimeType = "text/microsoft-urt/soap-serialized/base64"; private const string BinSerializedObjectMimeType = "application/x-microsoft.net.object.binary.base64"; - private const string SoapSerializedObjectMimeType = "application/x-microsoft.net.object.soap.base64"; - private const string DefaultSerializedObjectMimeType = BinSerializedObjectMimeType; private const string ByteArraySerializedObjectMimeType = "application/x-microsoft.net.object.bytearray.base64"; - private const string ResMimeType = "text/microsoft-resx"; - private const string StringTypeNamePrefix = "System.String, mscorlib,"; private const string StringTypeName40 = "System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089"; private const string MemoryStreamTypeNamePrefix = "System.IO.MemoryStream, mscorlib,"; From 1d848f994f6c7830449a32d3d3d186cd4a9d406f Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Sat, 8 Jan 2022 14:13:52 +1000 Subject: [PATCH 7/8] Fix remaining occurrences of CA1823 violations --- src/Tasks/ManifestUtil/SecurityUtil.cs | 2 +- src/Tasks/ManifestUtil/Util.cs | 5 +++-- src/Tasks/NativeMethods.cs | 12 +++++++----- src/Tasks/ResolveKeySource.cs | 2 ++ 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/Tasks/ManifestUtil/SecurityUtil.cs b/src/Tasks/ManifestUtil/SecurityUtil.cs index abdcce405f9..def1e2bdc0f 100644 --- a/src/Tasks/ManifestUtil/SecurityUtil.cs +++ b/src/Tasks/ManifestUtil/SecurityUtil.cs @@ -46,8 +46,8 @@ public static class SecurityUtilities #if !RUNTIME_TYPE_NETCORE private const int Fx2MajorVersion = 2; private const int Fx3MajorVersion = 3; -#endif private static readonly Version s_dotNet40Version = new Version("4.0"); +#endif private static readonly Version s_dotNet45Version = new Version("4.5"); #if !RUNTIME_TYPE_NETCORE diff --git a/src/Tasks/ManifestUtil/Util.cs b/src/Tasks/ManifestUtil/Util.cs index ebc38e86e2b..b3e543a1340 100644 --- a/src/Tasks/ManifestUtil/Util.cs +++ b/src/Tasks/ManifestUtil/Util.cs @@ -29,11 +29,12 @@ internal static class Util internal static readonly string logPath = GetLogPath(); private static readonly char[] s_fileNameInvalidChars = { '\\', '/', ':', '*', '?', '"', '<', '>', '|' }; private static StreamWriter s_logFileWriter; - // Major, Minor, Build and Revision of CLR v2.0 - private static readonly int[] s_clrVersion2 = { 2, 0, 50727, 0 }; #if RUNTIME_TYPE_NETCORE // Major, Minor, Build and Revision of CLR v4.0 private static readonly int[] s_clrVersion4 = { 4, 0, 30319, 0 }; +#else + // Major, Minor, Build and Revision of CLR v2.0 + private static readonly int[] s_clrVersion2 = { 2, 0, 50727, 0 }; #endif #region " Platform <-> ProcessorArchitecture mapping " diff --git a/src/Tasks/NativeMethods.cs b/src/Tasks/NativeMethods.cs index f2308684706..9adcab120cc 100644 --- a/src/Tasks/NativeMethods.cs +++ b/src/Tasks/NativeMethods.cs @@ -779,7 +779,9 @@ internal struct CRYPTOAPI_BLOB #region PInvoke private const string Crypt32DLL = "crypt32.dll"; private const string Advapi32DLL = "advapi32.dll"; +#if !RUNTIME_TYPE_NETCORE private const string MscoreeDLL = "mscoree.dll"; +#endif //------------------------------------------------------------------------------ // CreateHardLink @@ -1127,9 +1129,9 @@ internal static extern int CreateAssemblyNameObject( [DllImport(MscoreeDLL, SetLastError = true, CharSet = CharSet.Unicode)] internal static extern uint GetFileVersion(String szFullPath, StringBuilder szBuffer, int cchBuffer, out uint dwLength); #endif - #endregion +#endregion - #region Methods +#region Methods #if FEATURE_HANDLEPROCESSCORRUPTEDSTATEEXCEPTIONS /// /// Given a pointer to a metadata blob, read the string parameter from it. Returns true if @@ -1250,8 +1252,8 @@ internal static unsafe int CorSigUncompressData(IntPtr data, out int uncompresse return count; } - #endregion - #region InternalClass +#endregion +#region InternalClass #if FEATURE_COM_INTEROP /// /// This class is a wrapper over the native GAC enumeration API. @@ -1491,6 +1493,6 @@ public static string AssemblyPathFromStrongName(string strongName) } } #endif - #endregion +#endregion } } diff --git a/src/Tasks/ResolveKeySource.cs b/src/Tasks/ResolveKeySource.cs index 676d5a4d249..95e3a2ecf68 100644 --- a/src/Tasks/ResolveKeySource.cs +++ b/src/Tasks/ResolveKeySource.cs @@ -24,7 +24,9 @@ namespace Microsoft.Build.Tasks public class ResolveKeySource : TaskExtension { private const string pfxFileExtension = ".pfx"; +#if !RUNTIME_TYPE_NETCORE private const string pfxFileContainerPrefix = "VS_KEY_"; +#endif #region Properties From 4d1a1cd2aebda77472c40db03f668d137dc8b368 Mon Sep 17 00:00:00 2001 From: elachlan <2433737+elachlan@users.noreply.github.com> Date: Wed, 19 Jan 2022 09:09:49 +1000 Subject: [PATCH 8/8] Change clrVersion ordering --- src/Tasks/ManifestUtil/Util.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Tasks/ManifestUtil/Util.cs b/src/Tasks/ManifestUtil/Util.cs index b3e543a1340..984e66d8d3d 100644 --- a/src/Tasks/ManifestUtil/Util.cs +++ b/src/Tasks/ManifestUtil/Util.cs @@ -29,12 +29,12 @@ internal static class Util internal static readonly string logPath = GetLogPath(); private static readonly char[] s_fileNameInvalidChars = { '\\', '/', ':', '*', '?', '"', '<', '>', '|' }; private static StreamWriter s_logFileWriter; -#if RUNTIME_TYPE_NETCORE - // Major, Minor, Build and Revision of CLR v4.0 - private static readonly int[] s_clrVersion4 = { 4, 0, 30319, 0 }; -#else +#if !RUNTIME_TYPE_NETCORE // Major, Minor, Build and Revision of CLR v2.0 private static readonly int[] s_clrVersion2 = { 2, 0, 50727, 0 }; +#else + // Major, Minor, Build and Revision of CLR v4.0 + private static readonly int[] s_clrVersion4 = { 4, 0, 30319, 0 }; #endif #region " Platform <-> ProcessorArchitecture mapping "