[clangd] Fix a crash when renaming operator.

[hokein]

Summary

The renamelib uses a tricky way to calculate the end location by relying
on decl name, this is incorrect for the overloaded operator (the name is
"operator++" instead of "++"), which will cause out-of-file offset.

We also disable renaming operator symbol, this case is tricky, and
renamelib doesnt handle it properly..

[hokein]

Fix an accident change.

[ilya-biryukov]

LGTM to fix the crash. See the comment about more cases that could potentially break, though. Do we handle them gracefully now?

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

77

Should we more generally disable renaming all non-identifier names? Each of them seems to require special treatment.

A few examples that come to mind:

• conversion operators, e.g. operator int();
• user-defined literals, e.g. std::chrono::seconds operator ""s(int seconds);
• destructors;

[hokein]

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

77

I think so, that would be safer (this should be done in a separate patch), it seems that some cases are being handled by the rename library already.

• for constructor and destructor, the renamelib handle them well;
• for operator int()/user-defined literals, the renamelib doesn't find a corresponding decl, means that we will return "no symbol found error", though the message is a bit confusing;

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

 };
 
 // Check the symbol Decl is renameable (per the index) within the file.
 llvm::Optional<ReasonToReject> renamableWithinFile(const Decl &RenameDecl,
                                                    StringRef MainFile,
                                                    const SymbolIndex *Index) {
   if (llvm::isa<NamespaceDecl>(&RenameDecl))
     return ReasonToReject::UnsupportedSymbol;
+  if (const auto *FD = llvm::dyn_cast<FunctionDecl>(&RenameDecl)) {
+    if (FD->isOverloadedOperator())
+      return ReasonToReject::UnsupportedSymbol;
+  }
   auto &ASTCtx = RenameDecl.getASTContext();
   const auto &SM = ASTCtx.getSourceManager();
   bool MainFileIsHeader = ASTCtx.getLangOpts().IsHeaderFile;
   bool DeclaredInMainFile = isInsideMainFile(RenameDecl.getBeginLoc(), SM);
 
   // If the symbol is declared in the main file (which is not a header), we
   // rename it.
   if (DeclaredInMainFile && !MainFileIsHeader)

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

Case Cases[] = {
 
       {
           R"cpp(
          #define MACRO 1
          int s = MAC^RO;
        )cpp",
           "not a supported kind", HeaderFile},
 
+      {
+
+          R"cpp(
+        struct X { X operator++(int) {} };
+        void f(X x) {x+^+;})cpp",
+          "not a supported kind", HeaderFile},
+
       {R"cpp(// foo is declared outside the file.
         void fo^o() {}
-      )cpp", "used outside main file", !HeaderFile/*cc file*/},
+      )cpp",
+       "used outside main file", !HeaderFile/*cc file*/},
   };
   const char *CommonHeader = R"cpp(
     class Outside {};
     void foo();
   )cpp";
   TestTU OtherFile = TestTU::withCode("Outside s; auto ss = &foo;");
   OtherFile.HeaderCode = CommonHeader;
   OtherFile.Filename = "other.cc";

clang/include/clang/Tooling/Refactoring/RecursiveSymbolVisitor.h

private:
   const LangOptions &LangOpts;
 
   bool visit(const NamedDecl *ND, SourceLocation BeginLoc,
              SourceLocation EndLoc) {
     return static_cast<T *>(this)->visitSymbolOccurrence(
         ND, SourceRange(BeginLoc, EndLoc));
   }
   bool visit(const NamedDecl *ND, SourceLocation Loc) {
-    return visit(ND, Loc,
-                 Loc.getLocWithOffset(ND->getNameAsString().length() - 1));
+    return visit(ND, Loc, Lexer::getLocForEndOfToken(Loc, 0, SM, LangOpts));
   }
 };
 
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_REFACTOR_RECURSIVE_SYMBOL_VISITOR_H