[clang] Calculate PrimaryContext before adding the actual Decl in DeclContext::addDecl to prevent redundant lookup entries

[teemperor]

Summary

The tests Modules/cxx-templates.cpp are hitting Modules/template-default-args.cpp are currently failing with the
assert from D84827 to prevent duplicate entries in the lookup results.

The specific problem seems to be that we have duplicate entries in the lookup results when instantiating a templated class
with a (non-scoped) enum declared inside of it. The EnumConstantDecl inside the EnumDecl ends up being twice
in the lookup result of the surrounding class.

The unintentional addition to the lookup result happens with the following backtrace:

frame #1: clang`clang::DeclContext::makeDeclVisibleInContextImpl(this=, D=, Internal=false) at DeclBase.cpp:1940:21
frame #2: clang`clang::DeclContext::buildLookupImpl(this=, DCtx=, Internal=false) at DeclBase.cpp:1647:9
frame #3: clang`clang::DeclContext::buildLookupImpl(this=, DCtx=0x000000012180f368, Internal=false) at DeclBase.cpp:1654:9
frame #4: clang`clang::DeclContext::buildLookup(this=) at DeclBase.cpp:1621:5
frame #5: clang`clang::DeclContext::lookup(this=, Name="E" const at DeclBase.cpp:1713:43
frame #6: clang`clang::ASTReader::CompleteRedeclChain(this=, D=) at ASTReader.cpp:7177:13
frame #7: clang`clang::LazyGenerationalUpdatePtr<clang::Decl const*, clang::Decl*, &(clang::ExternalASTSource::CompleteRedeclChain(clang::Decl const*))>::get(this=, O=) at ExternalASTSource.h:443:9
frame #8: clang`clang::Redeclarable<clang::TagDecl>::DeclLink::getPrevious(this=, D=) const at Redeclarable.h:134:62
frame #9: clang`clang::Redeclarable<clang::TagDecl>::getNextRedeclaration(this=) const at Redeclarable.h:190:23
frame #10: clang`clang::Redeclarable<clang::TagDecl>::redecl_iterator::operator++(this=) at Redeclarable.h:271:34
frame #11: clang`clang::TagDecl::getDefinition(this=) const at Decl.cpp:4210:15
frame #12: clang`clang::DeclContext::getPrimaryContext(this=) at DeclBase.cpp:1266:31
frame #13: clang`clang::DeclContext::addDecl(this=, D=) at DeclBase.cpp:1577:27
frame #14: clang`clang::TemplateDeclInstantiator::InstantiateEnumDefinition(this=, Enum=, Pattern=) at SemaTemplateInstantiateDecl.cpp:1311:13

Essentially what happens here are the following steps which all come from the addDecl function:

void DeclContext::addDecl(Decl *D) {
  addHiddenDecl(D); //<- step 1

  if (auto *ND = dyn_cast<NamedDecl>(D))
    ND->getDeclContext()->getPrimaryContext()->makeDeclVisibleInContextWithFlags(ND, false, true);
  // call to `getPrimaryContext()` and `makeDeclVisibleInContextWithFlags`
}

1. We add the EnumConstantDecl as hidden to the EnumDecl (as preparation for making it visible) in addDecl.
2. We try to make the EnumConstantDecl visible now in the second part of addDecl.
3. For this we calculate the PrimaryContext of the EnumDecl via getPrimaryContext()
4. For the PrimaryContext we first try to find the Definition of the EnumDecl (as for every TagDecl).
5. To find the definition, we walk the redecl chain of the EnumDecl.
6. To walk the redecl chain, we do a lookup of the enum name in its containing DeclContext.
7. This will cause us to build the lookup for the EnumDecls' DeclContext which is the template class instantiation.
8. As the EnumDecl is a transparent DeclContext, building the lookup for its parent DeclContext will also build the lookup for our EnumDecl.
9. DeclContext::buildLookupImpl for the template class instantiation will find the EnumConstantDecl added in step 1 and make it visible.
10. Now we actually return from the getPrimaryContext() call from step 3 and call makeDeclVisibleInContextWithFlags on it to make our EnumConstantDecl visible.

As we already made it visible in Step 9, Step 10 just explicitly adds the EnumConstantDecl a second time to the lookup.

This patch moves the call to getPrimaryContext() up so that it is before we add the decl to the DeclContext. This way
when building the lookup we can never unintentionally add the decl we are about to add to any lookup.

[vsapsai]

I'm not familiar with this code, so my review can be pretty flawed. Current fix feels more like a workaround for getPrimaryContext() behavior (that it can make a decl visible). On the surface it looks like getPrimaryContext() shouldn't have drastic side-effects. Do you know if making EnumConstantDecl visible is critical for retrieving a valid primary context?

Another option is to make ASTReader safer. For example, finalize deserialization (and call CompleteRedeclChain) before even instantiating a template. Unfortunately, I don't know enough to give actionable recommendations and apologize for such a vague comment.

clang/include/clang/AST/DeclBase.h

public:
   static bool classof(const DeclContext *D) { return true; }
 
   void dumpDeclContext() const;
   void dumpLookups() const;
   void dumpLookups(llvm::raw_ostream &OS, bool DumpDecls = false,
                    bool Deserialize = false) const;
 
 private:
+  void addDeclImpl(Decl *D, bool Internal);
+
   /// Whether this declaration context has had externally visible
   /// storage added since the last lookup. In this case, \c LookupPtr's
   /// invariant may not hold and needs to be fixed before we perform
   /// another lookup.
   bool hasNeedToReconcileExternalVisibleStorage() const {
     return DeclContextBits.NeedToReconcileExternalVisibleStorage;
   }
 

clang/lib/AST/DeclBase.cpp

void DeclContext::addHiddenDecl(Decl *D) {
   // If this is a newly-created (not de-serialized) import declaration, wire
   // it in to the list of local import declarations.
   if (!D->isFromASTFile()) {
     if (auto *Import = dyn_cast<ImportDecl>(D))
       D->getASTContext().addedLocalImportDecl(Import);
   }
 }
 
-void DeclContext::addDecl(Decl *D) {
+void DeclContext::addDeclImpl(Decl *D, bool Internal) {
+  DeclContext *PrimaryContext = nullptr;
+  NamedDecl *ND = nullptr;
+  // Calculate the primary context before adding the decl. Calculating the
+  // primary context might end up unintentionally adding the new decl to the
+  // same lookup we will manually add it at the end of this function.
+  if ((ND = dyn_cast<NamedDecl>(D)))
+    PrimaryContext = ND->getDeclContext()->getPrimaryContext();
+
   addHiddenDecl(D);
 
-  if (auto *ND = dyn_cast<NamedDecl>(D))
-    ND->getDeclContext()->getPrimaryContext()->
-        makeDeclVisibleInContextWithFlags(ND, false, true);
+  if (ND)
+    PrimaryContext->makeDeclVisibleInContextWithFlags(ND, Internal, true);
 }
 
-void DeclContext::addDeclInternal(Decl *D) {
-  addHiddenDecl(D);
+void DeclContext::addDecl(Decl *D) { addDeclImpl(D, /*Internal*/ false); }
 
-  if (auto *ND = dyn_cast<NamedDecl>(D))
-    ND->getDeclContext()->getPrimaryContext()->
-        makeDeclVisibleInContextWithFlags(ND, true, true);
+void DeclContext::addDeclInternal(Decl *D) {
+  addDeclImpl(D, /*Internal*/ true);
 }
 
 /// buildLookup - Build the lookup data structure with all of the
 /// declarations in this DeclContext (and any other contexts linked
 /// to it or transparent contexts nested within it) and return it.
 ///
 /// Note that the produced map may miss out declarations from an
 /// external source. If it does, those entries will be marked with