Add 'Prefer Project Reference' Check (#10955)

* Add EvaluatedItemCheckData

* Fix build

* Add 'Prefer Project Reference' Check

* Remove extra code

* Fix typo

* Update Codes.md

* Update documentation/specs/BuildCheck/Codes.md

Co-authored-by: Chet Husk <[email protected]>

* Apply suggestions from code review

Co-authored-by: Jan Provazník <[email protected]>

* Reflect PR suggestions

* Reflected PR comments

---------

Co-authored-by: Chet Husk <[email protected]>
Co-authored-by: Jan Provazník <[email protected]>

documentation/specs/BuildCheck/Codes.md

@@ -2,14 +2,15 @@
 
 Report codes are chosen to conform to suggested guidelines. Those guidelines are currently in revew: https://github.com/dotnet/msbuild/pull/10088
 
-| Diagnostic Code | Default Severity | Reason |
-|:-----|-------|----------|
-| [BC0101](#bc0101---shared-output-path) | Warning | Shared output path. |
-| [BC0102](#bc0102---double-writes) | Warning | Double writes. |
-| [BC0103](#bc0103---used-environment-variable) | Suggestion | Used environment variable. |
-| [BC0201](#bc0201---usage-of-undefined-property) | Warning | Usage of undefined property. |
-| [BC0202](#bc0202---property-first-declared-after-it-was-used) | Warning | Property first declared after it was used. |
-| [BC0203](#bc0203----property-declared-but-never-used) | Suggestion | Property declared but never used. |
+| Diagnostic Code | Default Severity | Default Scope | Available from SDK | Reason |
+|:-----|-------|-------|-------|----------|
+| [BC0101](#bc0101---shared-output-path) | Warning | Project | 9.0.100 | Shared output path. |
+| [BC0102](#bc0102---double-writes) | Warning | Project | 9.0.100 | Double writes. |
+| [BC0103](#bc0103---used-environment-variable) | Suggestion | Project | 9.0.100 | Used environment variable. |
+| [BC0104](#bc0104---projectreference-is-preferred-to-reference) | Warning | Project | 9.0.200 | ProjectReference is preferred to Reference. |
+| [BC0201](#bc0201---usage-of-undefined-property) | Warning | Project | 9.0.100 | Usage of undefined property. |
+| [BC0202](#bc0202---property-first-declared-after-it-was-used) | Warning | Project | 9.0.100 | Property first declared after it was used. |
+| [BC0203](#bc0203----property-declared-but-never-used) | Suggestion | Project | 9.0.100 | Property declared but never used. |
 
 
 To enable verbose logging in order to troubleshoot issue(s), enable [binary logging](https://github.com/dotnet/msbuild/blob/main/documentation/wiki/Binary-Log.md#msbuild-binary-log-overview)
@@ -48,6 +49,15 @@ Relying on environment variables introduces variability and unpredictability, as
 
 This practice can result in inconsistent build outcomes and makes debugging difficult, since environment variables are external to project files and build scripts. To ensure consistent and reproducible builds, avoid using environment variables. Instead, explicitly pass properties using the /p option, which offers better control and traceability.
 
+<a name="BC0104"></a>
+## BC0104 - ProjectReference is preferred to Reference.
+
+"A project should not be referenced via 'Reference' to its output, but rather directly via 'ProjectReference'."
+
+It is not recommended to reference project outputs. Such practice leads to losing the explicit dependency between the projects. Build then might not order the projects properly, which can lead to randomly missing reference and hence undeterministic build.
+
+If you need to achieve more advanced dependency behavior - check [Controlling Dependencies Behavior](https://github.com/dotnet/msbuild/blob/main/documentation/wiki/Controlling-Dependencies-Behavior.md) document. If neither suits your needs - then you might need to disable this check for your build or for particular projects.
+
 <a name="BC0201"></a>
 ## BC0201 - Usage of undefined property.
 

src/Build/BuildCheck/.editorconfig

@@ -1,2 +1,3 @@
 [*.cs]
 csharp_style_namespace_declarations = file_scoped:warning
+dotnet_diagnostic.IDE0005.severity = warning

src/Build/BuildCheck/API/BuildCheckResult.cs

@@ -2,7 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
-using Microsoft.Build.Construction;
 using Microsoft.Build.Framework;
 using Microsoft.Build.Shared;
 

src/Build/BuildCheck/API/Check.cs

@@ -3,8 +3,6 @@
 
 using System;
 using System.Collections.Generic;
-using System.Collections.Immutable;
-using Microsoft.Build.Experimental.BuildCheck.Infrastructure;
 
 namespace Microsoft.Build.Experimental.BuildCheck;
 

src/Build/BuildCheck/API/CheckConfiguration.cs

@@ -1,7 +1,6 @@
 // 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.Infrastructure;
 using Microsoft.Build.Experimental.BuildCheck.Utilities;

src/Build/BuildCheck/API/ConfigurationContext.cs

@@ -1,9 +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;
 using System.Collections.Generic;
-using System.Linq;
 using Microsoft.Build.Experimental.BuildCheck.Infrastructure;
 
 namespace Microsoft.Build.Experimental.BuildCheck;

src/Build/BuildCheck/API/WorkerNodeCheck.cs

@@ -2,7 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
-using Microsoft.Build.Experimental.BuildCheck;
 
 namespace Microsoft.Build.Experimental.BuildCheck.Checks;
 

src/Build/BuildCheck/Acquisition/BuildCheckAcquisitionModule.cs

@@ -7,8 +7,9 @@
 using System.Reflection;
 using Microsoft.Build.Experimental.BuildCheck.Infrastructure;
 using Microsoft.Build.Framework;
-using Microsoft.Build.Framework.Telemetry;
+#if FEATURE_ASSEMBLYLOADCONTEXT
 using Microsoft.Build.Shared;
+#endif
 
 namespace Microsoft.Build.Experimental.BuildCheck.Acquisition;
 

src/Build/BuildCheck/Acquisition/CheckAcquisitionData.cs

@@ -1,13 +1,6 @@
 // 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.Linq;
-using System.Text;
-using System.Threading.Tasks;
-using Microsoft.Build.Experimental.BuildCheck;
-
 namespace Microsoft.Build.Experimental.BuildCheck.Acquisition;
 
 // https://github.com/dotnet/msbuild/issues/9633

src/Build/BuildCheck/Acquisition/IBuildCheckAcquisitionModule.cs

@@ -3,7 +3,6 @@
 
 using System.Collections.Generic;
 using Microsoft.Build.Experimental.BuildCheck.Infrastructure;
-using Microsoft.Build.Framework;
 
 namespace Microsoft.Build.Experimental.BuildCheck.Acquisition;
 

src/Build/BuildCheck/Checks/DoubleWritesCheck.cs

@@ -3,13 +3,10 @@
 
 using System;
 using System.Collections.Generic;
-using System.Collections.Immutable;
-using System.Diagnostics;
+#if !FEATURE_MSIOREDIST
 using System.IO;
+#endif
 using System.Linq;
-using Microsoft.Build.Experimental.BuildCheck.Infrastructure;
-using Microsoft.Build.Construction;
-using Microsoft.Build.Experimental.BuildCheck;
 using Microsoft.Build.Shared;
 using static Microsoft.Build.Experimental.BuildCheck.TaskInvocationCheckData;
 

src/Build/BuildCheck/Checks/PreferProjectReferenceCheck.cs

@@ -0,0 +1,109 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Collections.Generic;
+using System.IO;
+using Microsoft.Build.Collections;
+using Microsoft.Build.Construction;
+using Microsoft.Build.Framework;
+using Microsoft.Build.Shared;
+
+namespace Microsoft.Build.Experimental.BuildCheck.Checks;
+internal class PreferProjectReferenceCheck : Check
+{
+    private const string RuleId = "BC0104";
+    public static CheckRule SupportedRule = new CheckRule(RuleId, "PreferProjectReference",
+        ResourceUtilities.GetResourceString("BuildCheck_BC0104_Title")!,
+        ResourceUtilities.GetResourceString("BuildCheck_BC0104_MessageFmt")!,
+        new CheckConfiguration() { RuleId = RuleId, Severity = CheckResultSeverity.Warning });
+
+    public override string FriendlyName => "MSBuild.PreferProjectReferenceCheck";
+
+    public override IReadOnlyList<CheckRule> SupportedRules { get; } = [SupportedRule];
+
+    public override void Initialize(ConfigurationContext configurationContext)
+    {
+        /* This is it - no custom configuration */
+    }
+
+    public override void RegisterActions(IBuildCheckRegistrationContext registrationContext)
+    {
+        registrationContext.RegisterEvaluatedPropertiesAction(EvaluatedPropertiesAction);
+        registrationContext.RegisterEvaluatedItemsAction(EvaluatedItemsAction);
+    }
+
+    internal override bool IsBuiltIn => true;
+
+    private readonly Dictionary<string, (string, string)> _projectsPerReferencePath = new(MSBuildNameIgnoreCaseComparer.Default);
+    private readonly Dictionary<string, string> _projectsPerOutputPath = new(MSBuildNameIgnoreCaseComparer.Default);
+    private readonly HashSet<string> _projectsSeen = new(MSBuildNameIgnoreCaseComparer.Default);
+
+    private void EvaluatedPropertiesAction(BuildCheckDataContext<EvaluatedPropertiesCheckData> context)
+    {
+        // We want to avoid repeated checking of a same project (as it might be evaluated multiple times)
+        //  for this reason we use a hashset with already seen projects.
+        // We want to do the same prevention for both registered actions: EvaluatedPropertiesAction and EvaluatedItemsAction.
+        //  To avoid the need to have separate hashset for each of those functions - we use a single one and we use the fact that
+        //  both functions are always called after each other (EvaluatedPropertiesAction first, then EvaluatedItemsAction),
+        //  so this function just checks the hashset (not to prevent execution of EvaluatedItemsAction) and EvaluatedItemsAction
+        //  updates the hashset.
+        if (_projectsSeen.Contains(context.Data.ProjectFilePath))
+        {
+            return;
+        }
+
+        string? targetPath;
+
+        context.Data.EvaluatedProperties.TryGetValue(ItemMetadataNames.targetPath, out targetPath);
+
+        if (string.IsNullOrEmpty(targetPath))
+        {
+            return;
+        }
+
+        targetPath = BuildCheckUtilities.RootEvaluatedPath(targetPath, context.Data.ProjectFilePath);
+
+        _projectsPerOutputPath[targetPath] = context.Data.ProjectFilePath;
+
+        (string, string) projectProducingOutput;
+        if (_projectsPerReferencePath.TryGetValue(targetPath, out projectProducingOutput))
+        {
+            context.ReportResult(BuildCheckResult.Create(
+                SupportedRule,
+                // Populating precise location tracked via https://github.com/orgs/dotnet/projects/373/views/1?pane=issue&itemId=58661732
+                ElementLocation.EmptyLocation,
+                Path.GetFileName(context.Data.ProjectFilePath),
+                Path.GetFileName(projectProducingOutput.Item1),
+                projectProducingOutput.Item2));
+        }
+    }
+
+    private void EvaluatedItemsAction(BuildCheckDataContext<EvaluatedItemsCheckData> context)
+    {
+        // We want to avoid repeated checking of a same project (as it might be evaluated multiple times)
+        //  for this reason we use a hashset with already seen projects.
+        if (!_projectsSeen.Add(context.Data.ProjectFilePath))
+        {
+            return;
+        }
+
+        foreach (ItemData itemData in context.Data.EnumerateItemsOfType(ItemNames.reference))
+        {
+            string evaluatedReferencePath = itemData.EvaluatedInclude;
+            string referenceFullPath = BuildCheckUtilities.RootEvaluatedPath(evaluatedReferencePath, context.Data.ProjectFilePath);
+
+            _projectsPerReferencePath[referenceFullPath] = (context.Data.ProjectFilePath, evaluatedReferencePath);
+            string? projectReferencedViaOutput;
+            if (_projectsPerOutputPath.TryGetValue(referenceFullPath, out projectReferencedViaOutput))
+            {
+                context.ReportResult(BuildCheckResult.Create(
+                    SupportedRule,
+                    // Populating precise location tracked via https://github.com/orgs/dotnet/projects/373/views/1?pane=issue&itemId=58661732
+                    ElementLocation.EmptyLocation,
+                    Path.GetFileName(projectReferencedViaOutput),
+                    Path.GetFileName(context.Data.ProjectFilePath),
+                    evaluatedReferencePath));
+            }
+        }
+    }
+}

src/Build/BuildCheck/Checks/PropertiesUsageCheck.cs

@@ -3,12 +3,10 @@
 
 using System;
 using System.Collections.Generic;
-using System.Diagnostics;
 using System.Linq;
 using Microsoft.Build.BuildCheck.Infrastructure;
 using Microsoft.Build.Collections;
 using Microsoft.Build.Evaluation;
-using Microsoft.Build.Experimental.BuildCheck;
 using Microsoft.Build.Experimental.BuildCheck.Infrastructure;
 using Microsoft.Build.Shared;
 
@@ -19,17 +17,17 @@ internal class PropertiesUsageCheck : WorkerNodeCheck
     private static readonly CheckRule _usedBeforeInitializedRule = new CheckRule("BC0201", "PropertyUsedBeforeDeclared",
         ResourceUtilities.GetResourceString("BuildCheck_BC0201_Title")!,
         ResourceUtilities.GetResourceString("BuildCheck_BC0201_MessageFmt")!,
-        new CheckConfiguration() { Severity = CheckResultSeverity.Warning, EvaluationCheckScope = EvaluationCheckScope.ProjectFileOnly });
+        new CheckConfiguration() { RuleId = "BC0201", Severity = CheckResultSeverity.Warning, EvaluationCheckScope = EvaluationCheckScope.ProjectFileOnly });
 
     private static readonly CheckRule _initializedAfterUsedRule = new CheckRule("BC0202", "PropertyDeclaredAfterUsed",
         ResourceUtilities.GetResourceString("BuildCheck_BC0202_Title")!,
         ResourceUtilities.GetResourceString("BuildCheck_BC0202_MessageFmt")!,
-        new CheckConfiguration() { Severity = CheckResultSeverity.Warning, EvaluationCheckScope = EvaluationCheckScope.ProjectFileOnly });
+        new CheckConfiguration() { RuleId = "BC0202", Severity = CheckResultSeverity.Warning, EvaluationCheckScope = EvaluationCheckScope.ProjectFileOnly });
 
     private static readonly CheckRule _unusedPropertyRule = new CheckRule("BC0203", "UnusedPropertyDeclared",
         ResourceUtilities.GetResourceString("BuildCheck_BC0203_Title")!,
         ResourceUtilities.GetResourceString("BuildCheck_BC0203_MessageFmt")!,
-        new CheckConfiguration() { Severity = CheckResultSeverity.Suggestion, EvaluationCheckScope = EvaluationCheckScope.ProjectFileOnly });
+        new CheckConfiguration() { RuleId = "BC0203", Severity = CheckResultSeverity.Suggestion, EvaluationCheckScope = EvaluationCheckScope.ProjectFileOnly });
 
     internal static readonly IReadOnlyList<CheckRule> SupportedRulesList = [_usedBeforeInitializedRule, _initializedAfterUsedRule, _unusedPropertyRule];
 

src/Build/BuildCheck/Checks/SharedOutputPathCheck.cs

@@ -1,15 +1,11 @@
 // 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.Collections.Immutable;
-using System.Diagnostics;
 using System.IO;
-using Microsoft.Build.Experimental.BuildCheck.Infrastructure;
 using Microsoft.Build.Construction;
-using Microsoft.Build.Experimental.BuildCheck;
 using Microsoft.Build.Shared;
+using Microsoft.Build.Collections;
 
 namespace Microsoft.Build.Experimental.BuildCheck.Checks;
 
@@ -37,12 +33,14 @@ public override void RegisterActions(IBuildCheckRegistrationContext registration
 
     internal override bool IsBuiltIn => true;
 
-    private readonly Dictionary<string, string> _projectsPerOutputPath = new(StringComparer.CurrentCultureIgnoreCase);
-    private readonly HashSet<string> _projects = new(StringComparer.CurrentCultureIgnoreCase);
+    private readonly Dictionary<string, string> _projectsPerOutputPath = new(MSBuildNameIgnoreCaseComparer.Default);
+    private readonly HashSet<string> _projectsSeen = new(MSBuildNameIgnoreCaseComparer.Default);
 
     private void EvaluatedPropertiesAction(BuildCheckDataContext<EvaluatedPropertiesCheckData> context)
     {
-        if (!_projects.Add(context.Data.ProjectFilePath))
+        // We want to avoid repeated checking of a same project (as it might be evaluated multiple times)
+        //  for this reason we use a hashset with already seen projects
+        if (!_projectsSeen.Add(context.Data.ProjectFilePath))
         {
             return;
         }
@@ -56,8 +54,8 @@ private void EvaluatedPropertiesAction(BuildCheckDataContext<EvaluatedProperties
         // Check objPath only if it is different from binPath
         if (
             !string.IsNullOrEmpty(objPath) && !string.IsNullOrEmpty(absoluteBinPath) &&
-            !objPath.Equals(binPath, StringComparison.CurrentCultureIgnoreCase)
-            && !objPath.Equals(absoluteBinPath, StringComparison.CurrentCultureIgnoreCase)
+            !MSBuildNameIgnoreCaseComparer.Default.Equals(objPath, binPath)
+            && !MSBuildNameIgnoreCaseComparer.Default.Equals(objPath, absoluteBinPath)
         )
         {
             CheckAndAddFullOutputPath(objPath, context);
@@ -72,14 +70,7 @@ private void EvaluatedPropertiesAction(BuildCheckDataContext<EvaluatedProperties
         }
 
         string projectPath = context.Data.ProjectFilePath;
-
-        if (!Path.IsPathRooted(path))
-        {
-            path = Path.Combine(Path.GetDirectoryName(projectPath)!, path);
-        }
-
-        // Normalize the path to avoid false negatives due to different path representations.
-        path = FileUtilities.NormalizePath(path);
+        path = BuildCheckUtilities.RootEvaluatedPath(path!, projectPath);
 
         if (_projectsPerOutputPath.TryGetValue(path!, out string? conflictingProject))
         {

src/Build/BuildCheck/Infrastructure/BuildCheckConfigurationException.cs

@@ -2,10 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
-using System.Collections.Generic;
-using System.Linq;
-using System.Text;
-using System.Threading.Tasks;
 
 namespace Microsoft.Build.Experimental.BuildCheck.Infrastructure;
 

src/Build/BuildCheck/Infrastructure/BuildCheckConnectorLogger.cs

@@ -1,13 +1,6 @@
 // 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.Linq;
-using Microsoft.Build.BackEnd.Logging;
-using Microsoft.Build.Experimental.BuildCheck;
-using Microsoft.Build.Experimental.BuildCheck.Acquisition;
-using Microsoft.Build.Experimental.BuildCheck.Utilities;
 using Microsoft.Build.Framework;
 
 namespace Microsoft.Build.Experimental.BuildCheck.Infrastructure;