Another fix for overriding property decls (#1509)

* Another fix for overriding property decls

The central problem we keep running into with `property` decls in `interface`s comes down to two choices:

1. When a member lookup `obj.someName` or a simple lookup for `someName` produces an overloaded result, we make no attempt to resolve the overloading right away, and instead postpone disambiguation until the point where that expression gets *used*, in case the context where it gets used can help in disambiguation (a notable case being when there is a call expression `obj.someName(...)` or `someName(...)`).

2. When looking up members in a the scope of a type (either for `obj.someName` or `someName` in the context of a method), we include all results from base types in the set of overloads returned, even in cases where the type has a direct member that "overrides" the inherited one.

The combination of these factors means that when a `struct` type implements a `property` to satisfy a requirement of an inherited `interface`, then references to `obj.someProp` end up being ambiguous between the property in the concrete `struct` type and the property it inherits through the `interface`.

There is no quick fix possible for issue (2). It might seem that we could just skip over members inherited through `interface`s when doing lookup in a type, but that solution wouldn't apply to inheritance from another `struct` type, or any future scenario where we support default implementations of methods in interfaces. The simple idea of saying that a derived-type member named `M` hides all inherited members named `M` is possible, but would lead to a bad user interface when a type wants to support both a core "bottleneck" method and a bunch of convenience overloads with the same name.

That leaves us with issue (1), and trying to find a reasonable fix for it. The common case is that any expression `e` eventually gets used in a context where it will be be subject to disambiguation:

* If we form a call expression `e(...)`, then the overload resolution logic will (obviously) work to disambiguate which `e` was meant.

* If `e` is used as an argument to another call (`f(... e ...)` or `... + e`), then `e` will be coerced to the expected parameter type for its argument position, and that coercion will disambiguate it (this is the bit that was fixed in #1501)

* If `e` is used in another context where a type is expected/known, it will also be coerced: `if(e)`, `int v = e`, etc.

The problem case that is left behind is any scenario where `e` is not subject to one of the above resolution cases, which mostly amounts to cases where an expression is never coerced to a single fixed type. There are a few important cases where this occurs today:

* When the expression is used as the left-hand side of an assignement (`e = ...`).

* When an expression is used to initialize a variable with an implicit type (`let v = e`).

* When inferring generic arguments from the value arguments at a call site (`f(e)` where `f` is defined as `f<T>(T v)`)

The key connecting thread in each of these cases is that the front-end needs to determine the type of `e` to make progress.

Our semantic checking logic already has functions that try to draw a distinction between the two cases:

* The `CheckTerm()` operation is supposed to be used when we expect that we will eventually coerce or otherwise diambiguate the term, and also in cases where we don't yet know if a term should name a type or a value

* The `CheckExpr()` operation is supposed to be used when we do not expect that we will apply coercion/disambiguation to a term, and need to have assurances that it has been coerced into a non-overloaded expression with a reasonable type

The simple part of the fix made here is to make `CheckExpr()` actually do part of what it is suppsoed to (attempt to disambiguate overloaded terms), and then audit all the call sites to `CheckExpr()` to make sure they are actually ones that intend to opt into that logic.

The messier part of the fix is dealing with generic argument inference, because we need to extract the type of the disambiguated expression for the purposes of inference, but we don't want to disturb the actual argument list at a call site (because type coercion of the arguments is supposed to handle the disambiguation). This part is done with a bit of special-casing in the overload-resolution context, by adding a method that gets the type or an argument after disambiguation (when possible).

* fixup

Co-authored-by: Yong He <yonghe@outlook.com>

7 files changed, 42 insertions, 15 deletions

diff --git a/source/slang/slang-check-conversion.cpp b/source/slang/slang-check-conversion.cpp
index 0a0990ede..8b60b2725 100644
--- a/source/slang/slang-check-conversion.cpp
+++ b/source/slang/slang-check-conversion.cpp
@@ -488,6 +488,14 @@ namespace Slang
         // then we should start by trying to resolve the ambiguous reference
         // based on prioritization of the different candidates.
         //
+        // TODO: A more powerful model would be to try to coerce each
+        // of the constituent overload candidates, filtering down to
+        // those that are coercible, and then disambiguating the result.
+        // Such an approach would let us disambiguate between overloaded
+        // symbols based on their type (e.g., by casting the name of
+        // an overloaded function to the type of the overload we mean
+        // to reference).
+        //
         if( auto fromOverloadedExpr = as<OverloadedExpr>(fromExpr) )
         {
             auto resolvedExpr = maybeResolveOverloadedExpr(fromOverloadedExpr, LookupMask::Default, nullptr);
diff --git a/source/slang/slang-check-decl.cpp b/source/slang/slang-check-decl.cpp
index 4ea8aa7f6..f9a514d23 100644
--- a/source/slang/slang-check-decl.cpp
+++ b/source/slang/slang-check-decl.cpp
@@ -3152,7 +3152,7 @@ namespace Slang
         // that represents the explicit tag for this case.
         if(auto initExpr = decl->tagExpr)
         {
-            initExpr = CheckExpr(initExpr);
+            initExpr = CheckTerm(initExpr);
             initExpr = coerce(tagType, initExpr);
 
             // We want to enforce that this is an integer constant
@@ -3919,7 +3919,7 @@ namespace Slang
             // We must check the expression and coerce it to the
             // actual type of the parameter.
             //
-            initExpr = CheckExpr(initExpr);
+            initExpr = CheckTerm(initExpr);
             initExpr = coerce(typeExpr.type, initExpr);
             paramDecl->initExpr = initExpr;
 
diff --git a/source/slang/slang-check-expr.cpp b/source/slang/slang-check-expr.cpp
index 0cc3a55c5..0de7acae2 100644
--- a/source/slang/slang-check-expr.cpp
+++ b/source/slang/slang-check-expr.cpp
@@ -988,7 +988,7 @@ namespace Slang
         Expr* indexExpr = subscriptExpr->indexExpression;
         if (indexExpr)
         {
-            indexExpr = CheckExpr(indexExpr);
+            indexExpr = CheckTerm(indexExpr);
         }
 
         subscriptExpr->baseExpression = baseExpr;
@@ -1167,17 +1167,27 @@ namespace Slang
     {
         expr->left = CheckExpr(expr->left);
         expr->right = CheckTerm(expr->right);
+
         return checkAssignWithCheckedOperands(expr);
     }
 
-    Expr* SemanticsVisitor::CheckExpr(Expr* expr)
+    Expr* SemanticsVisitor::CheckExpr(Expr* uncheckedExpr)
     {
-        auto term = CheckTerm(expr);
+        auto checkedTerm = CheckTerm(uncheckedExpr);
+
+        // First, we want to do any disambiguation that is needed in order
+        // to turn the `term` into an expression that names a single
+        // value (and not something overloaded).
+        //
+        auto checkedExpr = maybeResolveOverloadedExpr(checkedTerm, LookupMask::Default, getSink());
 
-        // TODO(tfoley): Need a step here to ensure that the term actually
-        // resolves to a (single) expression with a real type.
+        // Next, we want to ensure that the `expr` actually has a type
+        // that is allowable in an expression context (e.g., make sure
+        // that `expr` names a value and not a type).
+        //
+        // TODO: Implement this step.
 
-        return term;
+        return checkedExpr;
     }
 
     Expr* SemanticsVisitor::CheckInvokeExprWithCheckedOperands(InvokeExpr *expr)
@@ -1255,11 +1265,11 @@ namespace Slang
     Expr* SemanticsExprVisitor::visitInvokeExpr(InvokeExpr *expr)
     {
         // check the base expression first
-        expr->functionExpr = CheckExpr(expr->functionExpr);
+        expr->functionExpr = CheckTerm(expr->functionExpr);
         // Next check the argument expressions
         for (auto & arg : expr->arguments)
         {
-            arg = CheckExpr(arg);
+            arg = CheckTerm(arg);
         }
 
         return CheckInvokeExprWithCheckedOperands(expr);
@@ -1306,7 +1316,7 @@ namespace Slang
         // Next check the argument expression (there should be only one)
         for (auto & arg : expr->arguments)
         {
-            arg = CheckExpr(arg);
+            arg = CheckTerm(arg);
         }
 
         // LEGACY FEATURE: As a backwards-compatibility feature
@@ -1822,7 +1832,7 @@ namespace Slang
 
     Expr* SemanticsExprVisitor::visitStaticMemberExpr(StaticMemberExpr* expr)
     {
-        expr->baseExpression = CheckExpr(expr->baseExpression);
+        expr->baseExpression = CheckTerm(expr->baseExpression);
 
         // Not sure this is needed -> but guess someone could do 
         expr->baseExpression = MaybeDereference(expr->baseExpression);
@@ -1855,7 +1865,7 @@ namespace Slang
     {
         auto baseExpr = inBaseExpr;
 
-        baseExpr = CheckExpr(baseExpr);
+        baseExpr = CheckTerm(baseExpr);
 
         baseExpr = MaybeDereference(baseExpr);
 
diff --git a/source/slang/slang-check-impl.h b/source/slang/slang-check-impl.h
index cdd319408..a9f861d3d 100644
--- a/source/slang/slang-check-impl.h
+++ b/source/slang/slang-check-impl.h
@@ -1191,6 +1191,13 @@ namespace Slang
                 else
                     return getArg(index)->type.type;
             }
+            Type* getArgTypeForInference(Index index, SemanticsVisitor* semantics)
+            {
+                if(argTypes)
+                    return argTypes[index];
+                else
+                    return semantics->maybeResolveOverloadedExpr(getArg(index), LookupMask::Default, nullptr)->type;
+            }
 
             bool disallowNestedConversions = false;
 
diff --git a/source/slang/slang-check-modifier.cpp b/source/slang/slang-check-modifier.cpp
index 9734df4db..b38018354 100644
--- a/source/slang/slang-check-modifier.cpp
+++ b/source/slang/slang-check-modifier.cpp
@@ -477,7 +477,7 @@ namespace Slang
                     }
                     if (!typeChecked)
                     {
-                        arg = CheckExpr(arg);
+                        arg = CheckTerm(arg);
                         arg = coerce(paramDecl->getType(), arg);
                     }
                 }
diff --git a/source/slang/slang-check-overload.cpp b/source/slang/slang-check-overload.cpp
index df5424659..0b2c61e18 100644
--- a/source/slang/slang-check-overload.cpp
+++ b/source/slang/slang-check-overload.cpp
@@ -1080,7 +1080,7 @@ namespace Slang
                 // So the question is then whether a mismatch during the
                 // unification step should be taken as an immediate failure...
 
-                TryUnifyTypes(constraints, context.getArgType(aa), getType(m_astBuilder, params[aa]));
+                TryUnifyTypes(constraints, context.getArgTypeForInference(aa, this), getType(m_astBuilder, params[aa]));
 #endif
             }
         }
diff --git a/tests/language-feature/properties/property-in-interface.slang b/tests/language-feature/properties/property-in-interface.slang
index 3622861e0..91cef0e14 100644
--- a/tests/language-feature/properties/property-in-interface.slang
+++ b/tests/language-feature/properties/property-in-interface.slang
@@ -21,6 +21,8 @@ struct YourCell : ICell
     int value;
 
     int getValue() { return value; }
+
+    [mutating] void setValue(int v) { value = v; }
 }
 
 int helper<C : ICell>(C cell)