Rubberduck: IModuleRewriter.Remove leaves Accessibility Token behind for declaration Lists

[BZngr]

Rubberduck version information
Version 2.4.1.24212
OS: Microsoft Windows NT 10.0.18362.0, x64
Host Product: Microsoft Office x86
Host Version: 16.0.12228.20364
Host Executable: EXCEL.EXE

Host Executable: [...]

Description
IModuleRewriter when acting on a variable list one by one, leaves the Accessibility token behind if all the variables are removed.

To Reproduce
A failing unit test:

        [Test]
        public void RewriterRemoveTest()
        {
            const string inputCode =
@"
Public mVar1 As Integer, mVar2 As String, mVar3 As Long

Private cVar1 As Integer, mVar4 As String";

            var vbe = TestVbe(inputCode, out _);
            var (state, rewritingManager) = MockParser.CreateAndParseWithRewritingManager(vbe);
            using (state)
            {
                var mVariables = state.DeclarationFinder.UserDeclarations(DeclarationType.Variable)
                    .Where(v => v.IdentifierName.StartsWith("m"));


                var rewriteSession = rewritingManager.CheckOutCodePaneSession();
                var rewriter = rewriteSession.CheckOutModuleRewriter(mVariables.First().QualifiedModuleName);
                foreach (var variable in mVariables)
                {
                    rewriter.Remove(variable);
                }

                StringAssert.DoesNotContain("Public", rewriter.GetText());
            }
        }

What is left behind in the above test scenario is:

Private cVar As Integer

The same is true when acting on a list of Constants except that it leaves Public Const behind.

[Vogel612]

This can not be the responsibility of IModule rewriter to resolve, because each rewriting request should be independent.

Instead the requesting quickfix or refactoring (note that this especially applies for "Fix All" invocations) needs to determine whether an additional context is required.

[MDoerner]

Removing a declaration is just a shorthand for removing its context. As can be seen from the grammar, the accessibility specifier is not part of any of the variables, but a part of the variableStmt, which contains said specifier and the variableListStmt, which itself contains the individual variableSubStmts of the variable declarations. Thus, the behaviour is as intended.

The rewriter API is very low-level. As stated above already, it is the caller's responsibility to look at the contexts involved in the rewrite operation and to pick exactly those wanted. If you want a functionality that removes variables, you will have to implement it as kind of a _mini-refactoring_ and then use that is your code.