From d782c60f199afadb8d389c36474d09456f4bb205 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Sun, 4 Oct 2020 16:42:44 +0000 Subject: [PATCH 1/3] Unify type comparisions for records. For records, same_type(r1, r2) would not check if the fields' attributes match as well. That seems like an oversight, and some callers of same_type() did indeed add that check on their end. This commit moves the check into same_type() itself. That generally doesn't seem make any differences except for a couple of places validating code, which we update a bit. That in turn leans to slightly different (better?) error messages for a couple of test cases. --- src/Expr.cc | 75 +++++++------------ src/Expr.h | 5 ++ src/Type.cc | 3 + .../Baseline/language.record-bad-ctor/out | 4 +- .../output | 10 ++- 5 files changed, 45 insertions(+), 52 deletions(-) diff --git a/src/Expr.cc b/src/Expr.cc index 2a08b630d2..535a267420 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -2168,22 +2168,14 @@ bool AssignExpr::TypeCheck(const AttributesPtr& attrs) if ( op1->GetType()->IsSet() ) op2 = make_intrusive( - cast_intrusive(op2), std::move(attr_copy)); + cast_intrusive(op2), std::move(attr_copy), op1->GetType()); else op2 = make_intrusive( - cast_intrusive(op2), std::move(attr_copy)); + cast_intrusive(op2), std::move(attr_copy), op1->GetType()); - if ( ! empty_list_assignment && ! same_type(op1->GetType(), op2->GetType()) ) - { - if ( op1->GetType()->IsSet() ) - ExprError("set type mismatch in assignment"); - else - ExprError("table type mismatch in assignment"); - - return false; - } - - return true; + // The constructor expressions are performing the type + // checking and will set op2 to an error state on failure. + return ! op2->IsError(); } if ( bt1 == TYPE_VECTOR ) @@ -2207,21 +2199,7 @@ bool AssignExpr::TypeCheck(const AttributesPtr& attrs) op2->GetType()->Tag() == TYPE_RECORD ) { if ( same_type(op1->GetType(), op2->GetType()) ) - { - RecordType* rt1 = op1->GetType()->AsRecordType(); - RecordType* rt2 = op2->GetType()->AsRecordType(); - - // Make sure the attributes match as well. - for ( int i = 0; i < rt1->NumFields(); ++i ) - { - const TypeDecl* td1 = rt1->FieldDecl(i); - const TypeDecl* td2 = rt2->FieldDecl(i); - - if ( same_attrs(td1->attrs.get(), td2->attrs.get()) ) - // Everything matches. - return true; - } - } + return true; // Need to coerce. op2 = make_intrusive(std::move(op2), op1->GetType()); @@ -3195,17 +3173,18 @@ TableConstructorExpr::TableConstructorExpr(ListExprPtr constructor_list, const auto& indices = type->AsTableType()->GetIndices()->GetTypes(); const ExprPList& cle = op->AsListExpr()->Exprs(); - // check and promote all index expressions in ctor list + // check and promote all assign expressions in ctor list for ( const auto& expr : cle ) { if ( expr->Tag() != EXPR_ASSIGN ) continue; - Expr* idx_expr = expr->AsAssignExpr()->Op1(); - - if ( idx_expr->Tag() != EXPR_LIST ) - continue; + auto idx_expr = expr->AsAssignExpr()->Op1(); + auto val_expr = expr->AsAssignExpr()->Op2(); + auto yield_type = GetType()->AsTableType()->Yield().get(); + // Promote LHS + assert(idx_expr->Tag() == EXPR_LIST); ExprPList& idx_exprs = idx_expr->AsListExpr()->Exprs(); if ( idx_exprs.length() != static_cast(indices.size()) ) @@ -3229,6 +3208,19 @@ TableConstructorExpr::TableConstructorExpr(ListExprPtr constructor_list, } ExprError("inconsistent types in table constructor"); + return; + } + + // Promote RHS + if ( auto promoted_val = check_and_promote_expr(val_expr, yield_type) ) + { + if ( promoted_val != val_expr ) + expr->AsAssignExpr()->SetOp2(promoted_val); + } + else + { + ExprError("inconsistent types in table constructor"); + return; } } } @@ -3328,8 +3320,8 @@ SetConstructorExpr::SetConstructorExpr(ListExprPtr constructor_list, Expr* ce = cle[i]; ListExpr* le = ce->AsListExpr(); - if ( ce->Tag() == EXPR_LIST && - check_and_promote_exprs(le, type->AsTableType()->GetIndices().get()) ) + assert(ce->Tag() == EXPR_LIST); + if ( check_and_promote_exprs(le, type->AsTableType()->GetIndices().get()) ) { if ( le != cle[i] ) cle.replace(i, le); @@ -4985,18 +4977,7 @@ ExprPtr check_and_promote_expr(Expr* const e, zeek::Type* t) RecordType* et_r = et->AsRecordType(); if ( same_type(t, et) ) - { - // Make sure the attributes match as well. - for ( int i = 0; i < t_r->NumFields(); ++i ) - { - const TypeDecl* td1 = t_r->FieldDecl(i); - const TypeDecl* td2 = et_r->FieldDecl(i); - - if ( same_attrs(td1->attrs.get(), td2->attrs.get()) ) - // Everything matches perfectly. - return {NewRef{}, e}; - } - } + return {NewRef{}, e}; if ( record_promotion_compatible(t_r, et_r) ) return make_intrusive( diff --git a/src/Expr.h b/src/Expr.h index 5bb2af6844..a1dd60ac33 100644 --- a/src/Expr.h +++ b/src/Expr.h @@ -504,6 +504,11 @@ public: ValPtr InitVal(const zeek::Type* t, ValPtr aggr) const override; bool IsPure() const override; + void SetOp2(ExprPtr e) + { + op2 = e; + } + protected: bool TypeCheck(const AttributesPtr& attrs = nullptr); bool TypeCheckArithmetics(TypeTag bt1, TypeTag bt2); diff --git a/src/Type.cc b/src/Type.cc index 80dafc7396..35b3cfdc94 100644 --- a/src/Type.cc +++ b/src/Type.cc @@ -1690,6 +1690,9 @@ bool same_type(const Type& arg_t1, const Type& arg_t2, if ( (match_record_field_names && ! util::streq(td1->id, td2->id)) || ! same_type(td1->type, td2->type, is_init, match_record_field_names) ) return false; + + if ( ! same_attrs(td1->attrs.get(), td2->attrs.get()) ) + return false; } return true; diff --git a/testing/btest/Baseline/language.record-bad-ctor/out b/testing/btest/Baseline/language.record-bad-ctor/out index e6ff4a8fd5..950175f145 100644 --- a/testing/btest/Baseline/language.record-bad-ctor/out +++ b/testing/btest/Baseline/language.record-bad-ctor/out @@ -1,2 +1,2 @@ -error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.record-bad-ctor/record-bad-ctor.zeek, line 6: no type given (asdfasdf) -expression error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.record-bad-ctor/record-bad-ctor.zeek, line 7: uninitialized list value ($ports=asdfasdf) +error in /Users/robin/bro/topic/testing/btest/.tmp/language.record-bad-ctor/record-bad-ctor.zeek, line 6: no type given (asdfasdf) +error in /Users/robin/bro/topic/testing/btest/.tmp/language.record-bad-ctor/record-bad-ctor.zeek, line 7: non-optional field "ports" missing in initialization ([ports=]) diff --git a/testing/btest/Baseline/language.table-list-assign-type-check/output b/testing/btest/Baseline/language.table-list-assign-type-check/output index 2062d3f4f3..d557c007b1 100644 --- a/testing/btest/Baseline/language.table-list-assign-type-check/output +++ b/testing/btest/Baseline/language.table-list-assign-type-check/output @@ -1,3 +1,7 @@ -error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, lines 15-20: table type mismatch in assignment (service_table_bad_yield = table(www, 80 = Internal Web Server, dns1, 53 = Internal DNS 1, dns2, 53 = Internal DNS 2, dhcp-for-wifi, 443 = DHCP Management interface for WiFi)) -error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, lines 23-28: table type mismatch in assignment (service_table_bad_index = table(www, 80 = Internal Web Server, dns1, 53 = Internal DNS 1, dns2, 53 = Internal DNS 2, dhcp-for-wifi, 443 = DHCP Management interface for WiFi)) -error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 31: set type mismatch in assignment (test_set_bad = set(1, 2, 3)) +error in count and /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 16: arithmetic mixed with non-arithmetic (count and Internal Web Server) +error in /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, lines 15-20: inconsistent types in table constructor (table(www, 80 = Internal Web Server, dns1, 53 = Internal DNS 1, dns2, 53 = Internal DNS 2, dhcp-for-wifi, 443 = DHCP Management interface for WiFi)) +error in count and /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 24: arithmetic mixed with non-arithmetic (count and 80) +error in /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, lines 23-28: inconsistent types in table constructor (table(www, 80 = Internal Web Server, dns1, 53 = Internal DNS 1, dns2, 53 = Internal DNS 2, dhcp-for-wifi, 443 = DHCP Management interface for WiFi)) +error in string and /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 31: arithmetic mixed with non-arithmetic (string and 1) +error in /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 31 and string: type mismatch (1 and string) +error in /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 31: inconsistent type in set constructor (set(1, 2, 3)) From e9aa531b837756f8a6855a6e6123e0926333bc99 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Fri, 2 Oct 2020 14:17:43 +0000 Subject: [PATCH 2/3] Optimize record constructor expression. We remove the inheritance from UnaryExpression because we know the type of the operand precisely and can skip a temporary when evaluating the expression. #425 --- src/Expr.cc | 32 ++++++++++++++++++++++++++------ src/Expr.h | 13 +++++++++++-- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/src/Expr.cc b/src/Expr.cc index 535a267420..e95c3db825 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -3055,7 +3055,7 @@ void HasFieldExpr::ExprDescribe(ODesc* d) const } RecordConstructorExpr::RecordConstructorExpr(ListExprPtr constructor_list) - : UnaryExpr(EXPR_RECORD_CONSTRUCTOR, std::move(constructor_list)) + : Expr(EXPR_RECORD_CONSTRUCTOR), op(std::move(constructor_list)) { if ( IsError() ) return; @@ -3108,22 +3108,30 @@ ValPtr RecordConstructorExpr::InitVal(const zeek::Type* t, ValPtr aggr) const return nullptr; } -ValPtr RecordConstructorExpr::Fold(Val* v) const +ValPtr RecordConstructorExpr::Eval(Frame* f) const { - ListVal* lv = v->AsListVal(); + if ( IsError() ) + return nullptr; + + const auto& exprs = op->Exprs(); auto rt = cast_intrusive(type); - if ( lv->Length() != rt->NumFields() ) + if ( exprs.length() != rt->NumFields() ) RuntimeErrorWithCallStack("inconsistency evaluating record constructor"); auto rv = make_intrusive(std::move(rt)); - for ( int i = 0; i < lv->Length(); ++i ) - rv->Assign(i, lv->Idx(i)); + for ( int i = 0; i < exprs.length(); ++i ) + rv->Assign(i, exprs[i]->Eval(f)); return rv; } +bool RecordConstructorExpr::IsPure() const + { + return op->IsPure(); + } + void RecordConstructorExpr::ExprDescribe(ODesc* d) const { d->Add("["); @@ -3131,6 +3139,18 @@ void RecordConstructorExpr::ExprDescribe(ODesc* d) const d->Add("]"); } +TraversalCode RecordConstructorExpr::Traverse(TraversalCallback* cb) const + { + TraversalCode tc = cb->PreExpr(this); + HANDLE_TC_EXPR_PRE(tc); + + tc = op->Traverse(cb); + HANDLE_TC_EXPR_PRE(tc); + + tc = cb->PostExpr(this); + HANDLE_TC_EXPR_POST(tc); + } + TableConstructorExpr::TableConstructorExpr(ListExprPtr constructor_list, std::unique_ptr> arg_attrs, TypePtr arg_type) diff --git a/src/Expr.h b/src/Expr.h index a1dd60ac33..4456b6fa4f 100644 --- a/src/Expr.h +++ b/src/Expr.h @@ -630,16 +630,25 @@ protected: int field; }; -class RecordConstructorExpr final : public UnaryExpr { +class RecordConstructorExpr final : public Expr { public: explicit RecordConstructorExpr(ListExprPtr constructor_list); ~RecordConstructorExpr() override; + ListExpr* Op() const { return op.get(); } + + ValPtr Eval(Frame* f) const override; + + bool IsPure() const override; + + TraversalCode Traverse(TraversalCallback* cb) const override; + protected: ValPtr InitVal(const zeek::Type* t, ValPtr aggr) const override; - ValPtr Fold(Val* v) const override; void ExprDescribe(ODesc* d) const override; + + ListExprPtr op; }; class TableConstructorExpr final : public UnaryExpr { From 553ce285008cc1c3ae5d7b832e2785950fe5e874 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Fri, 2 Oct 2020 15:45:03 +0000 Subject: [PATCH 3/3] Avoid unnecessary temporary value when coercing a record that's already the right type. The combination of this commit with the previous one now lets the examples in #425 all execute with the same performance. Closes #425. --- src/Expr.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Expr.cc b/src/Expr.cc index e95c3db825..b37a307ad8 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -3758,6 +3758,9 @@ ValPtr RecordCoerceExpr::InitVal(const zeek::Type* t, ValPtr aggr) const ValPtr RecordCoerceExpr::Fold(Val* v) const { + if ( same_type(GetType(), Op()->GetType()) ) + return IntrusivePtr{NewRef{}, v}; + auto val = make_intrusive(GetType()); RecordType* val_type = val->GetType()->AsRecordType();