[clangd] Add a NewName optional parameter to clangdServer::prepareRename.

[hokein]

Summary

If the NewName is provided, prepareRename would perform a name validation.

The motivation is to allow our internal embedder to implement the customized
"canRenameInto" functionality on top of prepareRename.

[sammccall]

clang-tools-extra/clangd/ClangdLSPServer.cpp

797–798

please make this optional rather than using "" as a sentinel value. The empty string is a plausible value here (though of course will always fail)

clang-tools-extra/clangd/ClangdServer.cpp

404

we can fail already here if the name is specified and empty

[sammccall]

Can you add a bit more context to the commit message?

And should we expose this as an extension on textDocument/prepareRename?

[hokein]

make NewName optional.

[hokein]

In D88881#2313863, @sammccall wrote:

Can you add a bit more context to the commit message?

And should we expose this as an extension on textDocument/prepareRename?

no needed right now. AFAIK, there are no lsp clients that would use that.

clang-tools-extra/clangd/ClangdServer.cpp

404

yeah, we could do that, but not sure this is a good idea:

• adding special-case code (I'd like to avoid)
• error-message might be re-prioritized -- if an invalid pos, and empty new name are passed through this API, we will emit error message "the name is empty" rather than "no symbol was found", I think "no symbol was found" is slightly more important than the "name is empty";

[danielmartin]

clang-tools-extra/clangd/ClangdServer.h

277

[hokein]

clang-tools-extra/clangd/ClangdServer.h

277

oh, thanks! Fixed in ba268d2fb09ceabfa03a6499258f973be62d7663.

clang-tools-extra/clangd/ClangdLSPServer.cpp

Server->workspaceSymbols(
 
         Reply(std::move(*Items));
       });
 }
 
 void ClangdLSPServer::onPrepareRename(const TextDocumentPositionParams &Params,
                                       Callback<llvm::Optional<Range>> Reply) {
   Server->prepareRename(
-      Params.textDocument.uri.file(), Params.position, Opts.Rename,
+      Params.textDocument.uri.file(), Params.position, /*NewName*/ llvm::None,
+      Opts.Rename,
       [Reply = std::move(Reply)](llvm::Expected<RenameResult> Result) mutable {
         if (!Result)
           return Reply(Result.takeError());
         return Reply(std::move(Result->Target));
       });
 }
 
 void ClangdLSPServer::onRename(const RenameParams &Params,

clang-tools-extra/clangd/ClangdServer.h

void formatFile(PathRef File, StringRef Code,
                   Callback<tooling::Replacements> CB);
 
   /// Run formatting after \p TriggerText was typed at \p Pos in \p File with
   /// content \p Code.
   void formatOnType(PathRef File, StringRef Code, Position Pos,
                     StringRef TriggerText, Callback<std::vector<TextEdit>> CB);
 
   /// Test the validity of a rename operation.
+  ///
+  /// If NewName is provided, it peforms a name validation.
   void prepareRename(PathRef File, Position Pos,
+                     llvm::Optional<std::string> NewName,
                      const RenameOptions &RenameOpts,
                      Callback<RenameResult> CB);
 
   /// Rename all occurrences of the symbol at the \p Pos in \p File to
   /// \p NewName.
   /// If WantFormat is false, the final TextEdit will be not formatted,
   /// embedders could use this method to get all occurrences of the symbol (e.g.
   /// highlighting them in prepare stage).

clang-tools-extra/clangd/ClangdServer.cpp

for (const tooling::Replacement &R :
          formatIncremental(Code, CursorPos, TriggerText, *Style))
       Result.push_back(replacementToEdit(Code, R));
     return CB(Result);
   };
   WorkScheduler.run("FormatOnType", File, std::move(Action));
 }
 
 void ClangdServer::prepareRename(PathRef File, Position Pos,
+                                 llvm::Optional<std::string> NewName,
                                  const RenameOptions &RenameOpts,
                                  Callback<RenameResult> CB) {
-  auto Action = [Pos, File = File.str(), CB = std::move(CB), RenameOpts,
+  auto Action = [Pos, File = File.str(), CB = std::move(CB),
+                 NewName = std::move(NewName), RenameOpts,
                  this](llvm::Expected<InputsAndAST> InpAST) mutable {
     if (!InpAST)
       return CB(InpAST.takeError());
     // prepareRename is latency-sensitive:
     //  - for single-file rename, performing rename isn't substantially more
     //    expensive than doing an AST-based check (the index is used to see if
     //    the rename is complete);
     //  - for cross-file rename, we deliberately pass a nullptr index to save
     //    the cost, thus the result may be incomplete as it only contains
     //    main-file occurrences;
     auto Results = clangd::rename(
-        {Pos, /*NewName=*/"__clangd_rename_dummy", InpAST->AST, File,
+        {Pos, NewName.getValueOr("__clangd_rename_dummy"), InpAST->AST, File,
          RenameOpts.AllowCrossFile ? nullptr : Index, RenameOpts});
     if (!Results) {
       // LSP says to return null on failure, but that will result in a generic
       // failure message. If we send an LSP error response, clients can surface
       // the message to users (VSCode does).
       return CB(Results.takeError());
     }
     return CB(*Results);

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

TEST(RenameTest, PrepareRename) {
   auto ServerOpts = ClangdServer::optsForTest();
   ServerOpts.BuildDynamicSymbolIndex = true;
 
   MockCompilationDatabase CDB;
   ClangdServer Server(CDB, FS, ServerOpts);
   runAddDocument(Server, FooHPath, FooH.code());
   runAddDocument(Server, FooCCPath, FooCC.code());
 
-  auto Results =
-      runPrepareRename(Server, FooCCPath, FooCC.point(), {/*CrossFile=*/true});
+  auto Results = runPrepareRename(Server, FooCCPath, FooCC.point(),
+                                  /*NewName=*/llvm::None, {/*CrossFile=*/true});
   // verify that for multi-file rename, we only return main-file occurrences.
   ASSERT_TRUE(bool(Results)) << Results.takeError();
   // We don't know the result is complete in prepareRename (passing a nullptr
   // index internally), so GlobalChanges should be empty.
   EXPECT_TRUE(Results->GlobalChanges.empty());
   EXPECT_THAT(FooCC.ranges(),
               testing::UnorderedElementsAreArray(Results->LocalChanges));
 
-  // single-file rename on global symbols, we should report an error.
+  // verify name validation.
   Results =
-      runPrepareRename(Server, FooCCPath, FooCC.point(), {/*CrossFile=*/false});
+      runPrepareRename(Server, FooCCPath, FooCC.point(),
+                       /*NewName=*/std::string("int"), {/*CrossFile=*/true});
+  EXPECT_FALSE(Results);
+  EXPECT_THAT(llvm::toString(Results.takeError()),
+              testing::HasSubstr("keyword"));
+
+  // single-file rename on global symbols, we should report an error.
+  Results = runPrepareRename(Server, FooCCPath, FooCC.point(),
+                             /*NewName=*/llvm::None, {/*CrossFile=*/false});
   EXPECT_FALSE(Results);
   EXPECT_THAT(llvm::toString(Results.takeError()),
               testing::HasSubstr("is used outside"));
 }
 
 TEST(CrossFileRenameTests, DirtyBuffer) {
   Annotations FooCode("class [[Foo]] {};");
   std::string FooPath = testPath("foo.cc");

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

 runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos);
 
 llvm::Expected<RenameResult> runRename(ClangdServer &Server, PathRef File,
                                        Position Pos, StringRef NewName,
                                        const clangd::RenameOptions &RenameOpts);
 
 llvm::Expected<RenameResult>
 runPrepareRename(ClangdServer &Server, PathRef File, Position Pos,
+                 llvm::Optional<std::string> NewName,
                  const clangd::RenameOptions &RenameOpts);
 
 llvm::Expected<tooling::Replacements>
 runFormatFile(ClangdServer &Server, PathRef File, StringRef Code);
 
 SymbolSlab runFuzzyFind(const SymbolIndex &Index, StringRef Query);
 SymbolSlab runFuzzyFind(const SymbolIndex &Index, const FuzzyFindRequest &Req);
 RefSlab getRefs(const SymbolIndex &Index, SymbolID ID);

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

 llvm::Expected<RenameResult> runRename(ClangdServer &Server, PathRef File,
                                        Position Pos, llvm::StringRef NewName,
                                        const RenameOptions &RenameOpts) {
   llvm::Optional<llvm::Expected<RenameResult>> Result;
   Server.rename(File, Pos, NewName, RenameOpts, capture(Result));
   return std::move(*Result);
 }
 
-llvm::Expected<RenameResult> runPrepareRename(ClangdServer &Server,
-                                              PathRef File, Position Pos,
-                                              const RenameOptions &RenameOpts) {
+llvm::Expected<RenameResult>
+runPrepareRename(ClangdServer &Server, PathRef File, Position Pos,
+                 llvm::Optional<std::string> NewName,
   llvm::Optional<llvm::Expected<RenameResult>> Result;
-  Server.prepareRename(File, Pos, RenameOpts, capture(Result));
+  Server.prepareRename(File, Pos, NewName, RenameOpts, capture(Result));
   return std::move(*Result);
 }
 
 llvm::Expected<tooling::Replacements>
 runFormatFile(ClangdServer &Server, PathRef File, StringRef Code) {
   llvm::Optional<llvm::Expected<tooling::Replacements>> Result;
   Server.formatFile(File, Code, capture(Result));
   return std::move(*Result);