Hook functions now directly callable instead of w/ "hook" statements.

The return value of the call is an implicit boolean value of T if all
hook handlers ran, or F if one hook handler exited as a result of a
break statement and potentially prevented other handlers from running.

Scripts don't need to declare hooks with an explicit return type of bool
(internally, that's assumed), and any values given to (optional) return
statements in handler definitions are just ignored.

Addresses #918.

doc/scripts/builtins.rst

@@ -505,15 +505,14 @@ The Bro scripting language supports the following built-in types.
     A hook is another flavor of function that shares characteristics of
     both a :bro:type:`function` and a :bro:type:`event`.  They are like
     events in that many handler bodies can be defined for the same hook
-    identifier, they have no return vale, and the order of execution
+    identifier and the order of execution can be enforced with
-    can be enforced with :bro:attr:`&priority`.  They are more like
+    :bro:attr:`&priority`.  They are more like functions in the way they
-    functions in the way they are invoked/called, because, unlike
+    are invoked/called, because, unlike events, their execution is
-    events, their execution is immediate and they do not get scheduled
+    immediate and they do not get scheduled through an event queue.
-    through an event queue.  Also, a unique feature of a hook is that
+    Also, a unique feature of a hook is that a given hook handler body
-    a given hook handler body can short-circuit the execution of
+    can short-circuit the execution of remaining hook handlers simply by
-    remaining hook handlers simply by exiting from the body as a result
+    exiting from the body as a result of a ``break`` statement (as
-    of a ``break`` statement (as opposed to a ``return`` or just
+    opposed to a ``return`` or just reaching the end of the body).
-    reaching the end of the body).
 
     A hook type is declared like::
 
@@ -553,12 +552,12 @@ The Bro scripting language supports the following built-in types.
     a hook type isn't strictly required, when it is provided, the
     argument types must match.
 
-    To invoke immediate execution of all hook handler bodies, a ``hook``
+    To invoke immediate execution of all hook handler bodies, they
-    statement must be used:
+    can be called just like a function:
 
     .. code:: bro
 
-        hook myhook("hi");
+        myhook("hi");
 
     And the output would like like::
 
@@ -568,6 +567,12 @@ The Bro scripting language supports the following built-in types.
     Note how the modification to arguments can be seen by remaining
     hook handlers.
 
+    The return value of a hook call is an implicit :bro:type:`bool`
+    value with ``T`` meaning that all handlers for the hook were
+    executed and ``F`` meaning that only some of the handlers may have
+    executed due to one handler body exiting as a result of a ``break``
+    statement.
+
 Attributes
 ----------
 

src/Expr.cc

@@ -4374,7 +4374,7 @@ bool InExpr::DoUnserialize(UnserialInfo* info)
 	return true;
 	}
 
-CallExpr::CallExpr(Expr* arg_func, ListExpr* arg_args, bool in_hook)
+CallExpr::CallExpr(Expr* arg_func, ListExpr* arg_args)
 : Expr(EXPR_CALL)
 	{
 	func = arg_func;
@@ -4415,13 +4415,8 @@ CallExpr::CallExpr(Expr* arg_func, ListExpr* arg_args, bool in_hook)
 				break;
 
 			case FUNC_FLAVOR_HOOK:
-				// It's fine to not have a yield if it's known that the call
+				Error("hook has no yield type");
-				// is being done from a hook statement.
-				if ( ! in_hook )
-					{
-					Error("hook called in expression, use hook statement instead");
 				SetError();
-					}
 				break;
 
 			default:

src/Expr.h

@@ -959,7 +959,7 @@ protected:
 
 class CallExpr : public Expr {
 public:
-	CallExpr(Expr* func, ListExpr* args, bool in_hook = false);
+	CallExpr(Expr* func, ListExpr* args);
 	~CallExpr();
 
 	Expr* Func() const	{ return func; }

src/Func.cc

@@ -349,16 +349,31 @@ Val* BroFunc::Call(val_list* args, Frame* parent) const
 			break;
 			}
 
-		if ( flow == FLOW_BREAK && Flavor() == FUNC_FLAVOR_HOOK )
+		if ( Flavor() == FUNC_FLAVOR_HOOK )
+			{
+			// Ignore any return values of hook bodies, final return value
+			// depends on whether a body returns as a result of break statement
+			Unref(result);
+			result = 0;
+
+			if ( flow == FLOW_BREAK )
 				{
 				// short-circuit execution of remaining hook handler bodies
+				result = new Val(false, TYPE_BOOL);
 				break;
 				}
 			}
+		}
+
+	if ( Flavor() == FUNC_FLAVOR_HOOK )
+		{
+		if ( ! result )
+			result = new Val(true, TYPE_BOOL);
+		}
 
 	// Warn if the function returns something, but we returned from
 	// the function without an explicit return, or without a value.
-	if ( FType()->YieldType() && FType()->YieldType()->Tag() != TYPE_VOID &&
+	else if ( FType()->YieldType() && FType()->YieldType()->Tag() != TYPE_VOID &&
 	     (flow != FLOW_RETURN /* we fell off the end */ ||
 	      ! result /* explicit return with no result */) &&
 	     ! f->HasDelayed() )

src/SerialTypes.h

@@ -165,7 +165,6 @@ SERIAL_STMT(EVENT_BODY_LIST, 16)
 SERIAL_STMT(INIT_STMT, 17)
 SERIAL_STMT(NULL_STMT, 18)
 SERIAL_STMT(WHEN_STMT, 19)
-SERIAL_STMT(HOOK_STMT, 20)
 
 #define SERIAL_TYPE(name, val) SERIAL_CONST(name, val, BRO_TYPE)
 SERIAL_TYPE(BRO_TYPE, 1)

src/Stmt.cc

@@ -23,7 +23,7 @@ const char* stmt_name(BroStmtTag t)
 		"print", "event", "expr", "if", "when", "switch",
 		"for", "next", "break", "return", "add", "delete",
 		"list", "bodylist",
-		"<init>", "hook",
+		"<init>",
 		"null",
 	};
 
@@ -933,52 +933,6 @@ bool EventStmt::DoUnserialize(UnserialInfo* info)
 	return event_expr != 0;
 	}
 
-HookStmt::HookStmt(CallExpr* arg_e) : ExprStmt(STMT_HOOK, arg_e)
-	{
-	call_expr = arg_e;
-	}
-
-Val* HookStmt::Exec(Frame* f, stmt_flow_type& flow) const
-	{
-	RegisterAccess();
-
-	Val* ret = call_expr->Eval(f);
-	Unref(ret);
-
-	flow = FLOW_NEXT;
-
-	return 0;
-	}
-
-TraversalCode HookStmt::Traverse(TraversalCallback* cb) const
-	{
-	TraversalCode tc = cb->PreStmt(this);
-	HANDLE_TC_STMT_PRE(tc);
-
-	// call expr is stored in base class's "e" field.
-	tc = e->Traverse(cb);
-	HANDLE_TC_STMT_PRE(tc);
-
-	tc = cb->PostStmt(this);
-	HANDLE_TC_STMT_POST(tc);
-	}
-
-IMPLEMENT_SERIAL(HookStmt, SER_HOOK_STMT);
-
-bool HookStmt::DoSerialize(SerialInfo* info) const
-	{
-	DO_SERIALIZE(SER_HOOK_STMT, ExprStmt);
-	return call_expr->Serialize(info);
-	}
-
-bool HookStmt::DoUnserialize(UnserialInfo* info)
-	{
-	DO_UNSERIALIZE(ExprStmt);
-
-	call_expr = (CallExpr*) Expr::Unserialize(info, EXPR_CALL);
-	return call_expr != 0;
-	}
-
 ForStmt::ForStmt(id_list* arg_loop_vars, Expr* loop_expr)
 : ExprStmt(STMT_FOR, loop_expr)
 	{
@@ -1378,7 +1332,10 @@ ReturnStmt::ReturnStmt(Expr* arg_e) : ExprStmt(STMT_RETURN, arg_e)
 		}
 
 	else if ( ! e )
+		{
+		if ( ft->Flavor() != FUNC_FLAVOR_HOOK )
 			Error("return statement needs expression");
+		}
 
 	else
 		(void) check_and_promote_expr(e, yt);
@@ -1990,7 +1947,6 @@ int same_stmt(const Stmt* s1, const Stmt* s2)
 	case STMT_RETURN:
 	case STMT_EXPR:
 	case STMT_EVENT:
-	case STMT_HOOK:
 		{
 		const ExprStmt* e1 = (const ExprStmt*) s1;
 		const ExprStmt* e2 = (const ExprStmt*) s2;

src/Stmt.h

@@ -286,24 +286,6 @@ protected:
 	EventExpr* event_expr;
 };
 
-class HookStmt : public ExprStmt {
-public:
-	HookStmt(CallExpr* e);
-
-	Val* Exec(Frame* f, stmt_flow_type& flow) const;
-
-	TraversalCode Traverse(TraversalCallback* cb) const;
-
-protected:
-	friend class Stmt;
-
-	HookStmt() { call_expr = 0; }
-
-	DECLARE_SERIAL(HookStmt);
-
-	CallExpr* call_expr;
-};
-
 class ForStmt : public ExprStmt {
 public:
 	ForStmt(id_list* loop_vars, Expr* loop_expr);

src/StmtEnums.h

@@ -15,7 +15,7 @@ typedef enum {
 	STMT_RETURN,
 	STMT_ADD, STMT_DELETE,
 	STMT_LIST, STMT_EVENT_BODY_LIST,
-	STMT_INIT, STMT_HOOK,
+	STMT_INIT,
 	STMT_NULL
 #define NUM_STMTS (int(STMT_NULL) + 1)
 } BroStmtTag;

src/Var.cc

@@ -294,12 +294,12 @@ void add_type(ID* id, BroType* t, attr_list* attr, int /* is_event */)
 void begin_func(ID* id, const char* module_name, function_flavor flavor,
 		int is_redef, FuncType* t)
 	{
-	if ( flavor == FUNC_FLAVOR_EVENT || flavor == FUNC_FLAVOR_HOOK )
+	if ( flavor == FUNC_FLAVOR_EVENT )
 		{
 		const BroType* yt = t->YieldType();
 
 		if ( yt && yt->Tag() != TYPE_VOID )
-			id->Error("event/hook cannot yield a value", t);
+			id->Error("event cannot yield a value", t);
 
 		t->ClearYieldType(flavor);
 		}

src/parse.y

@@ -2,7 +2,7 @@
 // See the file "COPYING" in the main distribution directory for copyright.
 %}
 
-%expect 88
+%expect 87
 
 %token TOK_ADD TOK_ADD_TO TOK_ADDR TOK_ANY
 %token TOK_ATENDIF TOK_ATELSE TOK_ATIF TOK_ATIFDEF TOK_ATIFNDEF
@@ -56,7 +56,6 @@
 %type <re> pattern
 %type <expr> expr init anonymous_function
 %type <event_expr> event
-%type <call_expr> hook
 %type <stmt> stmt stmt_list func_body for_head
 %type <type> type opt_type enum_body
 %type <func_type> func_hdr func_params
@@ -212,7 +211,6 @@ static std::list<std::string>* concat_opt_docs (std::list<std::string>* pre,
 	Val* val;
 	RE_Matcher* re;
 	Expr* expr;
-	CallExpr* call_expr;
 	EventExpr* event_expr;
 	Stmt* stmt;
 	ListExpr* list;
@@ -875,7 +873,7 @@ type:
 	|	TOK_HOOK '(' formal_args ')'
 				{
 				set_location(@1, @3);
-				$$ = new FuncType($3, 0, FUNC_FLAVOR_HOOK);
+				$$ = new FuncType($3, base_type(TYPE_BOOL), FUNC_FLAVOR_HOOK);
 				}
 
 	|	TOK_FILE TOK_OF type
@@ -1209,6 +1207,8 @@ func_hdr:
 			}
 	|	TOK_HOOK def_global_id func_params
 			{
+			$3->ClearYieldType(FUNC_FLAVOR_HOOK);
+			$3->SetYieldType(base_type(TYPE_BOOL));
 			begin_func($2, current_module.c_str(),
 				   FUNC_FLAVOR_HOOK, 0, $3);
 			$$ = $3;
@@ -1372,14 +1372,6 @@ stmt:
 			    brofiler.AddStmt($$);
 			}
 
-	|	TOK_HOOK hook ';' opt_no_test
-			{
-			set_location(@1, @4);
-			$$ = new HookStmt($2);
-			if ( ! $4 )
-			    brofiler.AddStmt($$);
-			}
-
 	|	TOK_IF '(' expr ')' stmt
 			{
 			set_location(@1, @4);
@@ -1533,14 +1525,6 @@ event:
 			}
 	;
 
-hook:
-		expr '(' opt_expr_list ')'
-			{
-			set_location(@1, @4);
-			$$ = new CallExpr($1, $3, true);
-			}
-	;
-
 case_list:
 		case_list case
 			{ $1->append($2); }

testing/btest/Baseline/language.hook/out

@@ -1,7 +1,15 @@
 myhook, &priority=10, [a=1156, b=hello world]
+myhook return F
+myhook return T
 myhook, &priority=5, [a=37, b=goobye world]
+F
 myhook3, 8
+T
 myhook4, 2
 myhook4, 1
+T
 myhook, &priority=10, [a=2, b=it works]
+myhook return F
+myhook return T
 myhook, &priority=5, [a=37, b=goobye world]
+F

testing/btest/Baseline/language.invalid_hook/out

@@ -1 +0,0 @@
-error in /Users/jsiwek/Projects/bro/bro/testing/btest/.tmp/language.invalid_hook/invalid_hook.bro, line 15: hook called in expression, use hook statement instead (myhook(nope))

testing/btest/core/leaks/hook.bro

@@ -39,7 +39,25 @@ hook myhook(r: rec) &priority=10
 	r$b = "goobye world";
 	# returning from the handler early, is fine, remaining handlers still run.
 	return;
-	print "ERROR: break statement should return from hook handler body";
+	print "ERROR: return statement should return from hook handler body";
+	}
+
+hook myhook(r: rec) &priority=9
+	{
+	print "myhook return F";
+	# return value is ignored, remaining handlers still run, final return
+	# value is whether any hook body returned via break statement
+	return F;
+	print "ERROR: return statement should return from hook handler body";
+	}
+
+hook myhook(r: rec) &priority=8
+	{
+	print "myhook return T";
+	# return value is ignored, remaining handlers still run, final return
+	# value is whether any hook body returned via break statement
+	return T;
+	print "ERROR: return statement should return from hook handler body";
 	}
 
 # hook function doesn't need a declaration, we can go straight to defining
@@ -63,16 +81,16 @@ event new_connection(c: connection)
 	{
 	print "new_connection", c$id;
 
-	hook myhook([$a=1156, $b="hello world"]);
+	print myhook([$a=1156, $b="hello world"]);
 
 	# A hook with no handlers is fine, it's just a no-op.
-	hook myhook2("nope");
+	print myhook2("nope");
 
-	hook myhook3(8);
+	print myhook3(8);
-	hook myhook4();
+	print myhook4();
 
 	# A hook can be treated like other data types and doesn't have to be
 	# invoked directly by name.
 	local h = myhook;
-	hook h([$a=2, $b="it works"]);
+	print h([$a=2, $b="it works"]);
 	}

testing/btest/language/hook.bro

@@ -34,7 +34,25 @@ hook myhook(r: rec) &priority=10
 	r$b = "goobye world";
 	# returning from the handler early, is fine, remaining handlers still run.
 	return;
-	print "ERROR: break statement should return from hook handler body";
+	print "ERROR: return statement should return from hook handler body";
+	}
+
+hook myhook(r: rec) &priority=9
+	{
+	print "myhook return F";
+	# return value is ignored, remaining handlers still run, final return
+	# value is whether any hook body returned via break statement
+	return F;
+	print "ERROR: return statement should return from hook handler body";
+	}
+
+hook myhook(r: rec) &priority=8
+	{
+	print "myhook return T";
+	# return value is ignored, remaining handlers still run, final return
+	# value is whether any hook body returned via break statement
+	return T;
+	print "ERROR: return statement should return from hook handler body";
 	}
 
 # hook function doesn't need a declaration, we can go straight to defining
@@ -56,16 +74,16 @@ hook myhook4() &priority=2
 
 event bro_init()
 	{
-	hook myhook([$a=1156, $b="hello world"]);
+	print myhook([$a=1156, $b="hello world"]);
 
 	# A hook with no handlers is fine, it's just a no-op.
-	hook myhook2("nope");
+	print myhook2("nope");
 
-	hook myhook3(8);
+	print myhook3(8);
-	hook myhook4();
+	print myhook4();
 
 	# A hook can be treated like other data types and doesn't have to be
 	# invoked directly by name.
 	local h = myhook;
-	hook h([$a=2, $b="it works"]);
+	print h([$a=2, $b="it works"]);
 	}

testing/btest/language/invalid_hook.bro

@@ -1,16 +0,0 @@
-# @TEST-EXEC-FAIL: bro -b %INPUT >out 2>&1
-# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff out
-
-global myhook: hook(s: string);
-
-hook myhook(s: string)
-	{
-	print "myhook", s;
-	}
-
-event bro_init()
-	{
-	# hooks must be invoked with a "hook", statement.  They have no return
-	# value and don't make sense to evaluate as arbitrary expressions.
-	local r = myhook("nope");
-	}