Make CodeActionEditHandlerService support multiple ApplyChangesOperations, and other Operations that update the solution #246
Description
Ported from TFS WorkItem: 1079724
Repro Steps:
Affected Branch:
Affected Build:
Affected Product Language:
Steps to reproduce:
Note: discussed this with HeeJae offline
The RenameTrackingCodeAction modifies the solution without extending from ApplyChangesCodeAction, and only one solution-changing CodeActionOperation is allowed/expected by CodeActionEditHandlerService, so a theoretical CodeAction that did what RenameTrackingCodeAction did and also provided another ApplyChangesOperation could trick that infrastructure into applying changes to stale solutions.
Talked with HeeJae about this and we agree that we should remove the restriction of having at most one ApplyChangesOperation, which would allow RenameTracking to accomplish its two-phase undo system by providing two ApplyChangesCodeAction instead of its own special CodeAction that applies multiple changes to the workspace. This also protects the CodeActionEditHandlerService from accidentally applying bad edits.
Revisions:
- Created By David Poeschl (11/10/2014 4:14:55 PM)
- Edited By Manish Vasani (12/11/2014 11:51:33 PM)
I think even this one needs to be discussed at the design meeting/team meeting as there are multiple possible scenarios for creating multiple ApplyChangesOperations in a CodeAction:
Sent: Thursday, December 11, 2014 5:18 PM
To: HeeJae Chang; David Poeschl; Srivatsn Narayanan
Subject: Bug 1079724: Make CodeActionEditHandlerService support multiple ApplyChangesOperations, and other Operations that update the solution
I am not sure I understand the exact design required for this bug. Allowing multiple ApplyChangesOperations could mean all of them are independent and based off the same base solution (batch edits) OR all of them are sequential and each one is based off previous ApplyChangesOperation’s updated solution OR there are a mix of these. For the batch case, we can compute merged changed solution with appropriate merge resolution (like linked file and Fix all batch fixer cases). We can add another field on ApplyChangesOperation ctor to take the base solution for it and then determine if it is batch or sequential case and do the appropriate edit. However, I am not sure if we want to/should support all these cases. Did we have something specific in mind here?
Maybe one of the possible outcome (which I haven't mentioned in the email above) is to retain the current design of allowing a single ApplyChangesOperation. When a code action needs multiple ApplyChanges, possibly interleaved with other operations, maybe its best to let the client deal with applying the changes themselves by using custom code action operations, rather then us having to create a complicated infrastructure of handling serial/batch changes and merging these together, when we don't even know if there would be many scenarios that require this support.
Our current support is quite similar to batch fix all provider, we provided infrastructure for simple code actions with single apply changes operation, but ask the clients to do it themselves for complicated code actions such as rename needing multiple solution updates.