[ASTImporter] Do not try to remove invisible Decls from DeclContext

[martong]

Summary

DeclContext is essentially a list of Decls and a lookup table (LookupPtr) but these are encapsulated.
E.g. LookupPtr is private. DeclContext::removeDecl has the responsibility to remove the given decl from the list and from the table too.
Template specializations cannot participate in normal (qualified) lookup.
Consequently no template specialization can be in the lookup table, but they will be in the list. When the lookup table is built or when a new Decl is added, then it is checked whether it is a template specialization and if so it is skipped from the lookup table.
Thus, whenever we want to remove a Decl from a DeclContext we must not reach the point to remove that from the lookup table (and that is what this patch do).

With respect to ASTImporter: At some point probably we will be reordering FriendDecls and possibly CXXMethodDecls in a RecordDecl at importDeclContext to be able to structurally compare two RecordDecls.
When that happens we most probably want to remove all Decls from a RecordDecl and then we would add them in the proper order.

[a.sidorin]

Hi Gabor,

I don't feel I'm a right person to review AST-related part so I'm adding other reviewers.
What I'm worrying about is that there is no test to check if our changes in removeDecl are correct. Maybe D44100 is a right patch for this but we should land it first or set dependencies properly.
Regarding ASTImporter, you can find my comments inline.

lib/AST/DeclBase.cpp

1410

specializations

unittests/AST/ASTImporterTest.cpp

1800

These tests seem to be for ImportDecl, not for ImportExpr.

1833

Check for namespaceDecl() looks too weak because import of NamespaceDecl succeeds even if import of its nested decls fails. Same below.

1857

For this change, we should create a separate patch.

[martong]

• Add test for removeDecl, remove unrelated tests

[martong]

Hi Aleksei,

Thanks for reviewing this.
I could synthesize a test which exercises only the DeclContext::removeDecl function. This test causes an assertion without the fix.
Removed the rest of the testcases, which are not strictly connected to this change.

unittests/AST/ASTImporterTest.cpp

1857

This test is disabled ATM, but I agree it would be better to bring this in when we fix the import of friends.

[martong]

• Remove unrelated CXX14 changes

[aaron.ballman]

Can you roll back the changes in MatchVerifier.h? Phab is telling me that the only changes are to whitespace.

lib/AST/DeclBase.cpp

1353

Rather than make a hanging forward declaration, can you just move the definition of this function here?

1411

The return should be on its own line per our usual conventions.

[martong]

Addressing review comments.

[aaron.ballman]

LGTM

[martong]

Closed by commit: rL332699

lib/AST/DeclBase.cpp

bool DeclContext::decls_empty() const {
   return !FirstDecl;
 }
 
 bool DeclContext::containsDecl(Decl *D) const {
   return (D->getLexicalDeclContext() == this &&
           (D->NextInContextAndBits.getPointer() || D == LastDecl));
 }
 
+/// shouldBeHidden - Determine whether a declaration which was declared
+/// within its semantic context should be invisible to qualified name lookup.
+static bool shouldBeHidden(NamedDecl *D) {
+  // Skip unnamed declarations.
+  if (!D->getDeclName())
+    return true;
+
+  // Skip entities that can't be found by name lookup into a particular
+  // context.
+  if ((D->getIdentifierNamespace() == 0 && !isa<UsingDirectiveDecl>(D)) ||
+      D->isTemplateParameter())
+    return true;
+
+  // Skip template specializations.
+  // FIXME: This feels like a hack. Should DeclarationName support
+  // template-ids, or is there a better way to keep specializations
+  // from being visible?
+  if (isa<ClassTemplateSpecializationDecl>(D))
+    return true;
+  if (auto *FD = dyn_cast<FunctionDecl>(D))
+    if (FD->isFunctionTemplateSpecialization())
+      return true;
+
+  return false;
+}
+
 void DeclContext::removeDecl(Decl *D) {
   assert(D->getLexicalDeclContext() == this &&
          "decl being removed from non-lexical context");
   assert((D->NextInContextAndBits.getPointer() || D == LastDecl) &&
          "decl is not in decls list");
 
   // Remove D from the decl chain.  This is O(n) but hopefully rare.
   if (D == FirstDecl) {
     if (D == LastDecl)
       FirstDecl = LastDecl = nullptr;
     else
       FirstDecl = D->NextInContextAndBits.getPointer();
   } else {
     for (Decl *I = FirstDecl; true; I = I->NextInContextAndBits.getPointer()) {
       assert(I && "decl not found in linked list");
       if (I->NextInContextAndBits.getPointer() == D) {
         I->NextInContextAndBits.setPointer(D->NextInContextAndBits.getPointer());
         if (D == LastDecl) LastDecl = I;
         break;
       }
     }
   }
-  
   // Mark that D is no longer in the decl chain.
   D->NextInContextAndBits.setPointer(nullptr);
 
   // Remove D from the lookup table if necessary.
   if (isa<NamedDecl>(D)) {
     auto *ND = cast<NamedDecl>(D);
 
+    // Do not try to remove the declaration if that is invisible to qualified
+    // lookup.  E.g. template specializations are skipped.
+    if (shouldBeHidden(ND))
+      return;
+
     // Remove only decls that have a name
-    if (!ND->getDeclName()) return;
+    if (!ND->getDeclName())
+      return;
 
     auto *DC = D->getDeclContext();
     do {
       StoredDeclsMap *Map = DC->getPrimaryContext()->LookupPtr;
       if (Map) {
         StoredDeclsMap::iterator Pos = Map->find(ND->getDeclName());
         assert(Pos != Map->end() && "no lookup entry for decl");
         if (Pos->second.getAsVector() || Pos->second.getAsDecl() == ND)

 void DeclContext::addDeclInternal(Decl *D) {
   addHiddenDecl(D);
 
   if (auto *ND = dyn_cast<NamedDecl>(D))
     ND->getDeclContext()->getPrimaryContext()->
         makeDeclVisibleInContextWithFlags(ND, true, true);
 }
 
-/// shouldBeHidden - Determine whether a declaration which was declared
-/// within its semantic context should be invisible to qualified name lookup.
-static bool shouldBeHidden(NamedDecl *D) {
-  // Skip unnamed declarations.
-  if (!D->getDeclName())
-    return true;
-
-  // Skip entities that can't be found by name lookup into a particular
-  // context.
-  if ((D->getIdentifierNamespace() == 0 && !isa<UsingDirectiveDecl>(D)) ||
-      D->isTemplateParameter())
-    return true;
-
-  // Skip template specializations.
-  // FIXME: This feels like a hack. Should DeclarationName support
-  // template-ids, or is there a better way to keep specializations
-  // from being visible?
-  if (isa<ClassTemplateSpecializationDecl>(D))
-    return true;
-  if (auto *FD = dyn_cast<FunctionDecl>(D))
-    if (FD->isFunctionTemplateSpecialization())
-      return true;
-
-  return false;
-}
-
 /// 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
 /// the 'hasExternalDecls' flag.
 StoredDeclsMap *DeclContext::buildLookup() {

unittests/AST/ASTImporterTest.cpp

testImport("struct S { template <typename T> void mem(); };"
              "  s.mem<U>();"
              "}"
              "void instantiate() { declToImport<int>(); }",
              Lang_CXX11, "", Lang_CXX11, Verifier,
              functionTemplateDecl(has(functionDecl(has(
                  compoundStmt(has(callExpr(has(unresolvedMemberExpr())))))))));
 }
 
+struct DeclContextTest : ASTImporterTestBase {};
+
+TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
+  Decl *TU = getTuDecl(
+      R"(
+      namespace NS {
+
+      template <typename T>
+      struct S {};
+      template struct S<int>;
+
+      inline namespace INS {
+        template <typename T>
+        struct S {};
+        template struct S<int>;
+      }
+
+      }
+      )", Lang_CXX11, "input0.cc");
+  auto *NS = FirstDeclMatcher<NamespaceDecl>().match(
+      TU, namespaceDecl());
+  auto *Spec = FirstDeclMatcher<ClassTemplateSpecializationDecl>().match(
+      TU, classTemplateSpecializationDecl());
+  ASSERT_TRUE(NS->containsDecl(Spec));
+
+  NS->removeDecl(Spec);
+  EXPECT_FALSE(NS->containsDecl(Spec));
+}
+
+INSTANTIATE_TEST_CASE_P(
+    ParameterizedTests, DeclContextTest,
+    ::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
+
 } // end namespace ast_matchers
 } // end namespace clang