Fix wrong paths in SharedOutputPathAnalyzer. (#10472)

Related to #10414

Context
PR #10238 added normalization of the path (which includes getting full path) before the project directory path is added to it. This led to the relative path being turned into a full path using CurrentDirectory instead of the project directory. This leads to enormous amount of false-positive analyzer's warnings (400 instead of 3) that cause the hang. This PR fixes the bug with paths and does not fix actual cause of a hang.

Changes Made
Reverted the PR #10238 and instead added normalization of the path later in the code when the project path is already combined with the relative path.

Testing
unit tests and local runs

src/Build/BuildCheck/Analyzers/SharedOutputPathAnalyzer.cs

@@ -46,8 +46,9 @@ private void EvaluatedPropertiesAction(BuildCheckDataContext<EvaluatedProperties
         }
 
         string? binPath, objPath;
-        context.Data.EvaluatedProperties.TryGetPathValue("OutputPath", out binPath);
-        context.Data.EvaluatedProperties.TryGetPathValue("IntermediateOutputPath", out objPath);
+
+        context.Data.EvaluatedProperties.TryGetValue("OutputPath", out binPath);
+        context.Data.EvaluatedProperties.TryGetValue("IntermediateOutputPath", out objPath);
 
         string? absoluteBinPath = CheckAndAddFullOutputPath(binPath, context);
         // Check objPath only if it is different from binPath
@@ -76,7 +77,7 @@ private void EvaluatedPropertiesAction(BuildCheckDataContext<EvaluatedProperties
         }
 
         // Normalize the path to avoid false negatives due to different path representations.
-        path = Path.GetFullPath(path);
+        path = FileUtilities.NormalizePath(path);
 
         if (_projectsPerOutputPath.TryGetValue(path!, out string? conflictingProject))
         {

src/BuildCheck.UnitTests/DoubleWritesAnalyzer_Tests.cs

@@ -15,36 +15,6 @@ namespace Microsoft.Build.BuildCheck.UnitTests
 {
     public sealed class DoubleWritesAnalyzer_Tests
     {
-        private sealed class MockBuildCheckRegistrationContext : IBuildCheckRegistrationContext
-        {
-            private event Action<BuildCheckDataContext<TaskInvocationAnalysisData>>? _taskInvocationAction;
-
-            public List<BuildCheckResult> Results { get; } = new();
-
-            public void RegisterEvaluatedPropertiesAction(Action<BuildCheckDataContext<EvaluatedPropertiesAnalysisData>> evaluatedPropertiesAction) => throw new NotImplementedException();
-            public void RegisterParsedItemsAction(Action<BuildCheckDataContext<ParsedItemsAnalysisData>> parsedItemsAction) => throw new NotImplementedException();
-
-            public void RegisterTaskInvocationAction(Action<BuildCheckDataContext<TaskInvocationAnalysisData>> taskInvocationAction)
-                => _taskInvocationAction += taskInvocationAction;
-
-            public void TriggerTaskInvocationAction(TaskInvocationAnalysisData data)
-            {
-                if (_taskInvocationAction is not null)
-                {
-                    BuildCheckDataContext<TaskInvocationAnalysisData> context = new BuildCheckDataContext<TaskInvocationAnalysisData>(
-                        null!,
-                        null!,
-                        null!,
-                        ResultHandler,
-                        data);
-                    _taskInvocationAction(context);
-                }
-            }
-
-            private void ResultHandler(BuildAnalyzerWrapper wrapper, IAnalysisContext context, BuildAnalyzerConfigurationEffective[] configs, BuildCheckResult result)
-                => Results.Add(result);
-        }
-
         private readonly DoubleWritesAnalyzer _analyzer;
 
         private readonly MockBuildCheckRegistrationContext _registrationContext;

src/BuildCheck.UnitTests/MockBuildCheckRegistrationContext.cs

@@ -0,0 +1,56 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections.Generic;
+using Microsoft.Build.Experimental.BuildCheck;
+using Microsoft.Build.Experimental.BuildCheck.Infrastructure;
+
+namespace Microsoft.Build.BuildCheck.UnitTests
+{
+    internal sealed class MockBuildCheckRegistrationContext : IBuildCheckRegistrationContext
+    {
+        private event Action<BuildCheckDataContext<TaskInvocationAnalysisData>>? _taskInvocationAction;
+        private event Action<BuildCheckDataContext<EvaluatedPropertiesAnalysisData>>? _evaluatedPropertiesAction;
+
+        public List<BuildCheckResult> Results { get; } = new();
+
+        public void RegisterEvaluatedPropertiesAction(Action<BuildCheckDataContext<EvaluatedPropertiesAnalysisData>> evaluatedPropertiesAction)
+            => _evaluatedPropertiesAction += evaluatedPropertiesAction;
+        public void RegisterParsedItemsAction(Action<BuildCheckDataContext<ParsedItemsAnalysisData>> parsedItemsAction) => throw new NotImplementedException();
+
+        public void RegisterTaskInvocationAction(Action<BuildCheckDataContext<TaskInvocationAnalysisData>> taskInvocationAction)
+            => _taskInvocationAction += taskInvocationAction;
+
+        public void TriggerTaskInvocationAction(TaskInvocationAnalysisData data)
+        {
+            if (_taskInvocationAction is not null)
+            {
+                BuildCheckDataContext<TaskInvocationAnalysisData> context = new BuildCheckDataContext<TaskInvocationAnalysisData>(
+                    null!,
+                    null!,
+                    null!,
+                    ResultHandler,
+                    data);
+                _taskInvocationAction(context);
+            }
+        }
+        public void TriggerEvaluatedPropertiesAction(EvaluatedPropertiesAnalysisData data)
+        {
+            if (_evaluatedPropertiesAction is not null)
+            {
+                BuildCheckDataContext<EvaluatedPropertiesAnalysisData> context = new BuildCheckDataContext<EvaluatedPropertiesAnalysisData>(
+                    null!,
+                    null!,
+                    null!,
+                    ResultHandler,
+                    data);
+                _evaluatedPropertiesAction(context);
+            }
+        }
+
+        private void ResultHandler(BuildAnalyzerWrapper wrapper, IAnalysisContext context, BuildAnalyzerConfigurationEffective[] configs, BuildCheckResult result)
+            => Results.Add(result);
+    }
+}
+

src/BuildCheck.UnitTests/SharedOutputPathAnalyzer_Tests.cs

@@ -0,0 +1,143 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+using Microsoft.Build.Experimental.BuildCheck;
+using Microsoft.Build.Experimental.BuildCheck.Analyzers;
+using Shouldly;
+using Xunit;
+
+namespace Microsoft.Build.BuildCheck.UnitTests
+{
+    public class SharedOutputPathAnalyzer_Tests
+    {
+        private readonly SharedOutputPathAnalyzer _analyzer;
+
+        private readonly MockBuildCheckRegistrationContext _registrationContext;
+
+        public SharedOutputPathAnalyzer_Tests()
+        {
+            _analyzer = new SharedOutputPathAnalyzer();
+            _registrationContext = new MockBuildCheckRegistrationContext();
+            _analyzer.RegisterActions(_registrationContext);
+        }
+
+        private EvaluatedPropertiesAnalysisData MakeEvaluatedPropertiesAction(
+            string projectFile,
+            Dictionary<string, string>? evaluatedProperties,
+            IReadOnlyDictionary<string, (string EnvVarValue, string File, int Line, int Column)>? evaluatedEnvVars)
+        {
+            return new EvaluatedPropertiesAnalysisData(
+                projectFile,
+                null,
+                evaluatedProperties ?? new Dictionary<string, string>(),
+                evaluatedEnvVars ?? new Dictionary<string, (string EnvVarValue, string File, int Line, int Column)>());
+        }
+
+        [Fact]
+        public void TestTwoProjectsWithSameRelativeOutputPath()
+        {
+            // Full output and intermediate paths are different: "C:/fake1/bin/Debug" and "C:/fake1/obj/Debug".
+            string projectFile1 = NativeMethodsShared.IsWindows ? "C:\\fake1\\project1.proj" : "/fake1/project1.proj";
+            _registrationContext.TriggerEvaluatedPropertiesAction(MakeEvaluatedPropertiesAction(
+                projectFile1,
+                new Dictionary<string, string> {
+                    { "OutputPath", "bin/Debug" },
+                    { "IntermediateOutputPath", "obj/Debug" },
+                },
+                null));
+
+            // Full output and intermediate paths are different: "C:/fake2/bin/Debug" and "C:/fake2/obj/Debug".
+            string projectFile2 = NativeMethodsShared.IsWindows ? "C:\\fake2\\project2.proj" : "/fake2/project2.proj";
+            _registrationContext.TriggerEvaluatedPropertiesAction(MakeEvaluatedPropertiesAction(
+                projectFile2,
+                new Dictionary<string, string> {
+                    { "OutputPath", "bin/Debug" },
+                    { "IntermediateOutputPath", "obj/Debug" },
+                },
+                null));
+
+            // Relative paths coincide but full does not. SharedOutputPathAnalyzer should not report it.
+            _registrationContext.Results.Count.ShouldBe(0);
+        }
+
+        [Fact]
+        public void TestProjectsWithDifferentPathsSeparators()
+        {
+            // Paths separators are messed up.
+            string projectFile1 = NativeMethodsShared.IsWindows ? "C:\\fake\\project1.proj" : "/fake/project1.proj";
+            string projectFile2 = NativeMethodsShared.IsWindows ? "C:\\fake\\project2.proj" : "/fake/project2.proj";
+
+            _registrationContext.TriggerEvaluatedPropertiesAction(MakeEvaluatedPropertiesAction(
+                projectFile1,
+                new Dictionary<string, string> {
+                    { "OutputPath", "bin/Debug" },
+                    { "IntermediateOutputPath", "obj\\Debug" },
+                },
+                null));
+
+            _registrationContext.TriggerEvaluatedPropertiesAction(MakeEvaluatedPropertiesAction(
+                projectFile2,
+                new Dictionary<string, string> {
+                    { "OutputPath", "bin/Debug" },
+                    { "IntermediateOutputPath", "obj\\Debug" },
+                },
+                null));
+
+            // 2 reports for bin and obj folders.
+            _registrationContext.Results.Count.ShouldBe(2);
+            _registrationContext.Results[0].BuildAnalyzerRule.Id.ShouldBe("BC0101");
+            _registrationContext.Results[1].BuildAnalyzerRule.Id.ShouldBe("BC0101");
+
+            // Check that paths are formed with correct paths separators
+            string wrongPathSeparator = NativeMethodsShared.IsWindows ? "/" : "\\";
+
+            foreach (string path in _registrationContext.Results[0].MessageArgs)
+            {
+                path.ShouldNotContain(wrongPathSeparator);
+            }
+            foreach (string path in _registrationContext.Results[1].MessageArgs)
+            {
+                path.ShouldNotContain(wrongPathSeparator);
+            }
+        }
+
+        [Fact]
+        public void TestThreeProjectsWithSameOutputPath()
+        {
+            string projectFolder = NativeMethodsShared.IsWindows ? "C:\\fake\\" : "/fake/";
+            string projectFile1 = $"{projectFolder}project1.proj";
+            string projectFile2 = $"{projectFolder}project2.proj";
+            string projectFile3 = $"{projectFolder}project3.proj";
+            var evaluatedProperties = new Dictionary<string, string> {
+                    { "OutputPath", "bin/Debug" },
+                    { "IntermediateOutputPath", "obj\\Debug" },};
+
+            _registrationContext.TriggerEvaluatedPropertiesAction(MakeEvaluatedPropertiesAction(
+                projectFile1,
+                evaluatedProperties,
+                null));
+
+            _registrationContext.TriggerEvaluatedPropertiesAction(MakeEvaluatedPropertiesAction(
+                projectFile2,
+                evaluatedProperties,
+                null));
+
+            _registrationContext.TriggerEvaluatedPropertiesAction(MakeEvaluatedPropertiesAction(
+                projectFile3,
+                evaluatedProperties,
+                null));
+
+            _registrationContext.Results.Count.ShouldBe(4); // 4 reports for two pairs of project: (1, 2) and (1, 3).
+            foreach (var result in _registrationContext.Results)
+            {
+                result.BuildAnalyzerRule.Id.ShouldBe("BC0101");
+            }
+        }
+    }
+}

src/Shared/FileUtilities.cs

@@ -576,26 +576,6 @@ internal static string MaybeAdjustFilePath(string value, string baseDirectory =
             return shouldAdjust ? newValue.ToString() : value;
         }
 
-        /// <summary>
-        /// Gets the path value that is associated with the specified key in a dictionary with <see cref="string"/> values.
-        /// Normalizes the value as a path.
-        /// </summary>
-        /// <param name="dictionary">The dictionary to search.</param>
-        /// <param name="key">The key to locate.</param>
-        /// <param name="value">When this method returns, the value associated with the specified key normalized as a path, if the key is found; otherwise <see langword="null"/>.</param>
-        /// <returns><see langword="true"/> if the dictionary contains an element that has the specified key; otherwise, <see langword="false"/>.</returns>
-        /// <remarks>Use this method to get paths from dictionaries of properties whose default values may contain backslashes.</remarks>
-        internal static bool TryGetPathValue<TKey>(this IReadOnlyDictionary<TKey, string> dictionary, TKey key, out string value)
-        {
-            bool result = dictionary.TryGetValue(key, out value);
-            if (result)
-            {
-                value = NormalizePath(value);
-            }
-
-            return result;
-        }
-
         /// <summary>
         /// If on Unix, convert backslashes to slashes for strings that resemble paths.
         /// This overload takes and returns ReadOnlyMemory of characters.