[PATCH] D153769: [clangd] Implement the 'Organize Imports' source action. Fix include-cleaner findings in batch.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

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?



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:24-25
+
+namespace clang {
+namespace clangd {
+namespace {
----------------
nit: `namespace clang::clangd {`


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:45-49
+std::string OrganizeImports::title() const {
+  return "Remove unused and add missing includes";
+}
+
+bool OrganizeImports::prepare(const Selection &Inputs) { return true; }
----------------
nit: i'd inline these into the class definition as well


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:51
+
+Expected<OrganizeImports::Effect>
+OrganizeImports::apply(const Selection &Inputs) {
----------------
s/OrganizeImports::Effect/Tweak::Effect


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:62
+  const auto &SM = Inputs.AST->getSourceManager();
+  std::set<std::string> Spellings;
+  for (const auto &Missing : Findings.MissingIncludes)
----------------
prefer `llvm::StringSet<>`


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:64-67
+    for (const auto &P : Missing.Providers)
+      Spellings.insert(include_cleaner::spellHeader(
+          {P, Inputs.AST->getPreprocessor().getHeaderSearchInfo(),
+           SM.getFileEntryForID(SM.getMainFileID())}));
----------------
this will insert all providers for a symbol (e.g. if we have both `foo.h` and `bar.h`, we'll insert both).


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:75
+  auto FileStyle =
+      format::getStyle(format::DefaultFormatStyle, Inputs.AST->tuPath(),
+                       format::DefaultFallbackStyle, Inputs.Code, Inputs.FS);
----------------



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:77
+                       format::DefaultFallbackStyle, Inputs.Code, Inputs.FS);
+  if (!FileStyle)
+    FileStyle = format::getLLVMStyle();
----------------
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.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:79-83
+  if (auto Final = format::cleanupAroundReplacements(Inputs.Code, Replacements,
+                                                     *FileStyle))
+    return Effect::mainFileEdit(SM, *Final);
+  else
+    return Final.takeError();
----------------
nit:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153769