[PATCH] D56267: [clangd] Interfaces for writing code actions

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

High level comments:

- this is an important concept and in hindsight it's amazing we didn't have it yet!
- I don't get a clear sense of the boundaries between "code action", "refactoring", and "action".  There are also "prepared actions" and "instant actions".  I think some combination of fewer concepts, clearer names, and more consistent usage would help a lot.
- it's pretty heavy on registries/factories/preparedX, maybe we can find a way to simplify
- many (most?) actions are going to do some "walk up AST looking for X", the interface could help make that easy



================
Comment at: clangd/ClangdLSPServer.cpp:346
+                  {ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND,
+                   ExecuteCommandParams::CLANGD_APPLY_CODE_ACTION}},
              }},
----------------
Seems a bit more direct to give each one its own command name? Maybe under a namespace like refactor/swapIfBranches


================
Comment at: clangd/ClangdLSPServer.cpp:662
+        std::vector<CodeAction> Actions = std::move(FixIts);
+        auto toCodeAction = [&](PreparedAction &&A) -> CodeAction {
+          CodeAction CA;
----------------
This seems like it could just be a helper function... what does it capture?
I think it might belong next to PreparedActions just like we have `toLSPDiags` in `Diag.h` - it's not ideal dependency-wise, but inlining everything into ClangdLSPServer seems dubious too. Happy to discuss offline...


================
Comment at: clangd/ClangdLSPServer.cpp:668
+            // This is an instant action, so we reply with a command to directly
+            // apply the edit that the action has already produced.
+            auto R = cantFail(std::move(A).computeEdits());
----------------
This optimization seems inessential, and complicates the model a bit. Can we leave it out for now?


================
Comment at: clangd/ClangdServer.h:213
+
+  /// Apply the previously precomputed code action. This will always fully
+  /// execute the action.
----------------
Unclear what the second sentence means, I'd suggest dropping it. Most obvious (to me) interpretation is that this can't fail, which isn't true.
nit: "Previously precomputed" is redundant :-)



================
Comment at: clangd/CodeActions.h:16
+
+#include "refactor/ActionProvider.h"
+
----------------
why is ActionProvider in refactor/ but CodeActions/ is here?


================
Comment at: clangd/CodeActions.h:23
+/// running the actions.
+class CodeActions {
+public:
----------------
I'm not sure this indirection is necessary, would a StringMap<u_p<ActionProvider>> suffice?

Since it's not actually complicated and there's just one caller, I think knowing what `prepareAll` is doing at the callsite would be an improvement.


================
Comment at: clangd/refactor/ActionProvider.h:67
+  // FIXME: provide a way to get sources and ASTs for other files.
+  // FIXME: cache some commonly required information (e.g. AST nodes under
+  //        cursor) to avoid redundant AST visit in every action.
----------------
I think we should consider (not necessarily adopt) designing the interface around walking up the AST node chain, rather than bolting it on later.


================
Comment at: clangd/refactor/ActionProvider.h:71
+
+using ActionID = size_t;
+/// Result of executing a first stage of the action. If computing the resulting
----------------
I'd prefer a string here, this will make the protocol easier to understand for human readers.


================
Comment at: clangd/refactor/ActionProvider.h:78
+/// so it's not safe to store the PreparedAction for long spans of time.
+class PreparedAction {
+public:
----------------
This is a concrete class that holds a type-erased unique_function with the actual logic. Similarly, the factories are just wrappers of unique_function.
This method of abstraction is harder (for me at least) to follow than expressing the commonality of the important objects (the refactorings) as inheritance. And more concretely, stack traces are going to be terrible :-)

The factory/prepared duality makes it hard to represent a refactoring as a single class, but let's have a think about this part.


================
Comment at: clangd/refactor/ActionProvider.h:87
+  /// CodeActions::prepare().
+  ActionID id() const { return ID; }
+  /// A title of the action for display in the UI.
----------------
Why is this dynamic and not a constructor param?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56267/new/

https://reviews.llvm.org/D56267