Merge remote-tracking branch 'origin/topic/jsiwek/hook'

* origin/topic/jsiwek/hook:
  Change hook calls to only be allowed when preceded by "hook" keyword.
  Clarification in hook documentation.
  Hook functions now directly callable instead of w/ "hook" statements.

Closes #918.

CHANGES

@@ -1,4 +1,20 @@
 
+2.1-188 | 2012-12-03 14:04:29 -0800
+
+  * Hook functions now callable with "hook" expression (i.e., hook is
+    no longer a statement). 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. (Jon Siwek)
+
+  * Clarification in hook documentation. (Jon Siwek)
+
 2.1-184 | 2012-12-03 13:59:50 -0800
 
   * Slightly fix up file name extraction from Content-Disposition

VERSION

@@ -1 +1 @@
-2.1-184
+2.1-188

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
-    can be enforced with :bro:attr:`&priority`.  They are more like
-    functions in the way they are invoked/called, because, unlike
-    events, their execution is immediate and they do not get scheduled
-    through an event queue.  Also, a unique feature of a hook is that
-    a given hook handler body can short-circuit the execution of
-    remaining hook handlers simply by exiting from the body as a result
-    of a ``break`` statement (as opposed to a ``return`` or just
-    reaching the end of the body).
+    identifier and the order of execution can be enforced with
+    :bro:attr:`&priority`.  They are more like functions in the way they
+    are invoked/called, because, unlike events, their execution is
+    immediate and they do not get scheduled through an event queue.
+    Also, a unique feature of a hook is that a given hook handler body
+    can short-circuit the execution of remaining hook handlers simply by
+    exiting from the body as a result of a ``break`` statement (as
+    opposed to a ``return`` or just reaching the end of the body).
 
     A hook type is declared like::
 
@@ -549,18 +548,26 @@ The Bro scripting language supports the following built-in types.
             print "not going to happen", s;
             }
 
-    Note that, although the first (forward) declaration of ``myhook`` as
-    a hook type isn't strictly required, when it is provided, the
-    argument types must match.
+    Note that the first (forward) declaration of ``myhook`` as a hook
+    type isn't strictly required.  Argument types must match for all
+    hook handlers and any forward declaration of a given hook.
 
-    To invoke immediate execution of all hook handler bodies, a ``hook``
-    statement must be used:
+    To invoke immediate execution of all hook handler bodies, they
+    are called similarly to a function, except preceded by the ``hook``
+    keyword:
 
     .. code:: bro
 
         hook myhook("hi");
 
-    And the output would like like::
+    or
+
+    .. code:: bro
+
+        if ( hook myhook("hi") )
+            print "all handlers ran";
+
+    And the output would look like::
 
         priority 10 myhook handler, hi
         break out of myhook handling, bye
@@ -568,6 +575,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

@@ -4394,6 +4394,13 @@ CallExpr::CallExpr(Expr* arg_func, ListExpr* arg_args, bool in_hook)
 		return;
 		}
 
+	if ( func_type->AsFuncType()->Flavor() == FUNC_FLAVOR_HOOK && ! in_hook )
+		{
+		func->Error("hook cannot be called directly, use hook operator");
+		SetError();
+		return;
+		}
+
 	if ( ! func_type->MatchesIndex(args) )
 		SetError("argument type mismatch in function call");
 	else
@@ -4415,13 +4422,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
-				// is being done from a hook statement.
-				if ( ! in_hook )
-					{
-					Error("hook called in expression, use hook statement instead");
-					SetError();
-					}
+				Error("hook has no yield type");
+				SetError();
 				break;
 
 			default:

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 )
 			{
-			// short-circuit execution of remaining hook handler bodies
-			break;
+			// 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 )
-		Error("return statement needs expression");
+		{
+		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

@@ -32,6 +32,7 @@
 
 %token TOK_NO_TEST
 
+%nonassoc TOK_HOOK
 %left ',' '|'
 %right '=' TOK_ADD_TO TOK_REMOVE_FROM
 %right '?' ':'
@@ -56,7 +57,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
@@ -119,6 +119,7 @@ extern const char* g_curr_debug_error;
 
 #define YYLTYPE yyltype
 
+static int in_hook = 0;
 int in_init = 0;
 int in_record = 0;
 bool resolving_global_ID = false;
@@ -212,7 +213,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;
@@ -517,7 +517,16 @@ expr:
 	|	expr '(' opt_expr_list ')'
 			{
 			set_location(@1, @4);
-			$$ = new CallExpr($1, $3);
+			$$ = new CallExpr($1, $3, in_hook > 0);
+			}
+
+	|	TOK_HOOK { ++in_hook; } expr
+			{
+			--in_hook;
+			set_location(@1, @3);
+			if ( $3->Tag() != EXPR_CALL )
+				$3->Error("not a valid hook call expression");
+			$$ = $3;
 			}
 
 	|	expr TOK_HAS_FIELD TOK_ID
@@ -875,7 +884,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 +1218,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 +1383,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 +1536,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,18 @@
 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
+myhook4, 2
+myhook4, 1
+myhook4 all handlers ran
 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.hook_calls/invalid.out

@@ -0,0 +1,10 @@
+error in ./invalid.bro, line 9: hook cannot be called directly, use hook operator (myhook)
+warning in ./invalid.bro, line 9: expression value ignored (myhook(3))
+error in ./invalid.bro, line 10: hook cannot be called directly, use hook operator (myhook)
+error in ./invalid.bro, line 11: hook cannot be called directly, use hook operator (myhook)
+error in ./invalid.bro, line 12: not a valid hook call expression (2 + 2)
+warning in ./invalid.bro, line 12: expression value ignored (2 + 2)
+error in ./invalid.bro, line 13: not a valid hook call expression (2 + 2)
+error in ./invalid.bro, line 15: hook cannot be called directly, use hook operator (h)
+warning in ./invalid.bro, line 15: expression value ignored (h(3))
+error in ./invalid.bro, line 16: hook cannot be called directly, use hook operator (h)

testing/btest/Baseline/language.hook_calls/valid.out

@@ -0,0 +1,42 @@
+myhook(), 3
+other myhook(), 3
+myhook(), 3
+other myhook(), 3
+T
+myhook(), 0
+F
+-----------
+indirect()
+myhook(), 3
+other myhook(), 3
+indirect()
+myhook(), 3
+other myhook(), 3
+T
+-----------
+really_indirect()
+indirect()
+myhook(), 3
+other myhook(), 3
+really_indirect()
+indirect()
+myhook(), 3
+other myhook(), 3
+T
+-----------
+myhook(), 3
+other myhook(), 3
+myhook(), 3
+other myhook(), 3
+T
+myhook(), 3
+other myhook(), 3
+yes
+myhook(), 0
+double yes
+-----------
+myhook(), 3
+other myhook(), 3
+myhook(), 3
+other myhook(), 3
+T

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,20 @@ event new_connection(c: connection)
 	{
 	print "new_connection", c$id;
 
-	hook myhook([$a=1156, $b="hello world"]);
+	print hook myhook([$a=1156, $b="hello world"]);
 
 	# A hook with no handlers is fine, it's just a no-op.
-	hook myhook2("nope");
+	print hook myhook2("nope");
 
-	hook myhook3(8);
-	hook myhook4();
+	print hook myhook3(8);
+	print hook myhook4();
+	if ( hook myhook4() )
+		{
+		print "myhook4 all handlers ran";
+		}
 
 	# 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 hook 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,20 @@ hook myhook4() &priority=2
 
 event bro_init()
 	{
-	hook myhook([$a=1156, $b="hello world"]);
+	print hook myhook([$a=1156, $b="hello world"]);
 
 	# A hook with no handlers is fine, it's just a no-op.
-	hook myhook2("nope");
+	print hook myhook2("nope");
 
-	hook myhook3(8);
-	hook myhook4();
+	print hook myhook3(8);
+	print hook myhook4();
+	if ( hook myhook4() )
+		{
+		print "myhook4 all handlers ran";
+		}
 
 	# 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 hook h([$a=2, $b="it works"]);
 	}

testing/btest/language/hook_calls.bro

@@ -0,0 +1,82 @@
+# @TEST-EXEC: bro -b valid.bro >valid.out
+# @TEST-EXEC: btest-diff valid.out
+# @TEST-EXEC-FAIL: bro -b invalid.bro > invalid.out 2>&1
+# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff invalid.out
+
+# hook functions must be called using the "hook" keyword as an operator...
+
+@TEST-START-FILE valid.bro
+hook myhook(i: count)
+    {
+    print "myhook()", i;
+	if ( i == 0 ) break;
+    }
+
+hook myhook(i: count) &priority=-1
+    {
+    print "other myhook()", i;
+	}
+
+function indirect(): hook(i: count)
+    {
+    print "indirect()";
+    return myhook;
+    }
+
+function really_indirect(): function(): hook(i: count)
+    {
+    print "really_indirect()";
+    return indirect;
+    }
+
+global t: table[count] of hook(i: count) = {
+    [0] = myhook,
+};
+
+event bro_init()
+	{
+	hook myhook(3);
+	print hook myhook(3);
+	print hook myhook(0);
+	print "-----------";
+	hook indirect()(3);
+	print hook indirect()(3);
+	print "-----------";
+	hook really_indirect()()(3);
+	print hook really_indirect()()(3);
+	print "-----------";
+	local h = t[0];
+	hook h(3);
+	print hook h(3);
+	if ( hook h(3) )
+		print "yes";
+	if ( ! hook h(0) )
+		print "double yes";
+	print "-----------";
+	hook t[0](3);
+	print hook t[0](3);
+	}
+
+@TEST-END-FILE
+
+@TEST-START-FILE invalid.bro
+hook myhook(i: count)
+	{
+	print "myhook()", i;
+	if ( i == 0 ) break;
+	}
+
+event bro_init()
+	{
+	myhook(3);
+	print myhook(3);
+	print myhook(0);
+	hook 2+2;
+	print hook 2+2;
+	local h = myhook;
+	h(3);
+	if ( h(3) )
+		print "hmm";
+	print "done";
+	}
+@TEST-END-FILE

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");
-	}