[clangd] Add basic keyword-name-validation in rename.

[sammccall]

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

124

nit: not sure we need a separate section for these, I can imagine at most keyword/conflict/shadow

215

nit: either "reserved [word|identifier|name]" or "keyword", but not both

This could be friendlier, what about "the chosen name is a keyword"?

479

Now that prepareRename only exposes ranges rather than edits, do we still need this empty special case, or can we go back to using "clangd_rename_dummy" or so?

(Sorry for the back and forth here)

[hokein]

address comments.

[hokein]

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

124

yeah, I would leave it as-is for now. we could factor this out when there are more.

479

oh, you're right. And the documentation of the prepareRename is stale. Updated.

A slight concern of using a dummy name is that it may cause a name validation, but it should rarely happen if we choose some name like clangd_rename_dummy.

[sammccall]

clang-tools-extra/clangd/ClangdServer.cpp

416

oh, reserved name is clever :-)

Though it occurs to me - we could actually warn on rename to a reserved name!
(We can still do this and just ignore __clangd_*)

[hokein]

clang-tools-extra/clangd/ClangdServer.cpp

416

giving a warning is nice, it seems too strict to forbidden users (library developers)) renaming a symbol to __a . The code is still compilable after all.

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.
-  ///
-  /// The returned result describes edits in the main-file only (all
-  /// occurrences of the renamed symbol are simply deleted.
   void prepareRename(PathRef File, Position Pos,
                      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.

clang-tools-extra/clangd/ClangdServer.cpp

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*/ "", InpAST->AST, File,
-                                   RenameOpts.AllowCrossFile ? nullptr : Index,
-                                   RenameOpts});
+    auto Results = clangd::rename(
+        {Pos, /*NewName=*/"__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/refactor/Rename.cpp

 
 enum class ReasonToReject {
   NoSymbolFound,
   NoIndexProvided,
   NonIndexable,
   UsedOutsideFile, // for within-file rename only.
   UnsupportedSymbol,
   AmbiguousSymbol,
+
+  // name validation.
+  RenameToKeywords,
 };
 
 llvm::Optional<ReasonToReject> renameable(const NamedDecl &RenameDecl,
                                           StringRef MainFilePath,
                                           const SymbolIndex *Index,
                                           bool CrossFile) {
   trace::Span Tracer("Renameable");
   // Filter out symbols that are unsupported in both rename modes.
auto Message = [](ReasonToReject Reason) {
     case ReasonToReject::UsedOutsideFile:
       return "the symbol is used outside main file";
     case ReasonToReject::NonIndexable:
       return "symbol may be used in other files (not eligible for indexing)";
     case ReasonToReject::UnsupportedSymbol:
       return "symbol is not a supported kind (e.g. namespace, macro)";
     case ReasonToReject::AmbiguousSymbol:
       return "there are multiple symbols at the given location";
+    case ReasonToReject::RenameToKeywords:
+      return "the chosen name is a keyword";
     }
     llvm_unreachable("unhandled reason kind");
   };
   return error("Cannot rename symbol: {0}", Message(Reason));
 }
 
 // Return all rename occurrences in the main file.
 std::vector<SourceLocation> findOccurrencesWithinFile(ParsedAST &AST,
llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
   if (locateMacroAt(*IdentifierToken, AST.getPreprocessor()))
     return makeError(ReasonToReject::UnsupportedSymbol);
 
   auto DeclsUnderCursor = locateDeclAt(AST, IdentifierToken->location());
   if (DeclsUnderCursor.empty())
     return makeError(ReasonToReject::NoSymbolFound);
   if (DeclsUnderCursor.size() > 1)
     return makeError(ReasonToReject::AmbiguousSymbol);
+  if (isKeyword(RInputs.NewName, AST.getLangOpts()))
+    return makeError(ReasonToReject::RenameToKeywords);
 
   const auto &RenameDecl =
       llvm::cast<NamedDecl>(*(*DeclsUnderCursor.begin())->getCanonicalDecl());
   auto Reject = renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index,
                            Opts.AllowCrossFile);
   if (Reject)
     return makeError(*Reject);
 

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

 }
 
 TEST(RenameTest, Renameable) {
   struct Case {
     const char *Code;
     const char* ErrorMessage; // null if no error
     bool IsHeaderFile;
     const SymbolIndex *Index;
+    llvm::StringRef NewName = "DummyName";
   };
   TestTU OtherFile = TestTU::withCode("Outside s; auto ss = &foo;");
   const char *CommonHeader = R"cpp(
     class Outside {};
     void foo();
   )cpp";
   OtherFile.HeaderCode = CommonHeader;
   OtherFile.Filename = "other.cc";
Case Cases[] = {
       )cpp",
        nullptr, HeaderFile, Index},
 
       {R"cpp(// allow -- symbol is indexable and has no refs in index.
         void [[On^lyInThisFile]]();
       )cpp",
        nullptr, HeaderFile, Index},
 
+      {R"cpp(
+        void ^f();
+      )cpp",
+       "keyword", HeaderFile, Index, "return"},
+
       {R"cpp(// disallow -- symbol is indexable and has other refs in index.
         void f() {
           Out^side s;
         }
       )cpp",
        "used outside main file", HeaderFile, Index},
 
       {R"cpp(// disallow -- symbol in anonymous namespace in header is not indexable.
for (const auto& Case : Cases) {
     TU.ExtraArgs.push_back("-fno-delayed-template-parsing");
     if (Case.IsHeaderFile) {
       // We open the .h file as the main file.
       TU.Filename = "test.h";
       // Parsing the .h file as C++ include.
       TU.ExtraArgs.push_back("-xobjective-c++-header");
     }
     auto AST = TU.build();
-    llvm::StringRef NewName = "dummyNewName";
+    llvm::StringRef NewName = Case.NewName;
     auto Results =
         rename({T.point(), NewName, AST, testPath(TU.Filename), Case.Index});
     bool WantRename = true;
     if (T.ranges().empty())
       WantRename = false;
     if (!WantRename) {
       assert(Case.ErrorMessage && "Error message must be set!");
       EXPECT_FALSE(Results)