Random cleanup (#7173)

* Use Shouldly

* Delete ToolsVersion and xmlns

* new() without type

* Use $ to format string literals

* Clean up exceptions

* Inline variable declaration

* => for simple methods

* Add space

* PR comments

* Use named functions to clarify meaning of exceptions

* Use ContainsKey

Author: Forgind

src/Build.OM.UnitTests/Definition/Project_Tests.cs

@@ -2109,10 +2109,6 @@ public void BuildEvaluationUsesCustomLoggers()
             {
                 result = project.Build(new ILogger[] { mockLogger });
             }
-            catch
-            {
-                throw;
-            }
             finally
             {
                 project.ProjectCollection.UnregisterAllLoggers();

src/Build.UnitTests/BackEnd/AssemblyTaskFactory_Tests.cs

@@ -11,6 +11,7 @@
 using Microsoft.Build.Construction;
 using InvalidProjectFileException = Microsoft.Build.Exceptions.InvalidProjectFileException;
 using Xunit;
+using Shouldly;
 
 #nullable disable
 
@@ -257,8 +258,8 @@ public void VerifyGoodTaskInstantiation()
                     new AppDomainSetup(),
 #endif
                     false);
-                Assert.NotNull(createdTask);
-                Assert.False(createdTask is TaskHostTask);
+                createdTask.ShouldNotBeNull();
+                createdTask.ShouldNotBeOfType<TaskHostTask>();
             }
             finally
             {

src/Build.UnitTests/BackEnd/BuildManager_Tests.cs

@@ -1681,11 +1681,11 @@ public void CancelledBuildWithDelay40()
         [Fact]
         public void CancelledBuildInTaskHostWithDelay40()
         {
-            string contents = CleanupFileContents(@"
+            string contents = CleanupFileContents(@$"
 <Project xmlns='msbuildnamespace' ToolsVersion='msbuilddefaulttoolsversion'>
  <UsingTask TaskName='Microsoft.Build.Tasks.Exec' AssemblyName='Microsoft.Build.Tasks.Core, Version=msbuildassemblyversion, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' TaskFactory='TaskHostFactory' />
  <Target Name='test'>
-    <Exec Command='" + Helpers.GetSleepCommand(TimeSpan.FromSeconds(10)) + @"'/>
+    <Exec Command='{Helpers.GetSleepCommand(TimeSpan.FromSeconds(10))}'/>
     <Message Text='[errormessage]'/>
  </Target>
 </Project>

src/Build.UnitTests/BackEnd/TargetBuilder_Tests.cs

@@ -1601,19 +1601,10 @@ private ProjectInstance CreateTestProject(string projectBodyContents, string ini
                     File.Create("testProject.proj").Dispose();
                     break;
                 }
-                catch (Exception ex)
+                // If all the retries failed, fail with the actual problem instead of some difficult-to-understand issue later.
+                catch (Exception ex) when (retries < 4)
                 {
-                    if (retries < 4)
-                    {
-                        Console.WriteLine(ex.ToString());
-                    }
-                    else
-                    {
-                        // All the retries have failed. We will now fail with the
-                        // actual problem now instead of with some more difficult-to-understand
-                        // issue later.
-                        throw;
-                    }
+                    Console.WriteLine(ex.ToString());
                 }
             }
 

src/Build.UnitTests/BackEnd/TaskHostFactory_Tests.cs

@@ -29,7 +29,7 @@ public void TaskNodesDieAfterBuild()
     </Target>
 </Project>";
                 TransientTestFile project = env.CreateFile("testProject.csproj", pidTaskProject);
-                ProjectInstance projectInstance = new ProjectInstance(project.Path);
+                ProjectInstance projectInstance = new(project.Path);
                 projectInstance.Build().ShouldBeTrue();
                 string processId = projectInstance.GetPropertyValue("PID");
                 string.IsNullOrEmpty(processId).ShouldBeFalse();

src/Build.UnitTests/EscapingInProjects_Tests.cs

@@ -22,6 +22,7 @@
 using Xunit;
 using Xunit.Abstractions;
 using Microsoft.Build.Shared;
+using Shouldly;
 
 #nullable disable
 
@@ -128,11 +129,11 @@ public void SemicolonInPropertyPassedIntoStringParam_UsingTaskHost()
         [Fact]
         public void SemicolonInPropertyPassedIntoITaskItemParam()
         {
-            MockLogger logger = Helpers.BuildProjectWithNewOMExpectSuccess(String.Format(@"
+            MockLogger logger = Helpers.BuildProjectWithNewOMExpectSuccess(@$"
 
                 <Project ToolsVersion=`msbuilddefaulttoolsversion` xmlns=`http://schemas.microsoft.com/developer/msbuild/2003`>
 
-                    <UsingTask TaskName=`Microsoft.Build.UnitTests.EscapingInProjects_Tests.MyTestTask` AssemblyFile=`{0}` />
+                    <UsingTask TaskName=`Microsoft.Build.UnitTests.EscapingInProjects_Tests.MyTestTask` AssemblyFile=`{new Uri(Assembly.GetExecutingAssembly().EscapedCodeBase).LocalPath}` />
 
                     <PropertyGroup>
                         <MyPropertyWithSemicolons>abc %3b def %3b ghi</MyPropertyWithSemicolons>
@@ -144,7 +145,7 @@ public void SemicolonInPropertyPassedIntoITaskItemParam()
 
                 </Project>
 
-                ", new Uri(Assembly.GetExecutingAssembly().EscapedCodeBase).LocalPath),
+                ",
                 logger: new MockLogger(_output));
 
             logger.AssertLogContains("Received TaskItemParam: 123 abc ; def ; ghi 789");
@@ -717,14 +718,14 @@ public void EscapedWildcardsShouldNotBeExpanded()
         [Trait("Category", "mono-osx-failing")]
         public void EscapedWildcardsShouldNotBeExpanded_InTaskHost()
         {
-            MockLogger logger = new MockLogger();
+            MockLogger logger = new();
 
             try
             {
                 // Populate the project directory with three physical files on disk -- a.weirdo, b.weirdo, c.weirdo.
                 EscapingInProjectsHelper.CreateThreeWeirdoFiles();
                 Project project = ObjectModelHelpers.CreateInMemoryProject(@"
-                <Project ToolsVersion=`msbuilddefaulttoolsversion` xmlns=`http://schemas.microsoft.com/developer/msbuild/2003`>
+                <Project>
                     <UsingTask TaskName=`Message` AssemblyFile=`$(MSBuildToolsPath)\Microsoft.Build.Tasks.Core.dll` TaskFactory=`TaskHostFactory` />
 
                     <Target Name=`t`>
@@ -736,8 +737,7 @@ public void EscapedWildcardsShouldNotBeExpanded_InTaskHost()
                 </Project>
                 ");
 
-                bool success = project.Build(logger);
-                Assert.True(success); // "Build failed.  See test output (Attachments in Azure Pipelines) for details"
+                project.Build(logger).ShouldBeTrue("Build failed.  See test output (Attachments in Azure Pipelines) for details");
                 logger.AssertLogContains("[*]");
             }
             finally

src/Build/BackEnd/BuildManager/BuildManager.cs

@@ -1727,10 +1727,9 @@ void IssueBuildSubmissionToSchedulerImpl(BuildSubmission submission, bool allowM
                         HandleNewRequest(Scheduler.VirtualNode, blocker);
                     }
                 }
-                catch (Exception ex) when (!ExceptionHandling.IsCriticalException(ex))
+                catch (Exception ex) when (IsInvalidProjectOrIORelatedException(ex))
                 {
-                    var projectException = ex as InvalidProjectFileException;
-                    if (projectException != null)
+                    if (ex is InvalidProjectFileException projectException)
                     {
                         if (!projectException.HasBeenLogged)
                         {
@@ -1739,10 +1738,6 @@ void IssueBuildSubmissionToSchedulerImpl(BuildSubmission submission, bool allowM
                             projectException.HasBeenLogged = true;
                         }
                     }
-                    else if ((ex is BuildAbortedException) || ExceptionHandling.NotExpectedException(ex))
-                    {
-                        throw;
-                    }
 
                     lock (_syncLock)
                     {
@@ -1752,7 +1747,7 @@ void IssueBuildSubmissionToSchedulerImpl(BuildSubmission submission, bool allowM
                             _legacyThreadingData.MainThreadSubmissionId = -1;
                         }
 
-                        if (projectException == null)
+                        if (ex is not InvalidProjectFileException)
                         {
                             var buildEventContext = new BuildEventContext(submission.SubmissionId, 1, BuildEventContext.InvalidProjectInstanceId, BuildEventContext.InvalidProjectContextId, BuildEventContext.InvalidTargetId, BuildEventContext.InvalidTaskId);
                             ((IBuildComponentHost)this).LoggingService.LogFatalBuildError(buildEventContext, ex, new BuildEventFileInfo(submission.BuildRequestData.ProjectFullPath));
@@ -1772,6 +1767,11 @@ void IssueBuildSubmissionToSchedulerImpl(BuildSubmission submission, bool allowM
             }
         }
 
+        private bool IsInvalidProjectOrIORelatedException(Exception e)
+        {
+            return !ExceptionHandling.IsCriticalException(e) && !ExceptionHandling.NotExpectedException(e) && e is not BuildAbortedException;
+        }
+
         private void ExecuteGraphBuildScheduler(GraphBuildSubmission submission)
         {
             try
@@ -1855,15 +1855,15 @@ private void ExecuteGraphBuildScheduler(GraphBuildSubmission submission)
                         submission.SubmissionId,
                         new ReadOnlyDictionary<ProjectGraphNode, BuildResult>(resultsPerNode ?? new Dictionary<ProjectGraphNode, BuildResult>())));
             }
-            catch (Exception ex) when (!ExceptionHandling.IsCriticalException(ex))
+            catch (Exception ex) when (IsInvalidProjectOrIORelatedException(ex))
             {
                 GraphBuildResult result = null;
 
                 // ProjectGraph throws an aggregate exception with InvalidProjectFileException inside when evaluation fails
                 if (ex is AggregateException aggregateException && aggregateException.InnerExceptions.All(innerException => innerException is InvalidProjectFileException))
                 {
                     // Log each InvalidProjectFileException encountered during ProjectGraph creation
-                    foreach (var innerException in aggregateException.InnerExceptions)
+                    foreach (Exception innerException in aggregateException.InnerExceptions)
                     {
                         var projectException = (InvalidProjectFileException) innerException;
                         if (!projectException.HasBeenLogged)
@@ -1881,23 +1881,16 @@ private void ExecuteGraphBuildScheduler(GraphBuildSubmission submission)
                     BuildEventContext projectBuildEventContext = new BuildEventContext(submission.SubmissionId, 1, BuildEventContext.InvalidProjectInstanceId, BuildEventContext.InvalidProjectContextId, BuildEventContext.InvalidTargetId, BuildEventContext.InvalidTaskId);
                     ((IBuildComponentHost)this).LoggingService.LogInvalidProjectFileError(projectBuildEventContext, new InvalidProjectFileException(ex.Message, ex));
                 }
-                else if (ex is BuildAbortedException || ExceptionHandling.NotExpectedException(ex))
-                {
-                    throw;
-                }
                 else
                 {
                     // Arbitrarily just choose the first entry point project's path
-                    var projectFile = submission.BuildRequestData.ProjectGraph?.EntryPointNodes.First().ProjectInstance.FullPath
+                    string projectFile = submission.BuildRequestData.ProjectGraph?.EntryPointNodes.First().ProjectInstance.FullPath
                         ?? submission.BuildRequestData.ProjectGraphEntryPoints?.First().ProjectFile;
                     BuildEventContext buildEventContext = new BuildEventContext(submission.SubmissionId, 1, BuildEventContext.InvalidProjectInstanceId, BuildEventContext.InvalidProjectContextId, BuildEventContext.InvalidTargetId, BuildEventContext.InvalidTaskId);
                     ((IBuildComponentHost)this).LoggingService.LogFatalBuildError(buildEventContext, ex, new BuildEventFileInfo(projectFile));
                 }
 
-                if (result == null)
-                {
-                    result = new GraphBuildResult(submission.SubmissionId, ex);
-                }
+                result ??= new GraphBuildResult(submission.SubmissionId, ex);
 
                 ReportResultsToSubmission(result);
 

src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEntry.cs

@@ -232,8 +232,7 @@ public bool ResolveConfigurationRequest(int unresolvedConfigId, int configId)
         {
             lock (GlobalLock)
             {
-                List<BuildRequest> requests = null;
-                if (_unresolvedConfigurations?.TryGetValue(unresolvedConfigId, out requests) != true)
+                if (_unresolvedConfigurations?.TryGetValue(unresolvedConfigId, out List<BuildRequest> requests) != true)
                 {
                     return false;
                 }

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

@@ -120,8 +120,7 @@ public BuildRequestConfiguration GetMatchingConfiguration(ConfigurationMetadata
             ErrorUtilities.VerifyThrowArgumentNull(configMetadata, nameof(configMetadata));
             lock (_lockObject)
             {
-                int configId;
-                if (!_configurationIdsByMetadata.TryGetValue(configMetadata, out configId))
+                if (!_configurationIdsByMetadata.TryGetValue(configMetadata, out int configId))
                 {
                     return null;
                 }
@@ -216,10 +215,9 @@ public List<int> ClearNonExplicitlyLoadedConfigurations()
             {
                 foreach (KeyValuePair<ConfigurationMetadata, int> metadata in _configurationIdsByMetadata)
                 {
-                    BuildRequestConfiguration configuration;
                     int configId = metadata.Value;
 
-                    if (_configurations.TryGetValue(configId, out configuration))
+                    if (_configurations.TryGetValue(configId, out BuildRequestConfiguration configuration))
                     {
                         // We do not want to retain this configuration
                         if (!configuration.ExplicitlyLoaded)

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

@@ -224,8 +224,7 @@ public void ClearResultsForConfiguration(int configurationId)
         {
             lock (_resultsByConfiguration)
             {
-                BuildResult removedResult;
-                _resultsByConfiguration.TryRemove(configurationId, out removedResult);
+                _resultsByConfiguration.TryRemove(configurationId, out BuildResult removedResult);
 
                 removedResult?.ClearCachedFiles();
             }