From d782c60f199afadb8d389c36474d09456f4bb205 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Sun, 4 Oct 2020 16:42:44 +0000 Subject: [PATCH] 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))