[clangd] Fix undefined behavior when generating error message at rename with an invalid name

[ArcsinX]

Summary

Message() lambda uses Reason.Details as an input parameter for llvm::formatv(), but Reason in Message() is a local object.
Return value of llvm::formatv() contains references to its input arguments, thus Message() returns an object which contains a reference to Details field of the local object Reason.
This patch fixes this behavior by passing Reason as a reference to Message() to ensure that return value of Message() contains references to alive object and also prevents copying of InvalidName structure at passing it to makeError().

Provided test passes on Linux+GCC with or without this patch, but fails on Windows+VisualStudio without this patch.

[sammccall]

Yikes, thank you!

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

case InvalidName::Conflict:
     return "Conflict";
   case InvalidName::BadIdentifier:
     return "BadIdentifier";
   }
   llvm_unreachable("unhandled InvalidName kind");
 }
 
 llvm::Error makeError(InvalidName Reason) {
-  auto Message = [](InvalidName Reason) {
+  auto Message = [](const InvalidName &Reason) {
     switch (Reason.K) {
     case InvalidName::Keywords:
       return llvm::formatv("the chosen name \"{0}\" is a keyword",
                            Reason.Details);
     case InvalidName::Conflict:
       return llvm::formatv("conflict with the symbol in {0}", Reason.Details);
     case InvalidName::BadIdentifier:
       return llvm::formatv("the chosen name \"{0}\" is not a valid identifier",
llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
   const auto &RenameDecl = **DeclsUnderCursor.begin();
   const auto *ID = RenameDecl.getIdentifier();
   if (!ID)
     return makeError(ReasonToReject::UnsupportedSymbol);
   if (ID->getName() == RInputs.NewName)
     return makeError(ReasonToReject::SameName);
   auto Invalid = checkName(RenameDecl, RInputs.NewName);
   if (Invalid)
-    return makeError(*Invalid);
+    return makeError(std::move(*Invalid));
 
   auto Reject = renameable(RenameDecl, RInputs.MainFilePath, RInputs.Index);
   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;

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

Case Cases[] = {
        nullptr, !HeaderFile, "Conflict"},
 
       {R"cpp(
         void func(int V^ar, int Conflict) {
         }
       )cpp",
        "conflict", !HeaderFile, "Conflict"},
 
+      {R"cpp(
+        int V^ar;
+      )cpp",
+       "\"const\" is a keyword", !HeaderFile, "const"},
+
       {R"cpp(// Trying to rename into the same name, SameName == SameName.
         void func() {
           int S^ameName;
         }
       )cpp",
        "new name is the same", !HeaderFile, "SameName"},
       {R"cpp(// Ensure it doesn't associate base specifier with base name.
         struct A {};