From 991185f80d09fad3a386faf5ccf7f15e6298103b Mon Sep 17 00:00:00 2001 From: Mike Rousos Date: Mon, 14 Jun 2021 17:23:23 -0400 Subject: [PATCH] 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 --- .../HttpContextCurrentAnalyzer.cs | 20 +++++++++----- .../TypeUpgradeAnalyzer.cs | 14 +++++++++- .../IdentifierUpgradeCodeFixer.cs | 26 ++++++++++++------- .../SourceUpdaterStep.cs | 6 +++-- .../TestHelper.cs | 3 ++- .../TestClasses/ControllerUpgrade.Fixed.cs | 12 ++++----- .../TestClasses/ControllerUpgrade.Fixed.vb | 6 ++--- .../assets/TestClasses/UA0008.Fixed.cs | 2 +- .../csharp/Upgraded/Views/Home/Index.cshtml | 2 +- 9 files changed, 59 insertions(+), 32 deletions(-) diff --git a/src/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers/HttpContextCurrentAnalyzer.cs b/src/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers/HttpContextCurrentAnalyzer.cs index b3859516..32a16e6f 100644 --- a/src/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers/HttpContextCurrentAnalyzer.cs +++ b/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 } /// - /// 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. /// /// The analysis context. - /// The accessedIdentifier that was found - /// Whether a symbol was found and was matched. + /// The accessedIdentifier that was found. + /// Returns true if the identifier's symbol matches ASP.NET's HttpContext or if no symbol was found. 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; } diff --git a/src/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers/TypeUpgradeAnalyzer.cs b/src/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers/TypeUpgradeAnalyzer.cs index e7079abf..2c255f50 100644 --- a/src/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers/TypeUpgradeAnalyzer.cs +++ b/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().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(); diff --git a/src/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.CodeFixes/IdentifierUpgradeCodeFixer.cs b/src/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.CodeFixes/IdentifierUpgradeCodeFixer.cs index b1b58e69..ba413628 100644 --- a/src/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.CodeFixes/IdentifierUpgradeCodeFixer.cs +++ b/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; } diff --git a/src/steps/Microsoft.DotNet.UpgradeAssistant.Steps.Source/SourceUpdaterStep.cs b/src/steps/Microsoft.DotNet.UpgradeAssistant.Steps.Source/SourceUpdaterStep.cs index 7cacb4cd..dc623d11 100644 --- a/src/steps/Microsoft.DotNet.UpgradeAssistant.Steps.Source/SourceUpdaterStep.cs +++ b/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()) diff --git a/tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/TestHelper.cs b/tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/TestHelper.cs index 903fb512..d27e0615 100644 --- a/tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/TestHelper.cs +++ b/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) { diff --git a/tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/assets/TestClasses/ControllerUpgrade.Fixed.cs b/tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/assets/TestClasses/ControllerUpgrade.Fixed.cs index 4090714e..43828d8c 100644 --- a/tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/assets/TestClasses/ControllerUpgrade.Fixed.cs +++ b/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 GetValues() @@ -23,7 +23,7 @@ namespace TestProject.TestClasses } } - public class MoviesController : ControllerBase + public class MoviesController : Microsoft.AspNetCore.Mvc.ControllerBase { // GET api/values public IEnumerable 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(); + var x = new List(); return new Controller2(); } } diff --git a/tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/assets/TestClasses/ControllerUpgrade.Fixed.vb b/tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/assets/TestClasses/ControllerUpgrade.Fixed.vb index a2307e92..3ee2770c 100644 --- a/tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/assets/TestClasses/ControllerUpgrade.Fixed.vb +++ b/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) diff --git a/tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/assets/TestClasses/UA0008.Fixed.cs b/tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/assets/TestClasses/UA0008.Fixed.cs index c633a5b6..56a7d60c 100644 --- a/tests/extensions/default/analyzers/Microsoft.DotNet.UpgradeAssistant.Extensions.Default.Analyzers.Test/assets/TestClasses/UA0008.Fixed.cs +++ b/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()); diff --git a/tests/tool/Integration.Tests/IntegrationScenarios/AspNetSample/csharp/Upgraded/Views/Home/Index.cshtml b/tests/tool/Integration.Tests/IntegrationScenarios/AspNetSample/csharp/Upgraded/Views/Home/Index.cshtml index 2b24d38a..e5ad80fc 100644 --- a/tests/tool/Integration.Tests/IntegrationScenarios/AspNetSample/csharp/Upgraded/Views/Home/Index.cshtml +++ b/tests/tool/Integration.Tests/IntegrationScenarios/AspNetSample/csharp/Upgraded/Views/Home/Index.cshtml @@ -14,7 +14,7 @@

Getting started

@{ - var x = new ActionResult(); + var x = new Microsoft.AspNetCore.Mvc.ActionResult(); }

ASP.NET MVC gives you a powerful, patterns-based way to build dynamic websites that