Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Merge pull request #5612 from BZngr/EF_ConflictFinderMeQualifier
EncapsulateFieldRefactoring - Fix false-positive conflict detections involving 'Me' qualifier
  • Loading branch information
retailcoder committed Dec 1, 2020
2 parents 9e4c73d + 1e9cf7b commit 4dc9bab
Show file tree
Hide file tree
Showing 14 changed files with 254 additions and 149 deletions.
Expand Up @@ -276,9 +276,14 @@ private bool HasConflictWithLocalDeclarationIdentifiers(IEncapsulateFieldCandida
.Select(f => f.Declaration));
}

bool IsQualifedReference(IdentifierReference identifierReference)
=> identifierReference.Context.Parent is VBAParser.MemberAccessExprContext
|| identifierReference.Context.Parent is VBAParser.WithMemberAccessExprContext;

//Only check IdentifierReferences in the declaring module because encapsulated field
//references in other modules will be module-qualified.
var candidateLocalReferences = candidate.Declaration.References.Where(rf => rf.QualifiedModuleName == candidate.QualifiedModuleName);
var candidateLocalReferences = candidate.Declaration.References.Where(rf => rf.QualifiedModuleName == candidate.QualifiedModuleName
&& !IsQualifedReference(rf));

var localDeclarationConflictCandidates = membersToEvaluate.Where(localDec => candidateLocalReferences
.Any(cr => cr.ParentScoping == localDec.ParentScopeDeclaration));
Expand Down
Expand Up @@ -9,14 +9,6 @@ namespace RubberduckTests.Refactoring.EncapsulateField
[TestFixture]
public class EncapsulateFieldCodeBuilderTests
{
private EncapsulateFieldTestSupport Support { get; } = new EncapsulateFieldTestSupport();

[SetUp]
public void ExecutesBeforeAllTests()
{
Support.ResetResolver();
}

[Test]
[Category("Refactorings")]
[Category("Encapsulate Field")]
Expand Down Expand Up @@ -103,8 +95,9 @@ public void BuildPropertyBlock_Set(string asTypeName)
var encapsulateTarget = state.AllUserDeclarations.Single(d => d.IdentifierName.Equals(prototypeIdentifier));

attrSet.Declaration = encapsulateTarget;
var resolver = EncapsulateFieldTestSupport.GetResolver(state);

return Support.Resolve<IEncapsulateFieldCodeBuilder>(state)
return resolver.Resolve<IEncapsulateFieldCodeBuilder>()
.BuildPropertyBlocks(attrSet);
}
}
Expand Down
@@ -0,0 +1,156 @@
using NUnit.Framework;
using Rubberduck.Parsing.Symbols;
using Rubberduck.Refactorings;
using Rubberduck.Refactorings.EncapsulateField;
using Rubberduck.VBEditor.SafeComWrappers;
using RubberduckTests.Mocks;
using System.Collections.Generic;
using System.Linq;

namespace RubberduckTests.Refactoring.EncapsulateField
{
[TestFixture]
public class EncapsulateFieldConflictFinderTests
{
[Test]
[Category("Refactorings")]
[Category("Encapsulate Field")]
[Category(nameof(EncapsulateFieldConflictFinder))]
public void RespectsMemberAccessExpressionUsingMe()
{
var targetField = "Fizz";

var inputCode =
$@"Public {targetField} As Integer
Public Sub NoConflict(Fizz As Integer)
Me.Fizz = Fizz
End Sub";

var result = TestIsConflictingIdentifier("Fizz", inputCode, targetField);
Assert.IsFalse(result);
}

[Test]
[Category("Refactorings")]
[Category("Encapsulate Field")]
[Category(nameof(EncapsulateFieldConflictFinder))]
public void RespectsWithMemberAccessExpressionUsingMe()
{
var targetField = "Fizz";

var inputCode =
$@"Public Fizz As Integer
Public Sub NoConflict(Fizz As Integer)
With Me
.Fizz = Fizz
End With
End Sub";
var result = TestIsConflictingIdentifier("Fizz", inputCode, targetField);
Assert.IsFalse(result);
}

[Test]
[Category("Refactorings")]
[Category("Encapsulate Field")]
[Category(nameof(EncapsulateFieldConflictFinder))]
public void RespectsWithMemberAccessExpressionUsingMe_Parameter()
{
var targetField = "Fizz";
var inputCode =
$@"Public Fizz As Integer
Public Sub CopyFromAnotherInstance(Fizz As MockVbeBuilder.TestModuleName)
Me.Fizz = Fizz.Fizz
End Sub";

var result = TestIsConflictingIdentifier("Fizz", inputCode, targetField);
Assert.IsFalse(result);
}

[TestCase("Dim thisConflicts As Long", null, true)]
[TestCase("Dim thisConflicts As Long", "Me.", false)]
[TestCase("Const thisConflicts As Long = 10", null, true)]
[TestCase("Const thisConflicts As Long = 10", "Me.", false)]
[Category("Refactorings")]
[Category("Encapsulate Field")]
[Category(nameof(EncapsulateFieldConflictFinder))]
public void LocalDeclarationsRespectsQualifier(string declaration, string qualifier, bool expected)
{
var targetField = "Fizz";
var inputCode =
$@"Public Fizz As Integer
Public Sub DoSomething()
{declaration}
{qualifier}Fizz = thisConflicts + 1
End Sub";

var result = TestIsConflictingIdentifier("thisConflicts", inputCode, targetField);
Assert.AreEqual(expected, result);
}

[TestCase("thisConFlicts", true)]
[TestCase("DoSomething", true)]
[TestCase("TestString", true)]
[TestCase("TestConst", true)]
[TestCase("FirstValue", false)]
[TestCase("LocalConst", false)]
[TestCase("LocalVariable", false)]
[Category("Refactorings")]
[Category("Encapsulate Field")]
[Category(nameof(EncapsulateFieldConflictFinder))]
public void DetectsModuleEntityConflicts(string newName, bool expected)
{
var targetField = "Fizz";
var inputCode =
$@"
Public Fizz As Integer
Private TestString As String
Private Const TestConst As Long = 10
Private Enum TestEnum
ThisConflicts
EnumTwo
End Enum
Private Type TestType
FirstValue As Long
End Type
Private Sub DoSomething()
Const localConst As String = ""Test""
Dim localVariable As Long
End Sub
";

var result = TestIsConflictingIdentifier(newName, inputCode, targetField);
Assert.AreEqual(expected, result);
}

private bool TestIsConflictingIdentifier(string testIdentifier, string inputCode, string targetFieldName)
{
var vbe = MockVbeBuilder.BuildFromModules((MockVbeBuilder.TestModuleName, inputCode, ComponentType.ClassModule));

var state = MockParser.CreateAndParse(vbe.Object);
using (state)
{

var field = state.DeclarationFinder.UserDeclarations(DeclarationType.Variable)
.Single(d => d.IdentifierName == targetFieldName);

var fieldModel = new FieldEncapsulationModel(field as VariableDeclaration);

var modelFactory = EncapsulateFieldTestSupport.GetResolver(state).Resolve<IEncapsulateFieldUseBackingFieldModelFactory>();
var model = modelFactory.Create(new List<FieldEncapsulationModel>() { fieldModel });

var efCandidate = model.EncapsulationCandidates.First(c => c.Declaration == field);
efCandidate.EncapsulateFlag = true;

return model.ConflictFinder.IsConflictingIdentifier(efCandidate, testIdentifier, out _);
}
}
}
}
Expand Up @@ -17,34 +17,21 @@ namespace RubberduckTests.Refactoring.EncapsulateField
{
public class EncapsulateFieldTestSupport : EncapsulateFieldInteractiveRefactoringTest
{
private EncapsulateFieldTestsResolver _testResolver;

public void ResetResolver()
{
_testResolver = null;
}

public T Resolve<T>(IDeclarationFinderProvider declarationFinderProvider, IRewritingManager rewritingManager = null, ISelectionService selectionService = null) where T : class
public EncapsulateFieldTestsResolver SetupResolver(IDeclarationFinderProvider declarationFinderProvider, IRewritingManager rewritingManager = null, ISelectionService selectionService = null)
{
SetupResolver(declarationFinderProvider, rewritingManager, selectionService);
return Resolve<T>() as T;
return GetResolver(declarationFinderProvider, rewritingManager, selectionService);
}

public T Resolve<T>() where T : class
=> _testResolver?.Resolve<T>() as T ?? throw new InvalidOperationException("Test Resolver not initialized. Call 'SetupResolver(...)' or use one of the 'Resolve<T>()' overloads");

public void SetupResolver(IDeclarationFinderProvider declarationFinderProvider, IRewritingManager rewritingManager = null, ISelectionService selectionService = null)
public static EncapsulateFieldTestsResolver GetResolver(IDeclarationFinderProvider declarationFinderProvider, IRewritingManager rewritingManager = null, ISelectionService selectionService = null)
{
if (declarationFinderProvider is null)
{
throw new ArgumentNullException("declarationFinderProvider is null");
}

if (_testResolver is null)
{
_testResolver = new EncapsulateFieldTestsResolver(declarationFinderProvider, rewritingManager, selectionService);
_testResolver.Install(new WindsorContainer(), null);
}
var resolver = new EncapsulateFieldTestsResolver(declarationFinderProvider, rewritingManager, selectionService);
resolver.Install(new WindsorContainer(), null);
return resolver;
}

public string RHSIdentifier => Rubberduck.Resources.Refactorings.Refactorings.CodeBuilder_DefaultPropertyRHSParam;
Expand Down Expand Up @@ -148,13 +135,13 @@ public string RefactoredCode(CodeString codeString, Func<EncapsulateFieldModel,
RefactoringUserInteraction<IEncapsulateFieldPresenter, EncapsulateFieldModel> userInteraction,
ISelectionService selectionService)
{
SetupResolver(state, rewritingManager, selectionService);
return new EncapsulateFieldRefactoring(Resolve<EncapsulateFieldRefactoringAction>(state, rewritingManager, selectionService),
Resolve<EncapsulateFieldPreviewProvider>(),
Resolve<IEncapsulateFieldModelFactory>(),
var resolver = SetupResolver(state, rewritingManager, selectionService);
return new EncapsulateFieldRefactoring(resolver.Resolve<EncapsulateFieldRefactoringAction>(),
resolver.Resolve<EncapsulateFieldPreviewProvider>(),
resolver.Resolve<IEncapsulateFieldModelFactory>(),
userInteraction,
selectionService,
Resolve<ISelectedDeclarationProvider>());
resolver.Resolve<ISelectedDeclarationProvider>());
}

public IDictionary<string, string> RefactoredCode(
Expand Down Expand Up @@ -202,9 +189,11 @@ public IEncapsulateFieldCandidate RetrieveEncapsulateFieldCandidate(IVBE vbe, st
var state = MockParser.CreateAndParse(vbe);
using (state)
{
var resolver = SetupResolver(state);

var match = state.DeclarationFinder.MatchName(fieldName).Where(m => m.DeclarationType.Equals(declarationType)).Single();

var model = Resolve<IEncapsulateFieldModelFactory>(state).Create(match);
var model = resolver.Resolve<IEncapsulateFieldModelFactory>().Create(match);

model.ConflictFinder.AssignNoConflictIdentifiers(model[match.IdentifierName]);

Expand Down Expand Up @@ -232,6 +221,32 @@ public EncapsulateFieldModel RetrieveUserModifiedModelPriorToRefactoring(IVBE vb
{
return SupportTestRefactoring(rewritingManager, state, userInteraction, selectionService);
}

public string RefactoredCode<TRefactoring,TModel>(string code, Func<RubberduckParserState, EncapsulateFieldTestsResolver, TModel> modelBuilder) where TRefactoring : CodeOnlyRefactoringActionBase<TModel> where TModel : class, IRefactoringModel
{
var vbe = MockVbeBuilder.BuildFromSingleStandardModule(code, out _).Object;
var componentName = vbe.SelectedVBComponent.Name;
var refactored = RefactoredCode<TRefactoring,TModel>(vbe, modelBuilder);
return refactored[componentName];
}

public IDictionary<string, string> RefactoredCode<TRefactoring,TModel>(IVBE vbe, Func<RubberduckParserState, EncapsulateFieldTestsResolver, TModel> modelBuilder) where TRefactoring: CodeOnlyRefactoringActionBase<TModel> where TModel: class, IRefactoringModel
{
var (state, rewritingManager) = MockParser.CreateAndParseWithRewritingManager(vbe);
using (state)
{
var resolver = GetResolver(state, rewritingManager);

var refactoring = resolver.Resolve<TRefactoring>();

var model = modelBuilder(state, resolver);

refactoring.Refactor(model);

return vbe.ActiveVBProject.VBComponents
.ToDictionary(component => component.Name, component => component.CodeModule.Content());
}
}
}

public class TestEncapsulationAttributes
Expand Down
Expand Up @@ -17,12 +17,6 @@ public class EncapsulateArrayFieldTests : EncapsulateFieldInteractiveRefactoring
{
private EncapsulateFieldTestSupport Support { get; } = new EncapsulateFieldTestSupport();

[SetUp]
public void ExecutesBeforeAllTests()
{
Support.ResetResolver();
}

[TestCase("Private", "mArray(5) As String", "mArray(5) As String")]
[TestCase("Public", "mArray(5) As String", "mArray(5) As String")]
[TestCase("Private", "mArray(5,2,3) As String", "mArray(5,2,3) As String")]
Expand Down
Expand Up @@ -21,12 +21,6 @@ public class EncapsulateFieldTests : EncapsulateFieldInteractiveRefactoringTest
{
private EncapsulateFieldTestSupport Support { get; } = new EncapsulateFieldTestSupport();

[SetUp]
public void ExecutesBeforeAllTests()
{
Support.ResetResolver();
}

[TestCase("fizz", true, "baz", true, "buzz", true)]
[TestCase("fizz", false, "baz", true, "buzz", true)]
[TestCase("fizz", false, "baz", false, "buzz", true)]
Expand Down

0 comments on commit 4dc9bab

Please sign in to comment.