Fix assertion when generating diagnostic for inline namespaces

[aaron.ballman]

Summary

When calculating the name to display for inline namespaces, we have custom logic to try to hide redundant inline namespaces from the diagnostic. Calculating these redundancies requires performing a lookup in the parent declaration context, but that lookup should not try to look through transparent declaration contexts, like linkage specifications. Instead, loop up the declaration context chain until we find a non-transparent context and use that instead.

This fixes PR49954.

[erichkeane]

This whole function seems a little suspect, but I don't have a good example of a place it would break. Is there no cases where a lookup could result in the same COUNT but different declaration set? I guess it is more the question of whether a transparent context can 'lose' a name lookup (perhaps a case of conflicting names?), then have it added by the local namespace.

clang/include/clang/AST/Decl.h

620

This loop seems useful enough to be its own function in DeclContext? I think I remember seeing us do this for a different patch, right?

[aaron.ballman]

In D108403#2955651, @erichkeane wrote:

This whole function seems a little suspect, but I don't have a good example of a place it would break. Is there no cases where a lookup could result in the same COUNT but different declaration set? I guess it is more the question of whether a transparent context can 'lose' a name lookup (perhaps a case of conflicting names?), then have it added by the local namespace.

I don't believe transparent contexts can lose a name lookup -- they're transparent, so the declarations aren't owned by that context, they're owned by the first non-transparent context they run into (as I understand it, anyway). However, the important bit on this patch is: lookup() asserts that you're not trying to call it on a transparent context, so that tells me that we should walk until we find a non-transparent context rather than the current approach of assuming the parent is not transparent.

clang/include/clang/AST/Decl.h

620

This pattern does appear in the code base a few places; I could add a new helper method.

[aaron.ballman]

Updating based on review comments.

[erichkeane]

In D108403#2957084, @aaron.ballman wrote:

In D108403#2955651, @erichkeane wrote:

This whole function seems a little suspect, but I don't have a good example of a place it would break. Is there no cases where a lookup could result in the same COUNT but different declaration set? I guess it is more the question of whether a transparent context can 'lose' a name lookup (perhaps a case of conflicting names?), then have it added by the local namespace.

I don't believe transparent contexts can lose a name lookup -- they're transparent, so the declarations aren't owned by that context, they're owned by the first non-transparent context they run into (as I understand it, anyway). However, the important bit on this patch is: lookup() asserts that you're not trying to call it on a transparent context, so that tells me that we should walk until we find a non-transparent context rather than the current approach of assuming the parent is not transparent.

Right... you're probably right here, the mechanism with 'lookup' just feels 1/2 too clever. Anyway, I think I believe that is a battle for another day. See the clang-format warning above, otherwise LGTM.

[aaron.ballman]

Thanks for the reviews! I've committed (with the formatting fix) in 48f73ee666a264d23716ff6bb671cad836b65ccf.

[cor3ntin]

clang/lib/AST/DeclBase.cpp

1222

Can getParent() be null? If it cam, then this function can return null which you don't check when you call that function. Or it can't and your test is not doing anything.

[erichkeane]

clang/lib/AST/DeclBase.cpp

1222

I don't think it can ever be null, a DC of transparent-context type is always going to have at least a TranslationUnit as a parent.

I might think an assertion here to replace the DC && part might be worth it.

[aaron.ballman]

clang/lib/AST/DeclBase.cpp

1222

Thanks for the suggestions! I switched to an assert in 65bcdeaa15b729dae40190c6a990cd67c12e9f10.

clang/include/clang/AST/Decl.h

void setInline(bool Inline) {
     AnonOrFirstNamespaceAndInline.setInt(Inline);
   }
 
   /// Returns true if the inline qualifier for \c Name is redundant.
   bool isRedundantInlineQualifierFor(DeclarationName Name) const {
     if (!isInline())
       return false;
     auto X = lookup(Name);
-    auto Y = getParent()->lookup(Name);
+    // We should not perform a lookup within a transparent context, so find a
+    // non-transparent parent context.
+    auto Y = getParent()->getNonTransparentContext()->lookup(Name);
     return std::distance(X.begin(), X.end()) ==
       std::distance(Y.begin(), Y.end());
   }
 
   /// Get the original (first) namespace declaration.
   NamespaceDecl *getOriginalNamespace();
 
   /// Get the original (first) namespace declaration.
   const NamespaceDecl *getOriginalNamespace() const;

clang/include/clang/AST/DeclBase.h

public:
   /// Find the nearest non-closure ancestor of this context,
   /// i.e. the innermost semantic parent of this context which is not
   /// a closure.  A context may be its own non-closure ancestor.
   Decl *getNonClosureAncestor();
   const Decl *getNonClosureAncestor() const {
     return const_cast<DeclContext*>(this)->getNonClosureAncestor();
   }
 
+  // Retrieve the nearest context that is not a transparent context.
+  DeclContext *getNonTransparentContext();
+  const DeclContext* getNonTransparentContext() const {
+    return const_cast<DeclContext *>(this)->getNonTransparentContext();
+  }
+
   /// getPrimaryContext - There may be many different
   /// declarations of the same entity (including forward declarations
   /// of classes, multiple definitions of namespaces, etc.), each with
   /// a different set of declarations. This routine returns the
   /// "primary" DeclContext structure, which will contain the
   /// information needed to perform name lookup into this context.
   DeclContext *getPrimaryContext();
   const DeclContext *getPrimaryContext() const {

clang/lib/AST/DeclBase.cpp

if (getPrimaryContext() != this)
     return getPrimaryContext()->Encloses(DC);
 
   for (; DC; DC = DC->getParent())
     if (DC->getPrimaryContext() == this)
       return true;
   return false;
 }
 
+DeclContext *DeclContext::getNonTransparentContext() {
+  DeclContext *DC = this;
+  while (DC && DC->isTransparentContext())
+    DC = DC->getParent();
+  return DC;
+}
+
 DeclContext *DeclContext::getPrimaryContext() {
   switch (getDeclKind()) {
   case Decl::ExternCContext:
   case Decl::LinkageSpec:
   case Decl::Export:
   case Decl::Block:
   case Decl::Captured:
   case Decl::OMPDeclareReduction:

clang/test/Misc/diag-inline-namespace.cpp

namespace N {
 
   template<typename> struct T; // expected-note 5{{here}}
   T<struct A::B::C::f> t1; // expected-error {{implicit instantiation of undefined template 'N::T<N::f>'}}
   T<struct A::B::C::g> t2; // expected-error {{implicit instantiation of undefined template 'N::T<N::B::C::g>'}}
   T<struct A::B::C::h> t3; // expected-error {{implicit instantiation of undefined template 'N::T<N::B::h>'}}
   T<struct A::B::C::i> t4; // expected-error {{implicit instantiation of undefined template 'N::T<N::A::i>'}}
   T<struct A::B::C::j> t5; // expected-error {{implicit instantiation of undefined template 'N::T<N::B::C::j>'}}
 }
+
+namespace dont_crash {
+// A malformed lookup involving inline namespaces in a linkage specification
+// would previous cause an assertion due to the way diagnostics are emitted.
+extern "C++" inline namespace {
+namespace a {
+  a : b // expected-error {{unexpected ':' in nested name specifier; did you mean '::'?}} \
+        // expected-error {{no type named 'b' in namespace 'dont_crash::a'}}
+} // expected-error {{expected unqualified-id}}
+} // inline namespace
+} // dont_crash