[clangd] Don't rename the namespace.

[hokein]

Summary

Also fix a small bug -- the extra argument "-xc++" doesn't overwrite the
language if the argument is present after the file name in the compiler
command.

[sammccall]

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

167 ↗

(On Diff #206402)

can't rename symbols of this kind?

"supported" lacks context I think.

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

137 ↗

(On Diff #206402)

(why this change?)

[hokein]

Address review comments.

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

137 ↗

(On Diff #206402)

for the cases here, we want the main file treat as a header file, using -xc++ here would make clang treat it as a .cc file.

[sammccall]

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

137 ↗

(On Diff #206402)

It sounds like this is unrelated to the current change, and is designed to address tests that were passing by mistake (rename was failing because the file was not a header, not for the desired reason.

Can we split up the fix into another patch, and verify it by asserting on the error message?

[hokein]

Verify the error message in the test.

[hokein]

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

137 ↗

(On Diff #206402)

Done in this patch, adding the error message when doing the verification.

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

llvm::Optional<std::string> getOtherRefFile(const NamedDecl &Decl,
   });
   return OtherFile;
 }
 
 enum ReasonToReject {
   NoIndexProvided,
   NonIndexable,
   UsedOutsideFile,
+  UnsupportedSymbol,
 };
 
 // Check the symbol Decl is renameable (per the index) within the file.
 llvm::Optional<ReasonToReject> renamableWithinFile(const NamedDecl &Decl,
                                                    StringRef MainFile,
                                                    const SymbolIndex *Index) {
+  if (llvm::isa<NamespaceDecl>(&Decl))
+    return ReasonToReject::UnsupportedSymbol;
   auto &ASTCtx = Decl.getASTContext();
   const auto &SM = ASTCtx.getSourceManager();
   bool MainFileIsHeader = ASTCtx.getLangOpts().IsHeaderFile;
   bool DeclaredInMainFile =
       SM.isWrittenInMainFile(SM.getExpansionLoc(Decl.getLocation()));
 
   // If the symbol is declared in the main file (which is not a header), we
   // rename it.
if (auto Reject = renamableWithinFile(*RenameDecl, File, Index)) {
     auto Message = [](ReasonToReject Reason) {
       switch (Reason) {
       case NoIndexProvided:
         return "symbol may be used in other files (no index available)";
       case UsedOutsideFile:
         return "the symbol is used outside main file";
       case NonIndexable:
         return "symbol may be used in other files (not eligible for indexing)";
+      case UnsupportedSymbol:
+        return "symbol is not a supported kind (e.g. namespace)";
       }
       llvm_unreachable("unhandled reason kind");
     };
     return llvm::make_error<llvm::StringError>(
         llvm::formatv("Cannot rename symbol: {0}", Message(*Reject)),
         llvm::inconvertibleErrorCode());
   }
 

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

for (const Test &T : Tests) {
     ASSERT_TRUE(bool(ApplyResult)) << ApplyResult.takeError();
 
     EXPECT_EQ(T.After, *ApplyResult) << T.Before;
   }
 }
 
 TEST(RenameTest, Renameable) {
   // Test cases where the symbol is declared in header.
-  const char *Headers[] = {
-      R"cpp(// allow -- function-local
+  struct Case {
+    const char* HeaderCode;
+    const char* ErrorMessage; // null if no error
+  };
+  Case Cases[] = {
+      {R"cpp(// allow -- function-local
         void f(int [[Lo^cal]]) {
           [[Local]] = 2;
         }
       )cpp",
+       nullptr},
 
-      R"cpp(// allow -- symbol is indexable and has no refs in index.
+      {R"cpp(// allow -- symbol is indexable and has no refs in index.
         void [[On^lyInThisFile]]();
       )cpp",
+       nullptr},
 
-      R"cpp(// disallow -- symbol is indexable and has other refs in index.
+      {R"cpp(// disallow -- symbol is indexable and has other refs in index.
         void f() {
           Out^side s;
         }
       )cpp",
+       "used outside main file"},
 
-      R"cpp(// disallow -- symbol is not indexable.
+      {R"cpp(// disallow -- symbol is not indexable.
         namespace {
         class Unin^dexable {};
         }
       )cpp",
+       "not eligible for indexing"},
+
+      {R"cpp(// disallow -- namespace symbol isn't supported
+        namespace fo^o {}
+      )cpp",
+       "not a supported kind"},
   };
   const char *CommonHeader = "class Outside {};";
   TestTU OtherFile = TestTU::withCode("Outside s;");
   OtherFile.HeaderCode = CommonHeader;
   OtherFile.Filename = "other.cc";
   // The index has a "Outside" reference.
   auto OtherFileIndex = OtherFile.index();
 
-  for (const char *Header : Headers) {
-    Annotations T(Header);
+  for (const auto& Case : Cases) {
+    Annotations T(Case.HeaderCode);
     // We open the .h file as the main file.
     TestTU TU = TestTU::withCode(T.code());
     TU.Filename = "test.h";
     TU.HeaderCode = CommonHeader;
-    TU.ExtraArgs.push_back("-xc++");
+    // Parsing the .h file as C++ include.
+    TU.ExtraArgs.push_back("-xobjective-c++-header");
     auto AST = TU.build();
 
     auto Results = renameWithinFile(AST, testPath(TU.Filename), T.point(),
                                     "dummyNewName", OtherFileIndex.get());
     bool WantRename = true;
     if (T.ranges().empty())
       WantRename = false;
     if (!WantRename) {
+      assert(Case.ErrorMessage && "Error message must be set!");
       EXPECT_FALSE(Results) << "expected renameWithinFile returned an error: "
                             << T.code();
-      llvm::consumeError(Results.takeError());
+      auto ActualMessage = llvm::toString(Results.takeError());
+      EXPECT_THAT(ActualMessage, testing::HasSubstr(Case.ErrorMessage));
     } else {
       EXPECT_TRUE(bool(Results)) << "renameWithinFile returned an error: "
                                  << llvm::toString(Results.takeError());
       std::vector<testing::Matcher<tooling::Replacement>> Expected;
       for (const auto &R : T.ranges())
         Expected.push_back(RenameRange(TU.Code, R));
       EXPECT_THAT(*Results, UnorderedElementsAreArray(Expected));
     }
   }
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/trunk/clangd/unittests/TestTU.cpp

ParsedAST TestTU::build() const {
   // guard without messing up offsets). In this case, use an intermediate file.
   std::string ThunkContents = "#import \"" + FullHeaderName + "\"\n";
 
   llvm::StringMap<std::string> Files(AdditionalFiles);
   Files[FullFilename] = Code;
   Files[FullHeaderName] = HeaderCode;
   Files[ImportThunk] = ThunkContents;
 
-  std::vector<const char *> Cmd = {"clang", FullFilename.c_str()};
+  std::vector<const char *> Cmd = {"clang"};
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
   if (!HeaderCode.empty()) {
     Cmd.push_back("-include");
     Cmd.push_back(ImplicitHeaderGuard ? ImportThunk.c_str()
                                       : FullHeaderName.c_str());
   }
   Cmd.insert(Cmd.end(), ExtraArgs.begin(), ExtraArgs.end());
+  // Put the file name at the end -- this allows the extra arg (-xc++) to
+  // override the language setting.
+  Cmd.push_back(FullFilename.c_str());
   ParseInputs Inputs;
   Inputs.CompileCommand.Filename = FullFilename;
   Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
   Inputs.CompileCommand.Directory = testRoot();
   Inputs.Contents = Code;
   Inputs.FS = buildTestFS(Files);
   Inputs.Opts = ParseOptions();
   Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;