[clang] Assert when trying to add the same Decl multiple times to StoredDeclsList

[teemperor]

Summary

StoredDeclsList is used to store lookup results within DeclContexts and is optimized to
store only a single Decl pointer (or nothing). It seems the idea is that each
StoredDeclsList instance should also only contain a list of unique Decl pointers as
this property is asserted on in StoredDeclsList::remove.

However, this check that every Decl pointer is unique is only made when removing
Decls from the list and not when adding new Decl pointers. As in Clang we rarely
remove Decls from a StoredDeclsList but usually only add subsequent ones, we
accumulated a bit of code that was able to add duplicate Decl pointers to the
lookup StoredDeclsLists (usually by making already visible Decls visible again).

Beside making this data structure less efficient, these redundant Decl pointers also
cause that Clang code that is removing Decls from DeclContexts will hit the
assert. The biggest problem here is the ASTImporter which has to reorder Decls
in some DeclContext instances by removing all Decls and adding them again in the
correct order (therefore hitting this assert on pretty frequently).

This patch just adds the same check when adding Decls to prevent that people
bring a StoredDeclsList into this invalid state and cause ASTImporter crashes
later on.

I'll fix the redundant adds in the linked parent revisions of this review.

[vsapsai]

Based on the description the added assertion seems to be sensible. And adding duplicate Decl pointers doesn't seem to be a goal. The change looks good to me, I still want to go through parent revisions to get more context.

clang/include/clang/AST/DeclContextInternals.h

201

You can use !llvm::is_contained(Vec, D) but personally I don't have an opinion on which one is more readable. Up to you which one you want to use.

clang/include/clang/AST/DeclContextInternals.h

void AddSubsequentDecl(NamedDecl *D) {
     // form.
     if (NamedDecl *OldD = getAsDecl()) {
       DeclsTy *VT = new DeclsTy();
       VT->push_back(OldD);
       Data = DeclsAndHasExternalTy(VT, false);
     }
 
     DeclsTy &Vec = *getAsVector();
+    assert(llvm::find(Vec, D) == Vec.end() &&
+           "Trying to add decl that already exists in list");
 
     // Using directives end up in a special entry which contains only
     // other using directives, so all this logic is wasted for them.
     // But avoiding the logic wastes time in the far-more-common case
     // that we're *not* adding a new using directive.
 
     // Tag declarations always go at the end of the list so that an
     // iterator which points at the first tag will start a span of