[PATCH] D56267: [clangd] Interfaces for writing code actions
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE352494: [clangd] Interfaces for writing code tweaks
(authored by ibiryukov, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D56267?vs=184039=184074#toc
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/refactor/Tweak.cpp
clangd/refactor/Tweak.h
clangd/refactor/tweaks/CMakeLists.txt
clangd/refactor/tweaks/Dummy.cpp
clangd/tool/CMakeLists.txt
test/clangd/fixits-command.test
test/clangd/initialize-params.test
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -421,6 +421,9 @@
const llvm::StringLiteral ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND =
"clangd.applyFix";
+const llvm::StringLiteral ExecuteCommandParams::CLANGD_APPLY_TWEAK =
+"clangd.applyTweak";
+
bool fromJSON(const llvm::json::Value , ExecuteCommandParams ) {
llvm::json::ObjectMapper O(Params);
if (!O || !O.map("command", R.command))
@@ -431,6 +434,8 @@
return Args && Args->size() == 1 &&
fromJSON(Args->front(), R.workspaceEdit);
}
+ if (R.command == ExecuteCommandParams::CLANGD_APPLY_TWEAK)
+return Args && Args->size() == 1 && fromJSON(Args->front(), R.tweakArgs);
return false; // Unrecognized command.
}
@@ -497,10 +502,13 @@
auto Cmd = llvm::json::Object{{"title", C.title}, {"command", C.command}};
if (C.workspaceEdit)
Cmd["arguments"] = {*C.workspaceEdit};
+ if (C.tweakArgs)
+Cmd["arguments"] = {*C.tweakArgs};
return std::move(Cmd);
}
const llvm::StringLiteral CodeAction::QUICKFIX_KIND = "quickfix";
+const llvm::StringLiteral CodeAction::REFACTOR_KIND = "refactor";
llvm::json::Value toJSON(const CodeAction ) {
auto CodeAction = llvm::json::Object{{"title", CA.title}};
@@ -544,6 +552,17 @@
return llvm::json::Object{{"changes", std::move(FileChanges)}};
}
+bool fromJSON(const llvm::json::Value , TweakArgs ) {
+ llvm::json::ObjectMapper O(Params);
+ return O && O.map("file", A.file) && O.map("selection", A.selection) &&
+ O.map("tweakID", A.tweakID);
+}
+
+llvm::json::Value toJSON(const TweakArgs ) {
+ return llvm::json::Object{
+ {"tweakID", A.tweakID}, {"selection", A.selection}, {"file", A.file}};
+}
+
llvm::json::Value toJSON(const ApplyWorkspaceEditParams ) {
return llvm::json::Object{{"edit", Params.edit}};
}
Index: clangd/CMakeLists.txt
===
--- clangd/CMakeLists.txt
+++ clangd/CMakeLists.txt
@@ -71,6 +71,8 @@
index/dex/PostingList.cpp
index/dex/Trigram.cpp
+ refactor/Tweak.cpp
+
LINK_LIBS
clangAST
clangASTMatchers
@@ -108,6 +110,7 @@
${CLANGD_ATOMIC_LIB}
)
+add_subdirectory(refactor/tweaks)
if( LLVM_LIB_FUZZING_ENGINE OR LLVM_USE_SANITIZE_COVERAGE )
add_subdirectory(fuzzer)
endif()
Index: clangd/tool/CMakeLists.txt
===
--- clangd/tool/CMakeLists.txt
+++ clangd/tool/CMakeLists.txt
@@ -3,6 +3,7 @@
add_clang_tool(clangd
ClangdMain.cpp
+ $
)
set(LLVM_LINK_COMPONENTS
Index: clangd/SourceCode.cpp
===
--- clangd/SourceCode.cpp
+++ clangd/SourceCode.cpp
@@ -141,6 +141,16 @@
return P;
}
+llvm::Expected sourceLocationInMainFile(const SourceManager ,
+Position P) {
+ llvm::StringRef Code = SM.getBuffer(SM.getMainFileID())->getBuffer();
+ auto Offset =
+ positionToOffset(Code, P, /*AllowColumnBeyondLineLength=*/false);
+ if (!Offset)
+return Offset.takeError();
+ return SM.getLocForStartOfFile(SM.getMainFileID()).getLocWithOffset(*Offset);
+}
+
Range halfOpenToRange(const SourceManager , CharSourceRange R) {
// Clang is 1-based, LSP uses 0-based indexes.
Position Begin = sourceLocToPosition(SM, R.getBegin());
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -16,11 +16,13 @@
#include "XRefs.h"
#include "index/FileIndex.h"
#include "index/Merge.h"
+#include "refactor/Tweak.h"
#include "clang/Format/Format.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Tooling/CompilationDatabase.h"
+#include "clang/Tooling/Core/Replacement.h"
#include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
#include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
#include "llvm/ADT/ArrayRef.h"
@@ -28,10 +30,12 @@
#include
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov updated this revision to Diff 184039.
ilya-biryukov added a comment.
- Update license headers in new files
- Add an empty cpp file to avoid cmake errors due to empty inputs
- clang-format
- Update the 'fixits-command.test' to unbreak it (the line number was larger
than the number of lines in the file)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
clang-tools-extra/clangd/refactor/tweaks/Dummy.cpp
clang-tools-extra/clangd/tool/CMakeLists.txt
clang-tools-extra/test/clangd/fixits-command.test
clang-tools-extra/test/clangd/initialize-params.test
clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp
Index: clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp
===
--- clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp
+++ clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp
@@ -11,6 +11,7 @@
#include "SourceCode.h"
#include "TestTU.h"
#include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Testing/Support/Error.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -18,6 +19,8 @@
namespace clangd {
namespace {
+using llvm::Failed;
+using llvm::HasValue;
using testing::ElementsAre;
using testing::Field;
using testing::IsEmpty;
@@ -338,6 +341,37 @@
EXPECT_THAT(AST.getLocalTopLevelDecls(), ElementsAre(DeclNamed("main")));
}
+TEST(ClangdUnitTest, SourceLocationInMainFile) {
+ Annotations Source(R"cpp(
+^in^t ^foo
+^bar
+^baz ^() {} {} {} {} { }^
+)cpp");
+
+ SourceManagerForFile Owner("foo.cpp", Source.code());
+ SourceManager = Owner.get();
+
+ SourceLocation StartOfFile = SM.getLocForStartOfFile(SM.getMainFileID());
+ EXPECT_THAT_EXPECTED(sourceLocationInMainFile(SM, pos(0, 0)),
+ HasValue(StartOfFile));
+ // End of file.
+ EXPECT_THAT_EXPECTED(
+ sourceLocationInMainFile(SM, pos(4, 0)),
+ HasValue(StartOfFile.getLocWithOffset(Source.code().size(;
+ // Column number is too large.
+ EXPECT_THAT_EXPECTED(sourceLocationInMainFile(SM, pos(0, 1)), Failed());
+ EXPECT_THAT_EXPECTED(sourceLocationInMainFile(SM, pos(0, 100)), Failed());
+ EXPECT_THAT_EXPECTED(sourceLocationInMainFile(SM, pos(4, 1)), Failed());
+ // Line number is too large.
+ EXPECT_THAT_EXPECTED(sourceLocationInMainFile(SM, pos(5, 0)), Failed());
+ // Check all positions mentioned in the test return valid results.
+ for (auto P : Source.points()) {
+size_t Offset = llvm::cantFail(positionToOffset(Source.code(), P));
+EXPECT_THAT_EXPECTED(sourceLocationInMainFile(SM, P),
+ HasValue(StartOfFile.getLocWithOffset(Offset)));
+ }
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/test/clangd/initialize-params.test
===
--- clang-tools-extra/test/clangd/initialize-params.test
+++ clang-tools-extra/test/clangd/initialize-params.test
@@ -25,7 +25,8 @@
# CHECK-NEXT: "documentSymbolProvider": true,
# CHECK-NEXT: "executeCommandProvider": {
# CHECK-NEXT:"commands": [
-# CHECK-NEXT: "clangd.applyFix"
+# CHECK-NEXT: "clangd.applyFix",
+# CHECK-NEXT: "clangd.applyTweak"
# CHECK-NEXT:]
# CHECK-NEXT: },
# CHECK-NEXT: "hoverProvider": true,
Index: clang-tools-extra/test/clangd/fixits-command.test
===
--- clang-tools-extra/test/clangd/fixits-command.test
+++ clang-tools-extra/test/clangd/fixits-command.test
@@ -23,7 +23,7 @@
# CHECK-NEXT:"uri": "file://{{.*}}/foo.c"
# CHECK-NEXT: }
---
-{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses"}]}}}
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses"}]}}}
# CHECK:
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Protocol.cpp:425 +const llvm::StringLiteral ExecuteCommandParams::CLANGD_APPLY_TWEAK = +"clangd.applyCodeAction"; + sammccall wrote: > tweak or applyTweak Done. Thanks for spotting this, totally missed it. Comment at: clang-tools-extra/clangd/SourceCode.h:59 +/// care to avoid comparing the result with expansion locations. +llvm::Expected sourceLocationInMainFile(const SourceManager , +Position P); sammccall wrote: > (can we add a unit test for this function?) Added a test in `ClangdUnitTests` for a lack of a better place. `SourceCodeTests` only test non-source-manager-related things, so adding a dependency on `SourceManager` there seemed weird. Happy to move them elsewhere, let me know what you think. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56267/new/ https://reviews.llvm.org/D56267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov updated this revision to Diff 183949.
ilya-biryukov marked 7 inline comments as done.
ilya-biryukov added a comment.
- s/applyCodeAction/applyTweak
- Added a comment to TweakArgs
- Added a unit test for sourceLocationInMainFile
- Extract a 'validateRegistry' function
- Do not log CancelledError.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
clang-tools-extra/clangd/tool/CMakeLists.txt
clang-tools-extra/test/clangd/initialize-params.test
clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp
Index: clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp
===
--- clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp
+++ clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp
@@ -11,6 +11,7 @@
#include "SourceCode.h"
#include "TestTU.h"
#include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Testing/Support/Error.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -18,6 +19,8 @@
namespace clangd {
namespace {
+using llvm::Failed;
+using llvm::HasValue;
using testing::ElementsAre;
using testing::Field;
using testing::IsEmpty;
@@ -338,6 +341,37 @@
EXPECT_THAT(AST.getLocalTopLevelDecls(), ElementsAre(DeclNamed("main")));
}
+TEST(ClangdUnitTest, SourceLocationInMainFile) {
+ Annotations Source(R"cpp(
+^in^t ^foo
+^bar
+^baz ^() {} {} {} {} { }^
+)cpp");
+
+ SourceManagerForFile Owner("foo.cpp", Source.code());
+ SourceManager = Owner.get();
+
+ SourceLocation StartOfFile = SM.getLocForStartOfFile(SM.getMainFileID());
+ EXPECT_THAT_EXPECTED(sourceLocationInMainFile(SM, pos(0, 0)),
+ HasValue(StartOfFile));
+ // End of file.
+ EXPECT_THAT_EXPECTED(
+ sourceLocationInMainFile(SM, pos(4, 0)),
+ HasValue(StartOfFile.getLocWithOffset(Source.code().size(;
+ // Column number is too large.
+ EXPECT_THAT_EXPECTED(sourceLocationInMainFile(SM, pos(0, 1)), Failed());
+ EXPECT_THAT_EXPECTED(sourceLocationInMainFile(SM, pos(0, 100)), Failed());
+ EXPECT_THAT_EXPECTED(sourceLocationInMainFile(SM, pos(4, 1)), Failed());
+ // Line number is too large.
+ EXPECT_THAT_EXPECTED(sourceLocationInMainFile(SM, pos(5, 0)), Failed());
+ // Check all positions mentioned in the test return valid results.
+ for (auto P : Source.points()) {
+size_t Offset = llvm::cantFail(positionToOffset(Source.code(), P));
+EXPECT_THAT_EXPECTED(sourceLocationInMainFile(SM, P),
+ HasValue(StartOfFile.getLocWithOffset(Offset)));
+ }
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/test/clangd/initialize-params.test
===
--- clang-tools-extra/test/clangd/initialize-params.test
+++ clang-tools-extra/test/clangd/initialize-params.test
@@ -25,7 +25,8 @@
# CHECK-NEXT: "documentSymbolProvider": true,
# CHECK-NEXT: "executeCommandProvider": {
# CHECK-NEXT:"commands": [
-# CHECK-NEXT: "clangd.applyFix"
+# CHECK-NEXT: "clangd.applyFix",
+# CHECK-NEXT: "clangd.applyTweak"
# CHECK-NEXT:]
# CHECK-NEXT: },
# CHECK-NEXT: "hoverProvider": true,
Index: clang-tools-extra/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/clangd/tool/CMakeLists.txt
@@ -3,6 +3,7 @@
add_clang_tool(clangd
ClangdMain.cpp
+ $
)
set(LLVM_LINK_COMPONENTS
Index: clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
@@ -0,0 +1,11 @@
+include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../..)
+
+# A target containing all code tweaks (i.e. mini-refactorings) provided by
+# clangd.
+# Built as an object library to make sure linker does not remove global
+# constructors that register individual tweaks in a global registry.
+# To enable these tweaks in exectubales or shared libraries, add
+# $ to a list of sources, see
+# clangd/tool/CMakeLists.txt for an example.
+add_clang_library(clangDaemonTweaks OBJECT
+ )
Index: clang-tools-extra/clangd/refactor/Tweak.h
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/Tweak.h
@@ -0,0 +1,99 @@
+//===--- Tweak.h
[PATCH] D56267: [clangd] Interfaces for writing code actions
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clang-tools-extra/clangd/Protocol.cpp:425
+const llvm::StringLiteral ExecuteCommandParams::CLANGD_APPLY_TWEAK =
+"clangd.applyCodeAction";
+
tweak or applyTweak
Comment at: clang-tools-extra/clangd/Protocol.h:634
+struct TweakArgs {
+ URIForFile file;
comment here?
Comment at: clang-tools-extra/clangd/SourceCode.h:59
+/// care to avoid comparing the result with expansion locations.
+llvm::Expected sourceLocationInMainFile(const SourceManager
,
+Position P);
(can we add a unit test for this function?)
Comment at: clang-tools-extra/clangd/refactor/Tweak.cpp:27
+std::vector> prepareTweaks(const Tweak::Selection ) {
+#ifndef NDEBUG
+ {
ilya-biryukov wrote:
> Please note I added these assertions here.
>
> It feels weird to traverse twice on every call to `prepareTweaks`, but that's
> the simplest option I could come up with.
Looks fine - you could consider extracting to a separate function
`validateRegistry` or so, but up to you
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.
Comment at: clang-tools-extra/clangd/refactor/Tweak.cpp:27
+std::vector> prepareTweaks(const Tweak::Selection ) {
+#ifndef NDEBUG
+ {
Please note I added these assertions here.
It feels weird to traverse twice on every call to `prepareTweaks`, but that's
the simplest option I could come up with.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov marked an inline comment as not done.
ilya-biryukov added inline comments.
Comment at: clangd/refactor/Tweak.h:40
+ struct Selection {
+static llvm::Optional create(llvm::StringRef File,
+llvm::StringRef Code,
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > 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? Passing an invalid range is always an error I guess.
> > The reason I added it is to avoid duplication between in the test code and
> > `ClangdServer`, which are the only two clients we have.
> > I expect this to be more useful when we add a way to traverse the subset of
> > the AST in the checks
> I understand. I think as things stand both callers would be clearer (if a
> couple of lines longer) without this helper.
>
> What the API should be in the future - happy to talk about that then.
I'd keep this function, here is my reasoning:
1. There are 4 usages already (2 in this patch, 2 in the tests), keeping them
in sync means an annoying (to my taste) amount of copy-paste.
2. This function is not completely trivial, as it makes a decision on what a
`CursorLoc` is and documents it.
3. Making changes is simpler: e.g. if we add more fields to `Tweak::Selection`,
we'd end up changing this function (possibly its signature too) and all of its
callers. There's almost no room for error. If we remove it, one would have to
find and update all usages by hand and make sure they're the same.
I did update the patch to inline it, but let me know if any of this convinces
you.
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;
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > 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)
> > That would mean no two tweaks are allowed to have the same class name. This
> > is probably fine, but somewhat contradicts C++, which would solve it with
> > namespaces.
> > To be fair, there's a simple trick to grep for the id to find its class, so
> > I'd keep as is.
> >
> > If we choose to adapt `REGISTER_TWEAK`, that would mean we force everyone
> > to put their tweaks **only** in `.cpp` files. That creates arbitrary
> > restrictions on how one should write a check and I'm somewhat opposed to
> > this. But happy to reconsider if you feel strongly about this.
> I don't care about the details (e.g. whether `REGISTER_TWEAK` sets the name,
> asserts the name, or none of the above).
>
> I do care that we don't add a second ID for a class that's not equal to the
> class name. This is both a bad idea from first principles and from experience
> with clang-tidy.
> If this were ever to become a real problem, I'm happy to include the
> namespace name in the ID.
Done, `REGISTER_TWEAK` now defines `id`.
Out of curiosity, what are the problems with clang-tidy checks that you
mention? Finding a check by its name being hard is something that comes to
mind. Anything else?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov updated this revision to Diff 182870.
ilya-biryukov marked 3 inline comments as done.
ilya-biryukov added a comment.
- Inline Tweak::Selection::create()
- Define Tweak::id() in REGISTER_TWEAK.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
clang-tools-extra/clangd/tool/CMakeLists.txt
clang-tools-extra/test/clangd/initialize-params.test
Index: clang-tools-extra/test/clangd/initialize-params.test
===
--- clang-tools-extra/test/clangd/initialize-params.test
+++ clang-tools-extra/test/clangd/initialize-params.test
@@ -25,7 +25,8 @@
# CHECK-NEXT: "documentSymbolProvider": true,
# CHECK-NEXT: "executeCommandProvider": {
# CHECK-NEXT:"commands": [
-# CHECK-NEXT: "clangd.applyFix"
+# CHECK-NEXT: "clangd.applyFix",
+# CHECK-NEXT: "clangd.applyCodeAction"
# CHECK-NEXT:]
# CHECK-NEXT: },
# CHECK-NEXT: "hoverProvider": true,
Index: clang-tools-extra/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/clangd/tool/CMakeLists.txt
@@ -3,6 +3,7 @@
add_clang_tool(clangd
ClangdMain.cpp
+ $
)
set(LLVM_LINK_COMPONENTS
Index: clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
@@ -0,0 +1,11 @@
+include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../..)
+
+# A target containing all code tweaks (i.e. mini-refactorings) provided by
+# clangd.
+# Built as an object library to make sure linker does not remove global
+# constructors that register individual tweaks in a global registry.
+# To enable these tweaks in exectubales or shared libraries, add
+# $ to a list of sources, see
+# clangd/tool/CMakeLists.txt for an example.
+add_clang_library(clangDaemonTweaks OBJECT
+ )
Index: clang-tools-extra/clangd/refactor/Tweak.h
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/Tweak.h
@@ -0,0 +1,99 @@
+//===--- Tweak.h -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+// Tweaks are small refactoring-like actions that run over the AST and produce
+// the set of edits as a result. They are local, i.e. they should take the
+// current editor context, e.g. the cursor position and selection into account.
+// The actions are executed in two stages:
+// - Stage 1 should check whether the action is available in a current
+// context. It should be cheap and fast to compute as it is executed for all
+// available actions on every client request, which happen quite frequently.
+// - Stage 2 is performed after stage 1 and can be more expensive to compute.
+// It is performed when the user actually chooses the action.
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_TWEAK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_TWEAK_H
+
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+namespace clang {
+namespace clangd {
+
+using TweakID = llvm::StringRef;
+
+/// An interface base for small context-sensitive refactoring actions.
+/// To implement a new tweak use the following pattern in a .cpp file:
+/// class MyTweak : public Tweak {
+/// public:
+/// TweakID id() const override final; // definition provided by REGISTER_TWEAK.
+/// // implement other methods here.
+/// };
+/// REGISTER_TWEAK(MyTweak);
+class Tweak {
+public:
+ /// Input to prepare and apply tweaks.
+ struct Selection {
+/// The text of the active document.
+llvm::StringRef Code;
+/// Parsed AST of the active file.
+ParsedAST
+/// A location of the cursor in the editor.
+SourceLocation Cursor;
+// FIXME: add selection when there are checks relying on it.
+// FIXME: provide a way to get sources and ASTs for other files.
+//
[PATCH] D56267: [clangd] Interfaces for writing code actions
sammccall added a comment.
Can live with `applyTweak` etc, though it makes me sad.
Comment at: clangd/refactor/Tweak.h:40
+ struct Selection {
+static llvm::Optional create(llvm::StringRef File,
+llvm::StringRef Code,
ilya-biryukov wrote:
> sammccall wrote:
> > 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? Passing an invalid range is always an error I guess.
> The reason I added it is to avoid duplication between in the test code and
> `ClangdServer`, which are the only two clients we have.
> I expect this to be more useful when we add a way to traverse the subset of
> the AST in the checks
I understand. I think as things stand both callers would be clearer (if a
couple of lines longer) without this helper.
What the API should be in the future - happy to talk about that then.
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;
ilya-biryukov wrote:
> sammccall wrote:
> > 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)
> That would mean no two tweaks are allowed to have the same class name. This
> is probably fine, but somewhat contradicts C++, which would solve it with
> namespaces.
> To be fair, there's a simple trick to grep for the id to find its class, so
> I'd keep as is.
>
> If we choose to adapt `REGISTER_TWEAK`, that would mean we force everyone to
> put their tweaks **only** in `.cpp` files. That creates arbitrary
> restrictions on how one should write a check and I'm somewhat opposed to
> this. But happy to reconsider if you feel strongly about this.
I don't care about the details (e.g. whether `REGISTER_TWEAK` sets the name,
asserts the name, or none of the above).
I do care that we don't add a second ID for a class that's not equal to the
class name. This is both a bad idea from first principles and from experience
with clang-tidy.
If this were ever to become a real problem, I'm happy to include the namespace
name in the ID.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov updated this revision to Diff 182534.
ilya-biryukov marked 10 inline comments as done.
ilya-biryukov added a comment.
- Update comments
- Return an ID+Title instead of a Tweak object
- Rename codeAction -> tweak everywhere
- Remove Tweak::Selection::File
- Return Expected instead of Optional
- Add a FIXME to apply the formatter on top of the actions
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
clang-tools-extra/clangd/tool/CMakeLists.txt
clang-tools-extra/test/clangd/initialize-params.test
Index: clang-tools-extra/test/clangd/initialize-params.test
===
--- clang-tools-extra/test/clangd/initialize-params.test
+++ clang-tools-extra/test/clangd/initialize-params.test
@@ -25,7 +25,8 @@
# CHECK-NEXT: "documentSymbolProvider": true,
# CHECK-NEXT: "executeCommandProvider": {
# CHECK-NEXT:"commands": [
-# CHECK-NEXT: "clangd.applyFix"
+# CHECK-NEXT: "clangd.applyFix",
+# CHECK-NEXT: "clangd.applyCodeAction"
# CHECK-NEXT:]
# CHECK-NEXT: },
# CHECK-NEXT: "hoverProvider": true,
Index: clang-tools-extra/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/clangd/tool/CMakeLists.txt
@@ -3,6 +3,7 @@
add_clang_tool(clangd
ClangdMain.cpp
+ $
)
set(LLVM_LINK_COMPONENTS
Index: clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
@@ -0,0 +1,11 @@
+include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../..)
+
+# A target containing all code tweaks (i.e. mini-refactorings) provided by
+# clangd.
+# Built as an object library to make sure linker does not remove global
+# constructors that register individual tweaks in a global registry.
+# To enable these tweaks in exectubales or shared libraries, add
+# $ to a list of sources, see
+# clangd/tool/CMakeLists.txt for an example.
+add_clang_library(clangDaemonTweaks OBJECT
+ )
Index: clang-tools-extra/clangd/refactor/Tweak.h
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/Tweak.h
@@ -0,0 +1,93 @@
+//===--- Tweak.h -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+// Tweaks are small refactoring-like actions that run over the AST and produce
+// the set of edits as a result. They are local, i.e. they should take the
+// current editor context, e.g. the cursor position and selection into account.
+// The actions are executed in two stages:
+// - Stage 1 should check whether the action is available in a current
+// context. It should be cheap and fast to compute as it is executed for all
+// available actions on every client request, which happen quite frequently.
+// - Stage 2 is performed after stage 1 and can be more expensive to compute.
+// It is performed when the user actually chooses the action.
+// To avoid extra round-trips and AST reads, actions can also produce results on
+// stage 1 in cases when the actions are fast to compute.
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_TWEAK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_TWEAK_H
+
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+namespace clang {
+namespace clangd {
+
+using TweakID = llvm::StringRef;
+
+/// An interface base for small context-sensitive refactoring actions.
+class Tweak {
+public:
+ /// Input to prepare and apply tweaks.
+ struct Selection {
+static llvm::Expected create(llvm::StringRef Code,
+ParsedAST , Range Selection);
+
+/// The text of the active document.
+llvm::StringRef Code;
+/// Parsed AST of the active file.
+ParsedAST
+/// A location of the cursor in the editor.
+SourceLocation Cursor;
+
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:363
+ return CB(A.takeError());
+return CB((*A)->apply(*Inputs));
+ };
sammccall wrote:
> we should `format::cleanUpAroundReplacements`... fine to leave this as a FIXME
Added a FIXME. We probably want a helper that does this, to reuse in tests and
`ClangdServer`.
Comment at: clangd/ClangdServer.h:212
+ /// Apply the code tweak with a specified \p ID.
+ void applyCodeTweak(PathRef File, Range Sel, TweakID ID,
+ Callback CB);
sammccall wrote:
> `tweak`? It's a verb...
That's too short for my taste, I went with `applyTweak` and it also feels
helpful if we don't switch between a verb and a noun to keep clear a parallel
between a `Tweak` returned by `enumerateTweaks` and a tweak applied in
`applyTweak`.
Let me know if you feel strongly about this :-)
Comment at: clangd/Protocol.cpp:426
+const llvm::StringLiteral ExecuteCommandParams::CLANGD_APPLY_CODE_ACTION =
+"clangd.applyCodeAction";
+
sammccall wrote:
> tweak
Used `applyTweak` for symmetry with `applyFix` (and other reasons mentioned in
the comments for `ClangdServer::tweak`)
Comment at: clangd/refactor/Tweak.h:40
+ struct Selection {
+static llvm::Optional create(llvm::StringRef File,
+llvm::StringRef Code,
sammccall wrote:
> 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? Passing an invalid range is always an error I guess.
The reason I added it is to avoid duplication between in the test code and
`ClangdServer`, which are the only two clients we have.
I expect this to be more useful when we add a way to traverse the subset of the
AST in the checks
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;
sammccall wrote:
> 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)
That would mean no two tweaks are allowed to have the same class name. This is
probably fine, but somewhat contradicts C++, which would solve it with
namespaces.
To be fair, there's a simple trick to grep for the id to find its class, so I'd
keep as is.
If we choose to adapt `REGISTER_TWEAK`, that would mean we force everyone to
put their tweaks **only** in `.cpp` files. That creates arbitrary restrictions
on how one should write a check and I'm somewhat opposed to this. But happy to
reconsider if you feel strongly about this.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
sammccall added inline comments. Comment at: clangd/ClangdServer.cpp:339 + return CB(llvm::createStringError(llvm::inconvertibleErrorCode(), +"could not create action context")); +CB(prepareTweaks(*Inputs)); (action context?) Comment at: clangd/ClangdServer.cpp:363 + return CB(A.takeError()); +return CB((*A)->apply(*Inputs)); + }; we should `format::cleanUpAroundReplacements`... fine to leave this as a FIXME Comment at: clangd/refactor/Tweak.h:45 +/// The path of an active document the action was invoked in. +llvm::StringRef File; +/// The text of the active document. Hmm, maybe we should drop this until we know how cross-file tweaks will work? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56267/new/ https://reviews.llvm.org/D56267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
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 , const URIForFile ,
+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>> 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>, 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>> 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>, 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 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 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 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
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov added a comment. This should be ready for another round of review. Let me know if I missed any of the older comments. There are two more things that I'd like to do before this lands: - go through the docs again and update/simplify them to make sure they're in sync with the new interface, - publish the tests I have, they're not 100% ready yet, so I'm keeping the, local for now. This could be a separate change, though I don't think this should matter much if the rest is dealt with. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56267/new/ https://reviews.llvm.org/D56267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:346
+ {ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND,
+ ExecuteCommandParams::CLANGD_APPLY_CODE_ACTION}},
}},
sammccall wrote:
> Seems a bit more direct to give each one its own command name? Maybe under a
> namespace like refactor/swapIfBranches
I'm not sure what we're winning here.
Currently we have: `command:{ name: "applyCodeAction", args:
["swap-if-branches", "…"] }`
In the proposed approach we'd have: `command: { name: "swap-if-branches", args:
[…] }`
The latter would definitely mean more fiddling at least to communicate the list
of supported actions. Both approaches don't seem to require any changes on the
client side, even though they affect the messages we send over the protocol.
Comment at: clangd/ClangdLSPServer.cpp:662
+std::vector Actions = std::move(FixIts);
+auto toCodeAction = [&](PreparedAction &) -> CodeAction {
+ CodeAction CA;
sammccall wrote:
> 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...
I've moved it into a helper function, moved away from using a lambda.
However, kept it in `ClangdLSPServer.cpp`, `Diag.h` does feel like the wrong
place for it, it would be nice to find a better place for both functions.
Comment at: clangd/refactor/Tweak.cpp:62
+ }
+ // Ensure deterministic order of the results.
+ llvm::sort(Available,
sammccall wrote:
> (nit: I'd make this `operator<` of `Tweak`? e.g. if priority is added, it
> should be private)
My personal preference would be to keep it in .cpp file to avoid complicating
the public interface with a function and a friend declaration for
`prepareTweaks` (let me know if I misinterpreted your proposal).
Let me know if you have strong preference wrt to this one
Comment at: clangd/refactor/Tweak.h:78
+/// Contains all actions supported by clangd.
+typedef llvm::Registry TweakRegistry;
+
sammccall wrote:
> nit: can we move this to the cpp file and out of the public interface?
> (or keep the registry public, but drop prepareTweak[s] and just make callers
> do it)
Done.
I don't think that makes a big difference, though: hiding the typedef would
hide the fact we're using the `Registry`, though. We still have
`REGISTER_TWEAK` that mentions it in the public interface.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov updated this revision to Diff 182315.
ilya-biryukov marked 9 inline comments as done.
ilya-biryukov added a comment.
- Remove intermediate StringMap, use registry directly
- Rename codeAction to codeTweak in ClangdServer
- Rename a helper to get position to sourceLocationInMainFile
- Move the Registry typedef into the .cpp file
- Make toCodeAction a helper function
- Update comments
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/refactor/Tweak.cpp
clangd/refactor/Tweak.h
clangd/refactor/tweaks/CMakeLists.txt
clangd/tool/CMakeLists.txt
test/clangd/initialize-params.test
Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -25,7 +25,8 @@
# CHECK-NEXT: "documentSymbolProvider": true,
# CHECK-NEXT: "executeCommandProvider": {
# CHECK-NEXT:"commands": [
-# CHECK-NEXT: "clangd.applyFix"
+# CHECK-NEXT: "clangd.applyFix",
+# CHECK-NEXT: "clangd.applyCodeAction"
# CHECK-NEXT:]
# CHECK-NEXT: },
# CHECK-NEXT: "hoverProvider": true,
Index: clangd/tool/CMakeLists.txt
===
--- clangd/tool/CMakeLists.txt
+++ clangd/tool/CMakeLists.txt
@@ -3,6 +3,7 @@
add_clang_tool(clangd
ClangdMain.cpp
+ $
)
set(LLVM_LINK_COMPONENTS
Index: clangd/refactor/tweaks/CMakeLists.txt
===
--- /dev/null
+++ clangd/refactor/tweaks/CMakeLists.txt
@@ -0,0 +1,11 @@
+include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../..)
+
+# A target containing all code tweaks (i.e. mini-refactorings) provided by
+# clangd.
+# Built as an object library to make sure linker does not remove global
+# constructors that register individual tweaks in a global registry.
+# To enable these tweaks in exectubales or shared libraries, add
+# $ to a list of sources, see
+# clangd/tool/CMakeLists.txt for an example.
+add_clang_library(clangDaemonTweaks OBJECT
+ )
Index: clangd/refactor/Tweak.h
===
--- /dev/null
+++ clangd/refactor/Tweak.h
@@ -0,0 +1,96 @@
+//===--- Tweak.h -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+// Tweaks are small refactoring-like actions that run over the AST and produce
+// the set of edits as a result. They are local, i.e. they should take the
+// current editor context, e.g. the cursor position and selection into account.
+// The actions are executed in two stages:
+// - Stage 1 should check whether the action is available in a current
+// context. It should be cheap and fast to compute as it is executed for all
+// available actions on every client request, which happen quite frequently.
+// - Stage 2 is performed after stage 1 and can be more expensive to compute.
+// It is performed when the user actually chooses the action.
+// To avoid extra round-trips and AST reads, actions can also produce results on
+// stage 1 in cases when the actions are fast to compute.
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_TWEAK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_TWEAK_H
+
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+namespace clang {
+namespace clangd {
+
+using TweakID = llvm::StringRef;
+
+/// An interface base for small context-sensitive refactoring actions.
+class Tweak {
+public:
+ /// Input to prepare and apply tweaks.
+ struct Selection {
+static llvm::Optional create(llvm::StringRef File,
+llvm::StringRef Code,
+ParsedAST , Range Selection);
+
+/// The path of an active document the action was invoked in.
+llvm::StringRef File;
+/// The text of the active document.
+llvm::StringRef Code;
+/// Parsed AST of the active file.
+ParsedAST
+/// A location of the cursor in the editor.
+SourceLocation Cursor;
+// FIXME: add selection when there are checks relying on it.
+// FIXME: provide a way to get sources and ASTs for other files.
+// FIXME: cache some commonly required
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov added a comment.
Haven't yet addressed all the comments, but switched to use the "object
library" (i.e. a collection of .o files) to make sure linker does not optimize
away global ctors required by registry.
Comment at: clangd/refactor/Tweak.cpp:38
+namespace {
+const llvm::StringMap()>> &
+tweaksRegistry() {
sammccall wrote:
> Can we drop this indirection and use the registry directly?
Sure, would mean linear time for prepareTweak, but we probably don't care.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov updated this revision to Diff 182272.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
- Build tweaks as an object library. To make sure linker does not optimize away
global constructors.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/refactor/Tweak.cpp
clangd/refactor/Tweak.h
clangd/refactor/tweaks/CMakeLists.txt
clangd/tool/CMakeLists.txt
test/clangd/initialize-params.test
Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -25,7 +25,8 @@
# CHECK-NEXT: "documentSymbolProvider": true,
# CHECK-NEXT: "executeCommandProvider": {
# CHECK-NEXT:"commands": [
-# CHECK-NEXT: "clangd.applyFix"
+# CHECK-NEXT: "clangd.applyFix",
+# CHECK-NEXT: "clangd.applyCodeAction"
# CHECK-NEXT:]
# CHECK-NEXT: },
# CHECK-NEXT: "hoverProvider": true,
Index: clangd/tool/CMakeLists.txt
===
--- clangd/tool/CMakeLists.txt
+++ clangd/tool/CMakeLists.txt
@@ -3,6 +3,7 @@
add_clang_tool(clangd
ClangdMain.cpp
+ $
)
set(LLVM_LINK_COMPONENTS
Index: clangd/refactor/tweaks/CMakeLists.txt
===
--- /dev/null
+++ clangd/refactor/tweaks/CMakeLists.txt
@@ -0,0 +1,11 @@
+include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../..)
+
+# A target containing all code tweaks (i.e. mini-refactorings) provided by
+# clangd.
+# Built as an object library to make sure linker does not remove global
+# constructors that register individual tweaks in a global registry.
+# To enable these tweaks in exectubales or shared libraries, add
+# $ to a list of sources, see
+# clangd/tool/CMakeLists.txt for an example.
+add_clang_library(clangDaemonTweaks OBJECT
+ )
Index: clangd/refactor/Tweak.h
===
--- /dev/null
+++ clangd/refactor/Tweak.h
@@ -0,0 +1,98 @@
+//===--- Tweak.h -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+// Tweaks are small refactoring-like actions that run over the AST and produce
+// the set of edits as a result. They are local, i.e. they should take the
+// current editor context, e.g. the cursor position and selection into account.
+// The actions are executed in two stages:
+// - Stage 1 should check whether the action is available in a current
+// context. It should be cheap and fast to compute as it is executed for all
+// available actions on every client request, which happen quite frequently.
+// - Stage 2 is performed after stage 1 and can be more expensive to compute.
+// It is performed when the user actually chooses the action.
+// To avoid extra round-trips and AST reads, actions can also produce results on
+// stage 1 in cases when the actions are fast to compute.
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_TWEAK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_TWEAK_H
+
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+namespace clang {
+namespace clangd {
+
+using TweakID = llvm::StringRef;
+
+/// An interface base for small context-sensitive refactoring actions.
+class Tweak {
+public:
+ /// Input to prepare and apply tweaks.
+ struct Selection {
+static llvm::Optional create(llvm::StringRef File,
+llvm::StringRef Code,
+ParsedAST , Range Selection);
+
+/// The path of an active document the action was invoked in.
+llvm::StringRef File;
+/// The text of the active document.
+llvm::StringRef Code;
+/// Parsed AST of the active file.
+ParsedAST
+/// A location of the cursor in the editor.
+SourceLocation Cursor;
+// FIXME: add selection when there are checks relying on it.
+// 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.
+ };
+ virtual ~Tweak() = default;
+ /// A unique id of the action.
[PATCH] D56267: [clangd] Interfaces for writing code actions
sammccall added a comment.
(Mostly just comments on Tweak, I think the comments on other parts still apply)
Comment at: clangd/SourceCode.h:58
+/// Return the file location, corresponding to \p P. Note that one should take
+/// care to avoid comparing the result with expansion locations.
naming nits:
- this should mention "main-file"
- doesn't need to mention "position" as it's just the param type
- 'sourceLoc' seems an odd abbreviation
Maybe `sourceLocationInMainFile`?
(Current name is consistent with `sourceLocToPosition` but that name is bad
too...)
Comment at: clangd/refactor/Tweak.cpp:38
+namespace {
+const llvm::StringMap()>> &
+tweaksRegistry() {
Can we drop this indirection and use the registry directly?
Comment at: clangd/refactor/Tweak.cpp:62
+ }
+ // Ensure deterministic order of the results.
+ llvm::sort(Available,
(nit: I'd make this `operator<` of `Tweak`? e.g. if priority is added, it
should be private)
Comment at: clangd/refactor/Tweak.h:78
+/// Contains all actions supported by clangd.
+typedef llvm::Registry TweakRegistry;
+
nit: can we move this to the cpp file and out of the public interface?
(or keep the registry public, but drop prepareTweak[s] and just make callers do
it)
Comment at: clangd/refactor/Tweak.h:93
+// If prepare() returns true, return the corresponding tweak.
+std::unique_ptr prepareTweak(TweakID ID, const Tweak::Selection );
+
should this return ErrorOr> so we can treat both missing ID
and prepare failed as errors with messages?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov updated this revision to Diff 182078.
ilya-biryukov added a comment.
Herald added a subscriber: mgrang.
- Rename ActionProvider to Tweak
- Update an interface of Tweak
- ActionInputs -> Tweak::Selection
- Remove CodeAction.h, use tweak registry instead
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/refactor/Tweak.cpp
clangd/refactor/Tweak.h
test/clangd/initialize-params.test
Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -25,7 +25,8 @@
# CHECK-NEXT: "documentSymbolProvider": true,
# CHECK-NEXT: "executeCommandProvider": {
# CHECK-NEXT:"commands": [
-# CHECK-NEXT: "clangd.applyFix"
+# CHECK-NEXT: "clangd.applyFix",
+# CHECK-NEXT: "clangd.applyCodeAction"
# CHECK-NEXT:]
# CHECK-NEXT: },
# CHECK-NEXT: "hoverProvider": true,
Index: clangd/refactor/Tweak.h
===
--- /dev/null
+++ clangd/refactor/Tweak.h
@@ -0,0 +1,98 @@
+//===--- Tweak.h -*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+// Tweaks are small refactoring-like actions that run over the AST and produce
+// the set of edits as a result. They are local, i.e. they should take the
+// current editor context, e.g. the cursor position and selection into account.
+// The actions are executed in two stages:
+// - Stage 1 should check whether the action is available in a current
+// context. It should be cheap and fast to compute as it is executed for all
+// available actions on every client request, which happen quite frequently.
+// - Stage 2 is performed after stage 1 and can be more expensive to compute.
+// It is performed when the user actually chooses the action.
+// To avoid extra round-trips and AST reads, actions can also produce results on
+// stage 1 in cases when the actions are fast to compute.
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_TWEAK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_TWEAK_H
+
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+namespace clang {
+namespace clangd {
+
+using TweakID = llvm::StringRef;
+
+/// An interface base for small context-sensitive refactoring actions.
+class Tweak {
+public:
+ /// Input to prepare and apply tweaks.
+ struct Selection {
+static llvm::Optional create(llvm::StringRef File,
+llvm::StringRef Code,
+ParsedAST , Range Selection);
+
+/// The path of an active document the action was invoked in.
+llvm::StringRef File;
+/// The text of the active document.
+llvm::StringRef Code;
+/// Parsed AST of the active file.
+ParsedAST
+/// A location of the cursor in the editor.
+SourceLocation Cursor;
+// FIXME: add selection when there are checks relying on it.
+// 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.
+ };
+ virtual ~Tweak() = default;
+ /// A unique id of the action. The convention is to
+ /// lower-case-with-dashes for the identifier.
+ virtual TweakID id() = 0;
+ /// Run the first stage of the action. The non-None result indicates that the
+ /// action is available and should be shown to the user. Returns None if the
+ /// action is not available.
+ /// This function should be fast, if the action requires non-trivial work it
+ /// should be moved into 'apply'.
+ /// Returns true iff the action is available and apply() can be called on it.
+ virtual bool prepare(const Selection ) = 0;
+ /// Run the second stage of the action that would produce the actual changes.
+ /// EXPECTS: prepare() was called and returned true.
+ virtual Expected apply(const Selection ) = 0;
+ /// A one-line title of the action that should be shown to the users in the
+ /// UI.
+ /// EXPECTS: prepare() was called and returned true.
+ virtual std::string title() const = 0;
+};
+
+/// Contains all actions supported by
[PATCH] D56267: [clangd] Interfaces for writing code actions
sammccall added a comment.
Offline discussion:
- CRTP doesn't buy anything. Simple inheritance is less clean than current
function-passing version but probably more familiar.
- `Tweak` is a good name: short, evocative but not commonly used, works as a
noun and a verb.
- Registry might be a good enough fit to use directly
Here's the result of my local, er, tweaking to `ActionProvider.h`.
//===--- Tweak.h -*-
C++-*-===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===--===//
// A tweak is a small context-sensitive refactoring-like action that is
// triggered at a cursor location, examines the AST and produces a set of
edits.
//
// It has two main pieces of logic:
// - prepare() quickly checks whether the action is applicable at the
cursor.
// It must be cheap as all tweaks are prepared to list them for the user.
// - apply() computes the actual edits.
// This can be more expensive, only the chosen tweak is applied.
//===--===//
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_ACTIONPROVIDER_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_ACTIONPROVIDER_H
#include "ClangdUnit.h"
#include "Protocol.h"
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/StringRef.h"
namespace clang {
namespace clangd {
// Abstract base for small context-sensitive refactoring actions.
class Tweak {
public:
// Information about the location where the tweak was triggered.
struct Selection {
// The path of an active document the action was invoked in.
llvm::StringRef File;
// The text of the active document.
llvm::StringRef Code;
// Parsed AST of the active file.
ParsedAST
// A location of the cursor in the editor.
SourceLocation Cursor;
// FIXME: full selection bounds.
// FIXME: AST node under cursor.
// FIXME: allow tweaks to specify matchers, and include match results.
// FIXME: allow access to index and other source files.
};
// Tweaks must have a no-op default constructor.
virtual ~Tweak() = default;
virtual const char* id() = 0;
// Determines whether this tweak can run at the selection.
// This function may record information to be used later.
// It should run as fast as possible, particularly when returning false.
virtual bool prepare(const Selection &) = 0;
// Generate the edits for this tweak.
// REQUIRES: prepare() was called and returned true.
virtual llvm::Expected
apply(const Selection &) = 0;
// Description of this tweak at the selected location.
// e.g. "Out-line definition of toString()".
// REQUIRES: prepare() was called and returned true.
virtual std::string describe() const;
};
// All tweaks must be registered, next to their definition.
#define REGISTER_TWEAK(Subclass)
\
static ::llvm::Registry<::clang::clangd::Tweak>::Add
\
TweakRegistrationFor##Subclass(Subclass{}.id(), #Subclass);
// Calls prepare() on all tweaks, returning those that can run on the
selection.
std::vector> prepareTweaks(const Tweak::Selection &);
// Calls prepare() on the tweak with a given ID.
// If prepare() returns false, returns an error.
Expected> prepareTweak(StringRef ID,
const Tweak::Selection &);
} // namespace clangd
} // namespace clang
#endif
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
sammccall added a comment. Name ideas: - "Refactoring" - I think the main drawback is the conflicts with potential global-refactorings that we do in clangd. I don't think the conflict with `tooling/refactor` is bad enough to avoid this. I also don't think "some of these don't look like refactoring to me" is a strong reason to avoid it. - "Code action" - I think the partial overlap with the protocol concept is confusing. (Worse, the protocol has two things called code action, that partially overlap). It's also not really a standard term outside LSP/VSCode, so we should be able to come up with something as good. - "Action" - much too vague, I think - "RefactoringAction" (or refactoring/Action, or refactoring::Action...) - not terrible but I think dominated by just "Refactoring" or "Refactor". - "AutoEdit" or something equally descriptive - this is bland along the lines of Code Action, may be ok if it's sufficiently short and unique - "Swizzle" or something equally unique-and-semantics-free - I'd be happy with this kind of alternative if we can't find something that clearly means what we want Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56267/new/ https://reviews.llvm.org/D56267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
sammccall added a comment.
To be a bit more concrete...
One approach which won't win any OO design points but might be quite easy to
navigate: combine the ActionProvider and PreparedAction into one class.
class SwapIfBranches : public MiniRefactoringThing {
SwapIfBranches(); // must have a default constructor.
const char* id() const override { return "swapIfBranches"; } // must be
constant for the subclass.
bool prepare(const Inputs&) override; // may only be called once. Returns
false if this operation doesn't apply.
std::string title() override; // REQUIRES: prepare() returned true
Expected apply() override; // REQUIRES: prepare()
returned true
}
note that the "subclasses of a common interface, have a default constructor"
fits exactly with the LLVM Registry pattern, if we want to move to that.
Otherwise, a `StringMap>`
A more... complicated version could use something like CRTP to give a generic
external interface while keeping the internal dataflow/signatures clean:
class SwapIfBranches : public MiniRefactoringThing {
struct State { ... }; // completing forward declaration from CRTP base
Optional prepare(const Inputs&) const override;
std::string getTitle(const State&) const;
Expected apply(const State&) const override;
}
const char MiniRefactoringThing::ID[] = "swapIfBranches";
Similarly we could use Registry or a
`StringMap>` where the CRTP base
implements the interface.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
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 Actions = std::move(FixIts);
+auto toCodeAction = [&](PreparedAction &) -> 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> 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov added a comment. I've added a few sample actions, please take a look. The major thing that's missing from the design is how we can define an interface for actions to get the nodes they interested in from the AST without doing an AST traversal in each of the actions separately. I have a few options in mind, will be happy to explore more of this. The other major problem that I've run into while playing around with the action in VSCode is increased latency: my suspicion is that the codeAction requests are not getting cancelled, I'll confirm and file an issue against VSCode if that's the case. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56267/new/ https://reviews.llvm.org/D56267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov updated this revision to Diff 181321.
ilya-biryukov added a comment.
- Remove 'using namespace llvm'
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/CodeActions.cpp
clangd/CodeActions.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/refactor/ActionProvider.cpp
clangd/refactor/ActionProvider.h
test/clangd/initialize-params.test
Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -25,7 +25,8 @@
# CHECK-NEXT: "documentSymbolProvider": true,
# CHECK-NEXT: "executeCommandProvider": {
# CHECK-NEXT:"commands": [
-# CHECK-NEXT: "clangd.applyFix"
+# CHECK-NEXT: "clangd.applyFix",
+# CHECK-NEXT: "clangd.applyCodeAction"
# CHECK-NEXT:]
# CHECK-NEXT: },
# CHECK-NEXT: "hoverProvider": true,
Index: clangd/refactor/ActionProvider.h
===
--- /dev/null
+++ clangd/refactor/ActionProvider.h
@@ -0,0 +1,115 @@
+//===--- ActionProvider.h *- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+// The code actions are small refactoring-like actions that run over the AST and
+// produce the set of edits as a result. They are local, i.e. they should take
+// the current editor context, e.g. the cursor position and selection into
+// account.
+// The actions are executed in two stages:
+// - Stage 1 should check whether the action is available in a current
+// context. It should be cheap and fast to compute as it is executed for all
+// available actions on every client request, which happen quite frequently.
+// - Stage 2 is performed after stage 1 and can be more expensive to compute.
+// It is performed when the user actually chooses the action.
+// To avoid extra round-trips and AST reads, actions can also produce results on
+// stage 1 in cases when the actions are fast to compute.
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_ACTIONPROVIDER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_ACTIONPROVIDER_H
+
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+namespace clang {
+namespace clangd {
+
+class PreparedAction;
+struct ActionInputs;
+
+/// Base interface for writing a code action.
+class ActionProvider {
+public:
+ virtual ~ActionProvider() = default;
+
+ /// Run the first stage of the action. The non-None result indicates that the
+ /// action is available and should be shown to the user. Returns None if the
+ /// action is not available.
+ /// This function should be fast, if the action requires non-trivial work it
+ /// should be moved into the second stage, i.e. the continuation function
+ /// provided in the resulting PreparedAction.
+ virtual llvm::Optional
+ prepare(const ActionInputs ) = 0;
+};
+
+/// A context used by the actions to identify
+struct ActionInputs {
+ static llvm::Optional create(llvm::StringRef File,
+ llvm::StringRef Code,
+ ParsedAST , Range Selection);
+
+ /// The path of an active document the action was invoked in.
+ llvm::StringRef File;
+ /// The text of the active document.
+ llvm::StringRef Code;
+ /// Parsed AST of the active file.
+ ParsedAST
+ /// A location of the cursor in the editor.
+ SourceLocation Cursor;
+ // FIXME: add selection when there are checks relying on it.
+ // 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.
+};
+
+using ActionID = size_t;
+/// Result of executing a first stage of the action. If computing the resulting
+/// WorkspaceEdit is fast, the actions should produce it right away.
+/// For non-trivial actions, a continuation function to compute the resulting
+/// edits should be provided instead. It is expected that PreparedAction is
+/// consumed immediately when created, so continuations can reference the AST,
+/// so it's not safe to store the PreparedAction for long spans of time.
+class PreparedAction {
+public:
+ PreparedAction(std::string Title,
+ llvm::unique_function()>
+
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov updated this revision to Diff 181309.
ilya-biryukov added a comment.
- Put more AST-centric information into ActionInputs
- Restructure and refactor the code
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56267/new/
https://reviews.llvm.org/D56267
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/CodeActions.cpp
clangd/CodeActions.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
clangd/refactor/ActionProvider.cpp
clangd/refactor/ActionProvider.h
test/clangd/initialize-params.test
Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -25,7 +25,8 @@
# CHECK-NEXT: "documentSymbolProvider": true,
# CHECK-NEXT: "executeCommandProvider": {
# CHECK-NEXT:"commands": [
-# CHECK-NEXT: "clangd.applyFix"
+# CHECK-NEXT: "clangd.applyFix",
+# CHECK-NEXT: "clangd.applyCodeAction"
# CHECK-NEXT:]
# CHECK-NEXT: },
# CHECK-NEXT: "hoverProvider": true,
Index: clangd/refactor/ActionProvider.h
===
--- /dev/null
+++ clangd/refactor/ActionProvider.h
@@ -0,0 +1,115 @@
+//===--- ActionProvider.h *- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+// The code actions are small refactoring-like actions that run over the AST and
+// produce the set of edits as a result. They are local, i.e. they should take
+// the current editor context, e.g. the cursor position and selection into
+// account.
+// The actions are executed in two stages:
+// - Stage 1 should check whether the action is available in a current
+// context. It should be cheap and fast to compute as it is executed for all
+// available actions on every client request, which happen quite frequently.
+// - Stage 2 is performed after stage 1 and can be more expensive to compute.
+// It is performed when the user actually chooses the action.
+// To avoid extra round-trips and AST reads, actions can also produce results on
+// stage 1 in cases when the actions are fast to compute.
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_ACTIONPROVIDER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_ACTIONPROVIDER_H
+
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+namespace clang {
+namespace clangd {
+
+class PreparedAction;
+struct ActionInputs;
+
+/// Base interface for writing a code action.
+class ActionProvider {
+public:
+ virtual ~ActionProvider() = default;
+
+ /// Run the first stage of the action. The non-None result indicates that the
+ /// action is available and should be shown to the user. Returns None if the
+ /// action is not available.
+ /// This function should be fast, if the action requires non-trivial work it
+ /// should be moved into the second stage, i.e. the continuation function
+ /// provided in the resulting PreparedAction.
+ virtual llvm::Optional
+ prepare(const ActionInputs ) = 0;
+};
+
+/// A context used by the actions to identify
+struct ActionInputs {
+ static llvm::Optional create(llvm::StringRef File,
+ llvm::StringRef Code,
+ ParsedAST , Range Selection);
+
+ /// The path of an active document the action was invoked in.
+ llvm::StringRef File;
+ /// The text of the active document.
+ llvm::StringRef Code;
+ /// Parsed AST of the active file.
+ ParsedAST
+ /// A location of the cursor in the editor.
+ SourceLocation Cursor;
+ // FIXME: add selection when there are checks relying on it.
+ // 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.
+};
+
+using ActionID = size_t;
+/// Result of executing a first stage of the action. If computing the resulting
+/// WorkspaceEdit is fast, the actions should produce it right away.
+/// For non-trivial actions, a continuation function to compute the resulting
+/// edits should be provided instead. It is expected that PreparedAction is
+/// consumed immediately when created, so continuations can reference the AST,
+/// so it's not safe to store the PreparedAction for long spans of time.
+class PreparedAction {
+public:
+ PreparedAction(std::string Title,
+
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov added a comment. That's the result of me playing around with writing a small code action for clangd. It's not final yet, e.g. missing tests and example actions, but still wanted to drop the change here to make sure it's not lost, I believe we'll need something like this in the near future anyway. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56267/new/ https://reviews.llvm.org/D56267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56267: [clangd] Interfaces for writing code actions
ilya-biryukov created this revision.
ilya-biryukov added reviewers: kadircet, ioeric, hokein, sammccall.
Herald added subscribers: arphaman, jkorous, MaskRay, mgorny.
The code actions can be run in two stages:
- Stage 1. Decides whether the action is available to the user and collects all
the information required to finish the action. Should be cheap, since this will
run over all the actions known to clangd on each textDocument/codeAction
request.
- Stage 2. Uses information from stage 1 to produce the actual edits that the
code action should perform. This stage can be expensive and will only run if
the user chooses to perform the specified action in the UI.
Trivial actions can also produce results in stage 1 to avoid an extra
round-trip and read of the AST to reconstruct the action on the server.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D56267
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/CodeActions.cpp
clangd/CodeActions.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/SourceCode.cpp
clangd/SourceCode.h
test/clangd/initialize-params.test
Index: test/clangd/initialize-params.test
===
--- test/clangd/initialize-params.test
+++ test/clangd/initialize-params.test
@@ -25,7 +25,8 @@
# CHECK-NEXT: "documentSymbolProvider": true,
# CHECK-NEXT: "executeCommandProvider": {
# CHECK-NEXT:"commands": [
-# CHECK-NEXT: "clangd.applyFix"
+# CHECK-NEXT: "clangd.applyFix",
+# CHECK-NEXT: "clangd.applyCodeAction"
# CHECK-NEXT:]
# CHECK-NEXT: },
# CHECK-NEXT: "hoverProvider": true,
Index: clangd/SourceCode.h
===
--- clangd/SourceCode.h
+++ clangd/SourceCode.h
@@ -65,6 +65,10 @@
std::pair offsetToClangLineColumn(llvm::StringRef Code,
size_t Offset);
+/// Return the substring, corresponding to the passed range.
+llvm::Expected rangeSubstr(llvm::StringRef Code,
+const Range );
+
/// From "a::b::c", return {"a::b::", "c"}. Scope is empty if there's no
/// qualifier.
std::pair
Index: clangd/SourceCode.cpp
===
--- clangd/SourceCode.cpp
+++ clangd/SourceCode.cpp
@@ -161,6 +161,20 @@
return {Lines + 1, Offset - StartOfLine + 1};
}
+llvm::Expected rangeSubstr(llvm::StringRef Code,
+const Range ) {
+ auto Begin = positionToOffset(Code, R.start, false);
+ if (!Begin)
+return Begin.takeError();
+ auto End = positionToOffset(Code, R.end, false);
+ if (!End)
+return End.takeError();
+ if (*End < *Begin)
+return createStringError(inconvertibleErrorCode(),
+ "invalid range passed to rangeSubstr");
+ return Code.substr(*Begin, *End - *Begin);
+}
+
std::pair splitQualifiedName(StringRef QName) {
size_t Pos = QName.rfind("::");
if (Pos == StringRef::npos)
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -632,6 +632,14 @@
bool fromJSON(const llvm::json::Value &, WorkspaceEdit &);
llvm::json::Value toJSON(const WorkspaceEdit );
+struct CodeActionArgs {
+ std::string file;
+ int64_t actionId;
+ Range selection;
+};
+bool fromJSON(const llvm::json::Value &, CodeActionArgs &);
+llvm::json::Value toJSON(const CodeActionArgs );
+
/// Exact commands are not specified in the protocol so we define the
/// ones supported by Clangd here. The protocol specifies the command arguments
/// to be "any[]" but to make this safer and more manageable, each command we
@@ -643,12 +651,15 @@
struct ExecuteCommandParams {
// Command to apply fix-its. Uses WorkspaceEdit as argument.
const static llvm::StringLiteral CLANGD_APPLY_FIX_COMMAND;
+ // Command to apply the code action. Uses action id as the argument.
+ const static llvm::StringLiteral CLANGD_APPLY_CODE_ACTION;
/// The command identifier, e.g. CLANGD_APPLY_FIX_COMMAND
std::string command;
// Arguments
llvm::Optional workspaceEdit;
+ llvm::Optional codeActionArgs;
};
bool fromJSON(const llvm::json::Value &, ExecuteCommandParams &);
@@ -670,6 +681,7 @@
/// Used to filter code actions.
llvm::Optional kind;
const static llvm::StringLiteral QUICKFIX_KIND;
+ const static llvm::StringLiteral REFACTOR_KIND;
/// The diagnostics that this code action resolves.
llvm::Optional> diagnostics;
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -418,6 +418,9 @@
const StringLiteral ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND =
"clangd.applyFix";
+const StringLiteral
