[clang-tools-extra] 0d893fd - [clangd] Add a symbol-name-based blacklist for rename.

[Haojian Wu via cfe-commits]

Author: Haojian Wu
Date: 2020-01-29T10:32:40+01:00
New Revision: 0d893fda4305f19be18bc60f56839f2143c78b38

URL: https://github.com/llvm/llvm-project/commit/0d893fda4305f19be18bc60f56839f2143c78b38
DIFF: https://github.com/llvm/llvm-project/commit/0d893fda4305f19be18bc60f56839f2143c78b38.diff

LOG: [clangd] Add a symbol-name-based blacklist for rename.

Summary:
This patch adds a simple mechanism to disallow global rename
on std symbols. We might extend it to other symbols, e.g. protobuf.

Reviewers: kadircet

Subscribers: mgorny, ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits

Tags: #clang

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

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 9518d848eff5..83f3a02d80d7 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -75,8 +75,8 @@ llvm::Optional<std::string> getOtherRefFile(const Decl &D, StringRef MainFile,
   return OtherFile;
 }
 
-llvm::DenseSet<const Decl *> locateDeclAt(ParsedAST &AST,
-                                          SourceLocation TokenStartLoc) {
+llvm::DenseSet<const NamedDecl *> locateDeclAt(ParsedAST &AST,
+                                               SourceLocation TokenStartLoc) {
   unsigned Offset =
       AST.getSourceManager().getDecomposedSpellingLoc(TokenStartLoc).second;
 
@@ -85,7 +85,7 @@ llvm::DenseSet<const Decl *> locateDeclAt(ParsedAST &AST,
   if (!SelectedNode)
     return {};
 
-  llvm::DenseSet<const Decl *> Result;
+  llvm::DenseSet<const NamedDecl *> Result;
   for (const NamedDecl *D :
        targetDecl(SelectedNode->ASTNode,
                   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
@@ -96,6 +96,15 @@ llvm::DenseSet<const Decl *> locateDeclAt(ParsedAST &AST,
   return Result;
 }
 
+bool isBlacklisted(const NamedDecl &RenameDecl) {
+  static const auto *StdSymbols = new llvm::DenseSet<llvm::StringRef>({
+#define SYMBOL(Name, NameSpace, Header) {#NameSpace #Name},
+#include "StdSymbolMap.inc"
+#undef SYMBOL
+  });
+  return StdSymbols->count(RenameDecl.getQualifiedNameAsString());
+}
+
 enum ReasonToReject {
   NoSymbolFound,
   NoIndexProvided,
@@ -105,7 +114,7 @@ enum ReasonToReject {
   AmbiguousSymbol,
 };
 
-llvm::Optional<ReasonToReject> renameable(const Decl &RenameDecl,
+llvm::Optional<ReasonToReject> renameable(const NamedDecl &RenameDecl,
                                           StringRef MainFilePath,
                                           const SymbolIndex *Index,
                                           bool CrossFile) {
@@ -120,6 +129,9 @@ llvm::Optional<ReasonToReject> renameable(const Decl &RenameDecl,
   if (RenameDecl.getParentFunctionOrMethod())
     return None;
 
+  if (isBlacklisted(RenameDecl))
+    return ReasonToReject::UnsupportedSymbol;
+
   // Check whether the symbol being rename is indexable.
   auto &ASTCtx = RenameDecl.getASTContext();
   bool MainFileIsHeader = isHeaderFile(MainFilePath, ASTCtx.getLangOpts());
@@ -131,12 +143,10 @@ llvm::Optional<ReasonToReject> renameable(const Decl &RenameDecl,
     IsMainFileOnly = false;
   else if (!DeclaredInMainFile)
     IsMainFileOnly = false;
-  bool IsIndexable =
-      isa<NamedDecl>(RenameDecl) &&
-      SymbolCollector::shouldCollectSymbol(
-          cast<NamedDecl>(RenameDecl), RenameDecl.getASTContext(),
-          SymbolCollector::Options(), IsMainFileOnly);
-  if (!IsIndexable) // If the symbol is not indexable, we disallow rename.
+  // If the symbol is not indexable, we disallow rename.
+  if (!SymbolCollector::shouldCollectSymbol(
+          RenameDecl, RenameDecl.getASTContext(), SymbolCollector::Options(),
+          IsMainFileOnly))
     return ReasonToReject::NonIndexable;
 
   if (!CrossFile) {
@@ -467,13 +477,10 @@ llvm::Expected<FileEdits> rename(const RenameInputs &RInputs) {
   if (DeclsUnderCursor.size() > 1)
     return makeError(ReasonToReject::AmbiguousSymbol);
 
-  const auto *RenameDecl = llvm::dyn_cast<NamedDecl>(*DeclsUnderCursor.begin());
-  if (!RenameDecl)
-    return makeError(ReasonToReject::UnsupportedSymbol);
-
-  auto Reject =
-      renameable(*RenameDecl->getCanonicalDecl(), RInputs.MainFilePath,
-                 RInputs.Index, RInputs.AllowCrossFile);
+  const auto &RenameDecl =
+      llvm::cast<NamedDecl>(*(*DeclsUnderCursor.begin())->getCanonicalDecl());
+  auto Reject = renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index,
+                           RInputs.AllowCrossFile);
   if (Reject)
     return makeError(*Reject);
 
@@ -486,7 +493,7 @@ llvm::Expected<FileEdits> rename(const RenameInputs &RInputs) {
   // To make cross-file rename work for local symbol, we use a hybrid solution:
   //   - run AST-based rename on the main file;
   //   - run index-based rename on other affected files;
-  auto MainFileRenameEdit = renameWithinFile(AST, *RenameDecl, RInputs.NewName);
+  auto MainFileRenameEdit = renameWithinFile(AST, RenameDecl, RInputs.NewName);
   if (!MainFileRenameEdit)
     return MainFileRenameEdit.takeError();
 
@@ -502,7 +509,7 @@ llvm::Expected<FileEdits> rename(const RenameInputs &RInputs) {
   // symbol if we don't have index.
   if (RInputs.Index) {
     auto OtherFilesEdits =
-        renameOutsideFile(*RenameDecl, RInputs.MainFilePath, RInputs.NewName,
+        renameOutsideFile(RenameDecl, RInputs.MainFilePath, RInputs.NewName,
                           *RInputs.Index, GetFileContent);
     if (!OtherFilesEdits)
       return OtherFilesEdits.takeError();

diff  --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index 95b960132a44..cb7911a4d94d 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -383,12 +383,12 @@ TEST(RenameTest, WithinFileRename) {
 
       // Typedef.
       R"cpp(
-        namespace std {
+        namespace ns {
         class basic_string {};
         typedef basic_string [[s^tring]];
-        } // namespace std
+        } // namespace ns
 
-        std::[[s^tring]] foo();
+        ns::[[s^tring]] foo();
       )cpp",
 
       // Variable.
@@ -540,6 +540,13 @@ TEST(RenameTest, Renameable) {
          void fo^o() {})cpp",
        "used outside main file", !HeaderFile, nullptr /*no index*/},
 
+      {R"cpp(// disallow rename on blacklisted symbols (e.g. std symbols)
+         namespace std {
+         class str^ing {};
+         }
+       )cpp",
+       "not a supported kind", !HeaderFile, Index},
+
       {R"cpp(
          void foo(int);
          void foo(char);