oslc: Add warning for detection of duplicate functions in the same scope. (#746)

Previously, if you for some reason defined two identical functions in
the same scope (including redefining a "built-in" function exactly in
the outermost scope), it would just silently accept it, and then you
wouldn't be sure which one would actually end up getting called.

For now it's a warning. After people have had a chance to scrub their
code bases, perhaps we will upgrade this to a full error in a future
release.

Author: lgritz

CMakeLists.txt

@@ -267,7 +267,7 @@ TESTSUITE ( and-or-not-synonyms aastep arithmetic array array-derivs array-range
             operator-overloading
             oslc-comma oslc-D
             oslc-err-arrayindex oslc-err-closuremul oslc-err-field
-            oslc-err-format oslc-err-intoverflow
+            oslc-err-format oslc-err-funcredef oslc-err-intoverflow
             oslc-err-noreturn oslc-err-notfunc
             oslc-err-outputparamvararray oslc-err-paramdefault
             oslc-err-struct-array-init oslc-err-struct-ctr

src/liboslcomp/ast.cpp

@@ -251,6 +251,45 @@ ASTNode::type_c_str (const TypeSpec &type) const
 
 
 
+void
+ASTNode::list_to_vec (const ref &A, std::vector<ref> &vec)
+{
+    vec.clear ();
+    for (ref node = A; node; node = node->next())
+        vec.push_back (node);
+}
+
+
+
+ASTNode::ref
+ASTNode::vec_to_list (std::vector<ref> &vec)
+{
+    if (vec.size()) {
+        for (size_t i = 0;  i < vec.size()-1;  ++i)
+            vec[i]->m_next = vec[i+1];
+        vec[vec.size()-1]->m_next = NULL;
+        return vec[0];
+    } else {
+        return ref();
+    }
+}
+
+
+
+std::string
+ASTNode::list_to_types_string (const ASTNode *node)
+{
+    std::ostringstream result;
+    for (int i = 0; node; node = node->nextptr(), ++i) {
+        if (i)
+            result << ", ";
+        result << node->typespec();
+    }
+    return result.str();
+}
+
+
+
 ASTshader_declaration::ASTshader_declaration (OSLCompilerImpl *comp,
                                 int stype, ustring name, ASTNode *form,
                                 ASTNode *stmts, ASTNode *meta)
@@ -304,29 +343,31 @@ ASTshader_declaration::shadertypename () const
 
 ASTfunction_declaration::ASTfunction_declaration (OSLCompilerImpl *comp,
                              TypeSpec type, ustring name,
-                             ASTNode *form, ASTNode *stmts, ASTNode *meta)
+                             ASTNode *form, ASTNode *stmts, ASTNode *meta,
+                             int sourceline_start)
     : ASTNode (function_declaration_node, comp, 0, meta, form, stmts),
       m_name(name), m_sym(NULL), m_is_builtin(false)
 {
-    m_typespec = type;
-    Symbol *f = comp->symtab().clash (name);
-    if (f && f->symtype() != SymTypeFunction) {
-        error ("\"%s\" already declared in this scope as a ", name.c_str(),
-               f->typespec().string().c_str());
-        // FIXME -- print the file and line of the other definition
-        f = NULL;
-    }
+    // Some trickery -- the compiler's idea of the "current" source line
+    // is the END of the function body, so if a hint was passed about the
+    // start of the declaration, substitute that.
+    if (sourceline_start >= 0)
+        m_sourceline = sourceline_start;
 
-    // FIXME -- allow multiple function declarations, but only if they
-    // aren't the same polymorphic type.
+    if (Strutil::starts_with (name, "___"))
+        error ("\"%s\" : sorry, can't start with three underscores", name);
 
-    if (name[0] == '_' && name[1] == '_' && name[2] == '_') {
-        error ("\"%s\" : sorry, can't start with three underscores",
-               name.c_str());
+    // Get a pointer to the first of the existing symbols of that name.
+    Symbol *existing_syms = comp->symtab().clash (name);
+    if (existing_syms && existing_syms->symtype() != SymTypeFunction) {
+        error ("\"%s\" already declared in this scope as a %s",
+               name, existing_syms->typespec());
+        // FIXME -- print the file and line of the other definition
+        existing_syms = NULL;
     }
 
-    m_sym = new FunctionSymbol (name, type, this);
-    func()->nextpoly ((FunctionSymbol *)f);
+    // Build up the argument signature for this declared function
+    m_typespec = type;
     std::string argcodes = oslcompiler->code_from_type (m_typespec);
     for (ASTNode *arg = form;  arg;  arg = arg->nextptr()) {
         const TypeSpec &t (arg->typespec());
@@ -341,6 +382,22 @@ ASTfunction_declaration::ASTfunction_declaration (OSLCompilerImpl *comp,
             v->error ("function parameter '%s' may not have a default initializer.",
                       v->name().c_str());
     }
+
+    // Allow multiple function declarations, but only if they aren't the
+    // same polymorphic type in the same scope.
+    int current_scope = oslcompiler->symtab().scopeid();
+    for (FunctionSymbol *f = reinterpret_cast<FunctionSymbol *>(existing_syms);
+         f; f = f->nextpoly()) {
+        if (f->argcodes() == argcodes && f->scope() == current_scope) {
+            warning ("Function '%s %s (%s)' declared twice in the same scope",
+                   type, name, list_to_types_string(form));
+        }
+    }
+
+
+    m_sym = new FunctionSymbol (name, type, this);
+    func()->nextpoly ((FunctionSymbol *)existing_syms);
+
     func()->argcodes (ustring (argcodes));
     oslcompiler->symtab().insert (m_sym);
 

src/liboslcomp/ast.h

@@ -248,24 +248,15 @@ class ASTNode : public OIIO::RefCnt {
 
     /// Flatten a list of nodes (headed by A) into a vector of node refs
     /// (vec).
-    static void list_to_vec (const ref &A, std::vector<ref> &vec) {
-        vec.clear ();
-        for (ref node = A; node; node = node->next())
-            vec.push_back (node);
-    }
+    static void list_to_vec (const ref &A, std::vector<ref> &vec);
 
     /// Turn a vector of node refs into a list of nodes, returning its
     /// head.
-    static ref vec_to_list (std::vector<ref> &vec) {
-        if (vec.size()) {
-            for (size_t i = 0;  i < vec.size()-1;  ++i)
-                vec[i]->m_next = vec[i+1];
-            vec[vec.size()-1]->m_next = NULL;
-            return vec[0];
-        } else {
-            return ref();
-        }
-    }
+    static ref vec_to_list (std::vector<ref> &vec);
+
+    /// Generate a comma-separated string of data types of the list headed
+    /// by node (for example, "float, int, string").
+    static std::string list_to_types_string (const ASTNode *node);
 
     /// Return the number of child nodes.
     ///
@@ -420,7 +411,8 @@ class ASTfunction_declaration : public ASTNode
 {
 public:
     ASTfunction_declaration (OSLCompilerImpl *comp, TypeSpec type, ustring name,
-                             ASTNode *form, ASTNode *stmts, ASTNode *meta=NULL);
+                             ASTNode *form, ASTNode *stmts, ASTNode *meta=NULL,
+                             int sourceline_start=-1);
     const char *nodetypename () const { return "function_declaration"; }
     const char *childname (size_t i) const;
     void print (std::ostream &out, int indentlevel=0) const;

src/liboslcomp/oslgram.y

@@ -194,11 +194,12 @@ shader_or_function_declaration
                         f = new ASTfunction_declaration (oslcompiler,
                                                          typespec_stack.top(),
                                                          ustring($2), $7 /*formals*/,
-                                                         $11 /*statements*/);
+                                                         $11 /*statements*/,
+                                                         NULL /*metadata*/,
+                                                         @2.first_line);
                         oslcompiler->remember_function_decl (f);
                         f->add_meta (concat($4, $10));
                         $$ = f;
-                        $$->sourceline (@2.first_line);
                         typespec_stack.pop ();
                     } else {
                         // Shader declaration

src/shaders/color2.h

@@ -181,18 +181,6 @@ color2 clamp(color2 c, float minval, float maxval)
     return clamp(c, color2(minval, minval), color2(maxval, maxval));
 }
 
-color2 pow(color2 base, color2 power)
-{
-    return color2(pow(base.r, power.r),
-                  pow(base.a, power.a));
-}
-
-color2 pow(color2 base, float power)
-{
-    return color2(pow(base.r, power),
-                  pow(base.a, power));
-}
-
 color2 max(color2 a, color2 b)
 {
     return color2(max(a.r, b.r),

src/shaders/stdosl.h

@@ -60,20 +60,20 @@
     color  name (color x) BUILTIN;              \
     float  name (float x) BUILTIN;
 
+// Declare name (T,T) for T in {triples,float}
 #define PERCOMP2(name)                          \
     normal name (normal x, normal y) BUILTIN;   \
     vector name (vector x, vector y) BUILTIN;   \
     point  name (point x, point y) BUILTIN;     \
     color  name (color x, color y) BUILTIN;     \
     float  name (float x, float y) BUILTIN;
 
+// Declare name(T,float) for T in {triples}
 #define PERCOMP2F(name)                         \
     normal name (normal x, float y) BUILTIN;    \
     vector name (vector x, float y) BUILTIN;    \
     point  name (point x, float y) BUILTIN;     \
-    color  name (color x, float y) BUILTIN;     \
-    float  name (float x, float y) BUILTIN;
-
+    color  name (color x, float y) BUILTIN;
 
 // Basic math
 normal degrees (normal x) { return x*(180.0/M_PI); }

testsuite/oslc-err-funcredef/ref/out.txt

@@ -0,0 +1,2 @@
+test.osl:1: warning: Function 'point abs (point)' declared twice in the same scope
+Compiled test.osl -> test.oso

testsuite/oslc-err-funcredef/run.py

@@ -0,0 +1,5 @@
+#!/usr/bin/env python
+
+# command = oslc("test.osl")
+# don't even need that -- it's automatic
+failureok = 1     # this test is expected to have oslc errors

testsuite/oslc-err-funcredef/test.osl

@@ -0,0 +1,12 @@
+point abs (point p)
+{
+    printf ("Alternate abs!\n");
+    return point (abs(p[0]), abs(p[1]), abs(p[2]));
+}
+
+
+shader test()
+{
+    printf ("abs(P)= %g\n", abs(P));
+}
+