[clang-tools-extra] 8a3cbb1 - [clangd] Add basic keyword-name-validation in rename.

[Haojian Wu via cfe-commits]

Author: Haojian Wu
Date: 2020-10-06T15:47:57+02:00
New Revision: 8a3cbb1535a92dcc0ac3bd8fc64216a465b8506a

URL: https://github.com/llvm/llvm-project/commit/8a3cbb1535a92dcc0ac3bd8fc64216a465b8506a
DIFF: https://github.com/llvm/llvm-project/commit/8a3cbb1535a92dcc0ac3bd8fc64216a465b8506a.diff

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

Differential Revision: https://reviews.llvm.org/D88875

Added: 
    

Modified: 
    clang-tools-extra/clangd/ClangdServer.cpp
    clang-tools-extra/clangd/ClangdServer.h
    clang-tools-extra/clangd/refactor/Rename.cpp
    clang-tools-extra/clangd/unittests/RenameTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 0840155fc8f9..d38e115a6796 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -412,9 +412,9 @@ void ClangdServer::prepareRename(PathRef File, Position Pos,
     //  - 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

diff  --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index a9d46fa5278f..d03f50069746 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -273,9 +273,6 @@ class ClangdServer {
                     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);

diff  --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 9de3302564fd..e072853dac9f 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -120,6 +120,9 @@ enum class ReasonToReject {
   UsedOutsideFile, // for within-file rename only.
   UnsupportedSymbol,
   AmbiguousSymbol,
+
+  // name validation.
+  RenameToKeywords,
 };
 
 llvm::Optional<ReasonToReject> renameable(const NamedDecl &RenameDecl,
@@ -208,6 +211,8 @@ llvm::Error makeError(ReasonToReject Reason) {
       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");
   };
@@ -471,6 +476,8 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
     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());

diff  --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index cc2454e9d04e..d925dfa36f50 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -516,6 +516,7 @@ TEST(RenameTest, Renameable) {
     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(
@@ -542,6 +543,11 @@ TEST(RenameTest, Renameable) {
       )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;
@@ -639,7 +645,7 @@ TEST(RenameTest, Renameable) {
       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;