cat_sep: Make fully vararg and do explicit runtime type checks

Using positional and vararg arguments for BIFs, it's not possible to do
proper runtime type checking on them as discussed in #2425. The bifcl produced
code unconditionally attempts to convert the positional arguments to StringVals,
but nothing ever type checks them. Instead of improving the vararg support in
Zeek script and bifcl, align cat_sep() with fmt() in making it fully vararg
and do implement type checks by hand.

With this change, passing wrong types for the separator and default argument
isn't a fatal error anymore and the error messages are also more descriptive.

It's a bit of a crutch working around varargs limitations.

Fixes #2425

NEWS

@@ -107,6 +107,9 @@ Changed Functionality
 - The parameter given to ``enum_names()`` can now be a string naming the
   enum type, rather than the type itself.
 
+- Passing non-string ``sep`` and ``def`` arguments to ``cat_sep()`` isn't a
+  fatal error anymore. More descriptive error messages are produced, too.
+
 Deprecated Functionality
 ------------------------
 

src/zeek.bif

@@ -1540,29 +1540,50 @@ function cat%(...%): string
 ## Concatenates all arguments, with a separator placed between each one. This
 ## function is similar to :zeek:id:`cat`, but places a separator between each
 ## given argument. If any of the variable arguments is an empty string it is
-## replaced by a given default string instead.
+## replaced by the given default string instead. This function takes at least
-##
+## two arguments. The first argument is the separator *sep* placed between
-## sep: The separator to place between each argument.
+## each argument, the second argument is the default string *def* used when
-##
+## an argument is the empty string.
-## def: The default string to use when an argument is the empty string.
 ##
 ## Returns: A concatenation of all arguments with *sep* between each one and
 ##          empty strings replaced with *def*.
 ##
 ## .. zeek:see:: cat string_cat
-function cat_sep%(sep: string, def: string, ...%): string
+function cat_sep%(...%): string
 	%{
+	if ( @ARGC@ < 2 )
+		{
+		zeek::emit_builtin_error(zeek::util::fmt("cat_sep() takes at least 2 arguments, got %ld",
+		                                         @ARGC@));
+		return nullptr;
+		}
+
+	zeek::ValPtr sep_arg = @ARG@[0];
+	zeek::ValPtr def_arg = @ARG@[1];
+
+	if ( sep_arg->GetType()->Tag() != zeek::TYPE_STRING )
+		{
+		zeek::emit_builtin_error(zeek::util::fmt("expected type string for separator, got %s",
+		                                         zeek::type_name(sep_arg->GetType()->Tag())));
+		return nullptr;
+		}
+
+	if ( def_arg->GetType()->Tag() != zeek::TYPE_STRING )
+		{
+		zeek::emit_builtin_error(zeek::util::fmt("expected type string for default, got %s",
+		                                         zeek::type_name(def_arg->GetType()->Tag())));
+		return nullptr;
+		}
+
+	zeek::StringVal* sep = sep_arg->AsStringVal();
+	zeek::StringVal* def = def_arg->AsStringVal();
+
 	zeek::ODesc d;
 	d.SetStyle(RAW_STYLE);
-
 	int pre_size = 0;
 
-	for ( auto i = 0u; i < @ARG@.size(); ++i )
+	for ( auto i = 2u; i < @ARGC@; ++i )
 		{
-		// Skip named parameters.
-		if ( i < 2 )
-			continue;
-
 		if ( i > 2 )
 			d.Add(sep->CheckString(), 0);
 

testing/btest/Baseline/bifs.cat/out

@@ -4,4 +4,5 @@ foo3T
 3T
 foo|3|T
 
+<empty>
 <empty>|3|T

testing/btest/Baseline/bifs.cat_sep_errors-2/.stderr

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/cat_sep_errors.zeek, line 3: cat_sep() takes at least 2 arguments, got 1 (cat_sep(sep))

testing/btest/Baseline/bifs.cat_sep_errors-3/.stderr

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/cat_sep_errors.zeek, line 4: expected type string for default, got count (cat_sep(sep, 1))

testing/btest/Baseline/bifs.cat_sep_errors-4/.stderr

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/cat_sep_errors.zeek, line 4: expected type string for separator, got count (cat_sep(1, default))

testing/btest/Baseline/bifs.cat_sep_errors-5/.stderr

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/cat_sep_errors.zeek, line 3: expected type string for separator, got record (cat_sep([$a=1], default))

testing/btest/Baseline/bifs.cat_sep_errors/.stderr

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/cat_sep_errors.zeek, line 6: cat_sep() takes at least 2 arguments, got 0 (cat_sep())

testing/btest/bifs/cat.zeek

@@ -17,6 +17,8 @@ event zeek_init()
 	print cat_sep("|", "<empty>", a, b, c);
 
 	print cat_sep("|", "<empty>");
-	
+
+	print cat_sep("|", "<empty>", "");
+
 	print cat_sep("|", "<empty>", "", b, c);
 	}

testing/btest/bifs/cat_sep_errors.zeek

@@ -0,0 +1,33 @@
+# @TEST-DOC: Runtime errors calling cat_sep()
+# @TEST-EXEC: zeek -b %INPUT
+# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff .stderr
+event zeek_init()
+	{
+	print cat_sep();
+	}
+
+@TEST-START-NEXT
+event zeek_init()
+	{
+	print cat_sep("sep");
+	}
+
+@TEST-START-NEXT
+# bad types 1
+event zeek_init()
+	{
+	print cat_sep("sep", 1);
+	}
+
+@TEST-START-NEXT
+# bad types 2
+event zeek_init()
+	{
+	print cat_sep(1, "default");
+	}
+
+@TEST-START-NEXT
+event zeek_init()
+	{
+	print cat_sep([$a=1], "default");
+	}