From 0461e8cef4e97cdd2fa477550864d749a47f701e Mon Sep 17 00:00:00 2001
From: Jan Krivanek <jankrivanek@microsoft.com>
Date: Tue, 11 Jul 2023 13:32:46 +0200
Subject: [PATCH] Reflect PR suggestions

---
 src/Tasks.UnitTests/Hash_Tests.cs | 49 +++++++++++++------------------
 src/Tasks/Hash.cs                 |  7 ++++-
 2 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/src/Tasks.UnitTests/Hash_Tests.cs b/src/Tasks.UnitTests/Hash_Tests.cs
index 611b6d92d55..aa41b686a6b 100644
--- a/src/Tasks.UnitTests/Hash_Tests.cs
+++ b/src/Tasks.UnitTests/Hash_Tests.cs
@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Linq;
 using Microsoft.Build.Framework;
 using Microsoft.Build.UnitTests;
 using Microsoft.Build.Utilities;
@@ -81,44 +82,36 @@ public void HashTaskLargeInputSizeTest()
             Assert.Equal(expectedHash, actualHash);
         }
 
-#pragma warning disable CA5350
         // This test verifies that hash computes correctly for various numbers of characters.
         // We would like to process edge of the buffer use cases regardless on the size of the buffer.
         [Fact]
         public void HashTaskDifferentInputSizesTest()
         {
             int maxInputSize = 2000;
-            string input = "";
-            using (var sha256 = System.Security.Cryptography.SHA256.Create())
+            MockEngine mockEngine = new();
+
+            var hashGroups =
+                Enumerable.Range(0, maxInputSize)
+                    .Select(cnt => new string('a', cnt))
+                    .Select(GetHash)
+                    .GroupBy(h => h)
+                    .Where(g => g.Count() > 1)
+                    .Select(g => g.Key);
+            // none of the hashes should repeat
+            Assert.Empty(hashGroups);
+
+            string GetHash(string input)
             {
-                var stringBuilder = new System.Text.StringBuilder(sha256.HashSize);
-                MockEngine mockEngine = new();
-                for (int i = 0; i < maxInputSize; i++)
+                Hash hashTask = new()
                 {
-                    input += "a";
-
-                    Hash hashTask = new()
-                    {
-                        BuildEngine = mockEngine,
-                        ItemsToHash = new ITaskItem[] { new TaskItem(input) },
-                        IgnoreCase = false
-                    };
-                    Assert.True(hashTask.Execute());
-                    string actualHash = hashTask.HashResult;
-
-                    byte[] hash = sha256.ComputeHash(System.Text.Encoding.UTF8.GetBytes(input + '\u2028'));
-                    stringBuilder.Clear();
-                    foreach (var b in hash)
-                    {
-                        stringBuilder.Append(b.ToString("x2"));
-                    }
-                    string expectedHash = stringBuilder.ToString();
-
-                    Assert.Equal(expectedHash, actualHash);
-                }
+                    BuildEngine = mockEngine,
+                    ItemsToHash = new ITaskItem[] { new TaskItem(input) },
+                    IgnoreCase = false
+                };
+                Assert.True(hashTask.Execute());
+                return hashTask.HashResult;
             }
         }
-#pragma warning restore CA5350
 
         [Fact]
         public void HashTaskIgnoreCaseTest()
diff --git a/src/Tasks/Hash.cs b/src/Tasks/Hash.cs
index 939dd7d9ac2..23615f93af7 100644
--- a/src/Tasks/Hash.cs
+++ b/src/Tasks/Hash.cs
@@ -15,7 +15,12 @@ namespace Microsoft.Build.Tasks
     /// </summary>
     /// <remarks>
     /// Currently uses SHA256. Implementation subject to change between MSBuild versions.
-    /// This class is not intended as a cryptographic security measure, only uniqueness between build executions.
+    /// This class is not intended as a cryptographic security measure, only uniqueness between build executions
+    /// - collisions can theoretically be possible in the future (should we move to noncrypto hash) and should be handled gracefully by the caller.
+    ///
+    /// Usage of cryptographic secure hash brings slight performance penalty, but it is considered acceptable.
+    /// Would this need to be revised - XxHash64 from System.Io.Hashing could be used instead for better performance.
+    /// (That however currently requires load of additional binary into VS process which has it's own costs)
     /// </remarks>
     public class Hash : TaskExtension
     {