Fix poorly defined behavior when choosing certain function overloads.

Previously the order of overload declarations would affect which was chosen.

Author: marsupial

CMakeLists.txt

@@ -248,6 +248,7 @@ TESTSUITE ( and-or-not-synonyms aastep arithmetic array array-derivs array-range
             error-dupes exit exponential
             fprintf
             function-earlyreturn function-simple function-outputelem
+            function-overloads
             geomath getattribute-camera getattribute-shader
             getsymbol-nonheap gettextureinfo
             group-outputs groupstring

src/liboslcomp/ast.h

@@ -889,17 +889,6 @@ class ASTfunction_call : public ASTNode
     }
 
 private:
-    /// Typecheck all polymorphic versions, return UNKNOWN if no match was
-    /// found, or a real type if there was a match.  Also, upon matching,
-    /// re-jigger m_sym to point to the specific polymorphic match.
-    /// Allow arguments to be coerced (e.g., substituting a vector where
-    /// a point was expected, or a float where a color was expected) only
-    /// if coerceargs is true.  For return values, allow spatial triples to
-    /// mutually match if 'equivreturn' is true, and allow any coercive
-    /// return type if 'expected' is TypeSpec() (i.e., unknown).
-    TypeSpec typecheck_all_poly (TypeSpec expected, bool coerceargs,
-                                 bool equivreturn);
-
     /// Handle all the special cases for built-ins.  This includes
     /// irregular patterns of which args are read vs written, special
     /// checks for printf- and texture-like, etc.

src/liboslcomp/typecheck.cpp

@@ -903,28 +903,229 @@ ASTNode::check_arglist (const char *funcname, ASTNode::ref arg,
 }
 
 
+class CandidateFunctions {
+    enum {
+        kExactMatch     = 100,
+        kIntegralToFP   = 80,
+        kArrayMatch     = 40,
+        kCoercable      = 20,
+        kMatchAnything  = 1,
+        kNoMatch        = 0,
+        
+        // Additional rules that don't match C++ behaviour
+        kFPToIntegral   = 60,          // = kIntegralToFP to match c++
+        kMatchReturn    = kExactMatch, // = 0 to match c++
+        kCoercehReturn  = kCoercable,  // = 0 to match c++
+    };
+    struct Candidate {
+        FunctionSymbol* sym;
+        TypeSpec rtype;
+        int ascore;
+        int rscore;
+
+        Candidate(FunctionSymbol *s, TypeSpec rt, int as, int rs) :
+            sym(s), rtype(rt), ascore(as), rscore(rs) {}
+
+        string_view name() const { return sym->name(); }
+    };
+    typedef std::vector<Candidate> Candidates;
+
+    OSLCompilerImpl* m_compiler;
+    Candidates m_candidates;
+    TypeSpec m_rval;
+    ASTNode::ref m_args;
+    size_t m_nargs;
+
+    const char* scoreWildcard(int& argscore, size_t& fargs, const char* args) const {
+        while (fargs < m_nargs) {
+            argscore += kMatchAnything;
+            ++fargs;
+        }
+        return args + 1;
+    }
 
-TypeSpec
-ASTfunction_call::typecheck_all_poly (TypeSpec expected, bool coerceargs,
-                                      bool equivreturn)
-{
-    for (FunctionSymbol *poly = func();  poly;  poly = poly->nextpoly()) {
-        const char *code = poly->argcodes().c_str();
+    int addCandidate(FunctionSymbol* func) {
         int advance;
-        TypeSpec returntype = m_compiler->type_from_code (code, &advance);
-        code += advance;
-        if (check_arglist (m_name.c_str(), args(), code, coerceargs)) {
-            // Return types also must match if not coercible
-            if (expected == returntype ||
-                (equivreturn && equivalent(expected,returntype)) ||
-                expected == TypeSpec()) {
-                m_sym = poly;
-                return returntype;
+        const char *formals = func->argcodes().c_str();
+        TypeSpec rtype = m_compiler->type_from_code (formals, &advance);
+        formals += advance;
+
+        int argscore = 0;
+        size_t fargs = 0;
+        for (ASTNode::ref arg = m_args; *formals && arg; ++fargs, arg = arg->next()) {
+            switch (*formals) {
+                case '*':  // Will match anything left
+                    formals = scoreWildcard(argscore, fargs, formals);
+                    ASSERT (*formals == 0);
+                    continue;
+
+                case '.':  // Token/value pairs
+                    if (arg->typespec().is_string() && arg->next()) {
+                        formals = scoreWildcard(argscore, fargs, formals);
+                        ASSERT (*formals == 0);
+                        continue;
+                    }
+                    return kNoMatch;
+
+                case '?':
+                    if (formals[1] == '[' && formals[2] == ']') {
+                        // Any array
+                        formals += 3;
+                        if (!arg->typespec().is_array())
+                            return kNoMatch; // wanted an array, didn't get one
+                        argscore += kMatchAnything;
+                    } else if (!arg->typespec().is_array()) {
+                        formals += 1;       // match anything
+                        argscore += kMatchAnything;
+                    } else
+                        return kNoMatch;    // wanted any scalar, got an array
+                    continue;
+
+                default:
+                    break;
             }
+            // To many arguments for the function, done without a match.
+            if (fargs >= m_nargs)
+                return kNoMatch;
+
+            TypeSpec argtype = arg->typespec();
+            TypeSpec formaltype = m_compiler->type_from_code (formals, &advance);
+            formals += advance;
+
+            if (argtype == formaltype)
+                argscore += kExactMatch;   // ok, move on to next arg
+            else if (!argtype.is_closure() && argtype.is_scalarnum() &&
+                     !formaltype.is_closure() && formaltype.is_scalarnum())
+                argscore += formaltype.is_int() ? kFPToIntegral : kIntegralToFP;
+            else if (formaltype.is_unsized_array() && argtype.is_sized_array() &&
+                  formaltype.elementtype() == argtype.elementtype()) {
+                // Allow a fixed-length array match to a formal array with
+                // unspecified length, if the element types are the same.
+                argscore += kArrayMatch;
+            } else if (assignable (formaltype, argtype))
+                argscore += kCoercable;
+            else
+                return kNoMatch;
         }
+
+        // Check any remaining arguments
+        switch (*formals) {
+            case '*':
+            case '.':
+                // Skip over the unused optional args
+                ++formals;
+                ++fargs;
+            case '\0':
+                if (fargs < m_nargs)
+                    return 0;
+                break;
+
+            default:
+                // TODO: Scoring default function arguments would go here
+                // Curently an unused formal argument, so no match at all.
+                return 0;
+        }
+        ASSERT (*formals == 0);
+
+        int highscore = m_candidates.empty() ? 0 : m_candidates.front().ascore;
+        if (argscore < highscore)
+            return 0;
+
+        
+        if (argscore == highscore) {
+            // Check for duplicate declarations
+            for (auto& candidate : m_candidates) {
+                if (candidate.sym->argcodes() == func->argcodes())
+                    return 0;
+            }
+        } else  // clear any prior ambiguous matches
+            m_candidates.clear();
+    
+        // append the latest high scoring function
+        m_candidates.emplace_back(func, rtype, argscore, rtype == m_rval ?
+         kMatchReturn : (equivalent(rtype, m_rval) ? kCoercehReturn : kNoMatch));
+
+        return argscore;
     }
-    return TypeSpec();
-}
+
+public:
+    CandidateFunctions(OSLCompilerImpl* compiler, TypeSpec rval, ASTNode::ref args, FunctionSymbol* func) :
+        m_compiler(compiler), m_rval(rval), m_args(args), m_nargs(0) {
+
+        //std::cerr << "Matching " << func->name() << " formals='" << (rval.simpletype().basetype != TypeDesc::UNKNOWN ?  compiler->code_from_type (rval) : " ");
+        for (ASTNode::ref arg = m_args; arg; arg = arg->next()) {
+            //std::cerr << compiler->code_from_type (arg->typespec());
+            ++m_nargs;
+        }
+        //std::cerr << "'\n";
+
+        while (func) {
+            //int score =
+            addCandidate(func);
+            //std::cerr << '\t' << func->name() << " formals='" << func->argcodes().c_str() << "'  " << score << ", " << (score ? m_candidates.back().rscore : 0) << "\n";
+            func = func->nextpoly();
+        }
+    }
+
+    void reportError(ASTfunction_call* caller, string_view name) {
+        std::string actualargs;
+        for (ASTNode::ref arg = m_args; arg; arg = arg->next()) {
+            if (actualargs.length())
+                actualargs += ", ";
+            actualargs += arg->typespec().string();
+        }
+        caller->error ("No matching function call to '%s (%s)'",
+               name.c_str(), actualargs.c_str());
+   }
+
+    void reportAmbiguity(FunctionSymbol* sym) const {
+        int advance;
+        const char *formals = sym->argcodes().c_str();
+        TypeSpec returntype = m_compiler->type_from_code (formals, &advance);
+        formals += advance;
+ 
+        auto& errh =  m_compiler->errhandler();
+        if (ASTNode* decl = sym->node())
+            errh.message("%s:%d ", decl->sourcefile(), decl->sourceline());
+
+        errh.message("candidate function:\n");
+        errh.message("\t%s %s (%s)\n",
+                            m_compiler->type_c_str(returntype), sym->name(),
+                            m_compiler->typelist_from_code(formals).c_str());
+    }
+
+    std::pair<FunctionSymbol*, TypeSpec> best(ASTNode* caller, bool strict = 0) {
+        switch (m_candidates.size()) {
+            case 0:  return { nullptr, TypeSpec() };
+            case 1:  return { m_candidates[0].sym, m_candidates[0].rtype };
+            default: break;
+        }
+
+        int ambiguity = 0;
+        std::pair<const Candidate*, int> c = { nullptr, -1 };
+        for (auto& candidate : m_candidates) {
+            // re-score based on matching return value
+            if (candidate.rscore > c.second)
+                c = std::make_pair(&candidate, candidate.rscore);
+            else if (candidate.rscore == c.second)
+                ambiguity = candidate.rscore;
+        }
+
+        if (ambiguity || strict) {
+            ASSERT (caller);
+            caller->warning( "call to '%s' is ambiguous", m_candidates[0].name());
+            for (auto& candidate : m_candidates) {
+                if (candidate.rscore >= ambiguity)
+                    reportAmbiguity(candidate.sym);
+            }
+        }
+
+        ASSERT (c.first);
+        return {c.first->sym, c.first->rtype};
+    }
+
+    bool empty() const { return m_candidates.empty(); }
+};
 
 
 
@@ -1191,47 +1392,10 @@ ASTfunction_call::typecheck (TypeSpec expected)
         return typecheck_struct_constructor ();
     }
 
-    bool match = false;
-
-    // Look for an exact match, including expected return type
-    m_typespec = typecheck_all_poly (expected, false, false);
-    if (m_typespec != TypeSpec())
-        match = true;
-
-    // Now look for an exact match for arguments, but equivalent return type
-    m_typespec = typecheck_all_poly (expected, false, true);
-    if (m_typespec != TypeSpec())
-        match = true;
+    CandidateFunctions candidates(m_compiler, expected, args(), func());
+    std::tie(m_sym, m_typespec) = candidates.best(this);
 
-    // Now look for an exact match on args, but any return type
-    if (! match && expected != TypeSpec()) {
-        m_typespec = typecheck_all_poly (TypeSpec(), false, false);
-        if (m_typespec != TypeSpec())
-            match = true;
-    }
-
-    // Now look for a coercible match of args, exact march on return type
-    if (! match) {
-        m_typespec = typecheck_all_poly (expected, true, false);
-        if (m_typespec != TypeSpec())
-            match = true;
-    }
-
-    // Now look for a coercible match of args, equivalent march on return type
-    if (! match) {
-        m_typespec = typecheck_all_poly (expected, true, true);
-        if (m_typespec != TypeSpec())
-            match = true;
-    }
-
-    // All that failed, try for a coercible match on everything
-    if (! match && expected != TypeSpec()) {
-        m_typespec = typecheck_all_poly (TypeSpec(), true, false);
-        if (m_typespec != TypeSpec())
-            match = true;
-    }
-
-    if (match) {
+    if (m_sym != nullptr) {
         if (is_user_function()) {
             if (func()->number_of_returns() == 0 &&
                 ! func()->typespec().is_void()) {
@@ -1245,35 +1409,18 @@ ASTfunction_call::typecheck (TypeSpec expected)
         return m_typespec;
     }
 
+    // Ambiguity has already been reported.
+    if (!candidates.empty())
+        return TypeSpec();
+
     // Couldn't find any way to match any polymorphic version of the
     // function that we know about.  OK, at least try for helpful error
     // message.
-    std::string choices ("");
-    for (FunctionSymbol *poly = func();  poly;  poly = poly->nextpoly()) {
-        const char *code = poly->argcodes().c_str();
-        int advance;
-        TypeSpec returntype = m_compiler->type_from_code (code, &advance);
-        code += advance;
-        if (choices.length())
-            choices += "\n";
-        choices += Strutil::format ("\t%s %s (%s)",
-                              type_c_str(returntype), m_name.c_str(),
-                              m_compiler->typelist_from_code(code).c_str());
-    }
+	candidates.reportError(this, m_name);
 
-    std::string actualargs;
-    for (ASTNode::ref arg = args();  arg;  arg = arg->next()) {
-        if (actualargs.length())
-            actualargs += ", ";
-        actualargs += arg->typespec().string();
-    }
+    for (FunctionSymbol *poly = func();  poly;  poly = poly->nextpoly())
+        candidates.reportAmbiguity(poly);
 
-    if (choices.size())
-        error ("No matching function call to '%s (%s)'\n    Candidates are:\n%s",
-               m_name.c_str(), actualargs.c_str(), choices.c_str());
-    else
-        error ("No matching function call to '%s (%s)'",
-               m_name.c_str(), actualargs.c_str());
     return TypeSpec();
 }
 

testsuite/function-overloads/a_fcn.h

@@ -0,0 +1,11 @@
+
+void testA(float a, float b, float c) {
+    printf("testA float\n");
+}
+void testA(color a, float b, float c) {
+    printf("testA color\n");
+}
+void testA(normal a, float b, float c) {
+    printf("testA normal\n");
+}
+

testsuite/function-overloads/a_ivp.h

@@ -0,0 +1,11 @@
+
+void testA(int a, float b, float c) {
+    printf("testA int\n");
+}
+void testA(vector a, float b, float c) {
+    printf("testA vector\n");
+}
+void testA(point a, float b, float c) {
+    printf("testA point\n");
+}
+