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

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 296438.
hokein marked an inline comment as done.
hokein added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88875

Files:
  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


Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -516,6 +516,7 @@
     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 @@
       )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 @@
       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;
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -120,6 +120,9 @@
   UsedOutsideFile, // for within-file rename only.
   UnsupportedSymbol,
   AmbiguousSymbol,
+
+  // name validation.
+  RenameToKeywords,
 };
 
 llvm::Optional<ReasonToReject> renameable(const NamedDecl &RenameDecl,
@@ -208,6 +211,8 @@
       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 @@
     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());
Index: clang-tools-extra/clangd/ClangdServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -273,9 +273,6 @@
                     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);
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -412,9 +412,9 @@
     //  - 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D88875.296438.patch
Type: text/x-patch
Size: 3970 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201006/0c129a21/attachment-0001.bin>