From 062ff656830f89bd3bca5b39a86c4d41b535a7bf Mon Sep 17 00:00:00 2001
From: Maria Matejka <mq@ucw.cz>
Date: Thu, 15 Jun 2023 13:25:40 +0200
Subject: [PATCH] Filter: functions can and should have typed return values

---
 doc/bird.sgml    | 14 ++++++++----
 filter/config.Y  | 59 ++++++++++++++++++++++++++++++++++++------------
 filter/f-inst.c  |  2 +-
 filter/f-inst.h  |  1 +
 filter/test.conf | 26 ++++++++++-----------
 5 files changed, 68 insertions(+), 34 deletions(-)

diff --git a/doc/bird.sgml b/doc/bird.sgml
index dc1d2285..46e34c04 100644
--- a/doc/bird.sgml
+++ b/doc/bird.sgml
@@ -539,7 +539,7 @@ include "tablename.conf";;
 	Define a filter. You can learn more about filters in the following
 	chapter.
 
-	<tag><label id="opt-function">function <m/name/ (<m/parameters/) <m/local variables/ { <m/commands/ }</tag>
+	<tag><label id="opt-function">function <m/type/ <m/name/ (<m/parameters/) <m/local variables/ { <m/commands/ }</tag>
 	Define a function. You can learn more about functions in the following chapter.
 
 	<tag><label id="opt-protocol">protocol rip|ospf|bgp|<m/.../ [<m/name/ [from <m/name2/]] { <m>protocol options</m> }</tag>
@@ -1296,18 +1296,22 @@ block of code conditional.
 
 <p>BIRD supports functions, so that you don't have to repeat the same blocks of
 code over and over. Functions can have zero or more parameters and they can have
-local variables. Recursion is not allowed. Function definitions look like this:
+local variables. You should always specify the function return type and always
+return it. No-return functions and multiple-type returning functions are deprecated.
+Direct recursion is possible. Function definitions look like this:
 
 <code>
-function name ()
+function int name ()
 {
 	int local_variable;
 	int another_variable = 5;
+	return 42;
 }
 
-function with_parameters (int parameter)
+function pair with_parameters (int parameter)
 {
 	print parameter;
+	return (1, 2);
 }
 </code>
 
@@ -1321,7 +1325,7 @@ may return values using the <cf>return <m/[expr]/</cf> command. Returning a
 value exits from current function (this is similar to C).
 
 <p>Filters are defined in a way similar to functions except they cannot have
-explicit parameters. They get a route table entry as an implicit parameter, it
+explicit parameters and cannot return. They get a route table entry as an implicit parameter, it
 is also passed automatically to any functions called. The filter must terminate
 with either <cf/accept/ or <cf/reject/ statement. If there is a runtime error in
 filter, the route is rejected.
diff --git a/filter/config.Y b/filter/config.Y
index b72e5ea7..6a39ef0c 100644
--- a/filter/config.Y
+++ b/filter/config.Y
@@ -19,6 +19,8 @@ static inline u32 pair(u32 a, u32 b) { return (a << 16) | b; }
 static inline u32 pair_a(u32 p) { return p >> 16; }
 static inline u32 pair_b(u32 p) { return p & 0xFFFF; }
 
+static struct symbol *this_function;
+
 static struct f_method_scope {
   struct f_inst *object;
   struct sym_scope scope;
@@ -374,7 +376,7 @@ CF_METHODS(IS_V4, TYPE, IP, RD, LEN, MAXLEN, ASN, SRC, DST, MASK,
 %type <f> filter where_filter
 %type <fl> filter_body function_body
 %type <flv> lvalue
-%type <i> type function_vars
+%type <i> type maybe_type function_vars
 %type <fa> function_argsn function_args
 %type <ecs> ec_kind
 %type <fret> break_command
@@ -390,8 +392,11 @@ CF_GRAMMAR
 
 conf: filter_def ;
 filter_def:
-   FILTER symbol { $2 = cf_define_symbol(new_config, $2, SYM_FILTER, filter, NULL); cf_push_scope( new_config, $2 ); }
-     filter_body {
+   FILTER symbol {
+     $2 = cf_define_symbol(new_config, $2, SYM_FILTER, filter, NULL);
+     cf_push_scope( new_config, $2 );
+     this_function = NULL;
+   } filter_body {
      struct filter *f = cfg_alloc(sizeof(struct filter));
      *f = (struct filter) { .sym = $2, .root = $4 };
      $2->filter = f;
@@ -511,7 +516,10 @@ filter:
      cf_assert_symbol($1, SYM_FILTER);
      $$ = $1->filter;
    }
- | { cf_push_scope(new_config, NULL); } filter_body {
+ | {
+     cf_push_scope(new_config, NULL);
+     this_function = NULL;
+   } filter_body {
      struct filter *f = cfg_alloc(sizeof(struct filter));
      *f = (struct filter) { .root = $2 };
      $$ = f;
@@ -535,29 +543,38 @@ function_body:
  ;
 
 conf: function_def ;
+maybe_type:
+   /* EMPTY */ { $$ = T_VOID; }
+ | type { $$ = $1; }
+ ;
+
 function_def:
-   FUNCTION symbol {
-     DBG( "Beginning of function %s\n", $2->name );
-     $2 = cf_define_symbol(new_config, $2, SYM_FUNCTION, function, NULL);
-     cf_push_scope(new_config, $2);
+   FUNCTION maybe_type symbol {
+     DBG( "Beginning of function %s\n", $3->name );
+     this_function = cf_define_symbol(new_config, $3, SYM_FUNCTION, function, NULL);
+/*   if ($2 == T_VOID) log(L_WARN "Support for functions without explicit return type will be removed soon" ); */
+     cf_push_scope(new_config, this_function);
    } function_args {
      /* Make dummy f_line for storing function prototype */
      struct f_line *dummy = cfg_allocz(sizeof(struct f_line));
-     $2->function = dummy;
+     this_function->function = dummy;
+
+     dummy->return_type = $2;
 
      /* Revert the args */
-     while ($4) {
-       struct f_arg *tmp = $4;
-       $4 = $4->next;
+     while ($5) {
+       struct f_arg *tmp = $5;
+       $5 = $5->next;
 
        tmp->next = dummy->arg_list;
        dummy->arg_list = tmp;
        dummy->args++;
      }
    } function_body {
-     $6->args = $2->function->args;
-     $6->arg_list = $2->function->arg_list;
-     $2->function = $6;
+     $7->args = this_function->function->args;
+     $7->arg_list = this_function->function->arg_list;
+     $7->return_type = this_function->function->return_type;
+     $3->function = $7;
      cf_pop_scope(new_config);
    }
  ;
@@ -998,6 +1015,18 @@ cmd:
    }
  | RETURN term ';' {
      DBG( "Ook, we'll return the value\n" );
+     if (!this_function)
+       cf_error("Can't return from a non-function, use accept or reject instead.");
+     if (this_function->function->return_type == T_VOID)
+     {
+       if ($2->type != T_VOID)
+	 log(L_WARN "Inferring function %s return type from its return value: %s", this_function->name, f_type_name($2->type));
+       ((struct f_line *) this_function->function)->return_type = $2->type;
+     }
+     else if (this_function->function->return_type != $2->type)
+       cf_error("Can't return type %s from function %s, expected %s",
+		f_type_name($2->type), this_function->name, f_type_name(this_function->function->return_type));
+
      $$ = f_new_inst(FI_RETURN, $2);
    }
  | dynamic_attr '=' term ';' {
diff --git a/filter/f-inst.c b/filter/f-inst.c
index b63fc1f4..9ecd8ffb 100644
--- a/filter/f-inst.c
+++ b/filter/f-inst.c
@@ -1242,7 +1242,7 @@
     SYMBOL;
 
     /* Fake result type declaration */
-    RESULT_TYPE(T_VOID);
+    RESULT_TYPE(sym->function->return_type);
 
     FID_NEW_BODY()
     ASSERT(sym->class == SYM_FUNCTION);
diff --git a/filter/f-inst.h b/filter/f-inst.h
index 3912df08..0913ace6 100644
--- a/filter/f-inst.h
+++ b/filter/f-inst.h
@@ -48,6 +48,7 @@ struct f_line {
   u8 args;				/* Function: Args required */
   u8 vars;
   u8 results;				/* Results left on stack: cmd -> 0, term -> 1 */
+  u8 return_type;			/* Type which the function returns */
   struct f_arg *arg_list;
   struct f_line_item items[0];		/* The items themselves */
 };
diff --git a/filter/test.conf b/filter/test.conf
index 6d786034..33389a17 100644
--- a/filter/test.conf
+++ b/filter/test.conf
@@ -21,17 +21,17 @@ attribute lclist mylclist;
 define one = 1;
 define ten = 10;
 
-function onef(int a)
+function int onef(int a)
 {
 	return 1;
 }
 
-function twof(int a)
+function int twof(int a)
 {
 	return 2;
 }
 
-function oneg(int a)
+function int oneg(int a)
 {
 	return 1;
 }
@@ -274,7 +274,7 @@ bt_test_suite(t_bytestring, "Testing bytestrings");
  * 	-------------
  */
 
-function 'mkpair-a'(int a)
+function pair 'mkpair-a'(int a)
 {
 	return (1, a);
 }
@@ -749,7 +749,7 @@ bt_test_suite(t_flowspec, "Testing flowspec routes");
  * 	-------------
  */
 
-function mkpath(int a; int b)
+function bgpmask mkpath(int a; int b)
 {
 	return [= a b 3 2 1 =];
 }
@@ -1133,7 +1133,7 @@ bt_test_suite(t_ec_set, "Testing sets of extended communities");
  * 	-------------------------
  */
 
-function mktrip(int a)
+function lc mktrip(int a)
 {
 	return (a, 2*a, 3*a);
 }
@@ -1363,7 +1363,7 @@ bt_test_suite(t_define, "Testing defined() function");
  *	 -------------------------
  */
 
-function callme(int arg1; int arg2)
+function int callme(int arg1; int arg2)
 int i;
 {
 	case arg1 {
@@ -1374,12 +1374,12 @@ int i;
 	return 0;
 }
 
-function callmeagain(int a; int b; int c)
+function int callmeagain(int a; int b; int c)
 {
 	return a + b + c;
 }
 
-function fifteen()
+function int fifteen()
 {
 	return 15;
 }
@@ -1412,28 +1412,28 @@ function local_vars(int j)
 	bt_assert(j = 35 && k = 20 && m = 100);
 }
 
-function factorial(int x)
+function int factorial(int x)
 {
 	if x = 0 then return 0;
 	if x = 1 then return 1;
 	else return x * factorial(x - 1);
 }
 
-function fibonacci(int x)
+function int fibonacci(int x)
 {
 	if x = 0 then return 0;
 	if x = 1 then return 1;
 	else return fibonacci(x - 1) + fibonacci(x - 2);
 }
 
-function hanoi_init(int a; int b)
+function bgppath hanoi_init(int a; int b)
 {
 	if b = 0
 	then return +empty+;
 	else return prepend(hanoi_init(a + 1, b - 1), a);
 }
 
-function hanoi_solve(int n; bgppath h_src; bgppath h_dst; bgppath h_aux; bool x; bool y)
+function bgppath hanoi_solve(int n; bgppath h_src; bgppath h_dst; bgppath h_aux; bool x; bool y)
 {
 	# x -> return src or dst
 	# y -> print state