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

[hokein]

Summary

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

[merge_guards_bot]

Unit tests: pass. 62202 tests passed, 0 failed and 815 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 1 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[kadircet]

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

145–146

nit: maybe just if (!shouldCollectSymbol...) return nonindexable; ?

474

locateDeclAt is already working on NamedDecls but returning a set of Decls could you rather update that helper to return a set of NamedDecls instead?

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

14 ↗

(On Diff #240497)

I don't think there's much value in having that in its own file, could you move it into the rename.cpp ?

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

454

maybe SCOPEDT_TRACE(T) instead?

[hokein]

address comments.

[hokein]

update the code.

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

474

I think the main problem is that NamedDecl->getCanonicalDecl() returns a Decl*, which we need to do a dyn_cast.

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

14 ↗

(On Diff #240497)

I thought we will extend this in the near future, moved to rename.cpp now as it is pretty short.

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

454

good point, reverted the change in this patch, and will do this in a separate patch.

[merge_guards_bot]

Unit tests: pass. 62202 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: pass. 62202 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[kadircet]

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

148

maybe move comment to be above if statement, and clang-format

474

ah right, but still it should be safe to perform just an llvm:cast here, as a NamedDecl shouldn't have an unnamed decl as its canonical declaration.

[hokein]

move the comment.

[hokein]

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

474

a NamedDecl shouldn't have an unnamed decl as its canonical declaration.

yeah, this is true for most cases, but it is not safe, we have corn cases, see the comment in SymbolCollector about ObjCPropertyDecl.

[merge_guards_bot]

Unit tests: pass. 62202 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[kadircet]

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

148

looks like it is still not clang-formatted

474

I don't think that's relevant in here though, it is performing the cast on a Decl. It is the ASTNode.OrigD that has been populated by libIndex.
So it doesn't need to be related with whatever nameddecl is being processed.

Am I missing something?

[hokein]

address comment.

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

474

ah, you are right. I thought ASTNode.OrigD was the decl getting from getCanonicalDecl, but it turns out not.

I also checked with the Decl::getCanonicalDecl default implementation, if the Decl subclass doesn't override this method, it just returns itself. so it is safe for NamedDecl.

[kadircet]

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

225

nit: maybe revert this one?

[merge_guards_bot]

Unit tests: pass. 62202 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

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

Index.refs(Req, [&](const Ref &R) {
     if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) {
       if (*RefFilePath != MainFile)
         OtherFile = *RefFilePath;
     }
   });
   return OtherFile;
 }
 
-llvm::DenseSet<const Decl *> locateDeclAt(ParsedAST &AST,
+llvm::DenseSet<const NamedDecl *> locateDeclAt(ParsedAST &AST,
                                                SourceLocation TokenStartLoc) {
   unsigned Offset =
       AST.getSourceManager().getDecomposedSpellingLoc(TokenStartLoc).second;
 
   SelectionTree Selection(AST.getASTContext(), AST.getTokens(), Offset);
   const SelectionTree::Node *SelectedNode = Selection.commonAncestor();
   if (!SelectedNode)
     return {};
 
-  llvm::DenseSet<const Decl *> Result;
+  llvm::DenseSet<const NamedDecl *> Result;
   for (const NamedDecl *D :
        targetDecl(SelectedNode->ASTNode,
                   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
     // Get to CXXRecordDecl from constructor or destructor.
     D = tooling::getCanonicalSymbolDeclaration(D);
     Result.insert(D);
   }
   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,
   NonIndexable,
   UsedOutsideFile, // for within-file rename only.
   UnsupportedSymbol,
   AmbiguousSymbol,
 };
 
-llvm::Optional<ReasonToReject> renameable(const Decl &RenameDecl,
+llvm::Optional<ReasonToReject> renameable(const NamedDecl &RenameDecl,
                                           StringRef MainFilePath,
                                           const SymbolIndex *Index,
                                           bool CrossFile) {
   // Filter out symbols that are unsupported in both rename modes.
   if (llvm::isa<NamespaceDecl>(&RenameDecl))
     return ReasonToReject::UnsupportedSymbol;
   if (const auto *FD = llvm::dyn_cast<FunctionDecl>(&RenameDecl)) {
     if (FD->isOverloadedOperator())
       return ReasonToReject::UnsupportedSymbol;
   }
   // function-local symbols is safe to rename.
   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());
   bool DeclaredInMainFile =
       isInsideMainFile(RenameDecl.getBeginLoc(), ASTCtx.getSourceManager());
   bool IsMainFileOnly = true;
   if (MainFileIsHeader)
     // main file is a header, the symbol can't be main file only.
     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) {
     if (!DeclaredInMainFile)
       // We are sure the symbol is used externally, bail out early.
       return ReasonToReject::UsedOutsideFile;
 
     // If the symbol is declared in the main file (which is not a header), we
std::vector<SourceLocation> findOccurrencesWithinFile(ParsedAST &AST,
   // FIXME: Get rid of the remaining tooling APIs.
   const auto *RenameDecl =
       ND.getDescribedTemplate() ? ND.getDescribedTemplate() : &ND;
   std::vector<std::string> RenameUSRs =
       tooling::getUSRsForDeclaration(RenameDecl, AST.getASTContext());
   llvm::DenseSet<SymbolID> TargetIDs;
   for (auto &USR : RenameUSRs)
     TargetIDs.insert(SymbolID(USR));
 
   std::vector<SourceLocation> Results;
   for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
     findExplicitReferences(TopLevelDecl, [&](ReferenceLoc Ref) {
       if (Ref.Targets.empty())
         return;
       for (const auto *Target : Ref.Targets) {
         auto ID = getSymbolID(Target);
         if (!ID || TargetIDs.find(*ID) == TargetIDs.end())
llvm::Expected<FileEdits> rename(const RenameInputs &RInputs) {
   // Renames should only triggered on identifiers.
   if (!IdentifierToken)
     return makeError(ReasonToReject::NoSymbolFound);
   // FIXME: Renaming macros is not supported yet, the macro-handling code should
   // be moved to rename tooling library.
   if (locateMacroAt(IdentifierToken->location(), 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);
 
-  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);
 
   // We have two implementations of the rename:
   //   - AST-based rename: used for renaming local symbols, e.g. variables
   //     defined in a function body;
   //   - index-based rename: used for renaming non-local symbols, and not
   //     feasible for local symbols (as by design our index don't index these
   //     symbols by design;
   // 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();
 
   if (!RInputs.AllowCrossFile) {
     // Within-file rename: just return the main file results.
     return FileEdits(
         {std::make_pair(RInputs.MainFilePath,
                         Edit{MainFileCode, std::move(*MainFileRenameEdit)})});
   }
 
   FileEdits Results;
   // Renameable safely guards us that at this point we are renaming a local
   // 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();
     Results = std::move(*OtherFilesEdits);
   }
   // Attach the rename edits for the main file.
   Results.try_emplace(RInputs.MainFilePath, MainFileCode,
                       std::move(*MainFileRenameEdit));

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

llvm::StringRef Tests[] = {
           }
           static void foo([[T]] value) {}
           [[T]] member;
         };
       )cpp",
 
       // 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.
       R"cpp(
         namespace A {
         int [[F^oo]];
         }
         int Foo;
for (llvm::StringRef T : Tests) {
     Annotations Code(T);
     auto TU = TestTU::withCode(Code.code());
     TU.ExtraArgs.push_back("-fno-delayed-template-parsing");
     auto AST = TU.build();
     llvm::StringRef NewName = "abcde";
     for (const auto &RenamePos : Code.points()) {
       auto RenameResult =
           rename({RenamePos, NewName, AST, testPath(TU.Filename)});
       ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError();
       ASSERT_EQ(1u, RenameResult->size());
       EXPECT_EQ(applyEdits(std::move(*RenameResult)).front().second,
                 expectedResult(Code, NewName));
     }
   }
 }
 
 TEST(RenameTest, Renameable) {
Case Cases[] = {
       )cpp",
        "used outside main file", !HeaderFile /*cc file*/, Index},
 
       {R"cpp(
          // We should detect the symbol is used outside the file from the AST.
          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);
          template <typename T> void f(T t) {
            fo^o(t);
          })cpp",
        "multiple symbols", !HeaderFile, nullptr /*no index*/},