simplify WhenInfo and Trigger classes given removal of old capture semantics

src/Stmt.cc

@@ -1867,7 +1867,8 @@ TraversalCode NullStmt::Traverse(TraversalCallback* cb) const
 WhenInfo::WhenInfo(ExprPtr arg_cond, FuncType::CaptureList* arg_cl, bool arg_is_return)
 	: cond(std::move(arg_cond)), cl(arg_cl), is_return(arg_is_return)
 	{
-	prior_vars = current_scope()->Vars();
+	if ( ! cl )
+		cl = new zeek::FuncType::CaptureList;
 
 	ProfileFunc cond_pf(cond.get());
 
@@ -1881,20 +1882,17 @@ WhenInfo::WhenInfo(ExprPtr arg_cond, FuncType::CaptureList* arg_cl, bool arg_is_
 		{
 		bool is_present = false;
 
-		if ( cl )
+		for ( auto& c : *cl )
-			{
+			if ( c.id == wl )
-			for ( auto& c : *cl )
-				if ( c.id == wl )
-					{
-					is_present = true;
-					break;
-					}
-
-			if ( ! is_present )
 				{
-				IDPtr wl_ptr = {NewRef{}, const_cast<ID*>(wl)};
+				is_present = true;
-				cl->emplace_back(FuncType::Capture{wl_ptr, false});
+				break;
 				}
+
+		if ( ! is_present )
+			{
+			IDPtr wl_ptr = {NewRef{}, const_cast<ID*>(wl)};
+			cl->emplace_back(FuncType::Capture{wl_ptr, false});
 			}
 
 		// In addition, don't treat them as external locals that
@@ -1926,32 +1924,12 @@ WhenInfo::WhenInfo(ExprPtr arg_cond, FuncType::CaptureList* arg_cl, bool arg_is_
 
 WhenInfo::WhenInfo(bool arg_is_return) : is_return(arg_is_return)
 	{
-	// This won't be needed once we remove the deprecated semantics.
 	cl = new zeek::FuncType::CaptureList;
 	BuildInvokeElems();
 	}
 
 void WhenInfo::Build(StmtPtr ws)
 	{
-	// This will call ws->Error() if it's deprecated and we can
-	// short-circuit.
-	if ( IsDeprecatedSemantics(ws) )
-		return;
-
-	if ( ! cl )
-		{
-		// This instance is compatible with new-style semantics,
-		// so create a capture list for it and populate with any
-		// when-locals.
-		cl = new zeek::FuncType::CaptureList;
-
-		for ( auto& wl : when_new_locals )
-			{
-			IDPtr wl_ptr = {NewRef{}, const_cast<ID*>(wl)};
-			cl->emplace_back(FuncType::Capture{wl_ptr, false});
-			}
-		}
-
 	lambda_ft->SetCaptures(*cl);
 
 	// Our general strategy is to construct a single lambda (so that
@@ -1996,38 +1974,30 @@ void WhenInfo::Build(StmtPtr ws)
 
 	auto shebang = make_intrusive<StmtList>(do_test, do_bodies, dummy_return);
 
-	auto ingredients = std::make_unique<function_ingredients>(current_scope(), shebang,
+	auto ingredients = std::make_unique<FunctionIngredients>(current_scope(), shebang,
-	                                                          current_module);
+	                                                         current_module);
-	auto outer_ids = gather_outer_ids(pop_scope(), ingredients->body);
+	auto outer_ids = gather_outer_ids(pop_scope(), ingredients->Body());
 
 	lambda = make_intrusive<LambdaExpr>(std::move(ingredients), std::move(outer_ids), ws);
 	}
 
 void WhenInfo::Instantiate(Frame* f)
 	{
-	if ( cl )
+	Instantiate(lambda->Eval(f));
-		Instantiate(lambda->Eval(f));
 	}
 
 void WhenInfo::Instantiate(ValPtr func)
 	{
-	if ( cl )
+	curr_lambda = make_intrusive<ConstExpr>(std::move(func));
-		curr_lambda = make_intrusive<ConstExpr>(std::move(func));
 	}
 
 ExprPtr WhenInfo::Cond()
 	{
-	if ( ! curr_lambda )
-		return cond;
-
 	return make_intrusive<CallExpr>(curr_lambda, invoke_cond);
 	}
 
 StmtPtr WhenInfo::WhenBody()
 	{
-	if ( ! curr_lambda )
-		return s;
-
 	auto invoke = make_intrusive<CallExpr>(curr_lambda, invoke_s);
 	return make_intrusive<ReturnStmt>(invoke, true);
 	}
@@ -2046,61 +2016,10 @@ double WhenInfo::TimeoutVal(Frame* f)
 
 StmtPtr WhenInfo::TimeoutStmt()
 	{
-	if ( ! curr_lambda )
-		return timeout_s;
-
 	auto invoke = make_intrusive<CallExpr>(curr_lambda, invoke_timeout);
 	return make_intrusive<ReturnStmt>(invoke, true);
 	}
 
-bool WhenInfo::IsDeprecatedSemantics(StmtPtr ws)
-	{
-	if ( cl )
-		return false;
-
-	// Which locals of the outer function are used in any of the "when"
-	// elements.
-	IDSet locals;
-
-	for ( auto& wl : when_new_locals )
-		prior_vars.erase(wl->Name());
-
-	for ( auto& bl : when_expr_locals )
-		if ( prior_vars.count(bl->Name()) > 0 )
-			locals.insert(bl);
-
-	ProfileFunc body_pf(s.get());
-	for ( auto& bl : body_pf.Locals() )
-		if ( prior_vars.count(bl->Name()) > 0 )
-			locals.insert(bl);
-
-	if ( timeout_s )
-		{
-		ProfileFunc to_pf(timeout_s.get());
-		for ( auto& tl : to_pf.Locals() )
-			if ( prior_vars.count(tl->Name()) > 0 )
-				locals.insert(tl);
-		}
-
-	if ( locals.empty() )
-		return false;
-
-	std::string vars;
-	for ( auto& l : locals )
-		{
-		if ( ! vars.empty() )
-			vars += ", ";
-		vars += l->Name();
-		}
-
-	std::string msg = util::fmt("\"when\" statement referring to locals without an "
-	                            "explicit [] capture: %s",
-	                            vars.c_str());
-	ws->Error(msg.c_str());
-
-	return true;
-	}
-
 void WhenInfo::BuildInvokeElems()
 	{
 	one_const = make_intrusive<ConstExpr>(val_mgr->Count(1));
@@ -2116,12 +2035,12 @@ WhenStmt::WhenStmt(WhenInfo* arg_wi) : Stmt(STMT_WHEN), wi(arg_wi)
 	{
 	wi->Build(ThisPtr());
 
-	auto cond = wi->Cond();
+	auto cond = wi->OrigCond();
 
 	if ( ! cond->IsError() && ! IsBool(cond->GetType()->Tag()) )
 		cond->Error("conditional in test must be boolean");
 
-	auto te = wi->TimeoutExpr();
+	auto te = wi->OrigTimeout();
 
 	if ( te )
 		{
@@ -2148,32 +2067,24 @@ ValPtr WhenStmt::Exec(Frame* f, StmtFlowType& flow)
 
 	auto timeout = wi->TimeoutVal(f);
 
-	if ( wi->Captures() )
+	std::vector<ValPtr> local_aggrs;
+	for ( auto& l : wi->WhenExprLocals() )
 		{
-		std::vector<ValPtr> local_aggrs;
+		IDPtr l_ptr = {NewRef{}, const_cast<ID*>(l)};
-		for ( auto& l : wi->WhenExprLocals() )
+		auto v = f->GetElementByID(l_ptr);
-			{
+		if ( v && v->Modifiable() )
-			IDPtr l_ptr = {NewRef{}, const_cast<ID*>(l)};
+			local_aggrs.emplace_back(std::move(v));
-			auto v = f->GetElementByID(l_ptr);
-			if ( v && v->Modifiable() )
-				local_aggrs.emplace_back(std::move(v));
-			}
-
-		new trigger::Trigger(wi, timeout, wi->WhenExprGlobals(), local_aggrs, f, location);
 		}
 
-	else
+	// The new trigger object will take care of its own deletion.
-		// The new trigger object will take care of its own deletion.
+	new trigger::Trigger(wi, timeout, wi->WhenExprGlobals(), local_aggrs, f, location);
-		new trigger::Trigger(wi->Cond(), wi->WhenBody(), wi->TimeoutStmt(), timeout, f,
-		                     wi->IsReturn(), location);
 
 	return nullptr;
 	}
 
 bool WhenStmt::IsPure() const
 	{
-	return wi->Cond()->IsPure() && wi->WhenBody()->IsPure() &&
+	return false;
-	       (! wi->TimeoutStmt() || wi->TimeoutStmt()->IsPure());
 	}
 
 void WhenStmt::StmtDescribe(ODesc* d) const
@@ -2183,35 +2094,35 @@ void WhenStmt::StmtDescribe(ODesc* d) const
 	if ( d->IsReadable() )
 		d->Add("(");
 
-	wi->Cond()->Describe(d);
+	wi->OrigCond()->Describe(d);
 
 	if ( d->IsReadable() )
 		d->Add(")");
 
 	d->SP();
 	d->PushIndent();
-	wi->WhenBody()->AccessStats(d);
+	wi->OrigBody()->AccessStats(d);
-	wi->WhenBody()->Describe(d);
+	wi->OrigBody()->Describe(d);
 	d->PopIndent();
 
-	if ( wi->TimeoutExpr() )
+	if ( wi->OrigTimeout() )
 		{
 		if ( d->IsReadable() )
 			{
 			d->SP();
 			d->Add("timeout");
 			d->SP();
-			wi->TimeoutExpr()->Describe(d);
+			wi->OrigTimeout()->Describe(d);
 			d->SP();
 			d->PushIndent();
-			wi->TimeoutStmt()->AccessStats(d);
+			wi->OrigTimeoutStmt()->AccessStats(d);
-			wi->TimeoutStmt()->Describe(d);
+			wi->OrigTimeoutStmt()->Describe(d);
 			d->PopIndent();
 			}
 		else
 			{
-			wi->TimeoutExpr()->Describe(d);
+			wi->OrigTimeout()->Describe(d);
-			wi->TimeoutStmt()->Describe(d);
+			wi->OrigTimeoutStmt()->Describe(d);
 			}
 		}
 	}
@@ -2231,22 +2142,22 @@ TraversalCode WhenStmt::Traverse(TraversalCallback* cb) const
 
 	else
 		{
-		tc = wi->Cond()->Traverse(cb);
+		tc = wi->OrigCond()->Traverse(cb);
 		HANDLE_TC_STMT_PRE(tc);
 
-		tc = wi->WhenBody()->Traverse(cb);
+		tc = wi->OrigBody()->Traverse(cb);
 		HANDLE_TC_STMT_PRE(tc);
 
-		if ( wi->TimeoutStmt() )
+		if ( wi->OrigTimeoutStmt() )
 			{
-			tc = wi->TimeoutStmt()->Traverse(cb);
+			tc = wi->OrigTimeoutStmt()->Traverse(cb);
 			HANDLE_TC_STMT_PRE(tc);
 			}
 		}
 
-	if ( wi->TimeoutExpr() )
+	if ( wi->OrigTimeout() )
 		{
-		tc = wi->TimeoutExpr()->Traverse(cb);
+		tc = wi->OrigTimeout()->Traverse(cb);
 		HANDLE_TC_STMT_PRE(tc);
 		}
 

src/Stmt.h

@@ -550,8 +550,7 @@ private:
 class WhenInfo
 	{
 public:
-	// Takes ownership of the CaptureList, which if nil signifies
+	// Takes ownership of the CaptureList.
-	// old-style frame semantics.
 	WhenInfo(ExprPtr cond, FuncType::CaptureList* cl, bool is_return);
 
 	// Constructor used by script optimization to create a stub.
@@ -582,18 +581,20 @@ public:
 	void Instantiate(Frame* f);
 	void Instantiate(ValPtr func);
 
-	// For old-style semantics, the following simply return the
+	// Return the original components used to construct the "when".
-	// individual "when" components.  For capture semantics, however,
+	const ExprPtr& OrigCond() const { return cond; }
-	// these instead return different invocations of a lambda that
+	const StmtPtr& OrigBody() const { return s; }
-	// manages the captures.
+	const ExprPtr& OrigTimeout() const { return timeout; }
+	const StmtPtr& OrigTimeoutStmt() const { return timeout_s; }
+
+	// Return different invocations of a lambda that manages the captures.
 	ExprPtr Cond();
 	StmtPtr WhenBody();
+	StmtPtr TimeoutStmt();
 
 	ExprPtr TimeoutExpr() const { return timeout; }
 	double TimeoutVal(Frame* f);
 
-	StmtPtr TimeoutStmt();
-
 	FuncType::CaptureList* Captures() { return cl; }
 
 	bool IsReturn() const { return is_return; }
@@ -605,18 +606,13 @@ public:
 	const IDSet& WhenExprGlobals() const { return when_expr_globals; }
 
 private:
-	// True if the "when" statement corresponds to old-style deprecated
-	// semantics (no captures, but needing captures).  Also triggers
-	// an error associated with "ws".
-	bool IsDeprecatedSemantics(StmtPtr ws);
-
 	// Build those elements we'll need for invoking our lambda.
 	void BuildInvokeElems();
 
 	ExprPtr cond;
 	StmtPtr s;
-	ExprPtr timeout;
 	StmtPtr timeout_s;
+	ExprPtr timeout;
 	FuncType::CaptureList* cl;
 
 	bool is_return = false;
@@ -648,10 +644,6 @@ private:
 
 	// Locals introduced via "local" in the "when" clause itself.
 	IDSet when_new_locals;
-
-	// Used for identifying deprecated instances.  Holds all of the local
-	// variables in the scope prior to parsing the "when" statement.
-	std::map<std::string, IDPtr, std::less<>> prior_vars;
 	};
 
 class WhenStmt final : public Stmt

src/Trigger.cc

@@ -99,13 +99,6 @@ protected:
 	double time;
 	};
 
-Trigger::Trigger(ExprPtr cond, StmtPtr body, StmtPtr timeout_stmts, double timeout, Frame* frame,
-                 bool is_return, const Location* location)
-	{
-	timeout_value = timeout;
-	Init(cond, body, timeout_stmts, frame, is_return, location);
-	}
-
 Trigger::Trigger(WhenInfo* wi, double timeout, const IDSet& _globals,
                  std::vector<ValPtr> _local_aggrs, Frame* f, const Location* loc)
 	{
@@ -114,34 +107,29 @@ Trigger::Trigger(WhenInfo* wi, double timeout, const IDSet& _globals,
 	local_aggrs = std::move(_local_aggrs);
 	have_trigger_elems = true;
 
-	Init(wi->Cond(), wi->WhenBody(), wi->TimeoutStmt(), f, wi->IsReturn(), loc);
+	cond = wi->Cond();
-	}
+	body = wi->WhenBody();
+	timeout_stmts = wi->TimeoutStmt();
+	is_return = wi->IsReturn();
 
-void Trigger::Init(ExprPtr arg_cond, StmtPtr arg_body, StmtPtr arg_timeout_stmts, Frame* arg_frame,
-                   bool arg_is_return, const Location* location)
-	{
-	cond = arg_cond;
-	body = arg_body;
-	timeout_stmts = arg_timeout_stmts;
 	timer = nullptr;
 	delayed = false;
 	disabled = false;
 	attached = nullptr;
-	is_return = arg_is_return;
 
 	if ( location )
 		name = util::fmt("%s:%d-%d", location->filename, location->first_line, location->last_line);
 	else
 		name = "<no-trigger-location>";
 
-	if ( arg_frame )
+	if ( f )
-		frame = arg_frame->Clone();
+		frame = f->LightClone();
 	else
 		frame = nullptr;
 
 	DBG_LOG(DBG_NOTIFIERS, "%s: instantiating", Name());
 
-	if ( is_return && frame && arg_frame )
+	if ( is_return && frame )
 		{
 		Trigger* parent = frame->GetTrigger();
 		if ( ! parent )
@@ -152,7 +140,7 @@ void Trigger::Init(ExprPtr arg_cond, StmtPtr arg_body, StmtPtr arg_timeout_stmts
 			}
 
 		parent->Attach(this);
-		arg_frame->SetDelayed();
+		f->SetDelayed();
 		}
 
 	// Make sure we don't get deleted if somebody calls a method like
@@ -262,7 +250,7 @@ bool Trigger::Eval()
 
 	try
 		{
-		f = frame->Clone();
+		f = frame->LightClone();
 		}
 	catch ( InterpreterException& )
 		{
@@ -364,7 +352,7 @@ void Trigger::Timeout()
 	if ( timeout_stmts )
 		{
 		StmtFlowType flow;
-		FramePtr f{AdoptRef{}, frame->Clone()};
+		FramePtr f{AdoptRef{}, frame->LightClone()};
 		ValPtr v;
 
 		try

src/Trigger.h

@@ -48,11 +48,6 @@ public:
 	// statements are executed immediately and the object is deleted
 	// right away.
 
-	// These first constructor is for the deprecated deep-copy semantics.
-	Trigger(ExprPtr cond, StmtPtr body, StmtPtr timeout_stmts, double timeout, Frame* f,
-	        bool is_return, const Location* loc);
-
-	// Used for capture-list semantics.
 	Trigger(WhenInfo* wi, double timeout, const IDSet& globals, std::vector<ValPtr> local_aggrs,
 	        Frame* f, const Location* loc);
 
@@ -112,9 +107,6 @@ public:
 private:
 	friend class TriggerTimer;
 
-	void Init(ExprPtr cond, StmtPtr body, StmtPtr timeout_stmts, Frame* frame, bool is_return,
-	          const Location* location);
-
 	void ReInit(std::vector<ValPtr> index_expr_results);
 
 	void Register(const ID* id);