Reload project file before simplifying it in source update step (#616)

This updates SourceUpdaterStep to reload the csproj (to make sure any changes made by code fixes are picked up) prior to simplifying the file.

Fixes #553

src/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers/HttpContextCurrentAnalyzer.cs

@@ -21,9 +21,9 @@ namespace Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers
         private const string Category = "Upgrade";
 
         private const string TargetTypeSimpleName = "HttpContext";
-        private const string TargetTypeSymbolName = "System.Web.HttpContext";
         private const string TargetMember = "Current";
 
+        private static readonly string[] TargetTypeSymbolNames = new[] { "System.Web.HttpContext", "Microsoft.AspNetCore.Http.HttpContext" };
         private static readonly LocalizableString Title = new LocalizableResourceString(nameof(Resources.HttpContextCurrentTitle), Resources.ResourceManager, typeof(Resources));
         private static readonly LocalizableString MessageFormat = new LocalizableResourceString(nameof(Resources.HttpContextCurrentMessageFormat), Resources.ResourceManager, typeof(Resources));
         private static readonly LocalizableString Description = new LocalizableResourceString(nameof(Resources.HttpContextCurrentDescription), Resources.ResourceManager, typeof(Resources));
@@ -119,19 +119,25 @@ namespace Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers
         }
 
         /// <summary>
-        /// Attempts to match against a symbol if there. If a symbol is resolved, it must match exactly. Otherwise, we just match on name.
+        /// Attempts to match against an identifier's symbol against ASP.NET's HttpContext.
+        /// If a symbol is resolved, it must match System.Web.HttpContext or
+        /// Microsoft.AspNetCore.Http.HttpContext exactly.
         /// </summary>
         /// <param name="context">The analysis context.</param>
-        /// <param name="accessedIdentifier">The accessedIdentifier that was found</param>
-        /// <returns>Whether a symbol was found and was matched.</returns>
+        /// <param name="accessedIdentifier">The accessedIdentifier that was found.</param>
+        /// <returns>Returns true if the identifier's symbol matches ASP.NET's HttpContext or if no symbol was found.</returns>
         private static bool TryMatchSymbol(SyntaxNodeAnalysisContext context, SyntaxNode accessedIdentifier)
         {
-            // If the accessed identifier resolves to a type symbol other than System.Web.HttpContext, then bail out
-            // since it means the user is calling some other similarly named API.
+            // If the accessed identifier resolves to a type symbol other than System.Web.HttpContext or
+            // Microsoft.AspNetCore.Http.HttpContext, then bail out since it means the user is calling
+            // some other similarly named API. This allows diagnostics on Microsoft.AspNetCore.Http.HttpContext
+            // in addition to System.Web.HttpContext since ASP.NET Core's HttpContext doesn't have a Current
+            // property so any attempt to access such a property probably indicates a partially upgraded
+            // call site that needs remedied.
             var accessedSymbol = context.SemanticModel.GetSymbolInfo(accessedIdentifier).Symbol;
             if (accessedSymbol is INamedTypeSymbol symbol)
             {
-                if (!symbol.ToDisplayString(NullableFlowState.NotNull).Equals(TargetTypeSymbolName, StringComparison.Ordinal))
+                if (!TargetTypeSymbolNames.Any(name => symbol.ToDisplayString(NullableFlowState.NotNull).Equals(name, StringComparison.Ordinal)))
                 {
                     return false;
                 }

src/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers/TypeUpgradeAnalyzer.cs

@@ -114,12 +114,24 @@ namespace Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers
             }
 
             // If the identifier resolves to an actual symbol that isn't the old identifier, bail out
-            if (context.SemanticModel.GetSymbolInfo(context.Node).Symbol is INamedTypeSymbol symbol
+            // Also refrain from adding a diagnostic if the symbol is ambiguous but a candidate symbol matches the
+            // new type. This is useful in cases where the type is referenced as a simple name and the new namespace
+            // has been added but the old namespace hasn't been removed yet.
+            var symbolInfo = context.SemanticModel.GetSymbolInfo(context.Node);
+
+            // Bail out if the node corresponds to a symbol that isn't the old type.
+            if (symbolInfo.Symbol is INamedTypeSymbol symbol
                 && !symbol.ToDisplayString(NullableFlowState.NotNull).Equals(mapping.OldName, StringComparison.Ordinal))
             {
                 return;
             }
 
+            // Bail out if the symbol might correspond to a symbol that is the new type.
+            if (symbolInfo.CandidateSymbols.OfType<INamedTypeSymbol>().Any(s => s.ToDisplayString(NullableFlowState.NotNull).Equals(mapping.NewName, StringComparison.Ordinal)))
+            {
+                return;
+            }
+
             // If the identifier is part of a fully qualified name and the qualified name exactly matches the new full name,
             // then bail out because the code is likely fine and the symbol is just unavailable because of missing references.
             var fullyQualifiedNameNode = context.Node.GetQualifiedName();

src/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.CodeFixes/IdentifierUpgradeCodeFixer.cs

@@ -86,18 +86,21 @@ namespace Microsoft.DotNet.UpgradeAssistant.Extensions.Default.CodeFixes
 
             // Split the new idenfitier into namespace and name components
             var namespaceDelimiterIndex = newIdentifier.LastIndexOf('.');
-            var (qualifier, name) = namespaceDelimiterIndex >= 0
-                ? (newIdentifier.Substring(0, namespaceDelimiterIndex), newIdentifier.Substring(namespaceDelimiterIndex + 1))
-                : ((string?)null, newIdentifier);
+            var qualifier = namespaceDelimiterIndex >= 0
+                ? newIdentifier.Substring(0, namespaceDelimiterIndex)
+                : null;
 
             // Create new identifier
-            // Note that the simplifier does not recognize using statements within namespaces, so this
-            // checks whether the necessary using statement is present or not and constructs the new identifer
-            // as either a simple name or qualified name based on that.
-            var updatedNode = (qualifier is not null && node.HasAccessToNamespace(qualifier))
-                ? generator.IdentifierName(name)
-                : GetUpdatedNode(node, generator, newIdentifier)
-                .WithAdditionalAnnotations(Simplifier.Annotation, Simplifier.AddImportsAnnotation);
+            var updatedNode = GetUpdatedNode(node, generator, newIdentifier)
+                .WithAdditionalAnnotations(Simplifier.Annotation);
+
+            // The simplifier does not recognize using statements within namespaces, so this
+            // checks whether the necessary using statement is present or not and adds the
+            // AddImportsAnnotation only if it's absent.
+            if (qualifier is not null && !node.HasAccessToNamespace(qualifier))
+            {
+                updatedNode = updatedNode.WithAdditionalAnnotations(Simplifier.AddImportsAnnotation);
+            }
 
             // Preserve white space and comments
             updatedNode = updatedNode.WithTriviaFrom(node);
@@ -114,6 +117,9 @@ namespace Microsoft.DotNet.UpgradeAssistant.Extensions.Default.CodeFixes
             // Add using declaration if needed
             updatedDocument = await ImportAdder.AddImportsAsync(updatedDocument, Simplifier.AddImportsAnnotation, null, cancellationToken).ConfigureAwait(false);
 
+            // Simplify the call, if possible
+            updatedDocument = await Simplifier.ReduceAsync(updatedDocument, Simplifier.Annotation, null, cancellationToken).ConfigureAwait(false);
+
             return updatedDocument;
         }
 

src/steps/Microsoft.DotNet.UpgradeAssistant.Steps.Source/SourceUpdaterStep.cs

@@ -177,10 +177,12 @@ namespace Microsoft.DotNet.UpgradeAssistant.Steps.Source
                 throw new ArgumentNullException(nameof(context));
             }
 
+            // Reload the workspace in case code fixes modified the project file
+            await context.ReloadWorkspaceAsync(token).ConfigureAwait(false);
+
+            // Apply any necessary cleanup to the project file
             var file = context.CurrentProject.Required().GetFile();
-
             file.Simplify();
-
             await file.SaveAsync(token).ConfigureAwait(false);
 
             if (Diagnostics.Any())

tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/TestHelper.cs

@@ -124,7 +124,8 @@ namespace Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test
                 fixAttempts++;
                 diagnosticFixed = false;
                 project = solution.GetProject(projectId)!;
-                var diagnostics = await GetDiagnosticsFromProjectAsync(project, documentPath, diagnosticIds).ConfigureAwait(false);
+                var diagnostics = (await GetDiagnosticsFromProjectAsync(project, documentPath, diagnosticIds).ConfigureAwait(false))
+                    .OrderBy(d => d.Location.SourceSpan.Start);
 
                 foreach (var diagnostic in diagnostics)
                 {

tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/assets/TestClasses/ControllerUpgrade.Fixed.cs

@@ -5,7 +5,7 @@ using Microsoft.AspNetCore.Mvc;
 
 namespace TestProject.TestClasses
 {
-    public partial class ValuesController : ControllerBase
+    public partial class ValuesController : Microsoft.AspNetCore.Mvc.ControllerBase
     {
         // GET api/values
         public IEnumerable<string> GetValues()
@@ -23,7 +23,7 @@ namespace TestProject.TestClasses
         }
     }
 
-    public class MoviesController : ControllerBase
+    public class MoviesController : Microsoft.AspNetCore.Mvc.ControllerBase
     {
         // GET api/values
         public IEnumerable<string> GetValues()
@@ -39,14 +39,14 @@ namespace TestProject.TestClasses
 
     public partial class Controller2
     {
-        Controller Controller;
+        Microsoft.AspNetCore.Mvc.Controller Controller;
     }
 
-    public partial class Controller2 : Controller
+    public partial class Controller2 : Microsoft.AspNetCore.Mvc.Controller
     {
-        public Controller DoSomething(ControllerBase a)
+        public Controller DoSomething(Microsoft.AspNetCore.Mvc.ControllerBase a)
         {
-            var x = new List<Controller>();
+            var x = new List<Microsoft.AspNetCore.Mvc.Controller>();
             return new Controller2();
         }
     }

tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/assets/TestClasses/ControllerUpgrade.Fixed.vb

@@ -6,7 +6,7 @@ Imports Microsoft.AspNetCore.Mvc
 Namespace TestProject.TestClasses
 
     Public Class ValuesController2
-        Inherits ControllerBase
+        Inherits Microsoft.AspNetCore.Mvc.ControllerBase
 
         ' GET api/values
         Public Function GetValues() As IEnumerable(Of String)
@@ -17,7 +17,7 @@ Namespace TestProject.TestClasses
     End Class
 
     Public Class MoviesController2
-        Inherits ControllerBase
+        Inherits Microsoft.AspNetCore.Mvc.ControllerBase
 
         ' GET api/movies
         Public Function GetMovies() As IEnumerable(Of String)
@@ -46,7 +46,7 @@ Namespace TestProject.TestClasses
     End Class
 
     Public Class MoviesController3
-        Inherits Controller
+        Inherits Microsoft.AspNetCore.Mvc.Controller
 
         ' GET api/movies
         Public Function GetMovies() As IEnumerable(Of String)

tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/assets/TestClasses/UA0008.Fixed.cs

@@ -7,7 +7,7 @@ namespace TestProject.TestClasses
     {
         public IUrlHelper Method1(this IUrlHelper h)
         {
-            IUrlHelper x = h;
+            var x = h;
 
             h.ExtenstionMethod(new TestProject.MyNamespace.UrlHelper(), new UrlHelper());
 

tests/tool/Integration.Tests/IntegrationScenarios/AspNetSample/csharp/Upgraded/Views/Home/Index.cshtml

@@ -14,7 +14,7 @@
     <div class="col-md-4">
         <h2>Getting started</h2>
         @{ 
-            var x = new ActionResult();
+            var x = new Microsoft.AspNetCore.Mvc.ActionResult();
         }
         <p>
             ASP.NET MVC gives you a powerful, patterns-based way to build dynamic websites that