diff --git a/src/EditorFeatures/Core/Implementation/InlineRename/AbstractEditorInlineRenameService.InlineRenameLocationSet.cs b/src/EditorFeatures/Core/Implementation/InlineRename/AbstractEditorInlineRenameService.InlineRenameLocationSet.cs index 508c19a90d7c8a3d17b42ef997f8b4640905c571..b2a2b9d0e1593c1b81e4b2c4d4addd5a2eed8a1b 100644 --- a/src/EditorFeatures/Core/Implementation/InlineRename/AbstractEditorInlineRenameService.InlineRenameLocationSet.cs +++ b/src/EditorFeatures/Core/Implementation/InlineRename/AbstractEditorInlineRenameService.InlineRenameLocationSet.cs @@ -1,6 +1,6 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System.Collections.Generic; +using System.Collections.Immutable; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -17,13 +17,15 @@ private class InlineRenameLocationSet : IInlineRenameLocationSet private readonly RenameLocations _renameLocationSet; private readonly SymbolInlineRenameInfo _renameInfo; - public IList Locations { get; } + public ImmutableArray Locations { get; } public InlineRenameLocationSet(SymbolInlineRenameInfo renameInfo, RenameLocations renameLocationSet) { _renameInfo = renameInfo; _renameLocationSet = renameLocationSet; - this.Locations = renameLocationSet.Locations.Where(l => !l.IsCandidateLocation || l.IsMethodGroupReference).Select(ConvertLocation).ToList(); + this.Locations = renameLocationSet.Locations.Where(RenameLocation.ShouldRename) + .Select(ConvertLocation) + .ToImmutableArray(); } private InlineRenameLocation ConvertLocation(RenameLocation location) @@ -42,4 +44,4 @@ public async Task GetReplacementsAsync(string repl } } } -} +} \ No newline at end of file diff --git a/src/EditorFeatures/Core/Implementation/InlineRename/Dashboard/DashboardViewModel.cs b/src/EditorFeatures/Core/Implementation/InlineRename/Dashboard/DashboardViewModel.cs index 714f2ac14526293f28a5330dbcd10d1790219b02..413c8b06722db81d33aafe0676ab0565b26ad041 100644 --- a/src/EditorFeatures/Core/Implementation/InlineRename/Dashboard/DashboardViewModel.cs +++ b/src/EditorFeatures/Core/Implementation/InlineRename/Dashboard/DashboardViewModel.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; +using System.Collections.Immutable; using System.ComponentModel; using System.Linq; using System.Runtime.CompilerServices; @@ -49,10 +50,10 @@ public DashboardViewModel(InlineRenameSession session) public event PropertyChangedEventHandler PropertyChanged; - private void OnReferenceLocationsChanged(object sender, IList renameLocations) + private void OnReferenceLocationsChanged(object sender, ImmutableArray renameLocations) { int totalFilesCount = renameLocations.GroupBy(s => s.Document).Count(); - int totalSpansCount = renameLocations.Count; + int totalSpansCount = renameLocations.Length; UpdateSearchText(totalSpansCount, totalFilesCount); } diff --git a/src/EditorFeatures/Core/Implementation/InlineRename/IEditorInlineRenameService.cs b/src/EditorFeatures/Core/Implementation/InlineRename/IEditorInlineRenameService.cs index 0914bc6b8680649090cd94b75a7bdaae91045736..3d15ffded6cc1649df3f73995a9a6cdcc2f8a0ec 100644 --- a/src/EditorFeatures/Core/Implementation/InlineRename/IEditorInlineRenameService.cs +++ b/src/EditorFeatures/Core/Implementation/InlineRename/IEditorInlineRenameService.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Collections.Generic; +using System.Collections.Immutable; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -110,7 +111,7 @@ internal interface IInlineRenameLocationSet /// has entered in the inline rename session. These are the locations are all relative /// to the solution when the inline rename session began. /// - IList Locations { get; } + ImmutableArray Locations { get; } /// /// Returns the set of replacements and their possible resolutions if the user enters the diff --git a/src/EditorFeatures/Core/Implementation/InlineRename/InlineRenameSession.cs b/src/EditorFeatures/Core/Implementation/InlineRename/InlineRenameSession.cs index 69d11e978eea481a275d8948139fd6b932078db8..8f989438db5bfca82bdbf1dff047db81793c61c5 100644 --- a/src/EditorFeatures/Core/Implementation/InlineRename/InlineRenameSession.cs +++ b/src/EditorFeatures/Core/Implementation/InlineRename/InlineRenameSession.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; +using System.Collections.Immutable; using System.Diagnostics; using System.Linq; using System.Threading; @@ -265,7 +266,7 @@ private void UpdateReferenceLocationsTask(Task allRena public IInlineRenameUndoManager UndoManager { get; } - public event EventHandler> ReferenceLocationsChanged; + public event EventHandler> ReferenceLocationsChanged; public event EventHandler ReplacementsComputed; public event EventHandler ReplacementTextChanged; @@ -341,14 +342,14 @@ private void OnWorkspaceChanged(object sender, WorkspaceChangeEventArgs args) } } - private void RaiseSessionSpansUpdated(IList locations) + private void RaiseSessionSpansUpdated(ImmutableArray locations) { AssertIsForeground(); SetReferenceLocations(locations); ReferenceLocationsChanged?.Invoke(this, locations); } - private void SetReferenceLocations(IEnumerable locations) + private void SetReferenceLocations(ImmutableArray locations) { AssertIsForeground(); diff --git a/src/EditorFeatures/Test2/Rename/RenameEngineTests.vb b/src/EditorFeatures/Test2/Rename/RenameEngineTests.vb index 83db8701cc1f0f878dad9b4febce17ed2bf5f7fe..20306d59177e0501b8503029c9d63c6196bf3036 100644 --- a/src/EditorFeatures/Test2/Rename/RenameEngineTests.vb +++ b/src/EditorFeatures/Test2/Rename/RenameEngineTests.vb @@ -20,8 +20,7 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.Rename End Sub - - + Public Sub CannotRenameNamespaceAlias() Using result = RenameEngineResult.Create(_outputHelper, @@ -41,6 +40,54 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.Rename End Using End Sub + + + Public Sub RenameInaccessibleMember1() + Using result = RenameEngineResult.Create(_outputHelper, + + + +class C +{ + void Main() + { + D.[|field|] = 8; + } +} + +class D +{ + private static int [|$$field|]; +} + + , renameTo:="_field") + End Using + End Sub + + + + Public Sub RenameInaccessibleMember2() + Using result = RenameEngineResult.Create(_outputHelper, + + + +class C +{ + void Main() + { + D.[|$$field|] = 8; + } +} + +class D +{ + private static int [|field|]; +} + + , renameTo:="_field") + End Using + End Sub + Public Sub RenameTypeDoesNotRenameGeneratedConstructorCalls() diff --git a/src/VisualStudio/Xaml/Impl/Features/InlineRename/XamlEditorInlineRenameService.cs b/src/VisualStudio/Xaml/Impl/Features/InlineRename/XamlEditorInlineRenameService.cs index c01da5c6fa83242aa8aac02eed3337cf89640ce6..f87068ed02faac254111b47a0d990d5883f921fa 100644 --- a/src/VisualStudio/Xaml/Impl/Features/InlineRename/XamlEditorInlineRenameService.cs +++ b/src/VisualStudio/Xaml/Impl/Features/InlineRename/XamlEditorInlineRenameService.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Collections.Generic; +using System.Collections.Immutable; using System.Composition; using System.Linq; using System.Threading; @@ -66,14 +67,16 @@ public async Task FindRenameLocationsAsync(OptionSet o { var references = new List(); - IList renameLocations = await _renameInfo.FindRenameLocationsAsync( + var renameLocations = await _renameInfo.FindRenameLocationsAsync( renameInStrings: optionSet.GetOption(RenameOptions.RenameInStrings), renameInComments: optionSet.GetOption(RenameOptions.RenameInComments), cancellationToken: cancellationToken).ConfigureAwait(false); - references.AddRange(renameLocations.Select(ds => new InlineRenameLocation(ds.Document, ds.TextSpan))); + references.AddRange(renameLocations.Select( + ds => new InlineRenameLocation(ds.Document, ds.TextSpan))); - return new InlineRenameLocationSet(_renameInfo, _document.Project.Solution, references); + return new InlineRenameLocationSet(_renameInfo, _document.Project.Solution, + references.ToImmutableArray()); } public TextSpan? GetConflictEditSpan(InlineRenameLocation location, string replacementText, CancellationToken cancellationToken) @@ -129,14 +132,14 @@ private class InlineRenameLocationSet : IInlineRenameLocationSet private readonly IXamlRenameInfo _renameInfo; private readonly Solution _oldSolution; - public InlineRenameLocationSet(IXamlRenameInfo renameInfo, Solution solution, IList locations) + public InlineRenameLocationSet(IXamlRenameInfo renameInfo, Solution solution, ImmutableArray locations) { _renameInfo = renameInfo; _oldSolution = solution; Locations = locations; } - public IList Locations { get; } + public ImmutableArray Locations { get; } public bool IsReplacementTextValid(string replacementText) { diff --git a/src/Workspaces/Core/Portable/FindSymbols/ReferenceLocation.cs b/src/Workspaces/Core/Portable/FindSymbols/ReferenceLocation.cs index 29646e9c75ae8eeca751debfe4f0f43440fa1d7a..9c19aa25203c6f77d810893257e6c7edb257bd0f 100644 --- a/src/Workspaces/Core/Portable/FindSymbols/ReferenceLocation.cs +++ b/src/Workspaces/Core/Portable/FindSymbols/ReferenceLocation.cs @@ -67,13 +67,7 @@ internal ReferenceLocation(Document document, IAliasSymbol alias, Location locat /// "Foo()" could show up as an error tolerance location to a method "Foo(int i)" if no /// actual "Foo()" method existed. /// - public bool IsCandidateLocation - { - get - { - return this.CandidateReason != CandidateReason.None; - } - } + public bool IsCandidateLocation => this.CandidateReason != CandidateReason.None; public static bool operator ==(ReferenceLocation left, ReferenceLocation right) { diff --git a/src/Workspaces/Core/Portable/Rename/ConflictEngine/ConflictResolver.Session.cs b/src/Workspaces/Core/Portable/Rename/ConflictEngine/ConflictResolver.Session.cs index 1b6d20e344c5de31cb0d23cdc250f3dcf55b7e27..882fcb0a80cc1d0b95e69898fa611ffa7ca121da 100644 --- a/src/Workspaces/Core/Portable/Rename/ConflictEngine/ConflictResolver.Session.cs +++ b/src/Workspaces/Core/Portable/Rename/ConflictEngine/ConflictResolver.Session.cs @@ -715,7 +715,7 @@ private async Task AddDocumentsWithPotentialConflicts(IEnumerable docu Solution originalSolution, Solution partiallyRenamedSolution, HashSet documentIdsToRename, - IEnumerable renameLocations, + ISet renameLocations, RenamedSpansTracker renameSpansTracker, bool replacementTextValid) { @@ -725,15 +725,13 @@ private async Task AddDocumentsWithPotentialConflicts(IEnumerable docu { _cancellationToken.ThrowIfCancellationRequested(); - // We try to rewrite all locations that are not candidate locations. If there is only one location - // it must be the correct one (the symbol is ambiguous to something else) and we always try to rewrite it. var document = originalSolution.GetDocument(documentId); var semanticModel = await document.GetSemanticModelAsync(_cancellationToken).ConfigureAwait(false); var originalSyntaxRoot = await semanticModel.SyntaxTree.GetRootAsync(_cancellationToken).ConfigureAwait(false); // Get all rename locations for the current document. var allTextSpansInSingleSourceTree = renameLocations - .Where(l => l.DocumentId == documentId && !l.IsRenameInStringOrComment && (renameLocations.Count() == 1 || !l.IsCandidateLocation || l.IsMethodGroupReference)) + .Where(l => l.DocumentId == documentId && ShouldIncludeLocation(renameLocations, l)) .ToDictionary(l => l.Location.SourceSpan); // All textspan in the document documentId, that requires rename in String or Comment @@ -790,6 +788,25 @@ private async Task AddDocumentsWithPotentialConflicts(IEnumerable docu throw ExceptionUtilities.Unreachable; } } + + /// We try to rewrite all locations that are invalid candidate locations. If there is only + /// one location it must be the correct one (the symbol is ambiguous to something else) + /// and we always try to rewrite it. If there are multiple locations, we only allow it + /// if the candidate reason allows for it). + private bool ShouldIncludeLocation(ISet renameLocations, RenameLocation location) + { + if (location.IsRenameInStringOrComment) + { + return false; + } + + if (renameLocations.Count == 1) + { + return true; + } + + return RenameLocation.ShouldRename(location); + } } } -} +} \ No newline at end of file diff --git a/src/Workspaces/Core/Portable/Rename/RenameLocation.ReferenceProcessing.cs b/src/Workspaces/Core/Portable/Rename/RenameLocation.ReferenceProcessing.cs index 9fb9f3f8a3ba9fdbddf544c01b0d9a704b8b3569..c02bf439ee379d6bde88fbbee28badc91810b212 100644 --- a/src/Workspaces/Core/Portable/Rename/RenameLocation.ReferenceProcessing.cs +++ b/src/Workspaces/Core/Portable/Rename/RenameLocation.ReferenceProcessing.cs @@ -298,7 +298,10 @@ private static string TrimNameToAfterLastDot(string name) { if (location.IsInSource) { - results.Add(new RenameLocation(location, solution.GetDocument(location.SourceTree).Id, isRenamableAccessor: isRenamableAccessor)); + results.Add(new RenameLocation( + location, + solution.GetDocument(location.SourceTree).Id, + isRenamableAccessor: isRenamableAccessor)); } } @@ -375,7 +378,7 @@ internal static async Task> GetRenamableReferenceLoc if (location.Alias.Name == referencedSymbol.Name) { results.Add(new RenameLocation(location.Location, location.Document.Id, - isCandidateLocation: location.IsCandidateLocation, isRenamableAliasUsage: true, isWrittenTo: location.IsWrittenTo)); + candidateReason: location.CandidateReason, isRenamableAliasUsage: true, isWrittenTo: location.IsWrittenTo)); // We also need to add the location of the alias itself var aliasLocation = location.Alias.Locations.Single(); @@ -389,8 +392,7 @@ internal static async Task> GetRenamableReferenceLoc location.Location, location.Document.Id, isWrittenTo: location.IsWrittenTo, - isCandidateLocation: location.IsCandidateLocation, - isMethodGroupReference: location.IsCandidateLocation && location.CandidateReason == CandidateReason.MemberGroup, + candidateReason: location.CandidateReason, isRenamableAccessor: await IsPropertyAccessorOrAnOverride(referencedSymbol, solution, cancellationToken).ConfigureAwait(false))); } } diff --git a/src/Workspaces/Core/Portable/Rename/RenameLocation.cs b/src/Workspaces/Core/Portable/Rename/RenameLocation.cs index aeb1599e2450b0c7a91ff524e6059ff235812b43..f7a6af639dc8346c2587b1dffdf2465958a473b0 100644 --- a/src/Workspaces/Core/Portable/Rename/RenameLocation.cs +++ b/src/Workspaces/Core/Portable/Rename/RenameLocation.cs @@ -10,7 +10,7 @@ internal struct RenameLocation : IEquatable { public readonly Location Location; public readonly DocumentId DocumentId; - public readonly bool IsCandidateLocation; + public readonly CandidateReason CandidateReason; public readonly bool IsRenamableAliasUsage; public readonly bool IsRenamableAccessor; public readonly TextSpan ContainingLocationForStringOrComment; @@ -18,13 +18,10 @@ internal struct RenameLocation : IEquatable public bool IsRenameInStringOrComment { get { return ContainingLocationForStringOrComment != default(TextSpan); } } - public bool IsMethodGroupReference { get; } - public RenameLocation( Location location, DocumentId documentId, - bool isCandidateLocation = false, - bool isMethodGroupReference = false, + CandidateReason candidateReason = CandidateReason.None, bool isRenamableAliasUsage = false, bool isRenamableAccessor = false, bool isWrittenTo = false, @@ -32,8 +29,7 @@ internal struct RenameLocation : IEquatable { Location = location; DocumentId = documentId; - IsCandidateLocation = isCandidateLocation; - IsMethodGroupReference = isMethodGroupReference; + CandidateReason = candidateReason; IsRenamableAliasUsage = isRenamableAliasUsage; IsRenamableAccessor = isRenamableAccessor; IsWrittenTo = isWrittenTo; @@ -42,8 +38,7 @@ internal struct RenameLocation : IEquatable public RenameLocation(ReferenceLocation referenceLocation, DocumentId documentId) : this(referenceLocation.Location, documentId, - isCandidateLocation: referenceLocation.IsCandidateLocation && referenceLocation.CandidateReason != CandidateReason.LateBound, - isMethodGroupReference: referenceLocation.IsCandidateLocation && referenceLocation.CandidateReason == CandidateReason.MemberGroup, + candidateReason: referenceLocation.CandidateReason, isWrittenTo: referenceLocation.IsWrittenTo) { } @@ -69,5 +64,118 @@ public override int GetHashCode() { return Location.GetHashCode(); } + + internal static bool ShouldRename(RenameLocation location) + => ShouldRename(location.CandidateReason); + + internal static bool ShouldRename(CandidateReason candidateReason) + { + if (candidateReason != CandidateReason.None) + { + // When we have a CandidateReason that means (for most reasons) the compiler + // encountered some sort of issue when binding the node. This means we're + // less certain about what the code meant and if the node bound to the actual + // symbol that we're trying to rename. However, for many of these reasons, + // even if the code is in error, we can still be confident enough that the + // node bound to the symbol we care about. + + switch (candidateReason) + { + case CandidateReason.NotATypeOrNamespace: + // We had a reference to the symbol in a location where we needed a + // type or namespace. This is usually a wildly broken situation. i.e. + // now due to hiding, something like a field/property is being referenced + // in a type location. It is highly likely that this should not be + // renamed. + return false; + + case CandidateReason.NotAnEvent: + case CandidateReason.NotAWithEventsMember: + // it's unlikely that someone would be referencing a non-event in an + // event context. Likely this location should be included. + return false; + + case CandidateReason.NotAnAttributeType: + // It's feasible that someon was referencing some type in an attribute + // location before making that type itself descend from System.Attribute. + // Still allow this type to be renamed. + return true; + + case CandidateReason.WrongArity: + // Someone may have provided the wrong number of type arguments to + // a type/method when calling it. We should still allow the reference + // to be updated. + return true; + + case CandidateReason.NotCreatable: + // Can happen when someone tries to do something like 'new' an + // abstract type. We still want to allow renaming this location. + return true; + + case CandidateReason.NotReferencable: + // Happens when the user does something like directly accessing + // the accessor of a normal property. In this case, we do still + // want to allow the rename to happen. + return true; + + case CandidateReason.Inaccessible: + // Can trivially occur in code that is in an initially broken state + // where inaccessible members are being referenced. We still want + // to update these references. + return true; + + case CandidateReason.NotAValue: + case CandidateReason.NotAVariable: + // Happens with code like "NS = 1". If "NS" is binding now to a + // namespace, then it's likely something has gone very wrong (similar to + // NotATypeOrNamespace), and we shouldn't update this reference. + return false; + + case CandidateReason.NotInvocable: + // Happens when something like a variable is being invoked, but the variable + // isn't a delegate type. This may be because the user intends to give + // this value a delegate type, but hasn't done so yet. We should still allow + // renaming this reference for now. + return false; + + case CandidateReason.StaticInstanceMismatch: + // Similar to 'Inaccessible', the code is currently broken, but it's fairly + // clear what the user's intent was. In this case, we want to update the + // references, even though the code isn't valid. + return true; + + case CandidateReason.OverloadResolutionFailure: + // Here we were renaming a method, and have a reference location that didn't + // bind properly to any methods. This case is simply hard to reason about. + // There are times where we might want to rename this, and times when we + // don't. As overloading methods is very common, we won't update this location + // as it might update code the user really doesn't want us to be touching. + return false; + + case CandidateReason.LateBound: + // This is a late bound call, so we should not update this location. + return false; + + case CandidateReason.Ambiguous: + // we shoudl not touch amgiguous code. We have no way to feel confident that + // this really is a location that we should be updating. + return false; + + case CandidateReason.MemberGroup: + // MemberGroup is not an error case. It happens in completely legal code, + // like nameof(int.ToString). Because of that, we do want to update the + // reference here. + return true; + + default: + // For cases added in the future, conservatively presume we can't update them. + // If we need to we can just add a case above this. + return false; + } + } + + // If there is no candidate reason, we can rename this reference. + return true; + } } -} +} \ No newline at end of file diff --git a/src/Workspaces/Core/Portable/Rename/RenameLocations.cs b/src/Workspaces/Core/Portable/Rename/RenameLocations.cs index 7fed7f8082e35c05baf2f2d7d012138ece9d47be..662dd61ac18ddf04bb9e34d4abbf31d3b5fb15a9 100644 --- a/src/Workspaces/Core/Portable/Rename/RenameLocations.cs +++ b/src/Workspaces/Core/Portable/Rename/RenameLocations.cs @@ -100,7 +100,7 @@ private class SearchResult { mergedLocations.AddRange(renameMethodGroupReferences ? result.Locations - : result.Locations.Where(x => !x.IsMethodGroupReference)); + : result.Locations.Where(x => x.CandidateReason != CandidateReason.MemberGroup)); mergedImplicitLocations.AddRange(result.ImplicitLocations); mergedReferencedSymbols.AddRange(result.ReferencedSymbols); @@ -109,7 +109,7 @@ private class SearchResult _mergedResult = new SearchResult(mergedLocations, mergedImplicitLocations, mergedReferencedSymbols); } - public ISet Locations { get { return _mergedResult.Locations; } } + public ISet Locations => _mergedResult.Locations; public SymbolAndProjectId SymbolAndProjectId => _symbolAndProjectId; public ISymbol Symbol => _symbolAndProjectId.Symbol; public Solution Solution { get { return _solution; } } diff --git a/src/Workspaces/Core/Portable/Rename/RenameUtilities.cs b/src/Workspaces/Core/Portable/Rename/RenameUtilities.cs index cd75a8219d8012674c0c2e78c0ee5b1df5aefbfb..c4c0edadc330eb4c70ad3bc2f3272c3dd4dcc305 100644 --- a/src/Workspaces/Core/Portable/Rename/RenameUtilities.cs +++ b/src/Workspaces/Core/Portable/Rename/RenameUtilities.cs @@ -136,6 +136,15 @@ private static bool ShouldRenameOnlyAffectDeclaringProject(ISymbol symbol) return TokenRenameInfo.CreateMemberGroupTokenInfo(symbolInfo.CandidateSymbols); } + if (RenameLocation.ShouldRename(symbolInfo.CandidateReason) && + symbolInfo.CandidateSymbols.Length == 1) + { + // TODO(cyrusn): We're allowing rename here, but we likely should let the user + // know that there is an error in the code and that rename results might be + // inaccurate. + return TokenRenameInfo.CreateSingleSymbolTokenInfo(symbolInfo.CandidateSymbols[0]); + } + return TokenRenameInfo.NoSymbolsTokenInfo; } }