Attempt to fix lookup for members that "override" (#1501)

Our current lookup process always finds *all* members of a type, which can include both an inherited member (e.g., from an `interface`) and one that logically overrides/implements it. If something downstream doesn't filter this result down and favor the derived member, then an ambiguity error will result.

To date, this has mostly been a non-issue because we haven't emphasized inheritance, and the main case we did support (`struct` types implemented `interface` methods) gets disambiguated as part of overload resolution for function calls.

Recent changes to support `property` declarations to `interface`s add the possibility for ambiguity between a "base" and "derived" declaration that can't rely on overload resolution for disambiguation.

The approach in this PR is to add disambiguation logic to the other main place where the results of lookup get used. If a lookup result is being assigned to a variable, passed to a function, or otherwise used in a case where a value of a specific type is needed, it will be "coerced" to the desired type. This change makes it so that the first step in the coercion logic is to try to disambiguate the expression that is being coerced.

In order to ensure that an overloaded expression can be detected and resolved even when just checking if coercion is possible, I needed to update the `canCoerce*()` functions to also take the expression that is being tested for coercibility, and not just its type. There is only one case (that I saw) where coercion checks were being made without an expression value available, and that case didn't actually need/want to handle overloading.

In order to test the fixes here, I added logic to the `property`-in-`interface` test to make sure that the critical cases work as expected (references to a derived member using "dot syntax" and "implicit `this`" syntax).

Alternatives Considered
-----------------------

The first attempt at this fix took a simpler approach: I added the disambiguation logic as a post-process on member lookup. That is, given `obj.foo` I would take the `LookupResult` for `foo` and immediately try to filter it to include only the most-derived members. This approach has the major benefit of catching even more use cases of values (and thus helping to ensure that we don't spend forever chasing down more of these ambiguity errors), but it also has two critical problems:

1. If we only trigger disambiguation when looking up `obj.foo`, then we can't do anything to help when `foo` is looked up as an ordinary identifier, but is actually equivalent to `this.foo`. A full fix would require doing this disambiguation on *every* name lookup, which leads to the second issue:

2. It is important that for a method call like `obj.m(...)` we do *not* disambiguate when looking up `obj.m`, and instead let the overload resolution for the call resolve things. That choice is what makes it possible to call an inherited `m` declaration even when there is a derived `m` with a different signature.

Issue (1) is covered by the test case that was added here, but we should probably have a test case for (2) to make sure we don't break that use case.

Caveats
-------

An important case that we don't solve in this PR is when the result of a lookup is captured in a variable without an explicit type:

    let f = obj.foo;

That case also needs disambiguation, and should be addressed in a later change.

A secondary issue is that our approach to prioritizing declarations during lookup is still quite naive. We really need a way for lookup to attach information about nesting of scopes to results (to be clear that results from inner scopes should be preferred over those from outer scopes), as well as have a robust mechanism for comparing the priority of members based on the inheritance graph of a type. This change doesn't do anything to make the situation better or worse.

6 files changed, 32 insertions, 12 deletions

diff --git a/source/slang/slang-check-conversion.cpp b/source/slang/slang-check-conversion.cpp
index ba4a9b8d9..0a0990ede 100644
--- a/source/slang/slang-check-conversion.cpp
+++ b/source/slang/slang-check-conversion.cpp
@@ -85,7 +85,7 @@ namespace Slang
         // we want to check for is whether a direct initialization
         // is possible (a type conversion exists).
         //
-        return canCoerce(toType, fromExpr->type);
+        return canCoerce(toType, fromExpr->type, fromExpr);
     }
 
     bool SemanticsVisitor::_readValueFromInitializerList(
@@ -484,6 +484,18 @@ namespace Slang
         Expr*    fromExpr,
         ConversionCost* outCost)
     {
+        // If we are about to try and coerce an overloaded expression,
+        // then we should start by trying to resolve the ambiguous reference
+        // based on prioritization of the different candidates.
+        //
+        if( auto fromOverloadedExpr = as<OverloadedExpr>(fromExpr) )
+        {
+            auto resolvedExpr = maybeResolveOverloadedExpr(fromOverloadedExpr, LookupMask::Default, nullptr);
+
+            fromExpr = resolvedExpr;
+            fromType = resolvedExpr->type;
+        }
+
         // An important and easy case is when the "to" and "from" types are equal.
         //
         if( toType->equals(fromType) )
@@ -756,6 +768,7 @@ namespace Slang
     bool SemanticsVisitor::canCoerce(
         Type*    toType,
         Type*    fromType,
+        Expr*    fromExpr,
         ConversionCost* outCost)
     {
         // As an optimization, we will maintain a cache of conversion results
@@ -795,7 +808,7 @@ namespace Slang
             toType,
             nullptr,
             fromType,
-            nullptr,
+            fromExpr,
             &cost);
 
         if (outCost)
@@ -877,7 +890,7 @@ namespace Slang
     {
         // Can we convert at all?
         ConversionCost conversionCost;
-        if(!canCoerce(toType, fromType, &conversionCost))
+        if(!canCoerce(toType, fromType, nullptr, &conversionCost))
             return false;
 
         // Is the conversion cheap enough to be done implicitly?
diff --git a/source/slang/slang-check-expr.cpp b/source/slang/slang-check-expr.cpp
index 4268ad99e..63367fa97 100644
--- a/source/slang/slang-check-expr.cpp
+++ b/source/slang/slang-check-expr.cpp
@@ -1963,9 +1963,6 @@ namespace Slang
                 return lookupMemberResultFailure(expr, baseType);
             }
 
-            // TODO: need to filter for declarations that are valid to refer
-            // to in this context...
-
             return createLookupResultExpr(
                 expr->name,
                 lookupResult,
diff --git a/source/slang/slang-check-impl.h b/source/slang/slang-check-impl.h
index 7907ef23b..cdd319408 100644
--- a/source/slang/slang-check-impl.h
+++ b/source/slang/slang-check-impl.h
@@ -670,6 +670,7 @@ namespace Slang
         bool canCoerce(
             Type*    toType,
             Type*    fromType,
+            Expr*    fromExpr,
             ConversionCost* outCost = 0);
 
         TypeCastExpr* createImplicitCastExpr();
diff --git a/source/slang/slang-check-overload.cpp b/source/slang/slang-check-overload.cpp
index 0a1294965..94638e473 100644
--- a/source/slang/slang-check-overload.cpp
+++ b/source/slang/slang-check-overload.cpp
@@ -255,7 +255,7 @@ namespace Slang
                     if (context.mode == OverloadResolveContext::Mode::JustTrying)
                     {
                         ConversionCost cost = kConversionCost_None;
-                        if (!canCoerce(getType(m_astBuilder, valParamRef), arg->type, &cost))
+                        if (!canCoerce(getType(m_astBuilder, valParamRef), arg->type, arg, &cost))
                         {
                             success = false;
                         }
@@ -344,7 +344,7 @@ namespace Slang
                     if(!getType(m_astBuilder, param)->equals(argType))
                         return false;
                 }
-                else if (!canCoerce(getType(m_astBuilder, param), argType, &cost))
+                else if (!canCoerce(getType(m_astBuilder, param), argType, arg, &cost))
                 {
                     return false;
                 }
diff --git a/tests/language-feature/properties/property-in-interface.slang b/tests/language-feature/properties/property-in-interface.slang
index ba6d5eaa9..3622861e0 100644
--- a/tests/language-feature/properties/property-in-interface.slang
+++ b/tests/language-feature/properties/property-in-interface.slang
@@ -19,6 +19,8 @@ struct MyCell : ICell
 struct YourCell : ICell
 {
     int value;
+
+    int getValue() { return value; }
 }
 
 int helper<C : ICell>(C cell)
@@ -31,7 +33,14 @@ int test(int value)
 {
     MyCell myCell = { value+1 };
     YourCell yourCell = { value };
-    return helper(myCell)*16 + helper(yourCell);
+
+    // Note: fetching `value` directly from `YourCell`
+    // to confirm that member lookup is prioritizing
+    // the concrete `YourCell::value` member of the inherited
+    // abstract member `ICell::value`.
+    //
+    int f = (yourCell.value + yourCell.getValue()) / 2;
+    return helper(myCell)*16 + helper(yourCell) + f;
 }
 
 //TEST_INPUT:ubuffer(data=[0 1 2 3], stride=4):out,name=outputBuffer
diff --git a/tests/language-feature/properties/property-in-interface.slang.expected.txt b/tests/language-feature/properties/property-in-interface.slang.expected.txt
index ba2ec282d..e9c86b523 100644
--- a/tests/language-feature/properties/property-in-interface.slang.expected.txt
+++ b/tests/language-feature/properties/property-in-interface.slang.expected.txt
@@ -1,4 +1,4 @@
 21
-32
-43
-54
+33
+45
+57