From f032885085b0c7c894f72f9fc7f5722637599eba Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Thu, 26 Mar 2020 15:48:18 -0700 Subject: [PATCH 1/2] Change record field anonymous functions to use lambda expressions There was an alternate syntax to assign anonymous functions to record fields that was never migrated to use the new lambda expression machinery (and so didn't allow referencing variables in outer scope): type myrec: record { foo: function(a: string); }; local o = "o"; local mr = myrec($foo(a: string) = { print a + o; }); --- src/Var.cc | 10 ------- src/parse.y | 23 +++++++-------- .../Baseline/language.lambda-record-field/out | 1 + .../scripts.base.frameworks.input.reread/out | 28 +++++++++---------- .../btest/language/lambda-record-field.zeek | 13 +++++++++ 5 files changed, 40 insertions(+), 35 deletions(-) create mode 100644 testing/btest/Baseline/language.lambda-record-field/out create mode 100644 testing/btest/language/lambda-record-field.zeek diff --git a/src/Var.cc b/src/Var.cc index c447517b53..d49323b7f3 100644 --- a/src/Var.cc +++ b/src/Var.cc @@ -476,16 +476,6 @@ void end_func(IntrusivePtr body) { auto ingredients = std::make_unique(pop_scope(), std::move(body)); - if ( streq(ingredients->id->Name(), "anonymous-function") ) - { - OuterIDBindingFinder cb(ingredients->scope.get()); - ingredients->body->Traverse(&cb); - - for ( size_t i = 0; i < cb.outer_id_references.size(); ++i ) - cb.outer_id_references[i]->Error( - "referencing outer function IDs not supported"); - } - if ( ingredients->id->HasVal() ) ingredients->id->ID_Val()->AsFunc()->AddBody( ingredients->body, diff --git a/src/parse.y b/src/parse.y index 7f379d44ec..dd22fbee04 100644 --- a/src/parse.y +++ b/src/parse.y @@ -57,7 +57,7 @@ %type init_class %type opt_init %type TOK_CONSTANT -%type expr opt_expr init anonymous_function index_slice opt_deprecated +%type expr opt_expr init anonymous_function lambda_body index_slice opt_deprecated %type event %type stmt stmt_list func_body for_head %type type opt_type enum_body @@ -504,7 +504,7 @@ expr: $$ = new FieldAssignExpr($2, {AdoptRef{}, $4}); } - | '$' TOK_ID func_params '=' + | '$' TOK_ID func_params '=' { func_hdr_location = @1; func_id = current_scope()->GenerateTemporary("anonymous-function"); @@ -512,11 +512,9 @@ expr: begin_func(func_id, current_module.c_str(), FUNC_FLAVOR_FUNCTION, 0, {AdoptRef{}, $3}); } - func_body + lambda_body { - $$ = new FieldAssignExpr($2, - make_intrusive( - IntrusivePtr{NewRef{}, func_id->ID_Val()})); + $$ = new FieldAssignExpr($2, IntrusivePtr{AdoptRef{}, $6}); Unref(func_id); } @@ -1237,9 +1235,7 @@ func_body: } ; -anonymous_function: - TOK_FUNCTION begin_func - +lambda_body: '{' { saved_in_init.push_back(in_init); @@ -1254,20 +1250,25 @@ anonymous_function: '}' { - set_location(@1, @7); + set_location(@1, @5); // Code duplication here is sad but needed. end_func actually instantiates the function // and associates it with an ID. We perform that association later and need to return // a lambda expression. // Gather the ingredients for a BroFunc from the current scope - auto ingredients = std::make_unique(IntrusivePtr{NewRef{}, current_scope()}, IntrusivePtr{AdoptRef{}, $5}); + auto ingredients = std::make_unique(IntrusivePtr{NewRef{}, current_scope()}, IntrusivePtr{AdoptRef{}, $3}); id_list outer_ids = gather_outer_ids(pop_scope().get(), ingredients->body.get()); $$ = new LambdaExpr(std::move(ingredients), std::move(outer_ids)); } ; +anonymous_function: + TOK_FUNCTION begin_func lambda_body + { $$ = $3; } + ; + begin_func: func_params { diff --git a/testing/btest/Baseline/language.lambda-record-field/out b/testing/btest/Baseline/language.lambda-record-field/out new file mode 100644 index 0000000000..31e0fce560 --- /dev/null +++ b/testing/btest/Baseline/language.lambda-record-field/out @@ -0,0 +1 @@ +helloworld diff --git a/testing/btest/Baseline/scripts.base.frameworks.input.reread/out b/testing/btest/Baseline/scripts.base.frameworks.input.reread/out index 19d323afcb..13adcdd31d 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.input.reread/out +++ b/testing/btest/Baseline/scripts.base.frameworks.input.reread/out @@ -39,7 +39,7 @@ print A::outfile, Left; print A::outfile, A::left; print A::outfile, Right; print A::outfile, A::right; -}, pred=anonymous-function +}, pred=lambda_<10906653936191056190> { print A::outfile, ============PREDICATE============; print A::outfile, A::typ; @@ -134,7 +134,7 @@ print A::outfile, Left; print A::outfile, A::left; print A::outfile, Right; print A::outfile, A::right; -}, pred=anonymous-function +}, pred=lambda_<10906653936191056190> { print A::outfile, ============PREDICATE============; print A::outfile, A::typ; @@ -241,7 +241,7 @@ print A::outfile, Left; print A::outfile, A::left; print A::outfile, Right; print A::outfile, A::right; -}, pred=anonymous-function +}, pred=lambda_<10906653936191056190> { print A::outfile, ============PREDICATE============; print A::outfile, A::typ; @@ -468,7 +468,7 @@ print A::outfile, Left; print A::outfile, A::left; print A::outfile, Right; print A::outfile, A::right; -}, pred=anonymous-function +}, pred=lambda_<10906653936191056190> { print A::outfile, ============PREDICATE============; print A::outfile, A::typ; @@ -593,7 +593,7 @@ print A::outfile, Left; print A::outfile, A::left; print A::outfile, Right; print A::outfile, A::right; -}, pred=anonymous-function +}, pred=lambda_<10906653936191056190> { print A::outfile, ============PREDICATE============; print A::outfile, A::typ; @@ -718,7 +718,7 @@ print A::outfile, Left; print A::outfile, A::left; print A::outfile, Right; print A::outfile, A::right; -}, pred=anonymous-function +}, pred=lambda_<10906653936191056190> { print A::outfile, ============PREDICATE============; print A::outfile, A::typ; @@ -843,7 +843,7 @@ print A::outfile, Left; print A::outfile, A::left; print A::outfile, Right; print A::outfile, A::right; -}, pred=anonymous-function +}, pred=lambda_<10906653936191056190> { print A::outfile, ============PREDICATE============; print A::outfile, A::typ; @@ -968,7 +968,7 @@ print A::outfile, Left; print A::outfile, A::left; print A::outfile, Right; print A::outfile, A::right; -}, pred=anonymous-function +}, pred=lambda_<10906653936191056190> { print A::outfile, ============PREDICATE============; print A::outfile, A::typ; @@ -1198,7 +1198,7 @@ print A::outfile, Left; print A::outfile, A::left; print A::outfile, Right; print A::outfile, A::right; -}, pred=anonymous-function +}, pred=lambda_<10906653936191056190> { print A::outfile, ============PREDICATE============; print A::outfile, A::typ; @@ -1251,7 +1251,7 @@ print A::outfile, Left; print A::outfile, A::left; print A::outfile, Right; print A::outfile, A::right; -}, pred=anonymous-function +}, pred=lambda_<10906653936191056190> { print A::outfile, ============PREDICATE============; print A::outfile, A::typ; @@ -1304,7 +1304,7 @@ print A::outfile, Left; print A::outfile, A::left; print A::outfile, Right; print A::outfile, A::right; -}, pred=anonymous-function +}, pred=lambda_<10906653936191056190> { print A::outfile, ============PREDICATE============; print A::outfile, A::typ; @@ -1357,7 +1357,7 @@ print A::outfile, Left; print A::outfile, A::left; print A::outfile, Right; print A::outfile, A::right; -}, pred=anonymous-function +}, pred=lambda_<10906653936191056190> { print A::outfile, ============PREDICATE============; print A::outfile, A::typ; @@ -1410,7 +1410,7 @@ print A::outfile, Left; print A::outfile, A::left; print A::outfile, Right; print A::outfile, A::right; -}, pred=anonymous-function +}, pred=lambda_<10906653936191056190> { print A::outfile, ============PREDICATE============; print A::outfile, A::typ; @@ -1463,7 +1463,7 @@ print A::outfile, Left; print A::outfile, A::left; print A::outfile, Right; print A::outfile, A::right; -}, pred=anonymous-function +}, pred=lambda_<10906653936191056190> { print A::outfile, ============PREDICATE============; print A::outfile, A::typ; diff --git a/testing/btest/language/lambda-record-field.zeek b/testing/btest/language/lambda-record-field.zeek new file mode 100644 index 0000000000..bcb7e1bc75 --- /dev/null +++ b/testing/btest/language/lambda-record-field.zeek @@ -0,0 +1,13 @@ +# @TEST-EXEC: zeek -b %INPUT >out +# @TEST-EXEC: btest-diff out + +type myrec: record { + foo: function(a: string); +}; + +event zeek_init() + { + local w = "world"; + local mr = myrec($foo(a: string) = { print a + w; }); + mr$foo("hello"); + } From 1ca11f11c7afda44c13b4913d822f3b8522e0ea3 Mon Sep 17 00:00:00 2001 From: Jon Siwek Date: Thu, 26 Mar 2020 17:05:59 -0700 Subject: [PATCH 2/2] GH-725: fix logic for finding a lambda's usage of outer IDs --- src/Expr.cc | 5 +++ src/Expr.h | 3 ++ src/ID.cc | 1 - src/Var.cc | 35 ++++++++----------- .../Baseline/language.lambda-nested-copy/out | 2 ++ .../btest/language/lambda-nested-copy.zeek | 19 ++++++++++ 6 files changed, 43 insertions(+), 22 deletions(-) create mode 100644 testing/btest/Baseline/language.lambda-nested-copy/out create mode 100644 testing/btest/language/lambda-nested-copy.zeek diff --git a/src/Expr.cc b/src/Expr.cc index ad6c18432c..ce9ae47868 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -4366,6 +4366,11 @@ LambdaExpr::LambdaExpr(std::unique_ptr arg_ing, id->SetConst(); } +Scope* LambdaExpr::GetScope() const + { + return ingredients->scope.get(); + } + IntrusivePtr LambdaExpr::Eval(Frame* f) const { auto lamb = make_intrusive( diff --git a/src/Expr.h b/src/Expr.h index 95c1167abe..69ae43b4be 100644 --- a/src/Expr.h +++ b/src/Expr.h @@ -61,6 +61,7 @@ extern const char* expr_name(BroExprTag t); template class IntrusivePtr; class Stmt; class Frame; +class Scope; class ListExpr; class NameExpr; class IndexExpr; @@ -813,6 +814,8 @@ public: IntrusivePtr Eval(Frame* f) const override; TraversalCode Traverse(TraversalCallback* cb) const override; + Scope* GetScope() const; + protected: void ExprDescribe(ODesc* d) const override; diff --git a/src/ID.cc b/src/ID.cc index f8d4c97bd1..c7228a6901 100644 --- a/src/ID.cc +++ b/src/ID.cc @@ -12,7 +12,6 @@ #include "Scope.h" #include "Type.h" #include "File.h" -#include "Scope.h" #include "Traverse.h" #include "Val.h" #include "zeekygen/Manager.h" diff --git a/src/Var.cc b/src/Var.cc index d49323b7f3..461de5e99f 100644 --- a/src/Var.cc +++ b/src/Var.cc @@ -419,32 +419,25 @@ void begin_func(ID* id, const char* module_name, function_flavor flavor, class OuterIDBindingFinder : public TraversalCallback { public: OuterIDBindingFinder(Scope* s) - : scope(s) { } + { + scopes.emplace_back(s); + } virtual TraversalCode PreExpr(const Expr*); virtual TraversalCode PostExpr(const Expr*); - Scope* scope; + std::vector scopes; vector outer_id_references; - int lambda_depth = 0; - // Note: think we really ought to toggle this to false to prevent - // considering locals within inner-lambdas as "outer", but other logic - // for "selective cloning" and locating IDs in the closure chain may - // depend on current behavior and also needs to be changed. - bool search_inner_lambdas = true; }; TraversalCode OuterIDBindingFinder::PreExpr(const Expr* expr) { if ( expr->Tag() == EXPR_LAMBDA ) - ++lambda_depth; - - if ( lambda_depth > 0 && ! search_inner_lambdas ) - // Don't inspect the bodies of inner lambdas as they will have their - // own traversal to find outer IDs and we don't want to detect - // references to local IDs inside and accidentally treat them as - // "outer" since they can't be found in current scope. + { + auto le = static_cast(expr); + scopes.emplace_back(le->GetScope()); return TC_CONTINUE; + } if ( expr->Tag() != EXPR_NAME ) return TC_CONTINUE; @@ -454,8 +447,11 @@ TraversalCode OuterIDBindingFinder::PreExpr(const Expr* expr) if ( e->Id()->IsGlobal() ) return TC_CONTINUE; - if ( scope->Lookup(e->Id()->Name()) ) - return TC_CONTINUE; + for ( const auto& scope : scopes ) + if ( scope->Lookup(e->Id()->Name()) ) + // Shadowing is not allowed, so if it's found at inner scope, it's + // not something we have to worry about also being at outer scope. + return TC_CONTINUE; outer_id_references.push_back(e); return TC_CONTINUE; @@ -464,10 +460,7 @@ TraversalCode OuterIDBindingFinder::PreExpr(const Expr* expr) TraversalCode OuterIDBindingFinder::PostExpr(const Expr* expr) { if ( expr->Tag() == EXPR_LAMBDA ) - { - --lambda_depth; - assert(lambda_depth >= 0); - } + scopes.pop_back(); return TC_CONTINUE; } diff --git a/testing/btest/Baseline/language.lambda-nested-copy/out b/testing/btest/Baseline/language.lambda-nested-copy/out new file mode 100644 index 0000000000..b963e085f3 --- /dev/null +++ b/testing/btest/Baseline/language.lambda-nested-copy/out @@ -0,0 +1,2 @@ +106 +106 diff --git a/testing/btest/language/lambda-nested-copy.zeek b/testing/btest/language/lambda-nested-copy.zeek new file mode 100644 index 0000000000..6cf0e6952a --- /dev/null +++ b/testing/btest/language/lambda-nested-copy.zeek @@ -0,0 +1,19 @@ +# @TEST-EXEC: zeek -b %INPUT >out +# @TEST-EXEC: btest-diff out + +local outer = 100; + +local lambda = function() + { + local inner = function(a: count, b: count, c: count, d: count, e: count, f: count) + { + print outer + f; + }; + + inner(1, 2, 3, 4, 5, 6); + }; + +lambda(); + +local copyLambda = copy(copy(copy(lambda))); +copyLambda();