revert changes for "ResultsCache ignores some of the BuildRequest data,.." (dotnet#9718)

YuliiaKovalova · web-flow · commit 553649bd6d87 · 2024-02-08T10:52:43.000+01:00
diff --git a/documentation/wiki/ChangeWaves.md b/documentation/wiki/ChangeWaves.md
@@ -32,7 +32,6 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t
 - [Change Version switch output to finish with a newline](https://github.com/dotnet/msbuild/pull/9485)
 - [Load Microsoft.DotNet.MSBuildSdkResolver into default load context (MSBuild.exe only)](https://github.com/dotnet/msbuild/pull/9439)
 - [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)
 
 
diff --git a/src/Build.UnitTests/BackEnd/ResultsCache_Tests.cs b/src/Build.UnitTests/BackEnd/ResultsCache_Tests.cs
@@ -188,127 +188,6 @@ 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()
         {
diff --git a/src/Build/BackEnd/Components/Caching/ResultsCache.cs b/src/Build/BackEnd/Components/Caching/ResultsCache.cs
@@ -18,15 +18,6 @@ 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.
@@ -149,11 +140,10 @@ public BuildResult GetResultsForConfiguration(int configurationId)
 
         /// <summary>
         /// Attempts to satisfy the request from the cache.  The request can be satisfied only if:
-        /// 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
+        /// 1. 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.
-        /// 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
+        /// 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
         ///    in the cache.
         /// </summary>
         /// <param name="request">The request whose results we should return.</param>
@@ -173,53 +163,47 @@ public ResultsCacheResponse SatisfyRequest(BuildRequest request, List<string> co
             {
                 if (_resultsByConfiguration.TryGetValue(request.ConfigurationId, out BuildResult allResults))
                 {
-                    bool buildDataFlagsSatisfied = ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_10)
-                        ? CheckBuildDataFlagsResults(request.BuildRequestDataFlags, allResults.BuildRequestDataFlags) : true;
+                    // Check for targets explicitly specified.
+                    bool explicitTargetsSatisfied = CheckResults(allResults, request.Targets, response.ExplicitTargetsToBuild, skippedResultsDoNotCauseCacheMiss);
 
-                    if (buildDataFlagsSatisfied)
+                    if (explicitTargetsSatisfied)
                     {
-                        // Check for targets explicitly specified.
-                        bool explicitTargetsSatisfied = CheckResults(allResults, request.Targets, response.ExplicitTargetsToBuild, skippedResultsDoNotCauseCacheMiss);
+                        // All of the explicit targets, if any, have been satisfied
+                        response.Type = ResultsCacheResponseType.Satisfied;
 
-                        if (explicitTargetsSatisfied)
+                        // 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))
                         {
-                            // All of the explicit targets, if any, have been satisfied
-                            response.Type = ResultsCacheResponseType.Satisfied;
+                            response.Type = ResultsCacheResponseType.NotSatisfied;
+                        }
 
-                            // 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))
+                        // 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))
                             {
                                 response.Type = ResultsCacheResponseType.NotSatisfied;
                             }
+                        }
 
-                            // We could still be missing implicit targets, so check those...
-                            if (request.Targets.Count == 0)
+                        // 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)
                             {
-                                // 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;
-                                }
+                                targetsToAddResultsFor.AddRange(request.Targets);
                             }
-
-                            // Now report those results requested, if they are satisfied.
-                            if (response.Type == ResultsCacheResponseType.Satisfied)
+                            else
                             {
-                                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);
+                                targetsToAddResultsFor.AddRange(configDefaultTargets);
                             }
+
+                            response.Results = new BuildResult(request, allResults, targetsToAddResultsFor.ToArray(), null);
                         }
                     }
                 }
@@ -344,20 +328,6 @@ 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();
diff --git a/src/Build/BackEnd/Shared/BuildRequest.cs b/src/Build/BackEnd/Shared/BuildRequest.cs
@@ -119,11 +119,6 @@ private BuildRequest(
             _nodeRequestId = nodeRequestId;
             _buildRequestDataFlags = buildRequestDataFlags;
             _requestedProjectState = requestedProjectState;
-
-            if (_requestedProjectState != null)
-            {
-                _buildRequestDataFlags |= BuildRequestDataFlags.ProvideSubsetOfStateAfterBuild;
-            }
         }
 
         /// <summary>
diff --git a/src/Build/BackEnd/Shared/BuildResult.cs b/src/Build/BackEnd/Shared/BuildResult.cs
@@ -116,11 +116,6 @@ 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;
@@ -209,7 +204,6 @@ internal BuildResult(BuildRequest request, BuildResult existingResults, string[]
             _nodeRequestId = request.NodeRequestId;
             _circularDependency = false;
             _baseOverallResult = true;
-            _buildRequestDataFlags = request.BuildRequestDataFlags;
 
             if (existingResults == null)
             {
@@ -386,12 +380,6 @@ 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>
@@ -593,7 +581,6 @@ 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>
