Reorganizing Rewriting #4465

MDoerner · 2018-10-27T17:25:33Z

This PR is still WIP. However, I wanted to put it out for review to get featback on the setup before doing all the rewiring necessary.

This PR aims to provide more structure to how rewriting is handled in RD. Currently, Any feature wanting to rewrite modules simply gets the appropriate rewirters from the RubberduckParserState and then either calls Rewrite on the rewriters directly or calls RewriteAllModules on the RubberduckParserState. This is rather unstructured and has some caching problems because everybody gets the same rewriters cached on the state. Moreover, it is completely up to the feature whether a reparse gets initiated afterwards and whether a selection gets recovered.

This PR introduces the concept of a IRewriteSession, which is an independent unit of rewriting. It hands out individual IModuleRewriters that no longer have a Rewrite method. To apply the changes, Rewrite has to be called on the IRewriteSession. Doing so, causes all checked out rewriters to be applied. Moreover, it forces a reparse. (Here, other behaviour like recovering the selection or recreating member attributes could be added in future PRs.)

Individual IRewriteSessions can be acquired from the IRewirtingManager. It holds a list of all active sessions and will invalidate all of them once one of them actually applies its rewrites. Moreover, on each parse, all rewrite sessions will get invalidated.

Future steps will include:

Wire up all users of the rewriters to use appropriate IRewriteSessions. (done)
Store ITokenstreams on the ModuleStates instead of rewriters. (done)
Constructor inject the IRewritingManager wherever it gets acquired from the RubberduckParserState. (done)
Remove the RewritingManager property from the RubberduckParserState. (done)
Write unit tests for the RewritingManager and the IRewritesSession implementation. (done)

The extraction is performed into the new IExecutableModuleRewriter, which extends IModuleRewriter. This change allows to restrict the access to triggering the actual application of the changes. This will be important in the next commits. It was necessary to change some other functionality using the rewriter with dynamic arguments in order to avoid runtime binder exceptions. (It does not see the interfaces that get extended.) The first change was to the (I)ModuleRewriter itself. I added a new overload both for Replace and Remove taking an IParseTree. This is implemented using pattern matching to prefer execution for the ITerminalNode and then falling back to ParserRuleContext, which is always a valid downcast for IParseTree. This allows to avoid using dynamic to prefer the calls with ITerminalNode. The other changes were explicit casts of the second argument of Replace, which only accepts strings anyway and a change from dynamic to patterns matching in a quickfix.

These two are meant to build the foundation for a new way to manage rewriting modules in a consistent fashion. When a feature wants to rewrite a module, it needs to have the IRewritingManager injected. Then it can request a IRewriteSession from it, either for code pane rewrites or attributes rewrites. The sessions manage the rewriting of all checked out rewriters and function as a point to perform pre- and post-rewrite actions in a consistent manner. The manager and sessions are set up such that only one active rewrite session may ever perform a successful rewrite: the manager will invalidate all active rewrite sessions on the first rewrite of an active rewrite session. (To coordinate this a callback gets passed to the freshly created sessions.) The current setup of the StateTokenStreamCache is temporary only; a change to only storing ITokenStreams on the RubberduckParserState is envisioned for the future.

When parsing, all rewrite sessions get invalidated because any change, like removing components, changing referenced projects or their priority or reparsing components may invalidate the basis for a rewrite or make the underlying component inaccessible.

This change is to be reverted later. It allows to decouple the work necessary to change the users of the IModuleRewriter to use the IRewritingManager from the work necessary to constructor inject the IRewritingManager into them. (The latter is a lot of work in the tests.) Here, property injection is used because the construction of the RewritingManager ultimately depends on the RubberduckParserState to be there.

This is a first step to make them use the IRewriteSession model. Passing it is, will allow the QuickFixProvider to pass in the same session for all results when fixing more than one resulte.g. for FixAll.

This change ensures that whenever a fix is used for multiple results, the same IRewriteSession is used.

bclothier

Very nice! Mainly questions.

Rubberduck.CodeAnalysis/QuickFixes/RemoveDuplicatedAnnotationQuickFix.cs

Rubberduck.Parsing/Rewriter/AttributesRewriteSession.cs

Rubberduck.CodeAnalysis/Inspections/Abstract/QuickFixBase.cs

comintern · 2018-10-28T20:41:12Z

Linking #4356 - was going to take a look at that one, but based on the errors it would be better to wait for this PR to merge.

The tests are still pending; that is another undertaking.

In addition, the tests are streamlined to use methods from a base class in order to reduce code duplication.

This is now handled via IRewriteSessions.

This is only a first step. The rewriters are still stored on the ModuleStates because providing a new one each time a rewriter is requested from the RubberduckParserState breaks the refactoring tests. Ultimately, the rewriters should only be created via the IRewriteSession.

With the new rewriting setup the source of IModuleRewriters changed.

It was only ever needed because of a superfluous part of the execution.

…o them The quickfixes using a refactoring currently cannot pass on the IRewriteSession to the refactoring, which handles its own rewriting separately. Moreover, the AddIdentifierToWhitelistQuickfix does not need it because it is not changing code.

…tial selection Previously, it used a tuple of ICodeModule and Selection, which was leaking SCWs.

This includes a bug fix to the RewritingManager.

The ReorderParametersCommand and its tests had to change, too.

Also contains one correction to a test for the RenameRefactoring

All interaction with rewriters is to go through an IRewriteSession in the future, which is acquired from an IRewritingManager.

All users have been rewired. So, it does not have to be property injected anymore.

# Conflicts: # Rubberduck.CodeAnalysis/QuickFixes/RemoveUnassignedVariableUsageQuickFix.cs # RubberduckTests/QuickFixes/AccessSheetUsingCodeNameQuickFixTests.cs # RubberduckTests/Refactoring/MoveCloserToUsageTests.cs

Rubberduck.Parsing/Rewriter/RewritingManager.cs

Contains extraction of a partial interface from the RubberduckParserState (IParseManager) in order to allow mocking the corresponding functionality.

Also makes the IRewriteSession expose the target code kind in order to make the RewritingManager testable.

# Conflicts: # Rubberduck.CodeAnalysis/QuickFixes/RenameDeclarationQuickFix.cs

RubberduckTests/Rewriter/RewriteManagerTests.cs

RubberduckTests/Rewriter/CodePaneRewriteSessionTests.cs

This change on the IRewriteSession allows the user to see whether the rewrite actually happened successfully.

The Rewrite member is no longer on the interface.

Vogel612

I've skipped all the tests. 👍 from me, there's some tech-debt with how refactorings work, but that's out of scope for this PR.

Rubberduck.CodeAnalysis/QuickFixes/RemoveDuplicatedAnnotationQuickFix.cs

Rubberduck.CodeAnalysis/QuickFixes/RemoveEmptyIfBlockQuickFix.cs

Rubberduck.Parsing/Rewriter/RewriteSessionBase.cs

Rubberduck.Refactorings/ExtractInterface/ExtractInterfaceRefactoring.cs

Vogel612 · 2018-11-14T11:25:47Z

Rubberduck.Refactorings/RemoveParameters/RemoveParametersRefactoring.cs

+            AdjustReferences(_model.TargetDeclaration.References, _model.TargetDeclaration, rewritingSession);
+            AdjustSignatures(rewritingSession);
+
+            rewritingSession.TryRewrite();


is there a contract that refactorings have to trigger the sessions's rewrite themselves? If not, I'd prefer they worked like quickfixes do ..

I asked about the approach to take in this PR in chat. @retailcoder opted for this version. The alternative would have required to change the IRefactoring interface.

Vogel612 · 2018-11-14T11:28:05Z

Rubberduck.Refactorings/Rename/RenameRefactoring.cs

-                    Reparse();
+                    var rewriteSession = _rewritingManager.CheckOutCodePaneSession();
+                    Rename(rewriteSession);
+                    rewriteSession.TryRewrite();


I'm a bit annoyed that each Refactoring implementation needs to remember doing that. Is there a clean way to fix this?

I assume there's some issues with refactorings that use dialogs that makes it hard to correctly unify this...

They all require a session to be able to rewrite and that session and the changes need to be applied in the end. Before this PR, it was not much better: every refactoring had to call rewrite on the appropriate rewriters in the end.

Moreover, every refactoring uses a different path through the interface methods to apply the refactoring.

So, without further cleanup or providing the session via method injection (see another comment above) I do not see how to put this in a base class.

Removed a typo and a superfluous comment, changed to property use from backing field in one occasion and adapted the exception message in a switch statement.

MDoerner added 6 commits October 25, 2018 01:00

Wire up the IRewritingManager

31c4bb0

When parsing, all rewrite sessions get invalidated because any change, like removing components, changing referenced projects or their priority or reparsing components may invalidate the basis for a rewrite or make the underlying component inaccessible.

Optionally pass a IRewriteSession to quickfixes

e28fadd

This is a first step to make them use the IRewriteSession model. Passing it is, will allow the QuickFixProvider to pass in the same session for all results when fixing more than one resulte.g. for FixAll.

Provide IRewriteSessions to quickfixes in QuickFixProvider

ee12ff5

This change ensures that whenever a fix is used for multiple results, the same IRewriteSession is used.

MDoerner added PR-Status: WIP Pull request is work-in-progress, more commits should be expected. PR-Status: Review Requested No more commits, PR is ready for the eyes that need to see it. labels Oct 27, 2018

bclothier reviewed Oct 28, 2018

View reviewed changes

MDoerner added 4 commits October 31, 2018 12:50

Make the quickfixes use the IRewritesession (DOES NOT COMPILE)

0eb245d

The tests are still pending; that is another undertaking.

Adjust the quickfix tests to the new setup of rewriting

76426be

In addition, the tests are streamlined to use methods from a base class in order to reduce code duplication.

Remove RewriteAllModules from RubberduckParserState

13f7f41

This is now handled via IRewriteSessions.

MDoerner added the PR-Status: Conflicting PR can't be merged as it stands, conflicts must be resolved by the author. label Nov 1, 2018

MDoerner added 15 commits November 2, 2018 12:12

Rewire and streamline the ModuleRewriter tests

e28d67c

With the new rewriting setup the source of IModuleRewriters changed.

Remove _projectsProvider from ReorderParameter refactoring

a297eab

It was only ever needed because of a superfluous part of the execution.

Make the RenameRefactoring use a QualifiedSelection? to store the ini…

e927aa8

…tial selection Previously, it used a tuple of ICodeModule and Selection, which was leaking SCWs.

Rewire the RenameRefactoring and its tests

fa2dd3b

This includes a bug fix to the RewritingManager.

Rewire the ReorderParametersRefactoring and its tests

3d55d22

The ReorderParametersCommand and its tests had to change, too.

Rewire the MoveCloserToUsageRefactoring

df91612

Rewire the EncapsulateFieldRefactoring

245e0fc

Also contains one correction to a test for the RenameRefactoring

Rewire the IntroduceParameterRefactoring

15a9530

Rewire the IntroduceFieldRefactoring

2ad16f6

Rewire the ImplementInterface- and ExtractInterfaceRefactoring

d3267ce

Rewire the RemoveParameterRefactoring

5f5ffe7

Remove the rewriters from the ModuleStates and RubberduckParserState

f22b5cc

All interaction with rewriters is to go through an IRewriteSession in the future, which is acquired from an IRewritingManager.

Remove the RewriteManager from the RubberDuckParserState

7a81d8f

All users have been rewired. So, it does not have to be property injected anymore.

Merge branch 'next' into ReorganizingRewriting2

2c5bd70

# Conflicts: # Rubberduck.CodeAnalysis/QuickFixes/RemoveUnassignedVariableUsageQuickFix.cs # RubberduckTests/QuickFixes/AccessSheetUsingCodeNameQuickFixTests.cs # RubberduckTests/Refactoring/MoveCloserToUsageTests.cs

MDoerner removed the PR-Status: Conflicting PR can't be merged as it stands, conflicts must be resolved by the author. label Nov 3, 2018

bclothier reviewed Nov 3, 2018

View reviewed changes

Rubberduck.Parsing/Rewriter/RewritingManager.cs Show resolved Hide resolved

MDoerner mentioned this pull request Nov 4, 2018

The tests should more follow the DRY principle #4489

Open

MDoerner added 3 commits November 6, 2018 01:36

Add tests for IRewriteSession implementations

0ded26b

Contains extraction of a partial interface from the RubberduckParserState (IParseManager) in order to allow mocking the corresponding functionality.

Add tests for the RewritingManager

16a5607

Also makes the IRewriteSession expose the target code kind in order to make the RewritingManager testable.

Merge branch 'next' into ReorganizingRewriting2

7450c58

# Conflicts: # Rubberduck.CodeAnalysis/QuickFixes/RenameDeclarationQuickFix.cs

MDoerner removed the PR-Status: WIP Pull request is work-in-progress, more commits should be expected. label Nov 6, 2018

bclothier reviewed Nov 6, 2018

View reviewed changes

RubberduckTests/Rewriter/RewriteManagerTests.cs Show resolved Hide resolved

bclothier reviewed Nov 6, 2018

View reviewed changes

RubberduckTests/Rewriter/CodePaneRewriteSessionTests.cs Show resolved Hide resolved

MDoerner added 2 commits November 7, 2018 23:15

Change Rewrite to TryRewrite

4494b8d

This change on the IRewriteSession allows the user to see whether the rewrite actually happened successfully.

Amend doc-comments in IModuleRewriter

bd9c9c2

The Rewrite member is no longer on the interface.

bclothier approved these changes Nov 7, 2018

View reviewed changes

MDoerner added 2 commits November 12, 2018 15:38

Merge branch 'next' into ReorganizingRewriting2

57ebb43

Merge branch 'next' into ReorganizingRewriting2

0c22259

Vogel612 approved these changes Nov 14, 2018

View reviewed changes

MDoerner added 2 commits November 14, 2018 21:41

Make some minor stylistic adjustments

299e08a

Removed a typo and a superfluous comment, changed to property use from backing field in one occasion and adapted the exception message in a switch statement.

Merge branch 'next' into ReorganizingRewriting2

5986308

Vogel612 merged commit bf75416 into rubberduck-vba:next Nov 14, 2018

MDoerner mentioned this pull request Nov 15, 2018

RD should recover the selection after each rewrite in a consistent way #4524

Closed

MDoerner deleted the ReorganizingRewriting2 branch December 5, 2018 21:13

bclothier mentioned this pull request Jan 17, 2019

Same issue with two different "Refactor" methods #4356

Closed

MDoerner removed the PR-Status: Review Requested No more commits, PR is ready for the eyes that need to see it. label Feb 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganizing Rewriting #4465

Reorganizing Rewriting #4465

MDoerner commented Oct 27, 2018 •

edited

bclothier left a comment

comintern commented Oct 28, 2018

Vogel612 left a comment

Vogel612 Nov 14, 2018

MDoerner Nov 14, 2018 •

edited

Vogel612 Nov 14, 2018

MDoerner Nov 14, 2018

Reorganizing Rewriting #4465

Reorganizing Rewriting #4465

Conversation

MDoerner commented Oct 27, 2018 • edited

bclothier left a comment

Choose a reason for hiding this comment

comintern commented Oct 28, 2018

Vogel612 left a comment

Choose a reason for hiding this comment

Vogel612 Nov 14, 2018

Choose a reason for hiding this comment

MDoerner Nov 14, 2018 • edited

Choose a reason for hiding this comment

Vogel612 Nov 14, 2018

Choose a reason for hiding this comment

MDoerner Nov 14, 2018

Choose a reason for hiding this comment

MDoerner commented Oct 27, 2018 •

edited

MDoerner Nov 14, 2018 •

edited