[clangd] Don't rename on symbols from system headers.

[hokein]

Summary

Fixes https://github.com/clangd/clangd/issues/963.

[kbobyrev]

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

163

nit: "as renaming these symbols..."

164

nit: "unlikely to be good candidates for modification"

Maybe my wording is bad, too, but what I want to convey is that the problem isn't that they can't be modified but that we don't want them to be modified!

171

Any reason not to do this right now? (if we keep the comment, then it's probably better as "Remove this check because it is redundant in the presence of isInSystemHeader")

[kbobyrev]

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

1202

Nit: capitalization.

[hokein]

address comments.

[hokein]

remove accident changes.

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

171

We could do it in this patch, I slightly prefer to do it in a followup patch, it'd require some changes in the unittests. (this FIXME was added in the last minute -- at the very beginning I didn't notice that it could be simplified)

[kbobyrev]

LG, thanks!

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

171

Okay, makes sense, thanks for explaining!

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

for (const NamedDecl *D :
        targetDecl(SelectedNode->ASTNode,
                   DeclRelation::Alias | DeclRelation::TemplatePattern,
                   AST.getHeuristicResolver())) {
     Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
 
-// By default, we exclude C++ standard symbols and protobuf symbols as rename
-// these symbols would change system/generated files which are unlikely to be
-// modified.
+// By default, we exclude symbols from system headers and protobuf symbols as
+// renaming these symbols would change system/generated files which are unlikely
+// to be good candidates for modification.
 bool isExcluded(const NamedDecl &RenameDecl) {
-  if (isProtoFile(RenameDecl.getLocation(),
-                  RenameDecl.getASTContext().getSourceManager()))
+  const auto &SM = RenameDecl.getASTContext().getSourceManager();
+  if (SM.isInSystemHeader(RenameDecl.getLocation()))
     return true;
+  if (isProtoFile(RenameDecl.getLocation(), SM))
+    return true;
+  // FIXME: Remove this std symbol list, as they should be covered by the
+  // above isInSystemHeader check.
   static const auto *StdSymbols = new llvm::DenseSet<llvm::StringRef>({
 #define SYMBOL(Name, NameSpace, Header) {#NameSpace #Name},
 #include "StdSymbolMap.inc"
 #undef SYMBOL
   });
   return StdSymbols->count(printQualifiedName(RenameDecl));
 }
 
case ReasonToReject::UnsupportedSymbol:
       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::SameName:
       return "new name is the same as the old name";
     }
     llvm_unreachable("unhandled reason kind");
   };
+  // abc
   return error("Cannot rename symbol: {0}", Message(Reason));
 }
 
 // Return all rename occurrences in the main file.
 std::vector<SourceLocation> findOccurrencesWithinFile(ParsedAST &AST,
                                                       const NamedDecl &ND) {
   trace::Span Tracer("FindOccurrencesWithinFile");
   assert(canonicalRenameDecl(&ND) == &ND &&

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

TEST(RenameTest, MainFileReferencesOnly) {
   auto RenameResult =
       rename({Code.point(), NewName, AST, testPath(TU.Filename)});
   ASSERT_TRUE(bool(RenameResult)) << RenameResult.takeError() << Code.point();
   ASSERT_EQ(1u, RenameResult->GlobalChanges.size());
   EXPECT_EQ(applyEdits(std::move(RenameResult->GlobalChanges)).front().second,
             expectedResult(Code, NewName));
 }
 
+TEST(RenameTest, NoRenameOnSymbolsFromSystemHeaders) {
+  // Filter out references not from main file.
+  llvm::StringRef Test =
+      R"cpp(
+        #include <system>
+        SystemSym^bol abc;
+        )cpp";
+
+  Annotations Code(Test);
+  auto TU = TestTU::withCode(Code.code());
+  TU.AdditionalFiles["system"] = R"cpp(
+    class SystemSymbol {};
+    )cpp";
+  TU.ExtraArgs = {"-isystem", testRoot()};
+  auto AST = TU.build();
+  llvm::StringRef NewName = "abcde";
+
+  auto Results = rename({Code.point(), NewName, AST, testPath(TU.Filename)});
+  EXPECT_FALSE(Results) << "expected rename returned an error: " << Code.code();
+  auto ActualMessage = llvm::toString(Results.takeError());
+  EXPECT_THAT(ActualMessage, testing::HasSubstr("not a supported kind"));
+}
+
 TEST(RenameTest, ProtobufSymbolIsExcluded) {
   Annotations Code("Prot^obuf buf;");
   auto TU = TestTU::withCode(Code.code());
   TU.HeaderCode =
       R"cpp(// Generated by the protocol buffer compiler.  DO NOT EDIT!
       class Protobuf {};
       )cpp";
   TU.HeaderFilename = "protobuf.pb.h";