Clean up the way that lookup "through" a base type is encoded (shader-slang#1519)

Tim Foley · web-flow · commit bc0d0f9e2a5a · 2020-08-27T10:46:50.000-07:00
* Clean up the way that lookup "through" a base type is encoded

In order to undestand this change, it is important to undestand how lookup through base interfaces works prior to this change. In order to understand *that* it helps to be reminded of how inheritance relationships get encoded in the AST.

Suppose the user writes:

    struct Base { int val; }
    struct Derived : Base { ... }
    ...
    Derived d = ...;
    int v = d.val;

The question is how an expression like `d.val` gets semantically checked, and how it is encoded into the IR after semantic checking. You might assume it gets checked and encoded so that we end up with:

    int v = ((Base) d).val;

and that seems like it should Just Work... so of course that isn't what Slang has been doing. Instead, we relied on the fact that the inheritance relationship `Derived : Base` is represented as an `InheritanceDecl` member of the `Derived` type, and we ended up checking the code into something like:

   int v = d.&lt;anonymous&gt;.val;

where `&lt;anonymous&gt;` stands in for the name of the `InheritanceDecl` that represents inheritance from `Base`. This design choice makes a limited amount of sense when you consider how inheritance would typically be lowered to a C-like output language:

    // struct Derived : Base { ... }
    //   =&gt;
    struct Derived { Base base; ... }

The problem with that encoding is that it really doesn't make sense for almost any other scenario. In particular, if you have a generic type parameter `T` that was constrianed with `T : ISomething`, then the constraint isn't even technically a *member* of the type parameter `T`, so expressing thing as a member reference in the AST is completely incorrect. Unfortunately, by the time it was clear that we needed something better, a bunch of implementation work was done based on the existing representation.

This change tries to clean things up so that lookup of a super-type member through a value of a sub-type does the obvious thing: cast the value to the super-type and then look up the member (as in `((Base) d).val`).

The core of the change is that in lookup, instead of creating `Constraint` breadcrumbs whenever we are looking up in a super-type (with a reference to the `TypeConstraintDecl` being used) we instead use `SuperType` breadcrumbs (with a reference to a `SubtypeWitness`). Then when we create the expression from a `LookupResultItem`, we translate any `SuperType` breadcrumbs into `CastToSuperTypeExpr`s (an expression type that already existed).

This change also adds support for lookup through the `This` type in the context of an interface, and in order for that to work we need a new kind of subtype witness to represent the knowledge that a `This` type is a subtype of the enclosing interface. Making that work forces us to change the representation of `TransitiveSubtypeWitness` so that it takes a pair of subtype witnesses (and not one subtype witness plus one `TypeConstraintDecl`). For the most part this is a small change, but it raises the possibility that some pieces of the code aren't going to be robust against all possible shapes of subtype witnesses.

The IR lowering logic has relied on the weird `d.&lt;anonymous&gt;` representation in order to ensure that when looking up interface members we weren't always casting to the interface type (which would create a `makeExistential` instruction), and then calling using that. Basically, the IR lowering would ignore the `d.&lt;anonymous&gt;` part and just emit `d`, but we can't do that for `((Base) d)` or `((IThing) d)` because whehter or not we should actually perform the cast depends on context.

For now we solve that problem by adding specific logic to ignore up-casts to interface types when they appear in member expressions or method calls. A more robust solution might be needed down the line, but this seems to work in practice.

All of this work is cleanup that I found was needed in order to make `extension`s of `interface` types workable.

* fixup: disable an incorrect test
diff --git a/source/slang/slang-ast-serialize.cpp b/source/slang/slang-ast-serialize.cpp
@@ -1624,7 +1624,7 @@ SlangResult ASTSerialReader::load(const uint8_t* data, size_t dataCount, ASTBuil
                     {
                         typedef LookupResultItem::Breadcrumb Breadcrumb;
 
-                        auto breadcrumb = new LookupResultItem::Breadcrumb(Breadcrumb::Kind::Member, DeclRef<Decl>(), nullptr);
+                        auto breadcrumb = new LookupResultItem::Breadcrumb(Breadcrumb::Kind::Member, DeclRef<Decl>(), nullptr, nullptr);
                         m_scope.add(breadcrumb);
                         m_objects[i] = breadcrumb;
                         break;
diff --git a/source/slang/slang-ast-support-types.h b/source/slang/slang-ast-support-types.h
@@ -1118,7 +1118,7 @@ namespace Slang
                 // `T` is a subtype of some other type `U`, so that
                 // lookup was able to find a member through type `U`
                 // instead.
-                Constraint,
+                SuperType,
 
                 // The lookup process considered a member of an
                 // enclosing type as being in scope, so that any
@@ -1154,18 +1154,22 @@ namespace Slang
             //
             DeclRef<Decl> declRef;
 
+            Val* val = nullptr;
+
             // The next implicit step that the lookup process took to
             // arrive at a final value.
             RefPtr<Breadcrumb> next;
 
             Breadcrumb(
                 Kind                kind,
                 DeclRef<Decl>       declRef,
+                Val*                val,
                 RefPtr<Breadcrumb>  next,
                 ThisParameterMode   thisParameterMode = ThisParameterMode::Default)
                 : kind(kind)
                 , thisParameterMode(thisParameterMode)
                 , declRef(declRef)
+                , val(val)
                 , next(next)
             {}
         };
diff --git a/source/slang/slang-ast-val.cpp b/source/slang/slang-ast-val.cpp
@@ -363,7 +363,7 @@ bool TransitiveSubtypeWitness::_equalsValOverride(Val* val)
     return sub->equals(otherWitness->sub)
         && sup->equals(otherWitness->sup)
         && subToMid->equalsVal(otherWitness->subToMid)
-        && midToSup.equals(otherWitness->midToSup);
+        && midToSup->equalsVal(otherWitness->midToSup);
 }
 
 Val* TransitiveSubtypeWitness::_substituteImplOverride(ASTBuilder* astBuilder, SubstitutionSet subst, int * ioDiff)
@@ -373,7 +373,7 @@ Val* TransitiveSubtypeWitness::_substituteImplOverride(ASTBuilder* astBuilder, S
     Type* substSub = as<Type>(sub->substituteImpl(astBuilder, subst, &diff));
     Type* substSup = as<Type>(sup->substituteImpl(astBuilder, subst, &diff));
     SubtypeWitness* substSubToMid = as<SubtypeWitness>(subToMid->substituteImpl(astBuilder, subst, &diff));
-    DeclRef<Decl> substMidToSup = midToSup.substituteImpl(astBuilder, subst, &diff);
+    SubtypeWitness* substMidToSup = as<SubtypeWitness>(midToSup->substituteImpl(astBuilder, subst, &diff));
 
     // If nothing changed, then we can bail out early.
     if (!diff)
@@ -416,7 +416,7 @@ String TransitiveSubtypeWitness::_toStringOverride()
     sb << "TransitiveSubtypeWitness(";
     sb << this->subToMid->toString();
     sb << ", ";
-    sb << this->midToSup.toString();
+    sb << this->midToSup->toString();
     sb << ")";
     return sb.ProduceString();
 }
@@ -426,7 +426,7 @@ HashCode TransitiveSubtypeWitness::_getHashCodeOverride()
     auto hash = sub->getHashCode();
     hash = combineHash(hash, sup->getHashCode());
     hash = combineHash(hash, subToMid->getHashCode());
-    hash = combineHash(hash, midToSup.getHashCode());
+    hash = combineHash(hash, midToSup->getHashCode());
     return hash;
 }
 
diff --git a/source/slang/slang-ast-val.h b/source/slang/slang-ast-val.h
@@ -154,7 +154,7 @@ class TransitiveSubtypeWitness : public SubtypeWitness
     SubtypeWitness* subToMid = nullptr;
 
     // Witness that `mid : sup`
-    DeclRef<Decl> midToSup;
+    SubtypeWitness* midToSup = nullptr;
 
     // Overrides should be public so base classes can access
     bool _equalsValOverride(Val* val);
@@ -200,4 +200,9 @@ class TaggedUnionSubtypeWitness : public SubtypeWitness
     Val* _substituteImplOverride(ASTBuilder* astBuilder, SubstitutionSet subst, int* ioDiff);
 };
 
+class ThisTypeSubtypeWitness : public SubtypeWitness
+{
+    SLANG_CLASS(ThisTypeSubtypeWitness)
+};
+
 } // namespace Slang
diff --git a/source/slang/slang-check-conformance.cpp b/source/slang/slang-check-conformance.cpp
@@ -82,10 +82,15 @@ namespace Slang
             // where `[...]` represents the "hole" we leave
             // open to fill in next.
             //
+            DeclaredSubtypeWitness* declaredWitness = m_astBuilder->create<DeclaredSubtypeWitness>();
+            declaredWitness->sub = bb->sub;
+            declaredWitness->sup = bb->sup;
+            declaredWitness->declRef = bb->declRef;
+
             TransitiveSubtypeWitness* transitiveWitness = m_astBuilder->create<TransitiveSubtypeWitness>();
-            transitiveWitness->sub = bb->sub;
+            transitiveWitness->sub = subType;
             transitiveWitness->sup = bb->sup;
-            transitiveWitness->midToSup = bb->declRef;
+            transitiveWitness->midToSup = declaredWitness;
 
             // Fill in the current hole, and then set the
             // hole to point into the node we just created.
diff --git a/source/slang/slang-check-expr.cpp b/source/slang/slang-check-expr.cpp
@@ -321,21 +321,43 @@ namespace Slang
                 bb = ConstructDerefExpr(bb, loc);
                 break;
 
-            case LookupResultItem::Breadcrumb::Kind::Constraint:
+            case LookupResultItem::Breadcrumb::Kind::SuperType:
                 {
-                    // TODO: do we need to make something more
-                    // explicit here?
-                    auto expr = ConstructDeclRefExpr(
-                        breadcrumb->declRef,
-                        bb,
-                        loc);
-
-                    if(bb && bb->type.isLeftValue)
+                    // Note: a lookup through a super-type can
+                    // occur even in the case of a `static` member,
+                    // so we only modify the base expression here
+                    // if there is one.
+                    //
+                    if( bb )
                     {
-                        expr->type.isLeftValue = true;
-                    }
+                        // We know that the breadcrumb reprsents a
+                        // cast of the base expression to a super type,
+                        // so we construct that cast explicitly here.
+                        //
+                        auto witness = as<SubtypeWitness>(breadcrumb->val);
+                        SLANG_ASSERT(witness);
+                        auto expr = createCastToSuperTypeExpr(witness->sup, bb, witness);
+
+                        // Note that we allow a cast of an l-value to
+                        // be used as an l-value here because it enables
+                        // `[mutating]` methods to be called, and
+                        // mutable properties to be modified, but this
+                        // is probably not *technically* correct, since
+                        // treating an l-value of type `Derived` as
+                        // an l-value of type `Base` implies that we
+                        // can assign an arbitrary value of type `Base`
+                        // to that l-value (which would be an error).
+                        //
+                        // TODO: make sure we believe there are no
+                        // issues here.
+                        //
+                        if(bb && bb->type.isLeftValue)
+                        {
+                            expr->type.isLeftValue = true;
+                        }
 
-                    bb = expr;
+                        bb = expr;
+                    }
                 }
                 break;
 
diff --git a/source/slang/slang-check-overload.cpp b/source/slang/slang-check-overload.cpp
@@ -1014,6 +1014,7 @@ namespace Slang
         ctorItem.breadcrumbs = new LookupResultItem::Breadcrumb(
             LookupResultItem::Breadcrumb::Kind::Member,
             typeItem.declRef,
+            nullptr,
             typeItem.breadcrumbs);
 
         OverloadCandidate candidate;
diff --git a/source/slang/slang-lookup.cpp b/source/slang/slang-lookup.cpp
@@ -22,6 +22,7 @@ struct BreadcrumbInfo
     LookupResultItem::Breadcrumb::Kind kind;
     LookupResultItem::Breadcrumb::ThisParameterMode thisParameterMode = LookupResultItem::Breadcrumb::ThisParameterMode::Default;
     DeclRef<Decl> declRef;
+    Val* val = nullptr;
     BreadcrumbInfo* prev = nullptr;
 };
 
@@ -165,6 +166,7 @@ LookupResultItem CreateLookupResultItem(
         breadcrumbs = new LookupResultItem::Breadcrumb(
             bb->kind,
             bb->declRef,
+            bb->val,
             breadcrumbs,
             bb->thisParameterMode);
     }
@@ -267,33 +269,42 @@ LookupResult lookUpDirectAndTransparentMembers(
     return result;
 }
 
-
 static SubtypeWitness* _makeSubtypeWitness(
     ASTBuilder*                 astBuilder,
     Type*                       subType,
     SubtypeWitness*             subToMidWitness,
     Type*                       superType,
-    DeclRef<TypeConstraintDecl> midToSuperConstraint)
+    SubtypeWitness*             midtoSuperWitness)
 {
     if(subToMidWitness)
     {
         TransitiveSubtypeWitness* transitiveWitness = astBuilder->create<TransitiveSubtypeWitness>();
         transitiveWitness->subToMid = subToMidWitness;
-        transitiveWitness->midToSup = midToSuperConstraint;
+        transitiveWitness->midToSup = midtoSuperWitness;
         transitiveWitness->sub = subType;
         transitiveWitness->sup = superType;
         return transitiveWitness;
     }
     else
     {
-        DeclaredSubtypeWitness* declaredWitness = astBuilder->create<DeclaredSubtypeWitness>();
-        declaredWitness->declRef = midToSuperConstraint;
-        declaredWitness->sub = subType;
-        declaredWitness->sup = superType;
-        return declaredWitness;
+        return midtoSuperWitness;
     }
 }
 
+static SubtypeWitness* _makeSubtypeWitness(
+    ASTBuilder*                 astBuilder,
+    Type*                       subType,
+    SubtypeWitness*             subToMidWitness,
+    Type*                       superType,
+    DeclRef<TypeConstraintDecl> midToSuperConstraint)
+{
+    DeclaredSubtypeWitness* midToSuperWitness = astBuilder->create<DeclaredSubtypeWitness>();
+    midToSuperWitness->declRef = midToSuperConstraint;
+    midToSuperWitness->sub = subType;
+    midToSuperWitness->sup = superType;
+    return _makeSubtypeWitness(astBuilder, subType, subToMidWitness, superType, midToSuperWitness);
+}
+
 // Same as the above, but we are specializing a type instead of a decl-ref
 static Type* _maybeSpecializeSuperType(
     ASTBuilder*                 astBuilder, 
@@ -337,37 +348,16 @@ static void _lookUpMembersInSuperTypeImpl(
     LookupResult&           ioResult,
     BreadcrumbInfo*         inBreadcrumbs);
 
-
 static void _lookUpMembersInSuperType(
-    ASTBuilder*                 astBuilder, 
+    ASTBuilder*                 astBuilder,
     Name*                       name,
     Type*                       leafType,
-    SubtypeWitness*             leafIsIntermediateWitness,
-    DeclRef<TypeConstraintDecl> intermediateIsSuperConstraint,
+    Type*                       superType,
+    SubtypeWitness*             leafIsSuperWitness,
     LookupRequest const&        request,
     LookupResult&               ioResult,
     BreadcrumbInfo*             inBreadcrumbs)
 {
-    if( request.semantics )
-    {
-        ensureDecl(request.semantics, intermediateIsSuperConstraint, DeclCheckState::CanUseBaseOfInheritanceDecl);
-    }
-
-    // The super-type in the constraint (e.g., `Foo` in `T : Foo`)
-    // will tell us a type we should use for lookup.
-    //
-    auto superType = getSup(astBuilder, intermediateIsSuperConstraint);
-    //
-    // We will go ahead and perform lookup using `superType`,
-    // after dealing with some details.
-
-    auto leafIsSuperWitness = _makeSubtypeWitness(
-        astBuilder,
-        leafType,
-        leafIsIntermediateWitness,
-        superType,
-        intermediateIsSuperConstraint);
-
     // If we are looking up through an interface type, then
     // we need to be sure that we add an appropriate
     // "this type" substitution here, since that needs to
@@ -384,8 +374,8 @@ static void _lookUpMembersInSuperType(
     //
     BreadcrumbInfo breadcrumb;
     breadcrumb.prev = inBreadcrumbs;
-    breadcrumb.kind = LookupResultItem::Breadcrumb::Kind::Constraint;
-    breadcrumb.declRef = intermediateIsSuperConstraint;
+    breadcrumb.kind = LookupResultItem::Breadcrumb::Kind::SuperType;
+    breadcrumb.val = leafIsSuperWitness;
     breadcrumb.prev = inBreadcrumbs;
 
     // TODO: Need to consider case where this might recurse infinitely (e.g.,
@@ -399,6 +389,39 @@ static void _lookUpMembersInSuperType(
     _lookUpMembersInSuperTypeImpl(astBuilder, name, leafType, superType, leafIsSuperWitness, request, ioResult, &breadcrumb);
 }
 
+static void _lookUpMembersInSuperType(
+    ASTBuilder*                 astBuilder, 
+    Name*                       name,
+    Type*                       leafType,
+    SubtypeWitness*             leafIsIntermediateWitness,
+    DeclRef<TypeConstraintDecl> intermediateIsSuperConstraint,
+    LookupRequest const&        request,
+    LookupResult&               ioResult,
+    BreadcrumbInfo*             inBreadcrumbs)
+{
+    if( request.semantics )
+    {
+        ensureDecl(request.semantics, intermediateIsSuperConstraint, DeclCheckState::CanUseBaseOfInheritanceDecl);
+    }
+
+    // The super-type in the constraint (e.g., `Foo` in `T : Foo`)
+    // will tell us a type we should use for lookup.
+    //
+    auto superType = getSup(astBuilder, intermediateIsSuperConstraint);
+    //
+    // We will go ahead and perform lookup using `superType`,
+    // after dealing with some details.
+
+    auto leafIsSuperWitness = _makeSubtypeWitness(
+        astBuilder,
+        leafType,
+        leafIsIntermediateWitness,
+        superType,
+        intermediateIsSuperConstraint);
+
+    return _lookUpMembersInSuperType(astBuilder, name, leafType, superType, leafIsSuperWitness, request, ioResult, inBreadcrumbs);
+}
+
 static void _lookUpMembersInSuperTypeDeclImpl(
     ASTBuilder*             astBuilder,
     Name*                   name,
@@ -587,10 +610,32 @@ static void _lookUpMembersInSuperTypeImpl(
 
         _lookUpMembersInSuperTypeDeclImpl(astBuilder, name, leafType, superType, leafIsSuperWitness, declRef, request, ioResult, inBreadcrumbs);
     }
-    if (auto extractExistentialType = as<ExtractExistentialType>(superType))
+    else if (auto extractExistentialType = as<ExtractExistentialType>(superType))
     {
         _lookUpMembersInSuperTypeDeclImpl(astBuilder, name, leafType, superType, leafIsSuperWitness, extractExistentialType->interfaceDeclRef, request, ioResult, inBreadcrumbs);
     }
+    else if( auto thisType = as<ThisType>(superType) )
+    {
+        // We need to create a witness that represents the next link in the
+        // chain. The `leafIsSuperWitness` represents the knowledge that `leafType : superType`
+        // (and we know that `superType == thisType`,  but we now need to extend that
+        // with the knowledge that `thisType : thisType->interfaceTypeDeclRef`.
+        //
+        auto interfaceType = DeclRefType::create(astBuilder, thisType->interfaceDeclRef);
+
+        auto superIsInterfaceWitness = astBuilder->create<ThisTypeSubtypeWitness>();
+        superIsInterfaceWitness->sub = superType;
+        superIsInterfaceWitness->sup = interfaceType;
+
+        auto leafIsInterfaceWitness = _makeSubtypeWitness(
+            astBuilder,
+            leafType,
+            leafIsSuperWitness,
+            interfaceType,
+            superIsInterfaceWitness);
+
+        _lookUpMembersInSuperTypeDeclImpl(astBuilder, name, leafType, interfaceType, leafIsInterfaceWitness, thisType->interfaceDeclRef, request, ioResult, inBreadcrumbs);
+    }
 }
 
     /// Perform lookup for `name` in the context of `type`.
diff --git a/source/slang/slang-lower-to-ir.cpp b/source/slang/slang-lower-to-ir.cpp
diff --git a/tests/compute/extension-on-interface.slang b/tests/compute/extension-on-interface.slang

Original file line number	Diff line number	Diff line change
`@@ -1624,7 +1624,7 @@ SlangResult ASTSerialReader::load(const uint8_t* data, size_t dataCount, ASTBuil`
`1624`	`1624`	`{`
`1625`	`1625`	`typedef LookupResultItem::Breadcrumb Breadcrumb;`
`1626`	`1626`
`1627`		`- auto breadcrumb = new LookupResultItem::Breadcrumb(Breadcrumb::Kind::Member, DeclRef<Decl>(), nullptr);`
	`1627`	`+ auto breadcrumb = new LookupResultItem::Breadcrumb(Breadcrumb::Kind::Member, DeclRef<Decl>(), nullptr, nullptr);`
`1628`	`1628`	`m_scope.add(breadcrumb);`
`1629`	`1629`	`m_objects[i] = breadcrumb;`
`1630`	`1630`	`break;`