[clangd] Store Index in Tweak::Selection

[kadircet]

Summary

Incoming define out-of-line tweak requires access to index.

This patch simply propogates the index in ClangdServer to Tweak::Selection while
passing the AST. Also updates TweakTest to accommodate this change.

[ilya-biryukov]

Overall LG, just a few quick questions.

clang-tools-extra/clangd/refactor/Tweak.h

67

NIT: Let's avoid mentioning ClangdServer here. ClangdServer is a "glue" that brings things like tweaks, code completion and other code together... Mentioning it here is sort of a "reverse" dependency...

68

NIT: Maybe put it right before ParsedAST?

clang-tools-extra/clangd/unittests/TweakTesting.h

72

How is this index populated? Each test has to mock it?

[kadircet]

clang-tools-extra/clangd/unittests/TweakTesting.h

72

that's what we usually do with the indexes in the rest of the testing code.(see code completion tests for an example)

we might decide to have some defaults if there are a substantial amount of tests making use of the same pattern, wdyt?

[kadircet]

• Address comments

[ilya-biryukov]

clang-tools-extra/clangd/unittests/TweakTesting.h

72

I see two potential problems:

• Lifetime of the index. Why not make this field a unique_ptr<SymbolIndex>? Client code wouldn't need to worry about keeping the stale pointer in the Index field when exiting...
• Ease of discovery for common patterns. Searching for a proper helper does not usually take a lot of time, but I somehow always forget what it is. Should we maybe add a comment mentioning a function that is commonly used for mocking index in tests. WDYT?

[kadircet]

• Store unique_ptr instead of a raw one.

[kadircet]

clang-tools-extra/clangd/unittests/TweakTesting.h

72

Lifetime of the index. Why not make this field a unique_ptr<SymbolIndex>? Client code wouldn't need to worry about keeping the stale pointer in the Index field when exiting...

Done.

Ease of discovery for common patterns. Searching for a proper helper does not usually take a lot of time, but I somehow always forget what it is. Should we maybe add a comment mentioning a function that is commonly used for mocking index in tests. WDYT?

Unfortunately there's no such common helper, every test has different requirements while creating those indexes so has a specialized way to go from a bunch of "DSL-like input" into a memindex.

[ilya-biryukov]

LGTM. Thanks

clang-tools-extra/clangd/refactor/Tweak.h

51

NIT: move it closer to the ParsedAST in constructor as well?

54

s/code-base/codebase
or
"code base"

This suggestion was inspired by Wikipedia xD)

clang-tools-extra/clangd/unittests/TweakTesting.h

72

Unfortunately there's no such common helper, every test has different requirements while creating those indexes so has a specialized way to go from a bunch of "DSL-like input" into a memindex.

Ah, this is sad... Anyway, let's figure it out when an actual test using the index will get added...

[kadircet]

• Address comments

clang-tools-extra/clangd/ClangdServer.cpp

 static llvm::Expected<Tweak::Selection>
 tweakSelection(const Range &Sel, const InputsAndAST &AST) {
   auto Begin = positionToOffset(AST.Inputs.Contents, Sel.start);
   if (!Begin)
     return Begin.takeError();
   auto End = positionToOffset(AST.Inputs.Contents, Sel.end);
   if (!End)
     return End.takeError();
-  return Tweak::Selection(AST.AST, *Begin, *End);
+  return Tweak::Selection(AST.Inputs.Index, AST.AST, *Begin, *End);
 }
 
 void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
                                    Callback<std::vector<TweakRef>> CB) {
   auto Action = [File = File.str(), Sel, CB = std::move(CB),
                  this](Expected<InputsAndAST> InpAST) mutable {
     if (!InpAST)
       return CB(InpAST.takeError());

clang-tools-extra/clangd/refactor/Tweak.h

 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_TWEAK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_TWEAK_H
 
 #include "ParsedAST.h"
 #include "Path.h"
 #include "Protocol.h"
 #include "Selection.h"
 #include "SourceCode.h"
+#include "index/Index.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include <string>
 
 namespace clang {
 namespace clangd {
 
 /// 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:
 ///     const char* id() const override final; // defined by REGISTER_TWEAK.
 ///     // implement other methods here.
 ///   };
 ///   REGISTER_TWEAK(MyTweak);
 class Tweak {
 public:
   /// Input to prepare and apply tweaks.
   struct Selection {
-    Selection(ParsedAST &AST, unsigned RangeBegin, unsigned RangeEnd);
+    Selection(const SymbolIndex *Index, ParsedAST &AST, unsigned RangeBegin,
+              unsigned RangeEnd);
     /// The text of the active document.
     llvm::StringRef Code;
+    /// The Index for handling codebase related queries.
+    const SymbolIndex *Index = nullptr;
     /// Parsed AST of the active file.
     ParsedAST &AST;
     /// A location of the cursor in the editor.
     // FIXME: Cursor is redundant and should be removed
     SourceLocation Cursor;
     /// The begin offset of the selection
     unsigned SelectionBegin;
     /// The end offset of the selection
     unsigned SelectionEnd;
     /// The AST nodes that were selected.
     SelectionTree ASTSelection;
     // FIXME: provide a way to get sources and ASTs for other files.
   };
 
   /// Output of a tweak.
   enum Intent {
     /// Apply changes that preserve the behavior of the code.
     Refactor,
     /// Provide information to the user.
     Info,
   };

clang-tools-extra/clangd/refactor/Tweak.cpp

 //===--- Tweak.cpp -----------------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "Tweak.h"
 #include "Logger.h"
 #include "Path.h"
 #include "SourceCode.h"
+#include "index/Index.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Registry.h"
 #include <functional>
for (const auto &E : TweakRegistry::entries()) {
     assert(E.instantiate()->id() == E.getName() &&
            "id should be equal to class name");
     assert(Seen.try_emplace(E.getName()).second && "duplicate check id");
   }
 #endif
 }
 } // namespace
 
-Tweak::Selection::Selection(ParsedAST &AST, unsigned RangeBegin,
-                            unsigned RangeEnd)
-    : AST(AST), SelectionBegin(RangeBegin), SelectionEnd(RangeEnd),
+Tweak::Selection::Selection(const SymbolIndex *Index, ParsedAST &AST,
+                            unsigned RangeBegin, unsigned RangeEnd)
+    : Index(Index), AST(AST), SelectionBegin(RangeBegin),
+      SelectionEnd(RangeEnd),
       ASTSelection(AST.getASTContext(), AST.getTokens(), RangeBegin, RangeEnd) {
   auto &SM = AST.getSourceManager();
   Code = SM.getBufferData(SM.getMainFileID());
   Cursor = SM.getComposedLoc(SM.getMainFileID(), RangeBegin);
 }
 
 std::vector<std::unique_ptr<Tweak>>
 prepareTweaks(const Tweak::Selection &S,

clang-tools-extra/clangd/unittests/TweakTesting.h

 //===--- TweakTesting.h - Test helpers for refactoring actions ---*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TWEAKTESTING_H
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TWEAKTESTING_H
 
 #include "TestTU.h"
+#include "index/Index.h"
 #include "llvm/ADT/StringMap.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include <memory>
 #include <string>
 
 namespace clang {
 namespace clangd {
 
 // Fixture base for testing tweaks. Intended to be subclassed for each tweak.
 //
 // Usage:
protected:
   std::string Header;
 
   // Extra flags passed to the compilation in apply().
   std::vector<const char *> ExtraArgs;
 
   // Context in which snippets of code should be placed to run tweaks.
   CodeContext Context = File;
 
+  // Index to be passed into Tweak::Selection.
+  std::unique_ptr<const SymbolIndex> Index = nullptr;
+
   // Apply the current tweak to the range (or point) in MarkedCode.
   // MarkedCode will be wrapped according to the Context.
   //  - if the tweak produces edits, returns the edited code (without markings)
   //    for the main file.
   //    Populates \p EditedFiles if there were changes to other files whenever
   //    it is non-null. It is a mapping from absolute path of the edited file to
   //    its new contents. Passing a nullptr to \p EditedFiles when there are
   //    changes, will result in a failure.

clang-tools-extra/clangd/unittests/TweakTesting.cpp

 //===-- TweakTesting.cpp ------------------------------------------------*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "TweakTesting.h"
 
 #include "Annotations.h"
 #include "SourceCode.h"
 #include "TestFS.h"
 #include "refactor/Tweak.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 using Context = TweakTest::CodeContext;
 
if (A.points().size() != 0) {
     SelectionRng = Range{A.point(), A.point()};
   } else {
     SelectionRng = A.range();
   }
   return {cantFail(positionToOffset(A.code(), SelectionRng.start)),
           cantFail(positionToOffset(A.code(), SelectionRng.end))};
 }
 
-MATCHER_P4(TweakIsAvailable, TweakID, Ctx, Header, ExtraFiles,
+MATCHER_P5(TweakIsAvailable, TweakID, Ctx, Header, ExtraFiles, Index,
            (TweakID + (negation ? " is unavailable" : " is available")).str()) {
   std::string WrappedCode = wrap(Ctx, arg);
   Annotations Input(WrappedCode);
   auto Selection = rangeOrPoint(Input);
   TestTU TU;
   TU.HeaderCode = Header;
   TU.Code = Input.code();
   TU.AdditionalFiles = std::move(ExtraFiles);
   ParsedAST AST = TU.build();
-  Tweak::Selection S(AST, Selection.first, Selection.second);
+  Tweak::Selection S(Index, AST, Selection.first, Selection.second);
   auto PrepareResult = prepareTweak(TweakID, S);
   if (PrepareResult)
     return true;
   llvm::consumeError(PrepareResult.takeError());
   return false;
 }
 
 } // namespace
 
 std::string TweakTest::apply(llvm::StringRef MarkedCode,
                              llvm::StringMap<std::string> *EditedFiles) const {
   std::string WrappedCode = wrap(Context, MarkedCode);
   Annotations Input(WrappedCode);
   auto Selection = rangeOrPoint(Input);
 
   TestTU TU;
   TU.HeaderCode = Header;
   TU.AdditionalFiles = std::move(ExtraFiles);
   TU.Code = Input.code();
   TU.ExtraArgs = ExtraArgs;
   ParsedAST AST = TU.build();
-  Tweak::Selection S(AST, Selection.first, Selection.second);
+  Tweak::Selection S(Index.get(), AST, Selection.first, Selection.second);
 
   auto T = prepareTweak(TweakID, S);
   if (!T) {
     llvm::consumeError(T.takeError());
     return "unavailable";
   }
   llvm::Expected<Tweak::Effect> Result = (*T)->apply(S);
   if (!Result)
else {
       else
         EditedFiles->try_emplace(It.first(), Unwrapped.str());
     }
   }
   return EditedMainFile;
 }
 
 ::testing::Matcher<llvm::StringRef> TweakTest::isAvailable() const {
-  return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header,
-                          ExtraFiles);
+  return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header, ExtraFiles,
+                          Index.get());
 }
 
 std::vector<std::string> TweakTest::expandCases(llvm::StringRef MarkedCode) {
   Annotations Test(MarkedCode);
   llvm::StringRef Code = Test.code();
   std::vector<std::string> Cases;
   for (const auto& Point : Test.points()) {
     size_t Offset = llvm::cantFail(positionToOffset(Code, Point));