Mediate access to ContainerDecl members (#7242)

Most of what this change does is straightforward: take all the places in the code that used to operate directly on `ContainerDecl::members` and related fields, and instead have them call into a smaller set of accessor methods defined on `ContainerDecl`.

The primary motivation for making this change is that in order to implement on-demand loading of members from serialized AST modules, we need a way to identify and intercept the "demand" for those members.

On-demand loading benefits from having all accesses to the members of a `ContainerDecl` be as narrow as possible.
If a part of the code only need a member at a specific index, it should say so.
If it only needs access to members with a specific name, or a given subclass of `Decl`, then it should say so.

A secondary motivation for this change is that there have recently been several changes that added complexity and special cases by introducing code that operated on (and *mutated*) the member list of a container decl in ways that the existing code had never done before.

Any code that mutates the member list of a `ContainerDecl` needs to be sure to not disrupt the invariants that the lookup acceleration structures currently rely on.
One of the recent changes added a declaration-to-index map to the set of acceleration structures (with different validation/invalidation behavior than the others...) while other recent changes would remove or insert declarations in ways that could change the indices of other declarations in the same container.
It is not clear if any of these pieces of code were aware of the others, and the invariants that might be expected or broken along the way.

This change bottlenecks the vast majority of accesses to the members of a `ContainerDecl` through the following operations:

* Getting a `List` of all of the direct member declarations of a container

* Get the number of direct member declarations, and accessing them by index.

* Looking up the list of direct member declarations with a given name.

* Adding a new direct member declaration to the end of the list.

Some other operations are layered on top of those (e.g., getting a list of all the direct member declarations of a given C++ class).
These layered operations are still centralized on the `ContainerDecl`, with the intention that we *can* change them to be non-layered implementations if we ever need to for performance (e.g., by building a lookup structure for finding member declarations by their type).

The exceptional cases of access/mutation on the direct members of a `ContainerDecl` have also been encapsulated, but rather than expose what would risk appearing like general-purpose accessors (e.g., `removeDecl(d)`, `setDecl(index)`, etc.), these operations have been explicitly named after the specific use case that they serve in the codebase today, to discourage others from using them for more kinds of operations we'd rather not support.
These operations have also been given parameter signatures that match their use cases, to make it so that even somebody determined to abuse them would have to invent suitable arguments out of thin air.

In the case of the declaration-to-index mapping, this change eliminates that acceleration structure, in favor or slightly more complicated (and possibly inefficient, yes) code at the use site.

Over time, it would be good to closely scrutinize each of the use cases that requires more complicated interaction with the members of a `ContainerDecl`, to see whether any of them can be reframed in terms of the more basic operations, or if there is some clean abstraction we can introduce to make operations that mutate the member list feel like... hacky.

1 files changed, 189 insertions, 49 deletions

diff --git a/source/slang/slang-ast-decl.cpp b/source/slang/slang-ast-decl.cpp
index 4c5d32f71..5152f0901 100644
--- a/source/slang/slang-ast-decl.cpp
+++ b/source/slang/slang-ast-decl.cpp
@@ -49,76 +49,218 @@ bool isInterfaceRequirement(Decl* decl)
     return false;
 }
 
-void ContainerDecl::buildMemberDictionary()
+//
+// ContainerDecl
+//
+
+List<Decl*> const& ContainerDecl::getDirectMemberDecls()
+{
+    return _directMemberDecls.decls;
+}
+
+Count ContainerDecl::getDirectMemberDeclCount()
+{
+    return _directMemberDecls.decls.getCount();
+}
+
+Decl* ContainerDecl::getDirectMemberDecl(Index index)
+{
+    return _directMemberDecls.decls[index];
+}
+
+Decl* ContainerDecl::getFirstDirectMemberDecl()
+{
+    if (getDirectMemberDeclCount() == 0)
+        return nullptr;
+    return getDirectMemberDecl(0);
+}
+
+DeclsOfNameList ContainerDecl::getDirectMemberDeclsOfName(Name* name)
+{
+    return DeclsOfNameList(findLastDirectMemberDeclOfName(name));
+}
+
+Decl* ContainerDecl::findLastDirectMemberDeclOfName(Name* name)
+{
+    _ensureLookupAcceleratorsAreValid();
+    if (auto found = _directMemberDecls.accelerators.mapNameToLastDeclOfThatName.tryGetValue(name))
+        return *found;
+    return nullptr;
+}
+
+Decl* ContainerDecl::getPrevDirectMemberDeclWithSameName(Decl* decl)
+{
+    SLANG_ASSERT(decl);
+    SLANG_ASSERT(decl->parentDecl == this);
+
+    _ensureLookupAcceleratorsAreValid();
+    return decl->_prevInContainerWithSameName;
+}
+
+void ContainerDecl::addDirectMemberDecl(Decl* decl)
+{
+    if (!decl)
+        return;
+
+    decl->parentDecl = this;
+    _directMemberDecls.decls.add(decl);
+}
+
+List<Decl*> const& ContainerDecl::getTransparentDirectMemberDecls()
+{
+    _ensureLookupAcceleratorsAreValid();
+    return _directMemberDecls.accelerators.filteredListOfTransparentDecls;
+}
+
+bool ContainerDecl::_areLookupAcceleratorsValid()
+{
+    return _directMemberDecls.accelerators.declCountWhenLastUpdated ==
+           _directMemberDecls.decls.getCount();
+}
+
+void ContainerDecl::_invalidateLookupAccelerators()
+{
+    _directMemberDecls.accelerators.declCountWhenLastUpdated = -1;
+}
+
+void ContainerDecl::_ensureLookupAcceleratorsAreValid()
 {
-    // Don't rebuild if already built
-    if (isMemberDictionaryValid())
+    if (_areLookupAcceleratorsValid())
         return;
 
-    // If it's < 0 it means that the dictionaries are entirely invalid
-    if (dictionaryLastCount < 0)
+    // If the `declCountWhenLastUpdated` is less than zero, it means that
+    // the accelerators are entirely invalidated, and must be rebuilt
+    // from scratch.
+    //
+    if (_directMemberDecls.accelerators.declCountWhenLastUpdated < 0)
     {
-        dictionaryLastCount = 0;
-        memberDictionary.clear();
-        transparentMembers.clear();
+        _directMemberDecls.accelerators.declCountWhenLastUpdated = 0;
+        _directMemberDecls.accelerators.mapNameToLastDeclOfThatName.clear();
+        _directMemberDecls.accelerators.filteredListOfTransparentDecls.clear();
     }
 
     // are we a generic?
     GenericDecl* genericDecl = as<GenericDecl>(this);
 
-    const Index membersCount = members.getCount();
+    Count memberCount = _directMemberDecls.decls.getCount();
+    Count memberCountWhenLastUpdated = _directMemberDecls.accelerators.declCountWhenLastUpdated;
 
-    SLANG_ASSERT(dictionaryLastCount >= 0 && dictionaryLastCount <= membersCount);
+    SLANG_ASSERT(memberCountWhenLastUpdated >= 0 && memberCountWhenLastUpdated <= memberCount);
 
-    for (Index i = dictionaryLastCount; i < membersCount; ++i)
+    for (Index i = memberCountWhenLastUpdated; i < memberCount; ++i)
     {
-        Decl* m = members[i];
-
-        auto name = m->getName();
-
-        // Add any transparent members to a separate list for lookup
-        if (m->hasModifier<TransparentModifier>())
+        Decl* memberDecl = _directMemberDecls.decls[i];
+
+        // Transparent member declarations will go into a separate list,
+        // so that they can be conveniently queried later for lookup
+        // operations.
+        //
+        // TODO: Rather than track these using a separate table, we
+        // could design a scheme where transparent members are put into
+        // the same lookup dictionary as everything else, just under
+        // a pseudo-name that identifies transparent members.
+        //
+        if (memberDecl->hasModifier<TransparentModifier>())
         {
-            TransparentMemberInfo info;
-            info.decl = m;
-            transparentMembers.add(info);
+            _directMemberDecls.accelerators.filteredListOfTransparentDecls.add(memberDecl);
         }
 
-        // Ignore members with no name
-        if (!name)
+        // Members that don't have a name don't go into the lookup dictionary.
+        //
+        auto memberName = memberDecl->getName();
+        if (!memberName)
             continue;
 
-        // Ignore the "inner" member of a generic declaration
-        if (genericDecl && m == genericDecl->inner)
+        // As a special case, we ignore the `inner` member of a
+        // `GenericDecl`, since it will always have the same name
+        // as the outer generic, and should not be found by lookup.
+        //
+        // TODO: We really ought to change up our entire encoding
+        // of generic declarations in the AST.
+        //
+        if (genericDecl && memberDecl == genericDecl->inner)
             continue;
 
-        m->nextInContainerWithSameName = nullptr;
+        // It is possible that we have encountered previous declarations
+        // that have the same name as `memberDecl`, and in that
+        // case we want to wire them up into a singly-linked list.
+        //
+        // This list makes it easy for a lookup operation to find, e.g.,
+        // all of the overloaded functions with a given name.
+        //
+        Decl* prevMemberWithSameName = nullptr;
+        _directMemberDecls.accelerators.mapNameToLastDeclOfThatName.tryGetValue(
+            memberName,
+            prevMemberWithSameName);
+        memberDecl->_prevInContainerWithSameName = prevMemberWithSameName;
+
+        // Whether or not there was a previous declaration with this
+        // name, the current `memberDecl` is the last member declaration
+        // with that name encountered so far, and it is what we will
+        // store in the lookup dictionary.
+        //
+        _directMemberDecls.accelerators.mapNameToLastDeclOfThatName[memberName] = memberDecl;
+    }
 
-        Decl* next = nullptr;
-        if (memberDictionary.tryGetValue(name, next))
-            m->nextInContainerWithSameName = next;
+    _directMemberDecls.accelerators.declCountWhenLastUpdated = memberCount;
+    SLANG_ASSERT(_areLookupAcceleratorsValid());
+}
 
-        memberDictionary[name] = m;
-    }
+void ContainerDecl::
+    _invalidateLookupAcceleratorsBecauseUnscopedEnumAttributeWillBeTurnedIntoTransparentModifier(
+        UnscopedEnumAttribute* unscopedEnumAttr,
+        TransparentModifier* transparentModifier)
+{
+    SLANG_ASSERT(unscopedEnumAttr);
+    SLANG_ASSERT(transparentModifier);
+
+    SLANG_UNUSED(unscopedEnumAttr);
+    SLANG_UNUSED(transparentModifier);
 
-    dictionaryLastCount = membersCount;
-    SLANG_ASSERT(isMemberDictionaryValid());
+    _invalidateLookupAccelerators();
 }
 
-Index ContainerDecl::getDeclIndex(Decl* decl)
+void ContainerDecl::
+    _removeDirectMemberConstructorDeclBecauseSynthesizedAnotherDefaultConstructorInstead(
+        ConstructorDecl* decl)
 {
-    if (Index* ptr = mapDeclMemberToIndex.tryGetValue(decl))
-    {
-        return *ptr;
-    }
-    Index res = members.findFirstIndex([&](Decl* d) { return d == decl; });
-    if (res >= Index(0))
-    {
-        mapDeclMemberToIndex[decl] = res;
-    }
-    return res;
+    SLANG_ASSERT(decl);
+
+    _invalidateLookupAccelerators();
+    _directMemberDecls.decls.remove(decl);
+}
+
+void ContainerDecl::
+    _replaceDirectMemberBitFieldVariableDeclAtIndexWithPropertyDeclThatWasSynthesizedForIt(
+        Index index,
+        VarDecl* oldDecl,
+        PropertyDecl* newDecl)
+{
+    SLANG_ASSERT(oldDecl);
+    SLANG_ASSERT(newDecl);
+    SLANG_ASSERT(index >= 0 && index < getDirectMemberDeclCount());
+    SLANG_ASSERT(getDirectMemberDecl(index) == oldDecl);
+
+    SLANG_UNUSED(oldDecl);
+    _invalidateLookupAccelerators();
+    _directMemberDecls.decls[index] = newDecl;
 }
 
+void ContainerDecl::_insertDirectMemberDeclAtIndexForBitfieldPropertyBackingMember(
+    Index index,
+    VarDecl* backingVarDecl)
+{
+    SLANG_ASSERT(backingVarDecl);
+    SLANG_ASSERT(index >= 0 && index <= getDirectMemberDeclCount());
+
+    _invalidateLookupAccelerators();
+    _directMemberDecls.decls.insert(index, backingVarDecl);
+}
+
+//
+//
+//
+
 bool isLocalVar(const Decl* decl)
 {
     const auto varDecl = as<VarDecl>(decl);
@@ -137,14 +279,12 @@ bool isLocalVar(const Decl* decl)
 
 ThisTypeDecl* InterfaceDecl::getThisTypeDecl()
 {
-    for (auto member : members)
+    auto thisTypeDecl = findFirstDirectMemberDeclOfType<ThisTypeDecl>();
+    if (!thisTypeDecl)
     {
-        if (auto thisTypeDeclCandidate = as<ThisTypeDecl>(member))
-        {
-            return thisTypeDeclCandidate;
-        }
+        SLANG_UNEXPECTED("InterfaceDecl does not have a ThisType decl.");
     }
-    SLANG_UNREACHABLE("InterfaceDecl does not have a ThisType decl.");
+    return thisTypeDecl;
 }
 
 InterfaceDecl* ThisTypeConstraintDecl::getInterfaceDecl()