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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks really good, everything else is pretty superficial I think.

I think we'll want to add one lit test for the whole two-step tweak workflow, as well as TestTU/annotation-based unit-tests for each tweak.
As this patch has no actual tweaks in it, we can work out the details when landing the first one.



================
Comment at: clangd/ClangdLSPServer.cpp:38
+/// Transforms a tweak into a code action that would apply it if executed.
+CodeAction toCodeAction(const Tweak &T, const URIForFile &File,
+                        Range Selection) {
----------------
comment: tweak must have been successfully prepared?


================
Comment at: clangd/ClangdLSPServer.cpp:43
+  CA.kind = CodeAction::REFACTOR_KIND;
+  // This action requires an expensive second stage, we only run it if
+  // the user actually chooses the action. So we reply with a command to
----------------
Comment should be more tentative (calculating edits *may* be expensive) since we don't have the immediate-edit optimization yet.
Decent place for a FIXME or comment regarding the optimization too.


================
Comment at: clangd/ClangdServer.h:208
+  /// Enumerate the code tweaks available to the user at a specified point.
+  void enumerateCodeTweaks(PathRef File, Range Sel,
+                            Callback<std::vector<std::unique_ptr<Tweak>>> CB);
----------------
The returned `Tweak` object contains references to the AST, which may be dangling, so it's not actually safe to use.
It would be nice to allow callers to either directly apply a returned tweak (with progress saved) or to apply one "later" by name, but the way that transactions work in ClangdServer only the latter is really possible.

So I'd suggest this should return vector<pair<TweakID, string>>, which makes it clear(ish) how this relates to `applyCodeTweak`


================
Comment at: clangd/ClangdServer.h:208
+  /// Enumerate the code tweaks available to the user at a specified point.
+  void enumerateCodeTweaks(PathRef File, Range Sel,
+                            Callback<std::vector<std::unique_ptr<Tweak>>> CB);
----------------
sammccall wrote:
> The returned `Tweak` object contains references to the AST, which may be dangling, so it's not actually safe to use.
> It would be nice to allow callers to either directly apply a returned tweak (with progress saved) or to apply one "later" by name, but the way that transactions work in ClangdServer only the latter is really possible.
> 
> So I'd suggest this should return vector<pair<TweakID, string>>, which makes it clear(ish) how this relates to `applyCodeTweak`
`enumerateTweaks`?


================
Comment at: clangd/ClangdServer.h:212
+  /// Apply the code tweak with a specified \p ID.
+  void applyCodeTweak(PathRef File, Range Sel, TweakID ID,
+                      Callback<tooling::Replacements> CB);
----------------
`tweak`? It's a verb...


================
Comment at: clangd/Protocol.cpp:426
+const llvm::StringLiteral ExecuteCommandParams::CLANGD_APPLY_CODE_ACTION =
+    "clangd.applyCodeAction";
+
----------------
tweak


================
Comment at: clangd/Protocol.h:635
 
+struct CodeActionArgs {
+  URIForFile file;
----------------
TweakArgs?


================
Comment at: clangd/Protocol.h:637
+  URIForFile file;
+  std::string actionId;
+  Range selection;
----------------
"tweakId" or just "id"


================
Comment at: clangd/Protocol.h:655
+  // Command to apply the code action. Uses CodeActionArgs as argument.
+  const static llvm::StringLiteral CLANGD_APPLY_CODE_ACTION;
 
----------------
CLANGD_TWEAK_COMMAND


================
Comment at: clangd/refactor/Tweak.h:40
+  struct Selection {
+    static llvm::Optional<Selection> create(llvm::StringRef File,
+                                            llvm::StringRef Code,
----------------
Not convinced about this helper function.
 - much of it is just a passthrough to struct initialization. I think the code calling it would be clearer if it was initialising the fields one-by one
 - the only part that's not passthrough is already a function call with a clear name, calling it seems reasonable
 - I'm not sure it makes sense to have the Range -> SourceLocation conversion in this file, but the Tweak -> CodeAction conversion outside this file (and not unit-testable). There's an argument to be make to keep this file independent of LSP protocol structs, but I think that argument applies equally to this function.


================
Comment at: clangd/refactor/Tweak.h:40
+  struct Selection {
+    static llvm::Optional<Selection> create(llvm::StringRef File,
+                                            llvm::StringRef Code,
----------------
sammccall wrote:
> Not convinced about this helper function.
>  - much of it is just a passthrough to struct initialization. I think the code calling it would be clearer if it was initialising the fields one-by one
>  - the only part that's not passthrough is already a function call with a clear name, calling it seems reasonable
>  - I'm not sure it makes sense to have the Range -> SourceLocation conversion in this file, but the Tweak -> CodeAction conversion outside this file (and not unit-testable). There's an argument to be make to keep this file independent of LSP protocol structs, but I think that argument applies equally to this function.
Expected<Selection>? Passing an invalid range is always an error I guess.


================
Comment at: clangd/refactor/Tweak.h:59
+  /// A unique id of the action. The convention is to
+  /// lower-case-with-dashes for the identifier.
+  virtual TweakID id() const = 0;
----------------
nit: one of my lessons from clang-tidy is that mapping between IDs and implementations is annoying.
Since IDs are 1:1 with classes, can we just require this to be the class name?

(If you wanted, I think you could adapt REGISTER_TWEAK so that it goes inside the class defn, and then it could provide the override of id() itself)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56267