Merge remote-tracking branch 'origin/topic/vern/vector-holes'

* origin/topic/vern/vector-holes:
  Remove NEWS entry regarding changed vector-holes functionality
  Fix potential segfaults in VectorVal Insert/Remove methods
  Fix copy() to work with a vector that has trailing holes
  update test suite for vector holes now being supported for numeric types
  add vector tests for creating holes, "in" operator, "?" operator, copying vectors with holes
  restore support for vectors with holes remove vestigial comment
  fix using ++/-- to vectors that contain holes

CHANGES

@@ -1,4 +1,15 @@
 
+4.1.0-dev.537 | 2021-04-20 14:34:48 -0700
+
+  * Restore support for vectors with holes and improve test cases (Vern Paxson, Corelight)
+
+    There's no longer breaking behavior changes to how vector-holes work from
+    previous Zeek versions.
+
+  * Fix cloning/copying vectors that contain holes (Vern Paxson, Corelight)
+
+  * fix using ++/-- to vectors that contain holes (Vern Paxson, Corelight)
+
 4.1.0-dev.526 | 2021-04-16 16:03:06 -0700
 
   * GH-1506: Fix Broker unserialization of set/table function indices (Jon Siwek, Corelight)

NEWS

@@ -52,19 +52,6 @@ New Functionality
 Changed Functionality
 ---------------------
 
-- The ``in`` operator does not any more track whether a given vector element
-  has been assigned-to. It now simply equates to "is this index between zero
-  and the size-of-the-vector-minus-one, inclusive".  Previously, the ``in``
-  operator could have detected whether, by purpose or accident, a "hole" was
-  created in a vector by way of assigning to an index that's more than 1 beyond
-  the end of the vector (with intermediate elements becoming "holes") and
-  whether a "hole" had ever been subsequently filled by an assignment.
-
-  The behavior of creating a "hole" in a vector was never explicitly
-  documented, and should be thought of as undefined behavior and not something
-  the ``in`` operator can be used to detect.  For this kind of usage, a
-  ``table[count] of X`` is more appropriate than a vector.
-
 Removed Functionality
 ---------------------
 

VERSION

@@ -1 +1 @@
-4.1.0-dev.526
+4.1.0-dev.537

src/Expr.cc

@@ -1239,7 +1239,8 @@ ValPtr IncrExpr::Eval(Frame* f) const
 		for ( unsigned int i = 0; i < v_vec->Size(); ++i )
 			{
 			auto elt = v_vec->ValAt(i);
-			v_vec->Assign(i, DoSingleEval(f, elt.get()));
+			if ( elt )
+				v_vec->Assign(i, DoSingleEval(f, elt.get()));
 			}
 
 		op->Assign(f, std::move(v_vec));

src/Val.cc

@@ -3246,7 +3246,7 @@ ValPtr TypeVal::DoClone(CloneState* state)
 
 VectorVal::VectorVal(VectorTypePtr t) : Val(t)
 	{
-	vector_val = new vector<ZVal>();
+	vector_val = new vector<std::optional<ZVal>>();
 	yield_type = t->Yield();
 
 	auto y_tag = yield_type->Tag();
@@ -3260,14 +3260,19 @@ VectorVal::~VectorVal()
 		{
 		int n = yield_types->size();
 		for ( auto i = 0; i < n; ++i )
-			ZVal::DeleteIfManaged((*vector_val)[i], (*yield_types)[i]);
+			{
+			auto& elem = (*vector_val)[i];
+			if ( elem )
+				ZVal::DeleteIfManaged(*elem, (*yield_types)[i]);
+			}
 		delete yield_types;
 		}
 
 	else if ( managed_yield )
 		{
 		for ( auto& elem : *vector_val )
-			ZVal::DeleteManagedType(elem);
+			if ( elem )
+				ZVal::DeleteManagedType(*elem);
 		}
 
 	delete vector_val;
@@ -3325,7 +3330,9 @@ bool VectorVal::Assign(unsigned int index, ValPtr element)
 
 	if ( index >= n )
 		{
-		AddHoles(index - n);
+		if ( index > n )
+			AddHoles(index - n);
+
 		vector_val->resize(index + 1);
 		if ( yield_types )
 			yield_types->resize(index + 1);
@@ -3335,14 +3342,21 @@ bool VectorVal::Assign(unsigned int index, ValPtr element)
 		{
 		const auto& t = element->GetType();
 		(*yield_types)[index] = t;
-		ZVal::DeleteIfManaged((*vector_val)[index], t);
-		(*vector_val)[index] = ZVal(std::move(element), t);
+		auto& elem = (*vector_val)[index];
+		if ( elem )
+			ZVal::DeleteIfManaged(*elem, t);
+		elem = ZVal(std::move(element), t);
 		}
 	else
 		{
-		if ( managed_yield )
-			ZVal::DeleteManagedType((*vector_val)[index]);
-		(*vector_val)[index] = ZVal(std::move(element), yield_type);
+		auto& elem = (*vector_val)[index];
+		if ( managed_yield && elem )
+			ZVal::DeleteManagedType(*elem);
+
+		if ( element )
+			elem = ZVal(std::move(element), yield_type);
+		else
+			elem = std::nullopt;
 		}
 
 	Modified();
@@ -3366,7 +3380,7 @@ bool VectorVal::Insert(unsigned int index, ValPtr element)
 	if ( ! CheckElementType(element) )
 		return false;
 
-	vector<ZVal>::iterator it;
+	vector<std::optional<ZVal>>::iterator it;
 	vector<TypePtr>::iterator types_it;
 
 	auto n = vector_val->size();
@@ -3376,28 +3390,39 @@ bool VectorVal::Insert(unsigned int index, ValPtr element)
 		it = std::next(vector_val->begin(), index);
 		if ( yield_types )
 			{
-			ZVal::DeleteIfManaged(*it, element->GetType());
+			if ( *it )
+				ZVal::DeleteIfManaged(**it, element->GetType());
 			types_it = std::next(yield_types->begin(), index);
 			}
 		else if ( managed_yield )
-			ZVal::DeleteManagedType(*it);
+			{
+			if ( *it )
+				ZVal::DeleteManagedType(**it);
+			}
 		}
 	else
 		{
 		it = vector_val->end();
 		if ( yield_types )
 			types_it = yield_types->end();
-		AddHoles(index - n);
+
+		if ( index > n )
+			AddHoles(index - n);
 		}
 
-	if ( yield_types )
+	if ( element )
 		{
-		const auto& t = element->GetType();
-		yield_types->insert(types_it, t);
-		vector_val->insert(it, ZVal(std::move(element), t));
+		if ( yield_types )
+			{
+			const auto& t = element->GetType();
+			yield_types->insert(types_it, t);
+			vector_val->insert(it, ZVal(std::move(element), t));
+			}
+		else
+			vector_val->insert(it, ZVal(std::move(element), yield_type));
 		}
 	else
-		vector_val->insert(it, ZVal(std::move(element), yield_type));
+		vector_val->insert(it, std::nullopt);
 
 	Modified();
 	return true;
@@ -3410,7 +3435,7 @@ void VectorVal::AddHoles(int nholes)
 		fill_t = base_type(TYPE_ANY);
 
 	for ( auto i = 0; i < nholes; ++i )
-		vector_val->emplace_back(ZVal(fill_t));
+		vector_val->emplace_back(std::nullopt);
 	}
 
 bool VectorVal::Remove(unsigned int index)
@@ -3423,12 +3448,16 @@ bool VectorVal::Remove(unsigned int index)
 	if ( yield_types )
 		{
 		auto types_it = std::next(yield_types->begin(), index);
-		ZVal::DeleteIfManaged(*it, *types_it);
+		if ( *it )
+			ZVal::DeleteIfManaged(**it, *types_it);
 		yield_types->erase(types_it);
 		}
 
 	else if ( managed_yield )
-		ZVal::DeleteManagedType(*it);
+		{
+		if ( *it )
+			ZVal::DeleteManagedType(**it);
+		}
 
 	vector_val->erase(it);
 
@@ -3465,33 +3494,33 @@ ValPtr VectorVal::At(unsigned int index) const
 	if ( index >= vector_val->size() )
 		return Val::nil;
 
+	auto& elem = (*vector_val)[index];
+	if ( ! elem )
+		return Val::nil;
+
 	const auto& t = yield_types ? (*yield_types)[index] : yield_type;
 
-	return (*vector_val)[index].ToVal(t);
+	return elem->ToVal(t);
 	}
 
 static Func* sort_function_comp = nullptr;
 
 // Used for indirect sorting to support order().
-static std::vector<const ZVal*> index_map;
+static std::vector<const std::optional<ZVal>*> index_map;
 
 // The yield type of the vector being sorted.
 static TypePtr sort_type;
-static bool sort_type_is_managed = false;
 
-static bool sort_function(const ZVal& a, const ZVal& b)
+static bool sort_function(const std::optional<ZVal>& a, const std::optional<ZVal>& b)
 	{
-	// Missing values are only applicable for managed types.
-	if ( sort_type_is_managed )
-		{
-		if ( ! a.ManagedVal() )
-			return 0;
-		if ( ! b.ManagedVal() )
-			return 1;
-		}
+	if ( ! a )
+		return false;
 
-	auto a_v = a.ToVal(sort_type);
-	auto b_v = b.ToVal(sort_type);
+	if ( ! b )
+		return true;
+
+	auto a_v = a->ToVal(sort_type);
+	auto b_v = b->ToVal(sort_type);
 
 	auto result = sort_function_comp->Invoke(a_v, b_v);
 	int int_result = result->CoerceToInt();
@@ -3499,19 +3528,37 @@ static bool sort_function(const ZVal& a, const ZVal& b)
 	return int_result < 0;
 	}
 
-static bool signed_sort_function (const ZVal& a, const ZVal& b)
+static bool signed_sort_function(const std::optional<ZVal>& a, const std::optional<ZVal>& b)
 	{
-	return a.AsInt() < b.AsInt();
+	if ( ! a )
+		return false;
+
+	if ( ! b )
+		return true;
+
+	return a->AsInt() < b->AsInt();
 	}
 
-static bool unsigned_sort_function (const ZVal& a, const ZVal& b)
+static bool unsigned_sort_function(const std::optional<ZVal>& a, const std::optional<ZVal>& b)
 	{
-	return a.AsCount() < b.AsCount();
+	if ( ! a )
+		return false;
+
+	if ( ! b )
+		return true;
+
+	return a->AsCount() < b->AsCount();
 	}
 
-static bool double_sort_function (const ZVal& a, const ZVal& b)
+static bool double_sort_function(const std::optional<ZVal>& a, const std::optional<ZVal>& b)
 	{
-	return a.AsDouble() < b.AsDouble();
+	if ( ! a )
+		return false;
+
+	if ( ! b )
+		return true;
+
+	return a->AsDouble() < b->AsDouble();
 	}
 
 static bool indirect_sort_function(size_t a, size_t b)
@@ -3540,9 +3587,8 @@ void VectorVal::Sort(Func* cmp_func)
 		reporter->RuntimeError(GetLocationInfo(), "cannot sort a vector-of-any");
 
 	sort_type = yield_type;
-	sort_type_is_managed = ZVal::IsManagedType(sort_type);
 
-	bool (*sort_func)(const ZVal&, const ZVal&);
+	bool (*sort_func)(const std::optional<ZVal>&, const std::optional<ZVal>&);
 
 	if ( cmp_func )
 		{
@@ -3577,7 +3623,6 @@ VectorValPtr VectorVal::Order(Func* cmp_func)
 		}
 
 	sort_type = yield_type;
-	sort_type_is_managed = ZVal::IsManagedType(sort_type);
 
 	bool (*sort_func)(size_t, size_t);
 
@@ -3671,8 +3716,8 @@ ValPtr VectorVal::DoClone(CloneState* state)
 
 	for ( auto i = 0; i < n; ++i )
 		{
-		auto vc = At(i)->Clone(state);
-		vv->Append(std::move(vc));
+		auto elem = At(i);
+		vv->Assign(i, elem ? elem->Clone(state) : nullptr);
 		}
 
 	return vv;

src/Val.h

@@ -1505,10 +1505,6 @@ public:
 	 */
 	bool Assign(unsigned int index, ValPtr element);
 
-	// Note: the following nullptr method can also go upon removing the above.
-	void Assign(unsigned int index, std::nullptr_t)
-		{ Assign(index, ValPtr{}); }
-
 	/**
 	 * Assigns a given value to multiple indices in the vector.
 	 * @param index  The starting index to assign to.
@@ -1581,18 +1577,19 @@ public:
 	/**
 	 * Returns the given element in a given underlying representation.
 	 * Enables efficient vector access.  Caller must ensure that the
-	 * index lies within the vector's range.
+	 * index lies within the vector's range, and does not point to
+	 * a "hole".
 	 * @param index  The position in the vector of the element to return.
 	 * @return  The element's underlying value.
 	 */
 	bro_uint_t CountAt(unsigned int index) const
-		{ return (*vector_val)[index].uint_val; }
+		{ return (*vector_val)[index]->uint_val; }
 	const RecordVal* RecordValAt(unsigned int index) const
-		{ return (*vector_val)[index].record_val; }
+		{ return (*vector_val)[index]->record_val; }
 	bool BoolAt(unsigned int index) const
-		{ return static_cast<bool>((*vector_val)[index].uint_val); }
+		{ return static_cast<bool>((*vector_val)[index]->uint_val); }
 	const StringVal* StringValAt(unsigned int index) const
-		{ return (*vector_val)[index].string_val; }
+		{ return (*vector_val)[index]->string_val; }
 	const String* StringAt(unsigned int index) const
 		{ return StringValAt(index)->AsString(); }
 
@@ -1601,7 +1598,7 @@ protected:
 	 * Returns the element at a given index or nullptr if it does not exist.
 	 * @param index  The position in the vector of the element to return.
 	 * @return  The element at the given index or nullptr if the index
-	 * does not exist (it's greater than or equal to vector's current size).
+	 * does not exist.
 	 *
 	 * Protected to ensure callers pick one of the differentiated accessors
 	 * above, as appropriate, with ValAt() providing the original semantics.
@@ -1624,7 +1621,7 @@ private:
 	// Add the given number of "holes" to the end of a vector.
 	void AddHoles(int nholes);
 
-	std::vector<ZVal>* vector_val;
+	std::vector<std::optional<ZVal>>* vector_val;
 
 	// For homogeneous vectors (the usual case), the type of the
 	// elements.  Will be TYPE_VOID for empty vectors created using

testing/btest/Baseline/language.record-index-complex-fields/output

@@ -11,7 +11,7 @@ b
 }
 }
 {
-[a=13, tags_v=[0, 0, 2, 3], tags_t={
+[a=13, tags_v=[, , 2, 3], tags_t={
 [four] = 4,
 [five] = 5
 }, tags_s={

testing/btest/Baseline/language.vector/out

@@ -34,6 +34,11 @@ add element (PASS)
 access element (PASS)
 add element (PASS)
 access element (PASS)
+add above a hole (PASS)
+in operator for non-hole (PASS)
+in operator for hole (PASS)
+in operator for edge (PASS)
+in operator for out of range (PASS)
 add element (PASS)
 access element (PASS)
 overwrite element (PASS)
@@ -68,3 +73,8 @@ slicing assignment (PASS)
 slicing assignment (PASS)
 slicing assignment grow (PASS)
 slicing assignment shrink (PASS)
+? operator (PASS)
+copy of a vector with holes (PASS)
+copy of a vector with trailing holes, [0, 2, 3, 77, , ], [0, 2, 3, 77, , ]
+hole in vector of managed types, 5, [[a=T], [a=T], , , [a=T]]
+hole in vector of managed types after replacing slice, 3, [[a=T], [a=T], ]

testing/btest/Baseline/scripts.base.frameworks.logging.ascii-json/ssh.log

@@ -1,2 +1,2 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-{"b":true,"i":-42,"e":"SSH::LOG","c":21,"p":123,"sn":"10.0.0.0/24","a":"1.2.3.4","d":3.14,"t":XXXXXXXXXX.XXXXXX,"iv":100.0,"s":"hurz","sc":[4,2,3,1],"ss":["CC","BB","AA"],"se":[],"vc":[10,20,30],"ve":[],"vn":[0,0,2],"f":"SSH::foo\n{ \nif (0 < SSH::i) \n\treturn (Foo);\nelse\n\treturn (Bar);\n\n}"}
+{"b":true,"i":-42,"e":"SSH::LOG","c":21,"p":123,"sn":"10.0.0.0/24","a":"1.2.3.4","d":3.14,"t":XXXXXXXXXX.XXXXXX,"iv":100.0,"s":"hurz","sc":[4,2,3,1],"ss":["CC","BB","AA"],"se":[],"vc":[10,20,30],"ve":[],"vn":[0,null,2],"f":"SSH::foo\n{ \nif (0 < SSH::i) \n\treturn (Foo);\nelse\n\treturn (Bar);\n\n}"}

testing/btest/Baseline/scripts.base.utils.json/output

@@ -29,7 +29,7 @@ XXXXXXXXXX.XXXXXX
 ["1.2.3.4"]
 [[false,true]]
 [{"s":"test"}]
-[0,0,2]
+[0,null,2]
 []
 [2,1]
 ["1.2.3.4"]

testing/btest/language/vector.zeek

@@ -10,6 +10,11 @@ function test_case(msg: string, expect: bool)
 # Note: only global vectors can be initialized with curly braces
 global vg1: vector of string = { "curly", "braces" };
 
+type R: record {
+	a: bool &default=T;
+};
+
+
 event zeek_init()
 {
 	local v1: vector of string = vector( "test", "example" );
@@ -27,6 +32,7 @@ event zeek_init()
 	local v13 = vector( F, F, T );
 	local v14 = v12 && v13;
 	local v15 = v12 || v13;
+	local v18 = v12 ? v5 : v6;
 
 	# Type inference tests
 
@@ -110,6 +116,13 @@ event zeek_init()
 	test_case( "add element", |v5| == 4 );
 	test_case( "access element", v5[3] == 77 );
 
+	v5[10] = 10;
+	test_case( "add above a hole", |v5| == 11 );
+	test_case( "in operator for non-hole", 3 in v5 );
+	test_case( "in operator for hole", 4 !in v5 );
+	test_case( "in operator for edge", |v5|-1 in v5 );
+	test_case( "in operator for out of range", 44 !in v5 );
+
 	vg1[2] = "global";
 	test_case( "add element", |vg1| == 3 );
 	test_case( "access element", vg1[2] == "global" );
@@ -134,7 +147,7 @@ event zeek_init()
 	test_case( "access element", v4[0] == "new4" );
 
 	v5[0] = 0;
-	test_case( "overwrite element", |v5| == 4 );
+	test_case( "overwrite element", |v5| == 11 );
 	test_case( "access element", v5[0] == 0 );
 
 	vg1[1] = "new5";
@@ -144,11 +157,13 @@ event zeek_init()
 	# Test increment/decrement operators
 
 	++v5;
-	test_case( "++ operator", |v5| == 4 && v5[0] == 1 && v5[1] == 3
-			 && v5[2] == 4 && v5[3] == 78 );
+	test_case( "++ operator", |v5| == 11 && v5[0] == 1 && v5[1] == 3
+			 && v5[2] == 4 && v5[3] == 78 && v5[10] == 11
+			 && 4 !in v5 );
 	--v5;
-	test_case( "-- operator", |v5| == 4 && v5[0] == 0 && v5[1] == 2
-			 && v5[2] == 3 && v5[3] == 77 );
+	test_case( "-- operator", |v5| == 11 && v5[0] == 0 && v5[1] == 2
+			 && v5[2] == 3 && v5[3] == 77 && v5[10] == 10
+			 && 4 !in v5 );
 
 	# Test +,-,*,/,% of two vectors
 
@@ -183,4 +198,23 @@ event zeek_init()
 	test_case( "slicing assignment grow", all_set(v17 == vector(6, 2, 9, 10, 11, 5)) );
 	v17[2:5] = vector(9);
 	test_case( "slicing assignment shrink", all_set(v17 == vector(6, 2, 9, 5)) );
+
+	# Test boolean ? operator.
+	test_case( "? operator", all_set(v18 == vector(1, 20, 3)) );
+
+	# Test copying of a vector with holes, as this used to crash.
+	local v19 = copy(v5);
+	test_case( "copy of a vector with holes", |v5| == |v19| );
+	# Even after removing some elements at the end, any trailing holes should
+	# be preserved after copying;
+	v5[6:] = vector();
+	local v20 = copy(v5);
+	print "copy of a vector with trailing holes", v5, v20;
+
+	local v21 = vector(R(), R());
+	v21[4] = R();
+	print "hole in vector of managed types", |v21|, v21;
+	v21[3:] = vector();
+	print "hole in vector of managed types after replacing slice", |v21|, v21;
+
 }