Enhance MemberAttibutesRecoverer to deal with properties

Now, the key used to identify members is (memberName, memberType) instead od just memberName. This is necessary because Get, Let and Set properties share the same name.

Author: MDoerner

Rubberduck.Core/UI/Notifiers/MemberAttributeRecoveryFailureNotifier.cs

@@ -1,7 +1,9 @@
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using Rubberduck.Interaction;
 using Rubberduck.Parsing.Rewriter;
+using Rubberduck.Parsing.Symbols;
 using Rubberduck.VBEditor;
 
 namespace Rubberduck.UI.Notifiers
@@ -44,18 +46,19 @@ private static string RewriteFailureReasonMessage(RewriteSessionState sessionSta
             }
         }
 
-        public void NotifyMembersForRecoveryNotFound(IEnumerable<QualifiedMemberName> membersNotFound)
+        public void NotifyMembersForRecoveryNotFound(IEnumerable<(QualifiedMemberName memberName, DeclarationType memberType)> membersNotFound)
         {
             var message = MembersNotFoundMessage(membersNotFound);
             var caption = Resources.RubberduckUI.MemberAttributeRecoveryFailureCaption;
 
             _messageBox.NotifyWarn(message, caption);
         }
 
-        private string MembersNotFoundMessage(IEnumerable<QualifiedMemberName> membersNotFound)
+        private string MembersNotFoundMessage(IEnumerable<(QualifiedMemberName memberName, DeclarationType memberType)> membersNotFound)
         {
-            var missingMemberList = $"{Environment.NewLine}{string.Join(Environment.NewLine, membersNotFound)}";
-            return string.Format(Resources.RubberduckUI.MemberAttributeRecoveryMembersNotFoundMessage, missingMemberList); ;
+            var missingMemberTexts = membersNotFound.Select(tpl => $"{tpl.memberName} ({tpl.memberType})");
+            var missingMemberList = $"{Environment.NewLine}{string.Join(Environment.NewLine, missingMemberTexts)}";
+            return string.Format(Resources.RubberduckUI.MemberAttributeRecoveryMembersNotFoundMessage, missingMemberList);
         }
     }
 }

Rubberduck.Parsing/Rewriter/IMemberAttributeRecoveryFailureNotifier.cs

@@ -1,11 +1,12 @@
 using System.Collections.Generic;
+using Rubberduck.Parsing.Symbols;
 using Rubberduck.VBEditor;
 
 namespace Rubberduck.Parsing.Rewriter
 {
     public interface IMemberAttributeRecoveryFailureNotifier
     {
         void NotifyRewriteFailed(RewriteSessionState rewriteSessionState);
-        void NotifyMembersForRecoveryNotFound(IEnumerable<QualifiedMemberName> membersNotFound);
+        void NotifyMembersForRecoveryNotFound(IEnumerable<(QualifiedMemberName memberName, DeclarationType memberType)> membersNotFound);
     }
 }

Rubberduck.Parsing/Rewriter/MemberAttributeRecoverer.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
@@ -19,15 +20,17 @@ public sealed class MemberAttributeRecoverer : IMemberAttributeRecovererWithSett
         private IRewritingManager _rewritingManager;
         private readonly IMemberAttributeRecoveryFailureNotifier _failureNotifier;
 
-        private readonly
-            IDictionary<QualifiedModuleName, IDictionary<string, HashSet<AttributeNode>>> _attributesToRecover
-                = new Dictionary<QualifiedModuleName, IDictionary<string, HashSet<AttributeNode>>>();
-        private readonly HashSet<QualifiedMemberName> _missingMembers = new HashSet<QualifiedMemberName>();
+        private readonly IDictionary<QualifiedModuleName, IDictionary<(string memberName, DeclarationType memberType), HashSet<AttributeNode>>> _attributesToRecover
+                = new Dictionary<QualifiedModuleName, IDictionary<(string memberName, DeclarationType memberType), HashSet<AttributeNode>>>();
+        private readonly HashSet<(QualifiedMemberName memberName, DeclarationType memberType)> _missingMembers = new HashSet<(QualifiedMemberName memberName, DeclarationType memberType)>();
 
         private readonly Logger _logger = LogManager.GetCurrentClassLogger();
 
-        public MemberAttributeRecoverer(IDeclarationFinderProvider declarationFinderProvider,
-            IParseManager parseManager, IAttributesUpdater attributesUpdater, IMemberAttributeRecoveryFailureNotifier failureNotifier)
+        public MemberAttributeRecoverer(
+            IDeclarationFinderProvider declarationFinderProvider,
+            IParseManager parseManager, 
+            IAttributesUpdater attributesUpdater, 
+            IMemberAttributeRecoveryFailureNotifier failureNotifier)
         {
             _declarationFinderProvider = declarationFinderProvider;
             _parseManager = parseManager;
@@ -83,7 +86,7 @@ private void SaveAttributesToRecover(IDictionary<QualifiedModuleName, IEnumerabl
                 var attributesByMember = declarationsByModule[module]
                     .Where(decl => decl.Attributes.Any())
                     .ToDictionary(
-                        decl => decl.IdentifierName,
+                        decl => (decl.IdentifierName, decl.DeclarationType),
                         decl => (HashSet<AttributeNode>)decl.Attributes);
                 _attributesToRecover.Add(module, attributesByMember);
             }
@@ -154,41 +157,41 @@ private void CancelTheCurrentParse()
             _parseManager.OnParseCancellationRequested(this);
         }
 
-        private void RecoverAttributes(IRewriteSession rewriteSession, QualifiedModuleName module, IDictionary<string, HashSet<AttributeNode>> attributesByMember)
+        private void RecoverAttributes(IRewriteSession rewriteSession, QualifiedModuleName module, IDictionary<(string memberName, DeclarationType memberType), HashSet<AttributeNode>> attributesByMember)
         {
             var membersWithAttributesToRecover = attributesByMember.Keys.ToHashSet();
             var declarationFinder = _declarationFinderProvider.DeclarationFinder;
             var declarationsWithAttributesToRecover = declarationFinder.Members(module)
-                .Where(decl => membersWithAttributesToRecover.Contains(decl.IdentifierName) 
+                .Where(decl => membersWithAttributesToRecover.Contains((decl.IdentifierName, decl.DeclarationType)) 
                                && decl.ParentScopeDeclaration.DeclarationType.HasFlag(DeclarationType.Module))
                 .ToList();
 
             if (membersWithAttributesToRecover.Count != declarationsWithAttributesToRecover.Count)
             {
                 var membersWithoutDeclarations = MembersWithoutDeclarations(membersWithAttributesToRecover, declarationsWithAttributesToRecover);
                 LogFailureToRecoverAllAttributes(module, membersWithoutDeclarations);
-                _missingMembers.UnionWith(membersWithoutDeclarations.Select(memberName => new QualifiedMemberName(module, memberName)));
+                _missingMembers.UnionWith(membersWithoutDeclarations.Select(tpl => (new QualifiedMemberName(module, tpl.memberName), tpl.memberType)));
             }
 
             foreach (var declaration in declarationsWithAttributesToRecover)
             {
-                RecoverAttributes(rewriteSession, declaration, attributesByMember[declaration.IdentifierName]);
+                RecoverAttributes(rewriteSession, declaration, attributesByMember[(declaration.IdentifierName, declaration.DeclarationType)]);
             }
         }
 
-        private static ICollection<string> MembersWithoutDeclarations(HashSet<string> membersWithAttributesToRecover, IEnumerable<Declaration> declarationsWithAttributesToRecover)
+        private static ICollection<(string memberName, DeclarationType memberType)> MembersWithoutDeclarations(HashSet<(string memberName, DeclarationType memberType)> membersWithAttributesToRecover, IEnumerable<Declaration> declarationsWithAttributesToRecover)
         {
             var membersWithoutDeclarations = membersWithAttributesToRecover.ToHashSet();
-            membersWithoutDeclarations.ExceptWith(declarationsWithAttributesToRecover.Select(decl => decl.IdentifierName));
+            membersWithoutDeclarations.ExceptWith(declarationsWithAttributesToRecover.Select(decl => (decl.IdentifierName, decl.DeclarationType)));
             return membersWithoutDeclarations;
         }
 
-        private void LogFailureToRecoverAllAttributes(QualifiedModuleName module, IEnumerable<string> membersWithoutDeclarations)
+        private void LogFailureToRecoverAllAttributes(QualifiedModuleName module, IEnumerable<(string memberName, DeclarationType memberType)> membersWithoutDeclarations)
         {
             _logger.Warn("Could not recover the attributes for all members because one or more members could no longer be found.");
-            foreach (var member in membersWithoutDeclarations)
+            foreach (var (memberName, memberType) in membersWithoutDeclarations)
             {
-                _logger.Trace($"Could not recover the attributes for member {member} in module {module} because a member of that name exists no longer.");
+                _logger.Trace($"Could not recover the attributes for member {memberName} of type {memberType} in module {module} because a member of that name and type exists no longer.");
             }
         }
 

RubberduckTests/Inspections/EmptyModuleInspectionTests.cs

@@ -49,7 +49,7 @@ Option Explicit
         }
 
         [TestCase("Private Function Foo() As String\r\nEnd Function")]
-        [TestCase("Private Sub Foo() As String\r\nEnd Sub")]
+        [TestCase("Private Sub Foo()\r\nEnd Sub")]
         [TestCase("Public Property Get Foo()\r\nEnd Property")]
         [TestCase("Public Property Set Foo(rhs As Variant)\r\nEnd Property")]
         [TestCase("Public Property Let Foo(rhs As Variant)\r\nEnd Property")]

RubberduckTests/Rewriter/MemberAttributeRecovererTests.cs

@@ -250,5 +250,72 @@ Public Function Bar() As Variant
                 Assert.AreEqual(expectedCodeWithoutRecovery, actualCodeWithoutRecovery);
             }
         }
+
+        [Test]
+        [Category("Rewriter")]
+        [TestCase("Attribute Foo.VB_UserMemId = 0", "", "")]
+        [TestCase("", "Attribute Foo.VB_UserMemId = 0", "")]
+        [TestCase("", "", "Attribute Foo.VB_UserMemId = 0")]
+        [TestCase("Attribute Foo.VB_Description = \"myPropertyGet\"", "Attribute Foo.VB_Description = \"myPropertyLet\"", "")]
+        [TestCase("", "Attribute Foo.VB_Description = \"myPropertyLet\"", "Attribute Foo.VB_Description = \"myPropertySet\"")]
+        [TestCase("Attribute Foo.VB_Description = \"myPropertyGet\"", "", "Attribute Foo.VB_Description = \"myPropertySet\"")]
+        [TestCase("Attribute Foo.VB_Description = \"myPropertyGet\"", "Attribute Foo.VB_Description = \"myPropertyLet\"", "Attribute Foo.VB_Description = \"myPropertySet\"")]
+        public void RecoveringAttributesRecoversTheAttributesInTheModulesProvided_ViaModule_PropertiesAreHandlesSeparately(string getAttributes, string letAttributes, string setAttributes)
+        {
+            var inputCode =
+                $@"Public Property Get Foo() As Variant
+{getAttributes}
+End Property
+
+Public Property Let Foo(ByVal RHS As Long)
+{letAttributes}
+End Property
+
+Public Property Set Foo(ByVal RHs As Object)
+{setAttributes}
+End Property";
+
+            var expectedCodeWithRecovery = inputCode;
+
+            var expectedCodeWithoutRecovery =
+                $@"Public Property Get Foo() As Variant{(string.IsNullOrEmpty(getAttributes) ? Environment.NewLine : string.Empty)}
+End Property
+
+Public Property Let Foo(ByVal RHS As Long){(string.IsNullOrEmpty(letAttributes) ? Environment.NewLine : string.Empty)}
+End Property
+
+Public Property Set Foo(ByVal RHs As Object){(string.IsNullOrEmpty(setAttributes) ? Environment.NewLine : string.Empty)}
+End Property";
+
+            var vbe = MockVbeBuilder.BuildFromStdModules(("RecoveryModule", inputCode), ("NoRecoveryModule", inputCode)).Object;
+            var (state, rewritingManager) = MockParser.CreateAndParseWithRewritingManager(vbe);
+            using (state)
+            {
+                var attributesUpdater = new AttributesUpdater(state);
+                var mockFailureNotifier = new Mock<IMemberAttributeRecoveryFailureNotifier>();
+                var memberAttributeRecoverer = new MemberAttributeRecoverer(state, state, attributesUpdater, mockFailureNotifier.Object);
+                memberAttributeRecoverer.RewritingManager = rewritingManager;
+
+                var recoveryModule = state.DeclarationFinder.UserDeclarations(DeclarationType.Module)
+                    .First(decl => decl.IdentifierName.Equals("RecoveryModule")).QualifiedModuleName;
+                var noRecoveryModule = state.DeclarationFinder.UserDeclarations(DeclarationType.Module)
+                    .First(decl => decl.IdentifierName.Equals("NoRecoveryModule")).QualifiedModuleName;
+
+                var modulesToRecoverAttributesIn = new List<QualifiedModuleName> { recoveryModule };
+
+                memberAttributeRecoverer.RecoverCurrentMemberAttributesAfterNextParse(modulesToRecoverAttributesIn);
+
+                var rewriteSession = rewritingManager.CheckOutCodePaneSession();
+                var declarationsForWhichToRemoveAttributes = state.DeclarationFinder.UserDeclarations(DeclarationType.Property);
+                RemoveAttributes(declarationsForWhichToRemoveAttributes, rewriteSession);
+
+                ExecuteAndWaitForParserState(state, () => rewriteSession.TryRewrite(), ParserState.Ready);
+
+                var actualCodeWithRecovery = state.ProjectsProvider.Component(recoveryModule).CodeModule.Content();
+                var actualCodeWithoutRecovery = state.ProjectsProvider.Component(noRecoveryModule).CodeModule.Content();
+                Assert.AreEqual(expectedCodeWithRecovery, actualCodeWithRecovery);
+                Assert.AreEqual(expectedCodeWithoutRecovery, actualCodeWithoutRecovery);
+            }
+        }
     }
 }