[AST] lookup in parent DeclContext for transparent DeclContext

[ChuanqiXu]

Summary

The compiler would crash if we lookup for name in transparent decl context. The tests in this revision are two examples. godbolt example is here: https://llvm.godbolt.org/z/W497zGW5T (I didn't attach module examples in godbolt since I don't know how to do).

I think this should make sense since the member declared in transparent DeclContext are semantically defined in the enclosing (non-transparent) DeclContext, this is the definition for transparent DeclContext.

[erichkeane]

I had to do something similar for this at one point: https://github.com/llvm/llvm-project/commit/90010c2e1d60c6a9a4a0b30a113d4dae2b7214eb

I seem to remember hitting this assert, and from my end, I think I decided even calling 'lookup' with the linkage spec to be a mistake (BTW, you might consider updating that 'Encloses' for 'export' as well!).

Is there any mechanism in the parent call of 'lookup' here to make it get the right thing?

[ChuanqiXu]

In D116792#3227379, @erichkeane wrote:

I had to do something similar for this at one point: https://github.com/llvm/llvm-project/commit/90010c2e1d60c6a9a4a0b30a113d4dae2b7214eb

I seem to remember hitting this assert, and from my end, I think I decided even calling 'lookup' with the linkage spec to be a mistake (BTW, you might consider updating that 'Encloses' for 'export' as well!).

Yeah, it is another bug for 'export'. I've tried to address it in https://reviews.llvm.org/D116911 with the same style.

Is there any mechanism in the parent call of 'lookup' here to make it get the right thing?

And 'lookup' is called in various places. For example, from the stack trace of the crash, we could find that the parent of call is DeclareImplicitDeductionGuides. And I tried to handle it in DeclareImplicitDeductionGuides, then the compiler would crash again at LookupDirect in SemaLookup.cpp. I feel it is not good to add checks for places to call lookup. I agree that it is odd to lookup in a transparent DeclContext. But I feel it is not bad to lookup in the enclosing context from the definition of transparent DeclContext. Any thoughts?

[erichkeane]

In D116792#3230478, @ChuanqiXu wrote:

In D116792#3227379, @erichkeane wrote:

I had to do something similar for this at one point: https://github.com/llvm/llvm-project/commit/90010c2e1d60c6a9a4a0b30a113d4dae2b7214eb

I seem to remember hitting this assert, and from my end, I think I decided even calling 'lookup' with the linkage spec to be a mistake (BTW, you might consider updating that 'Encloses' for 'export' as well!).

Yeah, it is another bug for 'export'. I've tried to address it in https://reviews.llvm.org/D116911 with the same style.

Is there any mechanism in the parent call of 'lookup' here to make it get the right thing?

And 'lookup' is called in various places. For example, from the stack trace of the crash, we could find that the parent of call is DeclareImplicitDeductionGuides. And I tried to handle it in DeclareImplicitDeductionGuides, then the compiler would crash again at LookupDirect in SemaLookup.cpp. I feel it is not good to add checks for places to call lookup. I agree that it is odd to lookup in a transparent DeclContext. But I feel it is not bad to lookup in the enclosing context from the definition of transparent DeclContext. Any thoughts?

Hmm... I didn't realize this was the root 'lookup' function, and thank you for the above analysis. It seems to make more sense to me as well to have this be tolerant of this lookup setup. So LGTM.

clang/lib/AST/DeclBase.cpp

for (auto *D : DCtx->noload_decls()) {
     if (auto *InnerCtx = dyn_cast<DeclContext>(D))
       if (InnerCtx->isTransparentContext() || InnerCtx->isInlineNamespace())
         buildLookupImpl(InnerCtx, Internal);
   }
 }
 
 DeclContext::lookup_result
 DeclContext::lookup(DeclarationName Name) const {
-  assert(getDeclKind() != Decl::LinkageSpec &&
-         getDeclKind() != Decl::Export &&
-         "should not perform lookups into transparent contexts");
+  // For transparent DeclContext, we should lookup in their enclosing context.
+  if (getDeclKind() == Decl::LinkageSpec || getDeclKind() == Decl::Export)
+    return getParent()->lookup(Name);
 
   const DeclContext *PrimaryContext = getPrimaryContext();
   if (PrimaryContext != this)
     return PrimaryContext->lookup(Name);
 
   // If we have an external source, ensure that any later redeclarations of this
   // context have been loaded, since they may add names to the result of this
   // lookup (or add external visible storage).

clang/test/Modules/Inputs/template_name_lookup/foo.cppm

+export module foo;
+export template <typename T>
+class X {};

clang/test/Modules/template_name_lookup.cpp

+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: %clang_cc1 -std=c++20 %S/Inputs/template_name_lookup/foo.cppm -emit-module-interface -o %t/foo.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %s -fsyntax-only -verify
+
+import foo;
+void use() {
+  X x; // expected-error {{no viable constructor or deduction guide for deduction of template arguments of 'X'}}
+       // expected-note@Inputs/template_name_lookup/foo.cppm:3 {{candidate template ignored: couldn't infer template argument 'T'}}
+       // expected-note@Inputs/template_name_lookup/foo.cppm:3 {{candidate function template not viable: requires 1 argument, but 0 were provided}}
+}

clang/test/SemaCXX/lookup-template-name-extern-CXX.cpp

+// Tests that the lookup in transparent declaration context
+// (linkage specifiaction context) wouldn't cause compiler crash.
+// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify
+extern "C++" {
+template <class T>
+class X {}; // expected-note {{candidate template ignored: couldn't infer template argument 'T'}}
+            // expected-note@-1 {{candidate function template not viable: requires 1 argument, but 0 were provided}}
+}
+
+void foo() {
+  X x; // expected-error {{no viable constructor or deduction guide for deduction of template arguments of 'X'}}
+}