fix review comments

src/Build/BackEnd/Shared/BuildRequestConfiguration.cs

@@ -699,9 +699,9 @@ public void CacheIfPossible()
                     if (IsCacheable)
                     {
                         string cacheFile = GetCacheFile();
-                        Directory.CreateDirectory(Path.GetDirectoryName(cacheFile));
                         try
                         {
+                            Directory.CreateDirectory(Path.GetDirectoryName(cacheFile));
                             using Stream stream = File.Create(cacheFile);
                             using ITranslator translator = GetConfigurationTranslator(TranslationDirection.WriteToStream, stream);
 

src/Build/BackEnd/Shared/TargetResult.cs

@@ -252,7 +252,7 @@ internal void CacheItems(int configId, string targetName)
                 if (!FileSystems.Default.FileExists(cacheFile))
                 {
                     using Stream stream = File.Create(cacheFile);
-                    using ITranslator translator = GetResultsCacheTranslator(TranslationDirection.WriteToStream, stream, cacheFile);
+                    using ITranslator translator = GetResultsCacheTranslator(TranslationDirection.WriteToStream, stream);
 
                     // If the translator is null, it means these results were cached once before.  Since target results are immutable once they
                     // have been created, there is no point in writing them again.
@@ -289,7 +289,7 @@ private void RetrieveItemsFromCache()
                 {
                     string cacheFile = GetCacheFile(_cacheInfo.ConfigId, _cacheInfo.TargetName);
                     using Stream stream = File.OpenRead(cacheFile);
-                    using ITranslator translator = GetResultsCacheTranslator(TranslationDirection.ReadFromStream, stream, cacheFile);
+                    using ITranslator translator = GetResultsCacheTranslator(TranslationDirection.ReadFromStream, stream);
 
                     TranslateItems(translator);
                     _cacheInfo = new CacheInfo();
@@ -349,26 +349,10 @@ private void TranslateItems(ITranslator translator)
         /// <summary>
         /// Gets the translator for this configuration.
         /// </summary>
-        private static ITranslator GetResultsCacheTranslator(
-            TranslationDirection direction,
-            Stream stream,
-            string cacheFile)
-        {
-            if (direction == TranslationDirection.WriteToStream)
-            {
-                if (FileSystems.Default.FileExists(cacheFile))
-                {
-                    // If the file already exists, then we have cached this once before.  No need to cache it again since it cannot have changed.
-                    return null;
-                }
-
-                return BinaryTranslator.GetWriteTranslator(stream);
-            }
-            else
-            {
-                return BinaryTranslator.GetReadTranslator(stream, InterningBinaryReader.PoolingBuffer);
-            }
-        }
+        private static ITranslator GetResultsCacheTranslator(TranslationDirection direction, Stream stream) =>
+            direction == TranslationDirection.WriteToStream
+                    ? BinaryTranslator.GetWriteTranslator(stream)
+                    : BinaryTranslator.GetReadTranslator(stream, InterningBinaryReader.PoolingBuffer);
 
         /// <summary>
         /// Information about where the cache for the items in this result are stored.

src/Framework/NativeMethods.cs

@@ -1210,40 +1210,32 @@ internal static void KillTree(int processIdToKill)
 
                 try
                 {
-                    try
-                    {
-                        // Kill this process, so that no further children can be created.
-                        thisProcess.Kill();
-                    }
-                    catch (Win32Exception e) when (e.NativeErrorCode == ERROR_ACCESS_DENIED)
-                    {
-                        // Access denied is potentially expected -- it happens when the process that
-                        // we're attempting to kill is already dead.  So just ignore in that case.
-                    }
+                    // Kill this process, so that no further children can be created.
+                    thisProcess.Kill();
+                }
+                catch (Win32Exception e) when (e.NativeErrorCode == ERROR_ACCESS_DENIED)
+                {
+                    // Access denied is potentially expected -- it happens when the process that
+                    // we're attempting to kill is already dead.  So just ignore in that case.
+                }
 
-                    // Now enumerate our children.  Children of this process are any process which has this process id as its parent
-                    // and which also started after this process did.
-                    List<KeyValuePair<int, SafeProcessHandle>> children = GetChildProcessIds(processIdToKill, myStartTime);
+                // Now enumerate our children.  Children of this process are any process which has this process id as its parent
+                // and which also started after this process did.
+                List<KeyValuePair<int, SafeProcessHandle>> children = GetChildProcessIds(processIdToKill, myStartTime);
 
-                    try
-                    {
-                        foreach (KeyValuePair<int, SafeProcessHandle> childProcessInfo in children)
-                        {
-                            KillTree(childProcessInfo.Key);
-                        }
-                    }
-                    finally
+                try
+                {
+                    foreach (KeyValuePair<int, SafeProcessHandle> childProcessInfo in children)
                     {
-                        foreach (KeyValuePair<int, SafeProcessHandle> childProcessInfo in children)
-                        {
-                            childProcessInfo.Value.Dispose();
-                        }
+                        KillTree(childProcessInfo.Key);
                     }
                 }
                 finally
                 {
-                    // Release the handle.  After this point no more children of this process exist and this process has also exited.
-                    hProcess.Dispose();
+                    foreach (KeyValuePair<int, SafeProcessHandle> childProcessInfo in children)
+                    {
+                        childProcessInfo.Value.Dispose();
+                    }
                 }
             }
         }

src/MSBuild/XMake.cs

@@ -34,8 +34,6 @@
 using Microsoft.Build.Shared;
 using Microsoft.Build.Shared.Debugging;
 using Microsoft.Build.Shared.FileSystem;
-using Microsoft.Build.Utilities;
-using static Microsoft.Build.CommandLine.MSBuildApp;
 using BinaryLogger = Microsoft.Build.Logging.BinaryLogger;
 using ConsoleLogger = Microsoft.Build.Logging.ConsoleLogger;
 using FileLogger = Microsoft.Build.Logging.FileLogger;
@@ -820,33 +818,33 @@ public static ExitType Execute(
                     }
                     else if ((getProperty.Length > 0 || getItem.Length > 0) && (targets is null || targets.Length == 0))
                     {
-                        TextWriter output = null;
                         try
                         {
                             using (ProjectCollection collection = new(globalProperties, loggers, ToolsetDefinitionLocations.Default))
                             {
                                 Project project = collection.LoadProject(projectFile, globalProperties, toolsVersion);
-#pragma warning disable CA2000 // Dispose objects before losing scope is suppressed because the StreamWriter is disposed in the finally block
-                                output = getResultOutputFile.Length > 0
-                                    ? new StreamWriter(getResultOutputFile)
-                                    : Console.Out;
-#pragma warning restore CA2000 // Dispose objects before losing scope
-                                exitType = OutputPropertiesAfterEvaluation(getProperty, getItem, project, output);
+
+                                if (outputPropertiesItemsOrTargetResults && targets?.Length > 0 && result is not null)
+                                {
+                                    if (getResultOutputFile.Length == 0)
+                                    {
+                                        exitType = OutputPropertiesAfterEvaluation(getProperty, getItem, project, Console.Out);
+                                    }
+                                    else
+                                    {
+                                        using (var streamWriter = new StreamWriter(getResultOutputFile))
+                                        {
+                                            exitType = OutputPropertiesAfterEvaluation(getProperty, getItem, project, streamWriter);
+                                        }
+                                    }
+                                }
                                 collection.LogBuildFinishedEvent(exitType == ExitType.Success);
                             }
                         }
                         catch (InvalidProjectFileException)
                         {
                             exitType = ExitType.BuildError;
                         }
-                        finally
-                        {
-                            if (output is StreamWriter)
-                            {
-                                // dispose only if StreamWriter to avoid closing Console.Out
-                                output?.Dispose();
-                            }
-                        }
                     }
                     else // regular build
                     {
@@ -899,23 +897,18 @@ public static ExitType Execute(
 
                     string timerOutputFilename = Environment.GetEnvironmentVariable("MSBUILDTIMEROUTPUTS");
 
-                    TextWriter outputStream = null;
-                    try
+                    if (outputPropertiesItemsOrTargetResults && targets?.Length > 0 && result is not null)
                     {
-                        if (outputPropertiesItemsOrTargetResults && targets?.Length > 0 && result is not null)
+                        if (getResultOutputFile.Length == 0)
                         {
-#pragma warning disable CA2000 // Dispose objects before losing scope is suppressed because the StreamWriter is disposed in the finally block
-                            outputStream = getResultOutputFile.Length > 0 ? new StreamWriter(getResultOutputFile) : Console.Out;
-#pragma warning restore CA2000 // Dispose objects before losing scope
-                            exitType = OutputBuildInformationInJson(result, getProperty, getItem, getTargetResult, loggers, exitType, outputStream);
+                            exitType = OutputBuildInformationInJson(result, getProperty, getItem, getTargetResult, loggers, exitType, Console.Out);
                         }
-                    }
-                    finally
-                    {
-                        if (outputStream is StreamWriter)
+                        else
                         {
-                            // dispose only if StreamWriter to avoid closing Console.Out
-                            outputStream?.Dispose();
+                            using (var streamWriter = new StreamWriter(getResultOutputFile))
+                            {
+                                exitType = OutputBuildInformationInJson(result, getProperty, getItem, getTargetResult, loggers, exitType, streamWriter);
+                            }
                         }
                     }
 

src/Tasks/ManifestUtil/SecurityUtil.cs

@@ -205,9 +205,7 @@ private static XmlElement GetXmlElement(string targetZone, FrameworkName fn)
                         {
                             try
                             {
-#pragma warning disable CA2000 // Dispose objects before losing scope is suppressed because it is managed by FileStream above.
-                                var sr = new StreamReader(fs);
-#pragma warning restore CA2000 // Dispose objects before losing scope
+                                using var sr = new StreamReader(fs, Encoding.UTF8, detectEncodingFromByteOrderMarks: true, 1024, leaveOpen: true);
                                 string data = sr.ReadToEnd();
                                 if (!string.IsNullOrEmpty(data))
                                 {

src/Tasks/ManifestUtil/Util.cs

@@ -521,9 +521,7 @@ public static void WriteLogFile(string filename, Stream s)
             }
 
             string path = Path.Combine(logPath, filename);
-#pragma warning disable CA2000 // Dispose objects before losing scope is suppressed because the stream is returned to the caller and will be handled there.
-            StreamReader r = new StreamReader(s);
-#pragma warning restore CA2000 // Dispose objects before losing scope
+            using var r = new StreamReader(s, Encoding.UTF8, detectEncodingFromByteOrderMarks: true, 1024, leaveOpen: true);
             string text = r.ReadToEnd();
             try
             {