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

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.
Closed by commit rG0d893fda4305: [clangd] Add a symbol-name-based blacklist for 
rename. (authored by hokein).

Changed prior to commit:
  https://reviews.llvm.org/D73450?vs=240853&id=241068#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73450

Files:
  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
@@ -383,12 +383,12 @@
 
       // 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 @@
          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);
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -75,8 +75,8 @@
   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 @@
   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 @@
   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 @@
   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 @@
   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 @@
     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 @@
   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 @@
   // 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 @@
   // 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();

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits