Revert "revert changes for "ResultsCache ignores some of the BuildReq…

…uest data,.." (dotnet#9718)"

This reverts commit 553649b.

documentation/wiki/ChangeWaves.md

@@ -31,6 +31,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t
 - [Add Link metadata to Resources in AssignLinkMetadata target](https://github.com/dotnet/msbuild/pull/9464)
 - [Change Version switch output to finish with a newline](https://github.com/dotnet/msbuild/pull/9485)
 - [Load NuGet.Frameworks into secondary AppDomain (MSBuild.exe only)](https://github.com/dotnet/msbuild/pull/9446)
+- [ResultsCache ignores some of the BuildRequest data, may return incorrect results](https://github.com/dotnet/msbuild/pull/9565)
 - [Update Traits when environment has been changed](https://github.com/dotnet/msbuild/pull/9655)
 - [Exec task does not trim leading whitespaces for ConsoleOutput](https://github.com/dotnet/msbuild/pull/9722)
 - [Introduce [MSBuild]::StableStringHash overloads](https://github.com/dotnet/msbuild/issues/9519)

src/Build.UnitTests/BackEnd/ResultsCache_Tests.cs

@@ -188,6 +188,127 @@ public void TestRetrieveSubsetTargetsFromResult()
             Assert.Equal(BuildResultCode.Success, response.Results.OverallResult);
         }
 
+        [Fact]
+        public void TestCacheOnDifferentBuildFlagsPerRequest_ProvideProjectStateAfterBuild()
+        {
+            string targetName = "testTarget1";
+            int submissionId = 1;
+            int nodeRequestId = 0;
+            int configurationId = 1;
+
+            BuildRequest requestWithNoBuildDataFlags = new BuildRequest(
+               submissionId,
+               nodeRequestId,
+               configurationId,
+               new string[1] { targetName } /* escapedTargets */,
+               null /* hostServices */,
+               BuildEventContext.Invalid /* parentBuildEventContext */,
+               null /* parentRequest */,
+               BuildRequestDataFlags.None);
+
+            BuildRequest requestWithProjectStateFlag = new BuildRequest(
+               submissionId,
+               nodeRequestId,
+               configurationId,
+               new string[1] { targetName } /* escapedTargets */,
+               null /* hostServices */,
+               BuildEventContext.Invalid /* parentBuildEventContext */,
+               null /* parentRequest */,
+               BuildRequestDataFlags.ProvideProjectStateAfterBuild);
+
+            BuildRequest requestWithNoBuildDataFlags2 = new BuildRequest(
+               submissionId,
+               nodeRequestId,
+               configurationId,
+               new string[1] { targetName } /* escapedTargets */,
+               null /* hostServices */,
+               BuildEventContext.Invalid /* parentBuildEventContext */,
+               null /* parentRequest */,
+               BuildRequestDataFlags.None);
+
+            BuildResult resultForRequestWithNoBuildDataFlags = new(requestWithNoBuildDataFlags);
+            resultForRequestWithNoBuildDataFlags.AddResultsForTarget(targetName, BuildResultUtilities.GetEmptySucceedingTargetResult());
+            ResultsCache cache = new();
+            cache.AddResult(resultForRequestWithNoBuildDataFlags);
+
+            ResultsCacheResponse cacheResponseForRequestWithNoBuildDataFlags = cache.SatisfyRequest(
+               requestWithNoBuildDataFlags,
+               new List<string>(),
+               new List<string>(new string[] { targetName }),
+               skippedResultsDoNotCauseCacheMiss: false);
+
+            ResultsCacheResponse cachedResponseForProjectState = cache.SatisfyRequest(
+               requestWithProjectStateFlag,
+               new List<string>(),
+               new List<string>(new string[] { targetName }),
+               skippedResultsDoNotCauseCacheMiss: false);
+
+            ResultsCacheResponse cacheResponseForNoBuildDataFlags2 = cache.SatisfyRequest(
+               requestWithNoBuildDataFlags2,
+               new List<string>(),
+               new List<string>(new string[] { targetName }),
+               skippedResultsDoNotCauseCacheMiss: false);
+
+            Assert.Equal(ResultsCacheResponseType.Satisfied, cacheResponseForRequestWithNoBuildDataFlags.Type);
+
+            // Because ProvideProjectStateAfterBuildFlag was provided as a part of BuildRequest
+            Assert.Equal(ResultsCacheResponseType.NotSatisfied, cachedResponseForProjectState.Type);
+
+            Assert.Equal(ResultsCacheResponseType.Satisfied, cacheResponseForNoBuildDataFlags2.Type);
+        }
+
+        [Fact]
+        public void TestCacheOnDifferentBuildFlagsPerRequest_ProvideSubsetOfStateAfterBuild()
+        {
+            string targetName = "testTarget1";
+            int submissionId = 1;
+            int nodeRequestId = 0;
+            int configurationId = 1;
+
+            BuildRequest requestWithSubsetFlag1 = new BuildRequest(
+                submissionId,
+                nodeRequestId,
+                configurationId,
+                new string[1] { targetName } /* escapedTargets */,
+                null /* hostServices */,
+                BuildEventContext.Invalid /* parentBuildEventContext */,
+                null /* parentRequest */,
+                BuildRequestDataFlags.ProvideSubsetOfStateAfterBuild);
+
+            BuildRequest requestWithSubsetFlag2 = new BuildRequest(
+                submissionId,
+                nodeRequestId,
+                configurationId,
+                new string[1] { targetName } /* escapedTargets */,
+                null /* hostServices */,
+                BuildEventContext.Invalid /* parentBuildEventContext */,
+                null /* parentRequest */,
+                BuildRequestDataFlags.ProvideSubsetOfStateAfterBuild);
+
+            BuildResult resultForRequestWithSubsetFlag1 = new(requestWithSubsetFlag1);
+            resultForRequestWithSubsetFlag1.AddResultsForTarget(targetName, BuildResultUtilities.GetEmptySucceedingTargetResult());
+            ResultsCache cache = new();
+            cache.AddResult(resultForRequestWithSubsetFlag1);
+
+            ResultsCacheResponse cachedResponseWithSubsetFlag1 = cache.SatisfyRequest(
+            requestWithSubsetFlag1,
+            new List<string>(),
+            new List<string>(new string[] { targetName }),
+            skippedResultsDoNotCauseCacheMiss: false);
+
+            ResultsCacheResponse cachedResponseWithSubsetFlag2 = cache.SatisfyRequest(
+                requestWithSubsetFlag2,
+                new List<string>(),
+                new List<string>(new string[] { targetName }),
+                skippedResultsDoNotCauseCacheMiss: false);
+
+            // It was agreed not to return cache results if ProvideSubsetOfStateAfterBuild is passed,
+            // because RequestedProjectState may have different user filters defined.
+            // It is more reliable to ignore the cached value. 
+            Assert.Equal(ResultsCacheResponseType.NotSatisfied, cachedResponseWithSubsetFlag1.Type);
+            Assert.Equal(ResultsCacheResponseType.NotSatisfied, cachedResponseWithSubsetFlag2.Type);    
+        }
+
         [Fact]
         public void TestClearResultsCache()
         {

src/Build/BackEnd/Components/Caching/ResultsCache.cs

@@ -18,6 +18,15 @@ namespace Microsoft.Build.BackEnd
     /// </summary>
     internal class ResultsCache : IResultsCache
     {
+        /// <summary>
+        /// The presence of any of these flags affects build result for the specified request.
+        /// </summary>
+        private const BuildRequestDataFlags FlagsAffectingBuildResults =
+            BuildRequestDataFlags.ProvideProjectStateAfterBuild
+            | BuildRequestDataFlags.SkipNonexistentTargets
+            | BuildRequestDataFlags.IgnoreMissingEmptyAndInvalidImports
+            | BuildRequestDataFlags.FailOnUnresolvedSdk;
+
         /// <summary>
         /// The table of all build results.  This table is indexed by configuration id and
         /// contains BuildResult objects which have all of the target information.
@@ -140,10 +149,11 @@ public BuildResult GetResultsForConfiguration(int configurationId)
 
         /// <summary>
         /// Attempts to satisfy the request from the cache.  The request can be satisfied only if:
-        /// 1. All specified targets in the request have successful results in the cache or if the sequence of target results
+        /// 1. The passed BuildRequestDataFlags can not affect the result data.
+        /// 2. All specified targets in the request have successful results in the cache or if the sequence of target results
         ///    includes 0 or more successful targets followed by at least one failed target.
-        /// 2. All initial targets in the configuration for the request have non-skipped results in the cache.
-        /// 3. If there are no specified targets, then all default targets in the request must have non-skipped results
+        /// 3. All initial targets in the configuration for the request have non-skipped results in the cache.
+        /// 4. If there are no specified targets, then all default targets in the request must have non-skipped results
         ///    in the cache.
         /// </summary>
         /// <param name="request">The request whose results we should return.</param>
@@ -163,47 +173,53 @@ public ResultsCacheResponse SatisfyRequest(BuildRequest request, List<string> co
             {
                 if (_resultsByConfiguration.TryGetValue(request.ConfigurationId, out BuildResult allResults))
                 {
-                    // Check for targets explicitly specified.
-                    bool explicitTargetsSatisfied = CheckResults(allResults, request.Targets, response.ExplicitTargetsToBuild, skippedResultsDoNotCauseCacheMiss);
+                    bool buildDataFlagsSatisfied = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10)
+                        ? CheckBuildDataFlagsResults(request.BuildRequestDataFlags, allResults.BuildRequestDataFlags) : true;
 
-                    if (explicitTargetsSatisfied)
+                    if (buildDataFlagsSatisfied)
                     {
-                        // All of the explicit targets, if any, have been satisfied
-                        response.Type = ResultsCacheResponseType.Satisfied;
+                        // Check for targets explicitly specified.
+                        bool explicitTargetsSatisfied = CheckResults(allResults, request.Targets, response.ExplicitTargetsToBuild, skippedResultsDoNotCauseCacheMiss);
 
-                        // Check for the initial targets.  If we don't know what the initial targets are, we assume they are not satisfied.
-                        if (configInitialTargets == null || !CheckResults(allResults, configInitialTargets, null, skippedResultsDoNotCauseCacheMiss))
+                        if (explicitTargetsSatisfied)
                         {
-                            response.Type = ResultsCacheResponseType.NotSatisfied;
-                        }
+                            // All of the explicit targets, if any, have been satisfied
+                            response.Type = ResultsCacheResponseType.Satisfied;
 
-                        // We could still be missing implicit targets, so check those...
-                        if (request.Targets.Count == 0)
-                        {
-                            // Check for the default target, if necessary.  If we don't know what the default targets are, we
-                            // assume they are not satisfied.
-                            if (configDefaultTargets == null || !CheckResults(allResults, configDefaultTargets, null, skippedResultsDoNotCauseCacheMiss))
+                            // Check for the initial targets.  If we don't know what the initial targets are, we assume they are not satisfied.
+                            if (configInitialTargets == null || !CheckResults(allResults, configInitialTargets, null, skippedResultsDoNotCauseCacheMiss))
                             {
                                 response.Type = ResultsCacheResponseType.NotSatisfied;
                             }
-                        }
 
-                        // Now report those results requested, if they are satisfied.
-                        if (response.Type == ResultsCacheResponseType.Satisfied)
-                        {
-                            List<string> targetsToAddResultsFor = new List<string>(configInitialTargets);
-
-                            // Now report either the explicit targets or the default targets
-                            if (request.Targets.Count > 0)
+                            // We could still be missing implicit targets, so check those...
+                            if (request.Targets.Count == 0)
                             {
-                                targetsToAddResultsFor.AddRange(request.Targets);
+                                // Check for the default target, if necessary.  If we don't know what the default targets are, we
+                                // assume they are not satisfied.
+                                if (configDefaultTargets == null || !CheckResults(allResults, configDefaultTargets, null, skippedResultsDoNotCauseCacheMiss))
+                                {
+                                    response.Type = ResultsCacheResponseType.NotSatisfied;
+                                }
                             }
-                            else
+
+                            // Now report those results requested, if they are satisfied.
+                            if (response.Type == ResultsCacheResponseType.Satisfied)
                             {
-                                targetsToAddResultsFor.AddRange(configDefaultTargets);
+                                List<string> targetsToAddResultsFor = new List<string>(configInitialTargets);
+
+                                // Now report either the explicit targets or the default targets
+                                if (request.Targets.Count > 0)
+                                {
+                                    targetsToAddResultsFor.AddRange(request.Targets);
+                                }
+                                else
+                                {
+                                    targetsToAddResultsFor.AddRange(configDefaultTargets);
+                                }
+
+                                response.Results = new BuildResult(request, allResults, targetsToAddResultsFor.ToArray(), null);
                             }
-
-                            response.Results = new BuildResult(request, allResults, targetsToAddResultsFor.ToArray(), null);
                         }
                     }
                 }
@@ -328,6 +344,20 @@ private static bool CheckResults(BuildResult result, List<string> targets, HashS
             return returnValue;
         }
 
+        /// <summary>
+        /// Checks results for the specified build flags.
+        /// </summary>
+        /// <param name="buildRequestDataFlags">The current request build flags.</param>
+        /// <param name="buildResultDataFlags">The existing build result data flags.</param>
+        /// <returns>False if there is any difference in the data flags that can cause missed build data, true otherwise.</returns>
+        private static bool CheckBuildDataFlagsResults(BuildRequestDataFlags buildRequestDataFlags, BuildRequestDataFlags buildResultDataFlags) =>
+
+            // Even if both buildRequestDataFlags and buildResultDataFlags have ProvideSubsetOfStateAfterBuild flag,
+            // the underlying RequestedProjectState may have different user filters defined.
+            // It is more reliable to ignore the cached value. 
+            !buildRequestDataFlags.HasFlag(BuildRequestDataFlags.ProvideSubsetOfStateAfterBuild)
+            && (buildRequestDataFlags & FlagsAffectingBuildResults) == (buildResultDataFlags & FlagsAffectingBuildResults);
+
         public IEnumerator<BuildResult> GetEnumerator()
         {
             return _resultsByConfiguration.Values.GetEnumerator();

src/Build/BackEnd/Shared/BuildRequest.cs

@@ -119,6 +119,11 @@ private BuildRequest(
             _nodeRequestId = nodeRequestId;
             _buildRequestDataFlags = buildRequestDataFlags;
             _requestedProjectState = requestedProjectState;
+
+            if (_requestedProjectState != null)
+            {
+                _buildRequestDataFlags |= BuildRequestDataFlags.ProvideSubsetOfStateAfterBuild;
+            }
         }
 
         /// <summary>

src/Build/BackEnd/Shared/BuildResult.cs

@@ -116,6 +116,11 @@ public class BuildResult : INodePacket, IBuildResults
         /// </summary>
         private ProjectInstance _projectStateAfterBuild;
 
+        /// <summary>
+        /// The flags provide additional control over the build results and may affect the cached value.
+        /// </summary>
+        private BuildRequestDataFlags _buildRequestDataFlags;
+
         private string _schedulerInducedError;
 
         private HashSet<string> _projectTargets;
@@ -204,6 +209,7 @@ internal BuildResult(BuildRequest request, BuildResult existingResults, string[]
             _nodeRequestId = request.NodeRequestId;
             _circularDependency = false;
             _baseOverallResult = true;
+            _buildRequestDataFlags = request.BuildRequestDataFlags;
 
             if (existingResults == null)
             {
@@ -380,6 +386,12 @@ public ProjectInstance ProjectStateAfterBuild
             set => _projectStateAfterBuild = value;
         }
 
+        /// <summary>
+        /// Gets the flags that were used in the build request to which these results are associated.
+        /// See <see cref="Execution.BuildRequestDataFlags"/> for examples of the available flags.
+        /// </summary>
+        public BuildRequestDataFlags BuildRequestDataFlags => _buildRequestDataFlags;
+
         /// <summary>
         /// Returns the node packet type.
         /// </summary>
@@ -581,6 +593,7 @@ void ITranslatable.Translate(ITranslator translator)
             translator.Translate(ref _savedCurrentDirectory);
             translator.Translate(ref _schedulerInducedError);
             translator.TranslateDictionary(ref _savedEnvironmentVariables, StringComparer.OrdinalIgnoreCase);
+            translator.TranslateEnum(ref _buildRequestDataFlags, (int)_buildRequestDataFlags);
         }
 
         /// <summary>