Make fixes for the attribute-error test case. (#1215)

There are two main fixes here:

The first is to remove a memory leak in the reflection test tool, in the case where Slang compilation fails. There is no real reason to be using the reflection test tool for tests that produce diagnostics (we have the slangc tool for that), but it makes sense to go ahead and fix the leak rather than work around it. This was one of those leaks that could have been avoided with smart pointers and a COM-lite API.

The second issue was that the logic for constructing an `AttributeDecl` based on a user-defined `struct` was not correctly setting the parent of the attribute decl. The code in question was a little hard to follow and had a few steps that didn't seem strictly necessary given its goals, so I went ahead and scrubbed+commented it to just do what made sense to me (and the tests still pass...).

I'm not entirely happy with the design and implementation approach for user-defined attributes, so we might want to take another stab at it sooner or later. This change is not meant to address any design issues, and is just about fixing bugs in what is already there.

1 files changed, 102 insertions, 61 deletions

diff --git a/source/slang/slang-check-modifier.cpp b/source/slang/slang-check-modifier.cpp
index 169b4696e..473b5e5da 100644
--- a/source/slang/slang-check-modifier.cpp
+++ b/source/slang/slang-check-modifier.cpp
@@ -80,84 +80,125 @@ namespace Slang
 
     AttributeDecl* SemanticsVisitor::lookUpAttributeDecl(Name* attributeName, Scope* scope)
     {
-        // Look up the name and see what we find.
-        //
-        // TODO: This needs to have some special filtering or naming
-        // rules to keep us from seeing shadowing variable declarations.
-        auto lookupResult = lookUp(getSession(), this, attributeName, scope, LookupMask::Attribute);
-
-        // If the result was overloaded,
-        // then we aren't going to be able to extract a single decl.
-        if(lookupResult.isOverloaded())
-            return nullptr;
+        auto session = getSession();
 
-        if (lookupResult.isValid())
+        // We start by looking for an existing attribute matching
+        // the name `attributeName`.
+        //
         {
-            auto decl = lookupResult.item.declRef.getDecl();
-            if (auto attributeDecl = as<AttributeDecl>(decl))
-            {
-                return attributeDecl;
-            }
-            else
-            {
+            // Look up the name and see what attributes we find.
+            //
+            auto lookupResult = lookUp(session, this, attributeName, scope, LookupMask::Attribute);
+
+            // If the result was overloaded, then that means there
+            // are multiple attributes matching the name, and we
+            // aren't going to be able to narrow it down.
+            //
+            if(lookupResult.isOverloaded())
                 return nullptr;
-            }
-        }
 
-        // If we couldn't find a system attribute, try looking up as a user defined attribute
-        // A user defined attribute class is defined as a struct type with a "UserDefinedAttributeAttribute" modifier
-        lookupResult = lookUp(getSession(), this, getSession()->getNameObj(attributeName->text + "Attribute"), scope, LookupMask::type);
-        if (lookupResult.isOverloaded())
-        {
-            // see if we have already created an AttributeDecl for this attribute struct
-            for (auto alt : lookupResult.items)
+            // If there is a single valid result, and it names
+            // an existing attribute declaration, then we can
+            // use it as the result.
+            //
+            if (lookupResult.isValid())
             {
-                if (auto adecl = alt.declRef.as<AttributeDecl>())
-                    return adecl.getDecl();
+                auto decl = lookupResult.item.declRef.getDecl();
+                if (auto attributeDecl = as<AttributeDecl>(decl))
+                {
+                    return attributeDecl;
+                }
             }
         }
-        // If we still cannot find any thing, quit
+
+        // If there wasn't already an attribute matching the
+        // given name, then we will look for a `struct` type
+        // matching the name scheme for user-defined attributes.
+        //
+        // If the attribute was `[Something(...)]` then we will
+        // look for a `struct` named `SomethingAttribute`.
+        //
+        LookupResult lookupResult = lookUp(session, this, session->getNameObj(attributeName->text + "Attribute"), scope, LookupMask::type);
+        //
+        // If we didn't find a matching type name, then we give up.
+        //
         if (!lookupResult.isValid() || lookupResult.isOverloaded())
             return nullptr;
-        // Now construct an AttributeDecl for this user defined attribute class for future lookup
-        auto userDefAttribAttrib = lookupResult.item.declRef.decl->FindModifier<AttributeUsageAttribute>();
-        if (!userDefAttribAttrib)
+
+
+        // We only allow a `struct` type to be used as an attribute
+        // if the type itself has an `[AttributeUsage(...)]` attribute
+        // attached to it.
+        //
+        auto structDecl = lookupResult.item.declRef.as<StructDecl>().getDecl();
+        if(!structDecl)
             return nullptr;
-        // create an AttributeDecl for the user defined attribute
-        auto structAttribDef = lookupResult.item.declRef.as<StructDecl>().getDecl();
-        RefPtr<AttributeDecl> attribDecl = new AttributeDecl();
-        attribDecl->nameAndLoc = structAttribDef->nameAndLoc;
-        attribDecl->loc = structAttribDef->loc;
-        attribDecl->nextInContainerWithSameName = structAttribDef->nextInContainerWithSameName;
-        // create a __attributeTarget modifier for the attribute class definition
+        auto attrUsageAttr = structDecl->FindModifier<AttributeUsageAttribute>();
+        if (!attrUsageAttr)
+            return nullptr;
+
+        // We will now synthesize a new `AttributeDecl` to mirror
+        // what was declared on the `struct` type.
+        //
+        RefPtr<AttributeDecl> attrDecl = new AttributeDecl();
+        attrDecl->nameAndLoc.name = attributeName;
+        attrDecl->nameAndLoc.loc = structDecl->nameAndLoc.loc;
+        attrDecl->loc = structDecl->loc;
+
         RefPtr<AttributeTargetModifier> targetModifier = new AttributeTargetModifier();
-        targetModifier->syntaxClass = userDefAttribAttrib->targetSyntaxClass;
-        targetModifier->loc = structAttribDef->loc;
-        targetModifier->next = attribDecl->modifiers.first;
-        attribDecl->modifiers.first = targetModifier;
-        structAttribDef->nextInContainerWithSameName = attribDecl.Ptr();
-        // we should create UserDefinedAttribute nodes for all user defined attribute instances
-        attribDecl->syntaxClass = getSession()->findSyntaxClass(getSession()->getNameObj("UserDefinedAttribute"));
-        for (auto member : structAttribDef->Members)
-        {
-            if (auto varMember = as<VarDecl>(member))
+        targetModifier->syntaxClass = attrUsageAttr->targetSyntaxClass;
+        targetModifier->loc = attrUsageAttr->loc;
+        addModifier(attrDecl, targetModifier);
+
+        // Every attribute declaration is associated with the type
+        // of syntax nodes it constructs (via reflection/RTTI).
+        //
+        // User-defined attributes create instances of
+        // `UserDefinedAttribute`.
+        //
+        attrDecl->syntaxClass = session->findSyntaxClass(session->getNameObj("UserDefinedAttribute"));
+
+        // The fields of the user-defined `struct` type become
+        // the parameters of the new attribute.
+        //
+        // TODO: This step should skip `static` fields.
+        //
+        for(auto member : structDecl->Members)
+        {
+            if(auto varMember = as<VarDecl>(member))
             {
-                RefPtr<ParamDecl> param = new ParamDecl();
-                param->nameAndLoc = member->nameAndLoc;
-                param->type = varMember->type;
-                param->loc = member->loc;
-                attribDecl->Members.add(param);
+                ensureDecl(varMember, DeclCheckState::CanUseTypeOfValueDecl);
+
+                RefPtr<ParamDecl> paramDecl = new ParamDecl();
+                paramDecl->nameAndLoc = member->nameAndLoc;
+                paramDecl->type = varMember->type;
+                paramDecl->loc = member->loc;
+                paramDecl->SetCheckState(DeclCheckState::Checked);
+
+                paramDecl->ParentDecl = attrDecl;
+                attrDecl->Members.add(paramDecl);
             }
         }
-        // add the attribute class definition to the syntax tree, so it can be found
-        structAttribDef->ParentDecl->Members.add(attribDecl.Ptr());
-        structAttribDef->ParentDecl->memberDictionaryIsValid = false;
-        // do necessary checks on this newly constructed node
 
+        // We need to end by putting the new attribute declaration
+        // into the AST, so that it can be found via lookup.
+        //
+        auto parentDecl = structDecl->ParentDecl;
+        //
+        // TODO: handle the case where `parentDecl` is generic?
+        //
+        attrDecl->ParentDecl = parentDecl;
+        parentDecl->Members.add(attrDecl);
+        parentDecl->memberDictionaryIsValid = false;
+
+        // Finally, we perform any required semantic checks on
+        // the newly constructed attribute decl.
+        //
         // TODO: what check state is relevant here?
-        ensureDecl(attribDecl, DeclCheckState::Checked);
+        //
+        ensureDecl(attrDecl, DeclCheckState::Checked);
 
-        return attribDecl.Ptr();
+        return attrDecl;
     }
 
     bool SemanticsVisitor::hasIntArgs(Attribute* attr, int numArgs)