AvoidCopyAlways Check

documentation/specs/BuildCheck/Codes.md

@@ -9,6 +9,7 @@ Report codes are chosen to conform to suggested guidelines. Those guidelines are
 | [BC0103](#bc0103---used-environment-variable) | Suggestion | Project | 9.0.100 | Used environment variable. |
 | [BC0104](#bc0104---projectreference-is-preferred-to-reference) | Warning | N/A | 9.0.200 | ProjectReference is preferred to Reference. |
 | [BC0105](#bc0105---embeddedresource-should-specify-culture-metadata) | Warning | N/A | 9.0.200 | Culture specific EmbeddedResource should specify Culture metadata. |
+| [BC0106](#bc0106---copytooutputdirectoryalways-should-be-avoided) | Warning | N/A | 9.0.200 | CopyToOutputDirectory='Always' should be avoided. |
 | [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. |
@@ -76,7 +77,36 @@ Examples:
 
 <a name="RespectAlreadyAssignedItemCulture"></a>
 **Note:** In Full Framework version of MSBuild (msbuild.exe, Visual Studio) and in .NET SDK prior 9.0 a global or project specific property `RespectAlreadyAssignedItemCulture` needs to be set to `'true'` in order for the explicit `Culture` metadata to be respected. Otherwise the explicit culture will be overwritten by MSBuild engine and if different from the extension - a `MSB3002` warning is emitted (`"MSB3002: Explicitly set culture "{0}" for item "{1}" was overwritten with inferred culture "{2}", because 'RespectAlreadyAssignedItemCulture' property was not set."`)
-
+
+<a name="BC0106"></a>
+## BC0106 - CopyToOutputDirectory='Always' should be avoided.
+
+"It is recommended to avoid specifying 'Always' value of metadata 'CopyToOutputDirectory' as this can lead to unnecessary build performance degradation. Use 'PreserveNewest' or 'IfDifferent' metadata value, or set the 'SkipUnchangedFilesOnCopyAlways' property to true to employ more effective copying."
+
+[`CopyToOutputDirectory` metadata](https://learn.microsoft.com/en-us/visualstudio/msbuild/common-msbuild-project-items) has following recognized values:
+ * `Never`
+ * `Always`
+ * `PreserveNewest`
+ * `IfDifferent`
+
+The `Always` is not recommended - as it causes the files to be copied always - even if unnecesary.
+
+This might have been historicaly needed to workaround cases where the destination file could have changed between builds (e.g. a case of asset that can be changed during test run, but needs to be rest by the build). A `IfDifferent` value is currently offered to efficiently cover such scenario.
+
+In order to avoid a need for change all copy metada, it's now possible to specify `SkipUnchangedFilesOnCopyAlways` property with a value of `'True'` in order to flip all copy behavior of `CopyToOutputDirectory=Always` to behave identicaly as `CopyToOutputDirectory=IfDifferent`:
+
+```xml
+<PropertyGroup>
+    <SkipUnchangedFilesOnCopyAlways>True</SkipUnchangedFilesOnCopyAlways>
+</PropertyGroup>
+
+<ItemGroup>
+    <None Include="File1.txt" CopyToOutputDirectory="Always" />
+    <None Include="File2.txt" CopyToOutputDirectory="IfDifferent" />
+</ItemGroup>
+```
+
+Both items in above example are treated same and no BC0106 diagnostic is issued.
 
 <a name="BC0201"></a>
 ## BC0201 - Usage of undefined property.

src/Build/BuildCheck/Checks/CopyAlwaysCheck.cs

@@ -0,0 +1,97 @@
+// 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 CopyAlwaysCheck : Check
+{
+    private const string RuleId = "BC0106";
+    public static CheckRule SupportedRule = new CheckRule(RuleId, "AvoidCopyAlways",
+        ResourceUtilities.GetResourceString("BuildCheck_BC0106_Title")!,
+        ResourceUtilities.GetResourceString("BuildCheck_BC0106_MessageFmt")!,
+        new CheckConfiguration() { RuleId = RuleId, Severity = CheckResultSeverity.Warning });
+
+    public override string FriendlyName => "MSBuild.CopyAlwaysCheck";
+
+    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 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;
+        }
+
+        context.Data.EvaluatedProperties.TryGetValue("SkipUnchangedFilesOnCopyAlways", out string? skipUnchanged);
+
+        if (skipUnchanged.IsMSBuildTrueString())
+        {
+            // Now we know that copy logic is optimized - so we do not need to check items. Avoiding the items check by inserting into lookup.
+            _projectsSeen.Add(context.Data.ProjectFilePath);
+        }
+    }
+
+    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.EnumerateItemsOfTypes([ItemNames.Content, ItemNames.Compile, ItemNames.None, ItemNames.EmbeddedResource]))
+        {
+            // itemData.Type
+            // itemData.EvaluatedInclude
+
+            foreach (KeyValuePair<string, string> keyValuePair in itemData.EnumerateMetadata())
+            {
+                if (MSBuildNameIgnoreCaseComparer.Default.Equals(keyValuePair.Key, ItemMetadataNames.copyToOutputDirectory))
+                {
+                    if (MSBuildNameIgnoreCaseComparer.Default.Equals(keyValuePair.Value, ItemMetadataNames.copyAlways))
+                    {
+                        // Project {0} specifies '{0}' item '{1}', ...
+                        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),
+                            itemData.Type,
+                            itemData.EvaluatedInclude));
+                    }
+
+                    break;
+                }
+            }
+        }
+    }
+}

src/Build/BuildCheck/Checks/EmbeddedResourceCheck.cs

@@ -43,7 +43,7 @@ private void EvaluatedItemsAction(BuildCheckDataContext<EvaluatedItemsCheckData>
             return;
         }
 
-        foreach (ItemData itemData in context.Data.EnumerateItemsOfType("EmbeddedResource"))
+        foreach (ItemData itemData in context.Data.EnumerateItemsOfType(ItemNames.EmbeddedResource))
         {
             string evaluatedEmbedItem = itemData.EvaluatedInclude;
             bool hasDoubleExtension = HasDoubleExtension(evaluatedEmbedItem);

src/Build/BuildCheck/Checks/PreferProjectReferenceCheck.cs

@@ -87,7 +87,7 @@ private void EvaluatedItemsAction(BuildCheckDataContext<EvaluatedItemsCheckData>
             return;
         }
 
-        foreach (ItemData itemData in context.Data.EnumerateItemsOfType(ItemNames.reference))
+        foreach (ItemData itemData in context.Data.EnumerateItemsOfType(ItemNames.Reference))
         {
             string evaluatedReferencePath = itemData.EvaluatedInclude;
             string referenceFullPath = BuildCheckUtilities.RootEvaluatedPath(evaluatedReferencePath, context.Data.ProjectFilePath);

src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs

@@ -147,6 +147,7 @@ internal readonly record struct BuiltInCheckFactory(
             [
                 new BuiltInCheckFactory([SharedOutputPathCheck.SupportedRule.Id], SharedOutputPathCheck.SupportedRule.DefaultConfiguration.IsEnabled ?? false, Construct<SharedOutputPathCheck>),
                 new BuiltInCheckFactory([PreferProjectReferenceCheck.SupportedRule.Id], PreferProjectReferenceCheck.SupportedRule.DefaultConfiguration.IsEnabled ?? false, Construct<PreferProjectReferenceCheck>),
+                new BuiltInCheckFactory([CopyAlwaysCheck.SupportedRule.Id], CopyAlwaysCheck.SupportedRule.DefaultConfiguration.IsEnabled ?? false, Construct<CopyAlwaysCheck>),
                 new BuiltInCheckFactory([DoubleWritesCheck.SupportedRule.Id], DoubleWritesCheck.SupportedRule.DefaultConfiguration.IsEnabled ?? false, Construct<DoubleWritesCheck>),
                 new BuiltInCheckFactory([NoEnvironmentVariablePropertyCheck.SupportedRule.Id], NoEnvironmentVariablePropertyCheck.SupportedRule.DefaultConfiguration.IsEnabled ?? false, Construct<NoEnvironmentVariablePropertyCheck>),
                 new BuiltInCheckFactory([EmbeddedResourceCheck.SupportedRule.Id], EmbeddedResourceCheck.SupportedRule.DefaultConfiguration.IsEnabled ?? false, Construct<EmbeddedResourceCheck>),

src/Build/BuildCheck/OM/ParsedItemsCheckData.cs

@@ -79,6 +79,11 @@ internal EvaluatedItemsCheckData(
     /// Lazy enumerates evaluated items for a current project. Only items with matching type will be returned (case-insensitive, MSBuild valid names only).
     /// </summary>
     public IEnumerable<ItemData> EnumerateItemsOfType(string typeName) => _evaluationFinishedEventArgs.EnumerateItemsOfType(typeName);
+
+    /// <summary>
+    /// Lazy enumerates evaluated items for a current project. Only items with matching type will be returned (case-insensitive, MSBuild valid names only, matching any type from the given list).
+    /// </summary>
+    public IEnumerable<ItemData> EnumerateItemsOfTypes(string[] typeNames) => _evaluationFinishedEventArgs.EnumerateItemsOfTypes(typeNames);
 }
 
 /// <summary>

src/Build/Logging/BuildEventArgsExtensions.cs

@@ -45,6 +45,15 @@ public static IEnumerable<ItemData> EnumerateItemsOfType(
         this ProjectEvaluationFinishedEventArgs eventArgs, string typeName)
         => EnumerateItemsOfType(eventArgs.Items, typeName);
 
+    /// <summary>
+    /// Lazy enumerates and partially strong types items from Items property. Only items with any matching type will be returned (case-insensitive, MSBuild valid names only).
+    /// The actual item value is proxied via accessor methods - to be able to provide defined interface
+    /// </summary>
+    /// <returns></returns>
+    public static IEnumerable<ItemData> EnumerateItemsOfTypes(
+        this ProjectEvaluationFinishedEventArgs eventArgs, string[] typeNames)
+        => EnumerateItemsOfTypes(eventArgs.Items, typeNames);
+
     /// <summary>
     /// Lazy enumerates and strong types items from Items property.
     /// The actual item value is proxied via accessor methods - to be able to provide defined interface
@@ -62,6 +71,15 @@ public static IEnumerable<ItemData> EnumerateItemsOfType(
         this ProjectStartedEventArgs eventArgs, string typeName)
         => EnumerateItemsOfType(eventArgs.Items, typeName);
 
+    /// <summary>
+    /// Lazy enumerates and partially strong types items from Items property. Only items with any matching type will be returned (case-insensitive, MSBuild valid names only).
+    /// The actual item value is proxied via accessor methods - to be able to provide defined interface
+    /// </summary>
+    /// <returns></returns>
+    public static IEnumerable<ItemData> EnumerateItemsOfTypes(
+        this ProjectStartedEventArgs eventArgs, string[] typeNames)
+        => EnumerateItemsOfTypes(eventArgs.Items, typeNames);
+
     private static IEnumerable<PropertyData> EnumerateProperties(IEnumerable? properties)
         => Internal.Utilities.EnumerateProperties(properties);
 
@@ -70,4 +88,7 @@ private static IEnumerable<ItemData> EnumerateItems(IEnumerable? items)
 
     private static IEnumerable<ItemData> EnumerateItemsOfType(IEnumerable? items, string typeName)
         => Internal.Utilities.EnumerateItemsOfType(items, typeName);
+
+    private static IEnumerable<ItemData> EnumerateItemsOfTypes(IEnumerable? items, string[] typeNames)
+        => Internal.Utilities.EnumerateItemsOfTypes(items, typeNames);
 }

src/Build/Resources/Strings.resx

@@ -2176,11 +2176,19 @@ Utilization:          {0} Average Utilization: {1:###.0}</value>
   </data>
   <data name="BuildCheck_BC0105_Title" xml:space="preserve">
     <value>It is recommended to specify explicit 'Culture' metadata, or 'WithCulture=false' metadata with 'EmbeddedResource' item in order to avoid wrong or nondeterministic culture estimation.</value>
-	<comment>Terms in quotes are not to be translated.</comment>
+	  <comment>Terms in quotes are not to be translated.</comment>
   </data>
   <data name="BuildCheck_BC0105_MessageFmt" xml:space="preserve">
     <value>Project {0} specifies 'EmbeddedResource' item '{1}', that has possibly a culture denoting extension ('{2}'), but explicit 'Culture' nor 'WithCulture=false' metadata are not specified.</value>
-	<comment>Terms in quotes are not to be translated.</comment>
+	  <comment>Terms in quotes are not to be translated.</comment>
+  </data>
+  <data name="BuildCheck_BC0106_Title" xml:space="preserve">
+    <value>It is recommended to avoid specifying 'Always' value of metadata 'CopyToOutputDirectory' as this can lead to unnecessary build performance degradation. Use 'PreserveNewest' or 'IfDifferent' metadata value, or set the 'SkipUnchangedFilesOnCopyAlways' property to true to employ more effective copying.</value>
+	  <comment>Terms in quotes are not to be translated.</comment>
+  </data>
+  <data name="BuildCheck_BC0106_MessageFmt" xml:space="preserve">
+    <value>Project {0} specifies '{1}' item '{2}', that has 'CopyToOutputDirectory' set as 'Always'. Change the metadata or use 'CopyToOutputDirectory' property.</value>
+	  <comment>Terms in quotes are not to be translated.</comment>
   </data>
   <data name="BuildCheck_BC0201_Title" xml:space="preserve">
     <value>A property that is accessed should be declared first.</value>