[AST] [Modules] Introduce Decl::getNonTransparentDeclContext to handle exported friends

[ChuanqiXu]

Summary

Closing https://github.com/llvm/llvm-project/issues/56826.

The root cause for pr56826 is: when we collect the template args for the friend, we need to judge if the friend lives in file context. However, if the friend lives in ExportDecl lexically, the judgement here is invalid.

The solution is easy. We should judge the non transparent context and the ExportDecl is transparent context. So the solution should be good.

A main concern may be the patch doesn't handle all the places of the same defect. I think it might not be bad since the patch itself should be innocent.

Another alternative may let the Decl::getContext() return non transparent context but I feel it changes too drastically.

[ChuanqiXu]

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

6152

I'm pretty sure we should use getNonTransparentDeclContext here. But I don't want to do a NFC change without being covered by a test. So I left a FIXME here. And we could change it after we find the test case. The decision may be a little painful but I think it is more stable.

[ChuanqiXu]

clang/include/clang/AST/DeclBase.h

2020

Export declaration has already been the transparent declaration for a long time. But we just forget to edit the comment.

[erichkeane]

I think this is fine. I vaguely remember doing something like this in the past, but I can't seem to figure out what it was...

clang/lib/AST/DeclBase.cpp

1037

so why are we calling this on our DeclContext here, which is the 'parent' of the current Decl? Shouldn't this be casting itself to DeclContext and calling this function on it, so we don't end up 'skipping' the first parent?

[erichkeane]

LGTM, thanks!

clang/lib/AST/DeclBase.cpp

1037

Ah, I see... getNonTransparentContext works differently from the other getDeclContext type functions in that it does NOT get the 1st parent, it will return the 'current' DeclContext if necessary. So casting it would be incorrect here. Strange that this works opposite of the rest, but looks like you're right :)

clang/include/clang/AST/DeclBase.h

DeclContext *getDeclContext() {
     if (isInSemaDC())
       return getSemanticDC();
     return getMultipleDC()->SemanticDC;
   }
   const DeclContext *getDeclContext() const {
     return const_cast<Decl*>(this)->getDeclContext();
   }
 
+  /// Return the non transparent context.
+  /// See the comment of `DeclContext::isTransparentContext()` for the
+  /// definition of transparent context.
+  DeclContext *getNonTransparentDeclContext();
+  const DeclContext *getNonTransparentDeclContext() const {
+    return const_cast<Decl *>(this)->getNonTransparentDeclContext();
+  }
+
   /// Find the innermost non-closure ancestor of this declaration,
   /// walking up through blocks, lambdas, etc.  If that ancestor is
   /// not a code context (!isFunctionOrMethod()), returns null.
   ///
   /// A declaration may be its own non-closure context.
   Decl *getNonClosureContext();
   const Decl *getNonClosureContext() const {
     return const_cast<Decl*>(this)->getNonClosureContext();
public:
   /// @code
   /// enum E {
   ///   Val1
   /// };
   /// @endcode
   /// Here, E is a transparent context, so its enumerator (Val1) will
   /// appear (semantically) that it is in the same context of E.
   /// Examples of transparent contexts include: enumerations (except for
-  /// C++0x scoped enums), and C++ linkage specifications.
+  /// C++0x scoped enums), C++ linkage specifications and export declaration.
   bool isTransparentContext() const;
 
   /// Determines whether this context or some of its ancestors is a
   /// linkage specification context that specifies C linkage.
   bool isExternCContext() const;
 
   /// Retrieve the nearest enclosing C linkage specification context.
   const LinkageSpecDecl *getExternCContext() const;

clang/lib/AST/DeclBase.cpp

const FunctionType *Decl::getFunctionType(bool BlocksToo) const {
   else if (Ty->isFunctionReferenceType())
     Ty = Ty->castAs<ReferenceType>()->getPointeeType();
   else if (BlocksToo && Ty->isBlockPointerType())
     Ty = Ty->castAs<BlockPointerType>()->getPointeeType();
 
   return Ty->getAs<FunctionType>();
 }
 
+DeclContext *Decl::getNonTransparentDeclContext() {
+  assert(getDeclContext());
+  return getDeclContext()->getNonTransparentContext();
+}
+
 /// Starting at a given context (a Decl or DeclContext), look for a
 /// code context that is not a closure (a lambda, block, etc.).
 template <class T> static Decl *getNonClosureContext(T *D) {
   if (getKind(D) == Decl::CXXMethod) {
     auto *MD = cast<CXXMethodDecl>(D);
     if (MD->getOverloadedOperator() == OO_Call &&
         MD->getParent()->isLambda())
       return getNonClosureContext(MD->getParent()->getParent());

clang/lib/Sema/SemaTemplateInstantiate.cpp

else if (const auto *Function = dyn_cast<FunctionDecl>(Ctx)) {
                "Outer template not instantiated?");
       }
 
       // If this is a friend declaration and it declares an entity at
       // namespace scope, take arguments from its lexical parent
       // instead of its semantic parent, unless of course the pattern we're
       // instantiating actually comes from the file's context!
       if (Function->getFriendObjectKind() &&
-          Function->getDeclContext()->isFileContext() &&
+          Function->getNonTransparentDeclContext()->isFileContext() &&
           (!Pattern || !Pattern->getLexicalDeclContext()->isFileContext())) {
         Ctx = Function->getLexicalDeclContext();
         RelativeToPrimary = false;
         continue;
       }
     } else if (const auto *Rec = dyn_cast<CXXRecordDecl>(Ctx)) {
       if (ClassTemplateDecl *ClassTemplate = Rec->getDescribedClassTemplate()) {
         assert(Result.getNumSubstitutedLevels() == 0 &&

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

while (!DC->isFileContext()) {
 
         // Check whether we're in the process of instantiating a member class.
         if (isInstantiationOf(Record, InstRecord))
           return InstRecord;
       }
 
       // Move to the outer template scope.
       if (FunctionDecl *FD = dyn_cast<FunctionDecl>(DC)) {
+        // FIXME: We should use `getNonTransparentDeclContext()` here instead
+        // of `getDeclContext()` once we find the invalid test case.
         if (FD->getFriendObjectKind() && FD->getDeclContext()->isFileContext()){
           DC = FD->getLexicalDeclContext();
           continue;
         }
         // An implicit deduction guide acts as if it's within the class template
         // specialization described by its name and first N template params.
         auto *Guide = dyn_cast<CXXDeductionGuideDecl>(FD);
         if (Guide && Guide->isImplicit()) {
           TemplateDecl *TD = Guide->getDeducedTemplate();

clang/test/Modules/pr56826.cppm

+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+
+export module m3;
+export template <class> struct X {
+  template <class Self> friend void f(Self &&self) {
+    (Self&)self; // expected-warning {{expression result unused}}
+  }
+};
+void g() { f(X<void>{}); } // expected-note {{in instantiation of function template specialization}}

clang/unittests/AST/DeclTest.cpp

TEST(Decl, ModuleAndInternalLinkage) {
 
   b = selectFirst<VarDecl>("b", match(varDecl(hasName("b")).bind("b"), CtxTS));
   g = selectFirst<FunctionDecl>(
       "g", match(functionDecl(hasName("g")).bind("g"), CtxTS));
 
   EXPECT_EQ(b->getLinkageInternal(), ModuleLinkage);
   EXPECT_EQ(g->getLinkageInternal(), ModuleLinkage);
 }
+
+TEST(Decl, GetNonTransparentDeclContext) {
+  llvm::Annotations Code(R"(
+    export module m3;
+    export template <class> struct X {
+      template <class Self> friend void f(Self &&self) {
+        (Self&)self;
+      }
+    };)");
+
+  auto AST =
+      tooling::buildASTFromCodeWithArgs(Code.code(), /*Args=*/{"-std=c++20"});
+  ASTContext &Ctx = AST->getASTContext();
+
+  auto *f = selectFirst<FunctionDecl>(
+      "f", match(functionDecl(hasName("f")).bind("f"), Ctx));
+
+  EXPECT_TRUE(f->getNonTransparentDeclContext()->isFileContext());
+}