Do not do raw name replacement when FromDecl is a class forward-declaration.

[ioeric]

Summary

If the FromDecl is a class forward declaration, the reference is
still considered as referring to the original definition given the nature
of forward-declarations, so we can't do a raw name replacement in this case.

[bkramer]

lib/Tooling/Core/Lookup.cpp

131 ↗

(On Diff #79306)

the "given the nature" part doesn't add any useful information. Either drop it or explain in more detail what's the problem with forward decls.

[ioeric]

• Address comments.

cfe/trunk/lib/Tooling/Core/Lookup.cpp

 //===--- Lookup.cpp - Framework for clang refactoring tools ---------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines helper methods for clang tools performing name lookup.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/Core/Lookup.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
 using namespace clang;
 using namespace clang::tooling;
 
 // Gets all namespaces that \p Context is in as a vector (ignoring anonymous
 // namespaces). The inner namespaces come before outer namespaces in the vector.
 // For example, if the context is in the following namespace:
 //    `namespace a { namespace b { namespace c ( ... ) } }`,
 // the vector will be `{c, b, a}`.

 std::string tooling::replaceNestedName(const NestedNameSpecifier *Use,
                                        const DeclContext *UseContext,
                                        const NamedDecl *FromDecl,
                                        StringRef ReplacementString) {
   assert(ReplacementString.startswith("::") &&
          "Expected fully-qualified name!");
 
   // We can do a raw name replacement when we are not inside the namespace for
-  // the original function and it is not in the global namespace.  The
+  // the original class/function and it is not in the global namespace.  The
   // assumption is that outside the original namespace we must have a using
   // statement that makes this work out and that other parts of this refactor
-  // will automatically fix using statements to point to the new function
+  // will automatically fix using statements to point to the new class/function.
+  // However, if the `FromDecl` is a class forward declaration, the reference is
+  // still considered as referring to the original definition, so we can't do a
+  // raw name replacement in this case.
   const bool class_name_only = !Use;
   const bool in_global_namespace =
       isa<TranslationUnitDecl>(FromDecl->getDeclContext());
-  if (class_name_only && !in_global_namespace &&
+  const bool is_class_forward_decl =
+      isa<CXXRecordDecl>(FromDecl) &&
+      !cast<CXXRecordDecl>(FromDecl)->isCompleteDefinition();
+  if (class_name_only && !in_global_namespace && !is_class_forward_decl &&
       !usingFromDifferentCanonicalNamespace(FromDecl->getDeclContext(),
                                             UseContext)) {
     auto Pos = ReplacementString.rfind("::");
     return Pos != StringRef::npos ? ReplacementString.substr(Pos + 2)
                                   : ReplacementString;
   }
   // We did not match this because of a using statement, so we will need to
   // figure out how good a namespace match we have with our destination type.
   // We work backwards (from most specific possible namespace to least
   // specific).
   return getBestNamespaceSubstr(UseContext, ReplacementString,
                                 isFullyQualified(Use));
 }

cfe/trunk/unittests/Tooling/LookupTest.cpp

 //===- unittest/Tooling/LookupTest.cpp ------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "TestVisitor.h"
 #include "clang/Tooling/Core/Lookup.h"
 using namespace clang;
 
 namespace {
 struct GetDeclsVisitor : TestVisitor<GetDeclsVisitor> {
-  std::function<void(CallExpr *)> OnCall;
+  std::function<void(CallExpr *)> OnCall = [&](CallExpr *Expr) {};
+  std::function<void(RecordTypeLoc)> OnRecordTypeLoc = [&](RecordTypeLoc Type) {
+  };
   SmallVector<Decl *, 4> DeclStack;
 
   bool VisitCallExpr(CallExpr *Expr) {
     OnCall(Expr);
     return true;
   }
 
+  bool VisitRecordTypeLoc(RecordTypeLoc Loc) {
+    OnRecordTypeLoc(Loc);
+    return true;
+  }
+
   bool TraverseDecl(Decl *D) {
     DeclStack.push_back(D);
     bool Ret = TestVisitor::TraverseDecl(D);
     DeclStack.pop_back();
     return Ret;
   }
 };
 
-TEST(LookupTest, replaceNestedName) {
+TEST(LookupTest, replaceNestedFunctionName) {
   GetDeclsVisitor Visitor;
 
   auto replaceCallExpr = [&](const CallExpr *Expr,
                              StringRef ReplacementString) {
     const auto *Callee = cast<DeclRefExpr>(Expr->getCallee()->IgnoreImplicit());
     const ValueDecl *FD = Callee->getDecl();
     return tooling::replaceNestedName(
         Callee->getQualifier(), Visitor.DeclStack.back()->getDeclContext(), FD,
Visitor.OnCall = [&](CallExpr *Expr) {
     EXPECT_EQ("x::bar", replaceCallExpr(Expr, "::a::x::bar"));
   };
   Visitor.runOver("namespace a { namespace b { void foo(); } }\n"
                   "namespace a { namespace b { namespace c {"
                   "void f() { foo(); }"
                   "} } }\n");
 }
 
+TEST(LookupTest, replaceNestedClassName) {
+  GetDeclsVisitor Visitor;
+
+  auto replaceRecordTypeLoc = [&](RecordTypeLoc Loc,
+                                  StringRef ReplacementString) {
+    const auto *FD = cast<CXXRecordDecl>(Loc.getDecl());
+    return tooling::replaceNestedName(
+        nullptr, Visitor.DeclStack.back()->getDeclContext(), FD,
+        ReplacementString);
+  };
+
+  Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
+    // Filter Types by name since there are other `RecordTypeLoc` in the test
+    // file.
+    if (Type.getDecl()->getQualifiedNameAsString() == "a::b::Foo")
+      EXPECT_EQ("x::Bar", replaceRecordTypeLoc(Type, "::a::x::Bar"));
+  };
+  Visitor.runOver("namespace a { namespace b {\n"
+                  "class Foo;\n"
+                  "namespace c { Foo f();; }\n"
+                  "} }\n");
+
+  Visitor.OnRecordTypeLoc = [&](RecordTypeLoc Type) {
+    // Filter Types by name since there are other `RecordTypeLoc` in the test
+    // file.
+    // `a::b::Foo` in using shadow decl is not `TypeLoc`.
+    if (Type.getDecl()->getQualifiedNameAsString() == "a::b::Foo")
+      EXPECT_EQ("Bar", replaceRecordTypeLoc(Type, "::a::x::Bar"));
+  };
+  Visitor.runOver("namespace a { namespace b { class Foo {}; } }\n"
+                  "namespace c { using a::b::Foo; Foo f();; }\n");
+}
+
 } // end anonymous namespace