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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:752
   auto Action = [File = File.str(), Sel, TweakID = TweakID.str(),
-                 CB = std::move(CB),
+                 CB = std::move(CB), ActionKinds,
                  this](Expected<InputsAndAST> InpAST) mutable {
----------------
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`.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:49
+bool OrganizeImports::prepare(const Tweak::Selection &Inputs) {
+  if (std::find(Inputs.RequestedActionKinds.begin(),
+                Inputs.RequestedActionKinds.end(),
----------------
can we also bail out for ObjC


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:56
+    return false;
+  Range PreambleRange;
+  PreambleRange.start =
----------------
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)


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:56
+    return false;
+  Range PreambleRange;
+  PreambleRange.start =
----------------
kadircet wrote:
> 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)
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`.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:63
+            offsetToPosition(Inputs.Code, Inputs.SelectionEnd)};
+  return SelectionRange.start <= PreambleRange.end &&
+         PreambleRange.start <= SelectionRange.end;
----------------
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).


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:72
+  for (const auto *Inc : Findings.UnusedIncludes)
+    if (auto Err = Replacements.add(
+            tooling::Replacement{MainFilePath, UINT_MAX, 1, Inc->Written}))
----------------
nit: insertions of Replacements with offset `UINT_MAX` can't fail. so you just wrap this inside `llvm::cantFail` instead.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:87
+         SM.getFileEntryForID(SM.getMainFileID())});
+    if (auto Err = Replacements.add(tooling::Replacement{
+            MainFilePath, UINT_MAX, 0, "#include " + Spelling}))
----------------
same here for never failing


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:92
+
+  if (Replacements.empty())
+    return Tweak::Effect{"No edits to apply.", {}};
----------------
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)


================
Comment at: clang-tools-extra/clangd/unittests/tweaks/OrganizeImportsTests.cpp:19
+
+TEST_F(OrganizeImportsTest, All) {
+  Header = "void foo();";
----------------
can you also add some availability tests ?


================
Comment at: clang-tools-extra/clangd/unittests/tweaks/TweakTesting.cpp:73
+        Tweak::Selection S(Index, AST, Range.Begin, Range.End, std::move(ST),
+                           FS, {std::string{CodeAction::SOURCE_KIND}});
+        if (auto T = prepareTweak(TweakID, S, nullptr)) {
----------------
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}`;
for `TweakTest::isAvailable`, we can update `EXPECT_AVAILABLE_` to pass in `{}` and call `isAvailable` directly in `OrganizeImportsTests` with `SOURCE_KIND`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153769