From c588b48b5489cde080add2e807a00fd3b6789537 Mon Sep 17 00:00:00 2001
From: Michael Desa <mjdesa@gmail.com>
Date: Tue, 1 Aug 2017 16:48:56 -0400
Subject: [PATCH] Fix unhelpful error message for node evaluators

---
 CHANGELOG.md                         |  1 +
 tick/ast/node.go                     |  1 +
 tick/stateful/eval_binary_node.go    |  5 +++
 tick/stateful/eval_bool_node.go      |  5 +++
 tick/stateful/eval_duration_node.go  |  5 +++
 tick/stateful/eval_float_node.go     |  5 +++
 tick/stateful/eval_function_node.go  | 35 ++++++++++++++++--
 tick/stateful/eval_int_node.go       |  5 +++
 tick/stateful/eval_lambda_node.go    |  4 +++
 tick/stateful/eval_reference_node.go |  4 +++
 tick/stateful/eval_regex_node.go     |  4 +++
 tick/stateful/eval_string_node.go    |  4 +++
 tick/stateful/eval_unary_node.go     |  6 ++++
 tick/stateful/functions.go           | 54 ++++++++++++++++++++++++++--
 tick/stateful/node_evaluator.go      |  1 +
 tick/stateful/scope.go               |  8 +++++
 16 files changed, 143 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 437dc4395..315d26089 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -16,6 +16,7 @@
 - [#1400](https://github.com/influxdata/kapacitor/issues/1400): Allow for `.yml` file extensions in `define-topic-handler`
 - [#1402](https://github.com/influxdata/kapacitor/pull/1402): Fix http server error logging.
 - [#1500](https://github.com/influxdata/kapacitor/pull/1500): Fix bugs with stopping running UDF agent.
+- [#1470](https://github.com/influxdata/kapacitor/pull/1470): Fix error messages for missing fields which are arguments to functions are not clear
 
 ## v1.3.1 [2017-06-02]
 
diff --git a/tick/ast/node.go b/tick/ast/node.go
index 61b0cb1eb..a376f9e9f 100644
--- a/tick/ast/node.go
+++ b/tick/ast/node.go
@@ -236,6 +236,7 @@ func newBool(p position, text string, c *CommentNode) (*BoolNode, error) {
 func (n *BoolNode) String() string {
 	return fmt.Sprintf("BoolNode@%v{%v}%v", n.position, n.Bool, n.Comment)
 }
+
 func (n *BoolNode) Format(buf *bytes.Buffer, indent string, onNewLine bool) {
 	if n.Comment != nil {
 		n.Comment.Format(buf, indent, onNewLine)
diff --git a/tick/stateful/eval_binary_node.go b/tick/stateful/eval_binary_node.go
index 489dfbde4..ee0a0c7c2 100644
--- a/tick/stateful/eval_binary_node.go
+++ b/tick/stateful/eval_binary_node.go
@@ -117,6 +117,11 @@ func NewEvalBinaryNode(node *ast.BinaryNode) (*EvalBinaryNode, error) {
 	return b, nil
 }
 
+func (n *EvalBinaryNode) String() string {
+
+	return fmt.Sprintf("%s %s %s", n.leftEvaluator, n.operator, n.rightEvaluator)
+}
+
 func (n *EvalBinaryNode) Type(scope ReadOnlyScope) (ast.ValueType, error) {
 	if n.constReturnType == ast.InvalidType {
 		var err error
diff --git a/tick/stateful/eval_bool_node.go b/tick/stateful/eval_bool_node.go
index f60eb788a..bc93ab3d0 100644
--- a/tick/stateful/eval_bool_node.go
+++ b/tick/stateful/eval_bool_node.go
@@ -1,6 +1,7 @@
 package stateful
 
 import (
+	"fmt"
 	"regexp"
 	"time"
 
@@ -11,6 +12,10 @@ type EvalBoolNode struct {
 	Node *ast.BoolNode
 }
 
+func (n *EvalBoolNode) String() string {
+	return fmt.Sprintf("%v", n.Node.Bool)
+}
+
 func (n *EvalBoolNode) Type(scope ReadOnlyScope) (ast.ValueType, error) {
 	return ast.TBool, nil
 }
diff --git a/tick/stateful/eval_duration_node.go b/tick/stateful/eval_duration_node.go
index cbd45adf7..808a6ecf6 100644
--- a/tick/stateful/eval_duration_node.go
+++ b/tick/stateful/eval_duration_node.go
@@ -1,6 +1,7 @@
 package stateful
 
 import (
+	"fmt"
 	"regexp"
 	"time"
 
@@ -11,6 +12,10 @@ type EvalDurationNode struct {
 	Duration time.Duration
 }
 
+func (n *EvalDurationNode) String() string {
+	return fmt.Sprintf("%v", n.Duration)
+}
+
 func (n *EvalDurationNode) Type(scope ReadOnlyScope) (ast.ValueType, error) {
 	return ast.TDuration, nil
 }
diff --git a/tick/stateful/eval_float_node.go b/tick/stateful/eval_float_node.go
index 8e7098021..e45d7fe54 100644
--- a/tick/stateful/eval_float_node.go
+++ b/tick/stateful/eval_float_node.go
@@ -1,6 +1,7 @@
 package stateful
 
 import (
+	"fmt"
 	"regexp"
 	"time"
 
@@ -11,6 +12,10 @@ type EvalFloatNode struct {
 	Float64 float64
 }
 
+func (n *EvalFloatNode) String() string {
+	return fmt.Sprintf("%v", n.Float64)
+}
+
 func (n *EvalFloatNode) Type(scope ReadOnlyScope) (ast.ValueType, error) {
 	return ast.TFloat, nil
 }
diff --git a/tick/stateful/eval_function_node.go b/tick/stateful/eval_function_node.go
index 40471ee8a..c702c8447 100644
--- a/tick/stateful/eval_function_node.go
+++ b/tick/stateful/eval_function_node.go
@@ -7,6 +7,7 @@ import (
 	"time"
 
 	"github.com/influxdata/kapacitor/tick/ast"
+	"github.com/pkg/errors"
 )
 
 type EvalFunctionNode struct {
@@ -32,6 +33,15 @@ func NewEvalFunctionNode(funcNode *ast.FunctionNode) (*EvalFunctionNode, error)
 	return evalFuncNode, nil
 }
 
+func (n *EvalFunctionNode) String() string {
+	args := []string{}
+	for _, argEvaluator := range n.argsEvaluators {
+		args = append(args, fmt.Sprintf("%s", argEvaluator))
+	}
+
+	return n.funcName + "(" + strings.Join(args, ", ") + ")"
+}
+
 func (n *EvalFunctionNode) Type(scope ReadOnlyScope) (ast.ValueType, error) {
 	f := lookupFunc(n.funcName, builtinFuncs, scope)
 	if f == nil {
@@ -49,12 +59,33 @@ func (n *EvalFunctionNode) Type(scope ReadOnlyScope) (ast.ValueType, error) {
 	}
 
 	if gotLen, expLen := len(n.argsEvaluators), len(domain); gotLen > expLen {
-		return ast.InvalidType, ErrWrongFuncSignature{Name: n.funcName, DomainProvided: domain, Func: f}
+		err := ErrWrongFuncSignature{Name: n.funcName, DomainProvided: domain, Func: f}
+		return ast.InvalidType, errors.Wrapf(err, "too many arguments provided")
 	}
 
 	retType, ok := signature[domain]
 	if !ok {
-		return ast.InvalidType, ErrWrongFuncSignature{Name: n.funcName, DomainProvided: domain, Func: f}
+		args := []string{}
+		missing := []string{}
+		for i, argEvaluator := range n.argsEvaluators {
+			t, err := argEvaluator.Type(scope)
+			if err != nil {
+				// This should never happen
+				return ast.InvalidType, fmt.Errorf("Failed to handle %v argument: %v", i+1, err)
+			}
+
+			if t == ast.TMissing {
+				missing = append(missing, fmt.Sprintf("%s", argEvaluator))
+			}
+
+			args = append(args, fmt.Sprintf("%s", argEvaluator))
+		}
+
+		if len(missing) > 0 {
+			return ast.InvalidType, ErrMissingType{Name: n.funcName, Args: missing, Scope: scope.References()}
+		}
+
+		return ast.InvalidType, ErrWrongFuncSignature{Name: n.funcName, ArgLiterals: args, DomainProvided: domain, Func: f}
 	}
 
 	return retType, nil
diff --git a/tick/stateful/eval_int_node.go b/tick/stateful/eval_int_node.go
index b2816e2ec..d99857711 100644
--- a/tick/stateful/eval_int_node.go
+++ b/tick/stateful/eval_int_node.go
@@ -1,6 +1,7 @@
 package stateful
 
 import (
+	"fmt"
 	"regexp"
 	"time"
 
@@ -11,6 +12,10 @@ type EvalIntNode struct {
 	Int64 int64
 }
 
+func (n *EvalIntNode) String() string {
+	return fmt.Sprintf("%v", n.Int64)
+}
+
 func (n *EvalIntNode) Type(scope ReadOnlyScope) (ast.ValueType, error) {
 	return ast.TInt, nil
 }
diff --git a/tick/stateful/eval_lambda_node.go b/tick/stateful/eval_lambda_node.go
index 20a62a56e..533eb14a5 100644
--- a/tick/stateful/eval_lambda_node.go
+++ b/tick/stateful/eval_lambda_node.go
@@ -28,6 +28,10 @@ func NewEvalLambdaNode(lambda *ast.LambdaNode) (*EvalLambdaNode, error) {
 	}, nil
 }
 
+func (n *EvalLambdaNode) String() string {
+	return fmt.Sprintf("%s", n.nodeEvaluator)
+}
+
 func (n *EvalLambdaNode) Type(scope ReadOnlyScope) (ast.ValueType, error) {
 	if n.constReturnType == ast.InvalidType {
 		// We are dynamic and we need to figure out our type
diff --git a/tick/stateful/eval_reference_node.go b/tick/stateful/eval_reference_node.go
index 0e6e9f44c..115b794e3 100644
--- a/tick/stateful/eval_reference_node.go
+++ b/tick/stateful/eval_reference_node.go
@@ -25,6 +25,10 @@ func (n *EvalReferenceNode) getReferenceValue(scope *Scope) (interface{}, error)
 	return value, nil
 }
 
+func (n *EvalReferenceNode) String() string {
+	return "\"" + n.Node.Reference + "\""
+}
+
 func (n *EvalReferenceNode) Type(scope ReadOnlyScope) (ast.ValueType, error) {
 	value, err := n.getReferenceValue(scope.(*Scope))
 	if err != nil {
diff --git a/tick/stateful/eval_regex_node.go b/tick/stateful/eval_regex_node.go
index 9dce3e397..558a0bb4c 100644
--- a/tick/stateful/eval_regex_node.go
+++ b/tick/stateful/eval_regex_node.go
@@ -11,6 +11,10 @@ type EvalRegexNode struct {
 	Node *ast.RegexNode
 }
 
+func (n *EvalRegexNode) String() string {
+	return n.Node.Literal
+}
+
 func (n *EvalRegexNode) Type(scope ReadOnlyScope) (ast.ValueType, error) {
 	return ast.TRegex, nil
 }
diff --git a/tick/stateful/eval_string_node.go b/tick/stateful/eval_string_node.go
index 4dc24ec26..35a8bfd98 100644
--- a/tick/stateful/eval_string_node.go
+++ b/tick/stateful/eval_string_node.go
@@ -11,6 +11,10 @@ type EvalStringNode struct {
 	Node *ast.StringNode
 }
 
+func (n *EvalStringNode) String() string {
+	return n.Node.Literal
+}
+
 func (n *EvalStringNode) Type(scope ReadOnlyScope) (ast.ValueType, error) {
 	return ast.TString, nil
 }
diff --git a/tick/stateful/eval_unary_node.go b/tick/stateful/eval_unary_node.go
index 2e2bac3e4..8c231332c 100644
--- a/tick/stateful/eval_unary_node.go
+++ b/tick/stateful/eval_unary_node.go
@@ -9,6 +9,7 @@ import (
 )
 
 type EvalUnaryNode struct {
+	operator        ast.TokenType
 	nodeEvaluator   NodeEvaluator
 	constReturnType ast.ValueType
 }
@@ -24,6 +25,7 @@ func NewEvalUnaryNode(unaryNode *ast.UnaryNode) (*EvalUnaryNode, error) {
 	}
 
 	return &EvalUnaryNode{
+		operator:        unaryNode.Operator,
 		nodeEvaluator:   nodeEvaluator,
 		constReturnType: getConstantNodeType(unaryNode),
 	}, nil
@@ -33,6 +35,10 @@ func isValidUnaryOperator(operator ast.TokenType) bool {
 	return operator == ast.TokenNot || operator == ast.TokenMinus
 }
 
+func (n *EvalUnaryNode) String() string {
+	return fmt.Sprintf("%s%s", n.operator, n.nodeEvaluator)
+}
+
 func (n *EvalUnaryNode) Type(scope ReadOnlyScope) (ast.ValueType, error) {
 	if n.constReturnType == ast.InvalidType {
 		// We are dynamic and we need to figure out our type
diff --git a/tick/stateful/functions.go b/tick/stateful/functions.go
index 066f5288c..a1cf3b6de 100644
--- a/tick/stateful/functions.go
+++ b/tick/stateful/functions.go
@@ -23,15 +23,65 @@ const (
 	maxArgs = 4
 )
 
+type ErrMissingType struct {
+	Name  string
+	Args  []string
+	Scope []string
+}
+
+func (e ErrMissingType) Error() string {
+	s := "Cannot call function \"%s\" argument %s is missing, values in scope are [%s]"
+	if len(e.Args) > 1 {
+		s = "Cannot call function \"%s\" arguments %s are missing, values in scope are [%s]"
+	}
+
+	// remove missing values from scope
+	for _, a := range e.Args {
+		e.Scope = removeElement(e.Scope, a)
+	}
+
+	return fmt.Sprintf(s, e.Name, strings.Join(e.Args, ", "), strings.Join(e.Scope, ", "))
+}
+
+func removeElement(xs []string, el string) []string {
+	for i, x := range xs {
+		if x == el {
+			xs = append(xs[:i], xs[i+1:]...)
+			break
+		}
+	}
+	return xs
+}
+
 type ErrWrongFuncSignature struct {
 	Name           string
 	DomainProvided Domain
+	ArgLiterals    []string
 	Func           Func
 }
 
 func (e ErrWrongFuncSignature) Error() string {
-	return fmt.Sprintf("Cannot call function \"%s\" with args signature %s, available signatures are %s.",
-		e.Name, e.DomainProvided, FuncDomains(e.Func))
+	var argStringer fmt.Stringer = &argDomain{args: e.ArgLiterals, domain: e.DomainProvided}
+	if e.ArgLiterals == nil {
+		argStringer = e.DomainProvided
+	}
+	return fmt.Sprintf("Cannot call function \"%s\" with args %s, available signatures are %s.",
+		e.Name, argStringer, FuncDomains(e.Func))
+}
+
+type argDomain struct {
+	args   []string
+	domain Domain
+}
+
+func (a *argDomain) String() string {
+	input := []string{}
+	for j, el := range a.args {
+		t := a.domain[j]
+		input = append(input, fmt.Sprintf("%s: %s", el, t))
+	}
+
+	return "(" + strings.Join(input, ",") + ")"
 }
 
 var ErrNotFloat = errors.New("value is not a float")
diff --git a/tick/stateful/node_evaluator.go b/tick/stateful/node_evaluator.go
index 7a15663ce..d0eb2a868 100644
--- a/tick/stateful/node_evaluator.go
+++ b/tick/stateful/node_evaluator.go
@@ -22,6 +22,7 @@ func (e ErrTypeGuardFailed) Error() string {
 
 type ReadOnlyScope interface {
 	Get(name string) (interface{}, error)
+	References() []string
 	DynamicFunc(name string) *DynamicFunc
 }
 
diff --git a/tick/stateful/scope.go b/tick/stateful/scope.go
index 7a7ffee78..f47a9ffee 100644
--- a/tick/stateful/scope.go
+++ b/tick/stateful/scope.go
@@ -44,6 +44,14 @@ func NewScope() *Scope {
 	}
 }
 
+func (s *Scope) References() []string {
+	ks := []string{}
+	for k := range s.variables {
+		ks = append(ks, "\""+k+"\"")
+	}
+	return ks
+}
+
 // Set defines a name -> value pairing in the scope.
 func (s *Scope) Set(name string, value interface{}) {
 	s.variables[name] = value