Details
- Reviewers
kadircet
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
there are unittests for tweaks under clang-tools-extra/clangd/unittests/tweaks/ can you create one for OrganizeImports there and test this in there?
i've got another concern around triggering of this interaction.
in theory code-action context has a only field, dictating which code actions should be surfaced, but when it's unset (as it's usually not set by clients and hard to understand semantics of code action kinds) clangd just provides all of the available code actions for a given selection.
since organize imports (and other source actions) are going to be available for all selections, it means we'll now have some code action response all the time and clients that don't know about semantics for "source" code action kind, will likely just show it in their UIs. so we need to be somewhat careful about triggering.
two alternatives that I can think of are:
1- show "source" kind code actions, only when they're explicitly requested.
2- make use of trigger kind and return these only when code actions are "explicitly" triggered.
1 has the downside of never showing up on editors that don't know about "source" code action kind, and also complicates behaviour on clangd side.
2 has the downside of being introduced very recently (trigger kind is made part of the spec at LSP 3.17), hence it'll likely be absent in old clients.
so my inclination is towards alternative 2), which at least limits the blast radius and gives clients a reasonably easy way to control the behavior. WDYT?
clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp | ||
---|---|---|
25–26 | nit: namespace clang::clangd { | |
46–50 | nit: i'd inline these into the class definition as well | |
52 | s/OrganizeImports::Effect/Tweak::Effect | |
63 | prefer llvm::StringSet<> | |
65–68 | this will insert all providers for a symbol (e.g. if we have both foo.h and bar.h, we'll insert both). | |
76 | ||
78 | unfortunately we actually have to consume the error here, as getStyle returns an Expected<T> and destroying an unchecked Expected is undefined behavior :/ something like elog("Couldn't get style for {0}: {1}", MainFilePath, FileStyle.takeError()); should do the trick. | |
80–84 | nit: |
so as we were discussing offline (just to summarize it here) there's also the possibility of propagating context kind in selection, and making this code action fire when code actions are requested for "source code action kind" or the selection intersects with includes. which feels like the most useful alternative without annoying old clients.
Address review comments. Update the action to trigger when source action is requested explicitly or from the preamble range. Add unit test.
Thanks for the comments!
clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp | ||
---|---|---|
63 | I think it should actually be llvm::DenseSet<inlude_cleaner::Header>. We should first de-duplicate providers and only then spell the resulting headers. In this version, we were potentially spelling the same provider multiple times. | |
65–68 | you're right, thanks |
clang-tools-extra/clangd/ClangdServer.cpp | ||
---|---|---|
719–721 | we're capturing ActionKinds as a reference, but Action is going to execute async. you can instead change the input to be llvm::ArrayRef<std::string> ActionKinds and capture it by value with ActionKinds = ActionKinds.vec() nit: even better if you just changed the signature here to look like applyTweak(std::string TweakID, CodeActionInputs Inputs, Callback<Tweak::Effect> CB), as we want to consume all of these by value anyways. Then you can just move TweakID and Inputs into the Action. | |
clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp | ||
50 | can we also bail out for ObjC | |
57 | relying on MainFileIncludes for preamble range is not correct in general. we can have includes way down inside the main file that'll be part of this vector, but not preamble (also it's not sorted explicitly). we should instead use ComputePreambleBounds (nit: we can also store it in ParsedAST, instead of relexing the file one more time with each CodeAction request) | |
57 | Having a comment for reason would also be helpful, something like To accommodate clients without knowledge of source actions, we trigger without checking code action kinds when inside the preamble region. | |
64 | we should also make sure that RequestedActionKinds is empty in this case. Because we don't want to offer a code action of kind Source, if user is explicitly asking for some other kind(s). | |
73 | nit: insertions of Replacements with offset UINT_MAX can't fail. so you just wrap this inside llvm::cantFail instead. | |
88 | same here for never failing | |
93 | i think we should perform this check on Final instead (as replacements can still go empty after cleanups, and empty set of replacements will result in an empty set anyways) | |
clang-tools-extra/clangd/unittests/tweaks/OrganizeImportsTests.cpp | ||
20 | can you also add some availability tests ? | |
clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp | ||
87 | ATM this is passing SOURCE_KIND for all tweaks, which is conceptually wrong. can you instead take this in as a parameter? for TweakTest::apply, we can default it to be an empty vector, and just update OrganizeImportsTests to pass {CodeAction::SOURCE_KIND}; |
Thank you for the comments!
clang-tools-extra/clangd/ClangdServer.cpp | ||
---|---|---|
719–721 | Ok, I can change the signature, but it seems like I still need to move individual members of CodeActionInputs into the callback separately. File cannot be moved, since it's also needed after the callback as an argument to runWithAST. | |
clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp | ||
57 |
Seems like the data is already there, I just need to expose it. |
FYI, i don't think you uploaded the new version of the patch
clang-tools-extra/clangd/ClangdServer.cpp | ||
---|---|---|
719–721 | yeah, it's fine to make a copy | |
clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp | ||
57 |
not really, we calculate bounds when building a ParsedAST but we don't store it anywhere. |
I'm sorry I think the upload failed because I tried to upload without a message or something, and I didn't notice that it failed :(
I think I also managed to update the wrong patch. Not sure how an alternative patch got created in the first place :( Sorry for the mess, should be fine now.
Respond to comments.
clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp | ||
---|---|---|
57 | I was referring to https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/PrecompiledPreamble.cpp#L580. We store the PreambleData and that seems to store the PrecompiledPreamble which seems to know its bounds |
clang-tools-extra/clangd/ClangdLSPServer.cpp | ||
---|---|---|
810–815 | this is kind of hard to read | |
clang-tools-extra/clangd/ParsedAST.h | ||
124 | Unfortunately this won't work in presence of stale preambles. You can use the "patched bounds" we calculate when building a parsedast in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/ParsedAST.cpp#L646; store it in ParsedAST in a new field, and return it here. | |
clang-tools-extra/clangd/refactor/Tweak.h | ||
74 | prefer llvm::ArrayRef<std:string> here, Selections are not passed in between threads. So no need to make an extra copy when creating one. | |
clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp | ||
52 | nit: llvm::find(Inputs.RequestedActionKinds, CodeAction::SOURCE_KIND) | |
56 | nit: if (!Inputs.RequestedActionKinds.empty()) return llvm::find(Inputs.RequestedActionKinds, CodeAction::SOURCE_KIND) != Inputs.RequestedActionKinds.end(); // To accommodate clients without knowledge of source actions, we trigger // without checking code action kinds when inside the preamble region. return Inputs.SelectionEnd <= Inputs.AST->getPreambleBounds().Size; | |
68 | we should respect Config::Diagnostics::Includes::IgnoreHeader here, similar to finding integration, similarly for missing include fixes. | |
clang-tools-extra/clangd/unittests/ClangdTests.cpp | ||
1307 | nit: Again extracting CodeActionInputs, setting filename explicitly and passing that into the call would be great here. | |
clang-tools-extra/clangd/unittests/tweaks/OrganizeImportsTests.cpp | ||
35–41 | nit: you can use EXPECT_AVAILABLE and EXPECT_UNAVAILABLE directly for these two cases | |
clang-tools-extra/clangd/unittests/tweaks/TweakTesting.h | ||
101 | type is not really giving any ideas what the parameter is about here, can you name it ? |
Thanks!
clang-tools-extra/clangd/unittests/tweaks/OrganizeImportsTests.cpp | ||
---|---|---|
35–41 | I think it's the other way around. I can use the macros directly when I don't need to pass specific requested action kinds, which is the last (third) case. So it's probably not worth it. |
this is kind of hard to read