print more details about type errors

7 files changed, 117 insertions, 22 deletions

diff --git a/eval/src/tests/eval/node_types/node_types_test.cpp b/eval/src/tests/eval/node_types/node_types_test.cpp
index 07bca5b53a6..900e71aa899 100644
--- a/eval/src/tests/eval/node_types/node_types_test.cpp
+++ b/eval/src/tests/eval/node_types/node_types_test.cpp
@@ -29,6 +29,11 @@ void verify(const vespalib::string &type_expr, const vespalib::string &type_spec
         input_types.push_back(ValueType::from_spec(function->param_name(i)));
     }
     NodeTypes types(*function, input_types);
+    if (!types.errors().empty()) {
+        for (const auto &msg: types.errors()) {
+            fprintf(stderr, "type error: %s\n", msg.c_str());
+        }
+    }
     ValueType expected_type = ValueType::from_spec(type_spec);
     ValueType actual_type = types.get_type(function->root());
     EXPECT_EQUAL(expected_type, actual_type);
@@ -236,6 +241,13 @@ TEST("require that tensor create resolves correct type") {
     TEST_DO(verify("tensor(x[3]):{{x:0}:double,{x:1}:error,{x:2}:double}", "error"));
 }
 
+TEST("require that tensor lambda resolves correct type") {
+    TEST_DO(verify("tensor(x[3])(error)", "error"));
+    TEST_DO(verify("tensor(x[3])(double)", "tensor(x[3])"));
+    TEST_DO(verify("tensor<float>(x[3])(double)", "tensor<float>(x[3])"));
+    TEST_DO(verify("tensor(x[3])(tensor(x[2]))", "error"));
+}
+
 TEST("require that tensor peek resolves correct type") {
     TEST_DO(verify("tensor(x[3]){x:1}", "double"));
     TEST_DO(verify("tensor(x[3]){x:double}", "error"));
diff --git a/eval/src/vespa/eval/eval/node_types.cpp b/eval/src/vespa/eval/eval/node_types.cpp
index 37632e35693..afb51b5b605 100644
--- a/eval/src/vespa/eval/eval/node_types.cpp
+++ b/eval/src/vespa/eval/eval/node_types.cpp
@@ -3,6 +3,10 @@
 #include "check_type.h"
 #include "node_traverser.h"
 #include "node_types.h"
+#include <vespa/vespalib/util/stringfmt.h>
+#include <vespa/vespalib/util/classname.h>
+
+using vespalib::make_string_short::fmt;
 
 namespace vespalib::eval {
 namespace nodes {
@@ -13,11 +17,13 @@ class State
 private:
     const std::vector<ValueType>      &_params;
     std::map<const Node *, ValueType> &_type_map;
+    std::vector<vespalib::string>     &_errors;
 
 public:
     State(const std::vector<ValueType> &params,
-          std::map<const Node *, ValueType> &type_map)
-        : _params(params), _type_map(type_map) {}
+          std::map<const Node *, ValueType> &type_map,
+          std::vector<vespalib::string> &errors)
+        : _params(params), _type_map(type_map), _errors(errors) {}
 
     const ValueType &param_type(size_t idx) {
         assert(idx < _params.size());
@@ -33,20 +39,44 @@ public:
         assert(pos != _type_map.end());
         return pos->second;
     }
+    void add_error(const vespalib::string &msg) {
+        _errors.push_back(msg);
+    }
 };
 
 struct TypeResolver : public NodeVisitor, public NodeTraverser {
     State state;
     TypeResolver(const std::vector<ValueType> &params_in,
-                 std::map<const Node *, ValueType> &type_map_out);
+                 std::map<const Node *, ValueType> &type_map_out,
+                 std::vector<vespalib::string> &errors_out);
     ~TypeResolver();
 
     const ValueType &param_type(size_t idx) {
         return state.param_type(idx);
     }
 
-    void bind(const ValueType &type, const Node &node) {
-        state.bind(type, node);
+    void fail(const Node &node, const vespalib::string &msg, bool child_types = true) {
+        auto str = fmt("%s: %s", vespalib::getClassName(node).c_str(), msg.c_str());
+        if (child_types) {
+            str += ", child types: [";
+            for (size_t i = 0; i < node.num_children(); ++i) {
+                if (i > 0) {
+                    str += ", ";
+                }
+                str += state.type(node.get_child(i)).to_spec();
+            }
+            str += "]";
+        }
+        state.add_error(str);
+        state.bind(ValueType::error_type(), node);
+    }
+
+    void bind(const ValueType &type, const Node &node, bool check_error = true) {
+        if (check_error && type.is_error()) {
+            fail(node, "type resolving failed");
+        } else {
+            state.bind(type, node);
+        }
     }
 
     const ValueType &type(const Node &node) {
@@ -65,7 +95,7 @@ struct TypeResolver : public NodeVisitor, public NodeTraverser {
     bool check_error(const Node &node) {
         for (size_t i = 0; i < node.num_children(); ++i) {
             if (type(node.get_child(i)).is_error()) {
-                bind(ValueType::error_type(), node);
+                bind(ValueType::error_type(), node, false);
                 return true;
             }
         }
@@ -87,7 +117,7 @@ struct TypeResolver : public NodeVisitor, public NodeTraverser {
         bind(ValueType::double_type(), node);
     }
     void visit(const Symbol &node) override {
-        bind(param_type(node.id()), node);
+        bind(param_type(node.id()), node, false);
     }
     void visit(const String &node) override {
         bind(ValueType::double_type(), node);
@@ -100,7 +130,7 @@ struct TypeResolver : public NodeVisitor, public NodeTraverser {
                                type(node.false_expr())), node);
     }
     void visit(const Error &node) override {
-        bind(ValueType::error_type(), node);
+        bind(ValueType::error_type(), node, false);
     }
     void visit(const TensorMap &node) override { resolve_op1(node); }
     void visit(const TensorJoin &node) override { resolve_op2(node); }
@@ -109,12 +139,30 @@ struct TypeResolver : public NodeVisitor, public NodeTraverser {
                               type(node.get_child(1))), node);
     }
     void visit(const TensorReduce &node) override {
-        const ValueType &child = type(node.get_child(0));
-        bind(child.reduce(node.dimensions()), node);
+        auto my_type = type(node.get_child(0)).reduce(node.dimensions());
+        if (my_type.is_error()) {
+            auto str = fmt("aggr: %s, dimensions: [",
+                           AggrNames::name_of(node.aggr())->c_str());
+            for (const auto &dimension: node.dimensions()) {
+                str += ",";
+                str += dimension;
+            }
+            str += "]";
+            fail(node, str);
+        } else {
+            bind(my_type, node);
+        }
     }
     void visit(const TensorRename &node) override {
-        const ValueType &child = type(node.get_child(0));
-        bind(child.rename(node.from(), node.to()), node);
+        auto my_type = type(node.get_child(0)).rename(node.from(), node.to());
+        if (my_type.is_error()) {
+            auto str = fmt("%s -> %s",
+                           TensorRename::flatten(node.from()).c_str(),
+                           TensorRename::flatten(node.to()).c_str());
+            fail(node, str);
+        } else {
+            bind(my_type, node);
+        }
     }
     void visit(const TensorConcat &node) override {
         bind(ValueType::concat(type(node.get_child(0)),
@@ -123,7 +171,7 @@ struct TypeResolver : public NodeVisitor, public NodeTraverser {
     void visit(const TensorCreate &node) override {
         for (size_t i = 0; i < node.num_children(); ++i) {
             if (!type(node.get_child(i)).is_double()) {
-                return bind(ValueType::error_type(), node);
+                return fail(node, fmt("non-double child at index %zu", i), false);
             }
         }
         bind(node.type(), node);
@@ -139,7 +187,7 @@ struct TypeResolver : public NodeVisitor, public NodeTraverser {
         }
         NodeTypes lambda_types(node.lambda(), arg_types);
         if (!lambda_types.get_type(node.lambda().root()).is_double()) {
-            return bind(ValueType::error_type(), node);
+            return fail(node, "lambda function produces non-double result", false);
         }
         import(lambda_types);
         bind(node.type(), node);
@@ -151,20 +199,22 @@ struct TypeResolver : public NodeVisitor, public NodeTraverser {
             dimensions.push_back(dim.first);
             if (dim.second.is_expr()) {
                 if (!type(*dim.second.expr).is_double()) {
-                    return bind(ValueType::error_type(), node);
+                    return fail(node, fmt("non-double label expression for dimension %s", dim.first.c_str()));
                 }
             } else {
                 size_t dim_idx = param_type.dimension_index(dim.first);
                 if (dim_idx == ValueType::Dimension::npos) {
-                    return bind(ValueType::error_type(), node);
+                    return fail(node, fmt("dimension not in param: %s", dim.first.c_str()));
                 }
                 const auto &param_dim = param_type.dimensions()[dim_idx];
                 if (param_dim.is_indexed()) {
                     if (!is_number(dim.second.label)) {
-                        return bind(ValueType::error_type(), node);
+                        return fail(node, fmt("non-numeric label for dimension %s: '%s'",
+                                        dim.first.c_str(), dim.second.label.c_str()));
                     }
                     if (as_number(dim.second.label) >= param_dim.size) {
-                        return bind(ValueType::error_type(), node);
+                        return fail(node, fmt("out-of-bounds label for dimension %s: %s",
+                                        dim.first.c_str(), dim.second.label.c_str()));
                     }
                 }
             }
@@ -227,8 +277,9 @@ struct TypeResolver : public NodeVisitor, public NodeTraverser {
 };
 
 TypeResolver::TypeResolver(const std::vector<ValueType> &params_in,
-                           std::map<const Node *, ValueType> &type_map_out)
-    : state(params_in, type_map_out)
+                           std::map<const Node *, ValueType> &type_map_out,
+                           std::vector<vespalib::string> &errors_out)
+    : state(params_in, type_map_out, errors_out)
 {
 }
 
@@ -248,10 +299,12 @@ NodeTypes::NodeTypes(const Function &function, const std::vector<ValueType> &inp
       _type_map()
 {
     assert(input_types.size() == function.num_params());
-    nodes::TypeResolver resolver(input_types, _type_map);
+    nodes::TypeResolver resolver(input_types, _type_map, _errors);
     function.root().traverse(resolver);
 }
 
+NodeTypes::~NodeTypes() = default;
+
 const ValueType &
 NodeTypes::get_type(const nodes::Node &node) const
 {
diff --git a/eval/src/vespa/eval/eval/node_types.h b/eval/src/vespa/eval/eval/node_types.h
index a93f886a2eb..c072915ffb1 100644
--- a/eval/src/vespa/eval/eval/node_types.h
+++ b/eval/src/vespa/eval/eval/node_types.h
@@ -23,9 +23,12 @@ class NodeTypes
 private:
     ValueType _not_found;
     std::map<const nodes::Node*,ValueType> _type_map;
+    std::vector<vespalib::string> _errors;
 public:
     NodeTypes();
     NodeTypes(const Function &function, const std::vector<ValueType> &input_types);
+    ~NodeTypes();
+    const std::vector<vespalib::string> &errors() const { return _errors; }
     const ValueType &get_type(const nodes::Node &node) const;
     template <typename F>
     void each(F &&f) const {
diff --git a/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.cpp b/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.cpp
index 5423e37a755..26c61ba6b3c 100644
--- a/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.cpp
+++ b/searchlib/src/vespa/searchlib/features/rankingexpressionfeature.cpp
@@ -292,6 +292,9 @@ RankingExpressionBlueprint::setup(const fef::IIndexEnvironment &env,
     }
     ValueType root_type = node_types.get_type(rank_function->root());
     if (root_type.is_error()) {
+        for (const auto &type_error: node_types.errors()) {
+            LOG(warning, "type error: %s", type_error.c_str());
+        }
         return fail("rank expression contains type errors: %s\n", script.c_str());
     }
     auto compile_issues = CompiledFunction::detect_issues(*rank_function);
diff --git a/vespalib/src/tests/stringfmt/fmt.cpp b/vespalib/src/tests/stringfmt/fmt.cpp
index f9bd0ae12fb..e941c185e2f 100644
--- a/vespalib/src/tests/stringfmt/fmt.cpp
+++ b/vespalib/src/tests/stringfmt/fmt.cpp
@@ -4,6 +4,7 @@
 #include <vespa/vespalib/testkit/test_kit.h>
 
 using vespalib::make_string;
+using vespalib::make_string_short::fmt;
 
 TEST("test that make_string formats as one can expect.")
 {
@@ -39,4 +40,8 @@ TEST("test that make_string formats as one can expect.")
     EXPECT_TRUE(tst == make_string("%s", p));
 }
 
+TEST("require that short form make string can be used") {
+    EXPECT_EQUAL(fmt("format: %d", 123), vespalib::string("format: 123"));
+}
+
 TEST_MAIN() { TEST_RUN_ALL(); }
diff --git a/vespalib/src/vespa/vespalib/util/stringfmt.cpp b/vespalib/src/vespa/vespalib/util/stringfmt.cpp
index 266fb60714d..3c5ebce355c 100644
--- a/vespalib/src/vespa/vespalib/util/stringfmt.cpp
+++ b/vespalib/src/vespa/vespalib/util/stringfmt.cpp
@@ -50,4 +50,15 @@ vespalib::string make_string(const char *fmt, ...)
     return ret;
 }
 
+namespace make_string_short {
+vespalib::string fmt(const char *format, ...)
+{
+    va_list ap;
+    va_start(ap, format);
+    vespalib::string ret = make_string_va(format, ap);
+    va_end(ap);
+    return ret;
+}
+} // namespace vespalib::make_string_short
+
 } // namespace vespalib
diff --git a/vespalib/src/vespa/vespalib/util/stringfmt.h b/vespalib/src/vespa/vespalib/util/stringfmt.h
index e9ec1c433bb..1009f3d9275 100644
--- a/vespalib/src/vespa/vespalib/util/stringfmt.h
+++ b/vespalib/src/vespa/vespalib/util/stringfmt.h
@@ -16,5 +16,13 @@ extern vespalib::string make_string(const char *fmt, ...)
 #endif
     ;
 
-} // namespace vespalib
+namespace make_string_short {
+extern vespalib::string fmt(const char *format, ...)
+#ifdef __GNUC__
+        // Add printf format checks with gcc
+        __attribute__ ((format (printf,1,2)))
+#endif
+    ;
+} // namespace vespalib::make_string_short
 
+} // namespace vespalib