21489: Fixes logic bugs involving null values (#260)

src/Amalgam/Parser.cpp

@@ -651,7 +651,7 @@ EvaluableNode *Parser::ParseNextBlock()
 			//if specifying something unusual, then assume it's just a null
 			if(n->GetType() == ENT_NOT_A_BUILT_IN_TYPE)
 			{
-				n->SetType(ENT_NULL, evaluableNodeManager);
+				n->SetType(ENT_NULL, nullptr, false);
 				if(!originalSource.empty())
 					std::cerr << "Warning: "  << " Invalid opcode at line " << lineNumber + 1 << " of " << originalSource << std::endl;
 			}

src/Amalgam/entity/Entity.cpp

@@ -254,7 +254,7 @@ bool Entity::SetValueAtLabel(StringInternPool::StringID label_sid, EvaluableNode
 	if(!on_self)
 	{
 		if(IsLabelPrivate(label_sid))
-			return EvaluableNodeReference(nullptr, true);
+			return EvaluableNodeReference::Null();
 
 		//since it's not setting on self, another entity owns the data so it isn't unique to this entity
 		new_value.unique = false;
@@ -462,7 +462,7 @@ EvaluableNodeReference Entity::Execute(StringInternPool::StringID label_sid,
 	)
 {
 	if(!on_self && IsLabelPrivate(label_sid))
-		return EvaluableNodeReference(nullptr, true);
+		return EvaluableNodeReference::Null();
 
 	EvaluableNode *node_to_execute = nullptr;
 	if(label_sid == string_intern_pool.NOT_A_STRING_ID)	//if not specified, then use root

src/Amalgam/evaluablenode/EvaluableNode.cpp

@@ -592,22 +592,39 @@ void EvaluableNode::SetType(EvaluableNodeType new_type, EvaluableNodeManager *en
 		if(attempt_to_preserve_immediate_value)
 			number_value = EvaluableNode::ToNumber(this);
 
-		InitNumberValue();
-		GetNumberValueReference() = number_value;
+		if(FastIsNaN(number_value))
+		{
+			InitOrderedChildNodes();
+			SetNeedCycleCheck(false);
+		}
+		else
+		{
+			InitNumberValue();
+			GetNumberValueReference() = number_value;
 
-		//will check below if any reason to not be idempotent
-		SetIsIdempotent(true);
+			//will check below if any reason to not be idempotent
+			SetIsIdempotent(true);
+		}
 	}
 	else if(DoesEvaluableNodeTypeUseStringData(new_type))
 	{
-		StringInternPool::StringID sid = string_intern_pool.NOT_A_STRING_ID;
+		StringInternPool::StringID sid = string_intern_pool.emptyStringId;
 		if(attempt_to_preserve_immediate_value)
 			sid = EvaluableNode::ToStringIDWithReference(this);
-		InitStringValue();
-		GetStringIDReference() = sid;
 
-		//will check below if any reason to not be idempotent
-		SetIsIdempotent(new_type == ENT_STRING);
+		if(sid == string_intern_pool.NOT_A_STRING_ID)
+		{
+			InitOrderedChildNodes();
+			SetNeedCycleCheck(false);
+		}
+		else
+		{
+			InitStringValue();
+			GetStringIDReference() = sid;
+
+			//will check below if any reason to not be idempotent
+			SetIsIdempotent(new_type == ENT_STRING);
+		}
 	}
 	else if(DoesEvaluableNodeTypeUseAssocData(new_type))
 	{
@@ -722,7 +739,7 @@ void EvaluableNode::SetStringID(StringInternPool::StringID id)
 {
 	if(id == StringInternPool::NOT_A_STRING_ID)
 	{
-		SetType(ENT_NULL);
+		SetType(ENT_NULL, nullptr, false);
 	}
 	else
 	{
@@ -812,7 +829,7 @@ void EvaluableNode::SetStringIDWithReferenceHandoff(StringInternPool::StringID i
 {
 	if(id == StringInternPool::NOT_A_STRING_ID)
 	{
-		SetType(ENT_NULL);
+		SetType(ENT_NULL, nullptr, false);
 	}
 	else
 	{

src/Amalgam/evaluablenode/EvaluableNode.h

@@ -457,8 +457,8 @@ class EvaluableNode
 	// if enm is nullptr, then it will not necessarily keep child nodes
 	//if attempt_to_preserve_immediate_value is true, then it will try to preserve any relevant immediate value
 	// attempt_to_preserve_immediate_value should be set to false if the value will be immediately overwritten
-	void SetType(EvaluableNodeType new_type, EvaluableNodeManager *enm = nullptr,
-		bool attempt_to_preserve_immediate_value = true);
+	void SetType(EvaluableNodeType new_type, EvaluableNodeManager *enm,
+		bool attempt_to_preserve_immediate_value);
 
 	//sets up number value
 	void InitNumberValue();
@@ -487,6 +487,20 @@ class EvaluableNode
 		}
 	}
 
+	//changes the type by setting it to the string id value specified, handing off the reference
+	inline void SetTypeViaStringIdValueWithReferenceHandoff(StringInternPool::StringID v)
+	{
+		if(v == string_intern_pool.NOT_A_STRING_ID)
+		{
+			SetType(ENT_NULL, nullptr, false);
+		}
+		else
+		{
+			SetType(ENT_STRING, nullptr, false);
+			GetStringIDReference() = v;
+		}
+	}
+
 	//sets up the ability to contain a string
 	void InitStringValue();
 	__forceinline StringInternPool::StringID GetStringID()

src/Amalgam/evaluablenode/EvaluableNodeManagement.cpp

@@ -1010,7 +1010,7 @@ void EvaluableNodeManager::MarkAllReferencedNodesInUseRecurse(EvaluableNode *tre
 }
 
 #ifdef MULTITHREAD_SUPPORT
-void EvaluableNodeManager::MarkAllReferencedNodesInUseRecurseConcurrent(EvaluableNode* tree)
+void EvaluableNodeManager::MarkAllReferencedNodesInUseRecurseConcurrent(EvaluableNode *tree)
 {
 #ifdef AMALGAM_FAST_MEMORY_INTEGRITY
 	assert(tree->IsNodeValid());

src/Amalgam/evaluablenode/EvaluableNodeManagement.h

@@ -39,8 +39,10 @@ class EvaluableNodeReference
 		: value(value), unique(true)
 	{	}
 
-	__forceinline EvaluableNodeReference(StringInternPool::StringID string_id)
-		: value(string_intern_pool.CreateStringReference(string_id)), unique(true)
+	//if reference_handoff is true, it will assume ownership rather than creating a new reference
+	__forceinline EvaluableNodeReference(StringInternPool::StringID string_id, bool reference_handoff = false)
+		: value(reference_handoff ? string_id : string_intern_pool.CreateStringReference(string_id)),
+		unique(true)
 	{	}
 
 	__forceinline EvaluableNodeReference(const std::string &str)
@@ -161,7 +163,7 @@ class EvaluableNodeReference
 
 	__forceinline static EvaluableNodeReference Null()
 	{
-		return EvaluableNodeReference(nullptr, true);
+		return EvaluableNodeReference(static_cast<EvaluableNode *>(nullptr), true);
 	}
 
 	__forceinline void SetReference(EvaluableNode *_reference)
@@ -516,7 +518,7 @@ class EvaluableNodeManager
 		//perform a handoff in case candidate is the only value
 		string_intern_pool.CreateStringReference(value);
 		EvaluableNodeReference node = ReuseOrAllocNode(candidate, ENT_STRING);
-		node->SetStringIDWithReferenceHandoff(value);
+		node->SetTypeViaStringIdValueWithReferenceHandoff(value);
 		return node;
 	}
 
@@ -565,7 +567,7 @@ class EvaluableNodeManager
 		//perform a handoff in case one of the candidates is the only value
 		string_intern_pool.CreateStringReference(value);
 		EvaluableNodeReference node = ReuseOrAllocOneOfNodes(candidate_1, candidate_2, ENT_STRING);
-		node->SetStringIDWithReferenceHandoff(value);
+		node->SetTypeViaStringIdValueWithReferenceHandoff(value);
 		return node;
 	}
 
@@ -1008,7 +1010,7 @@ class EvaluableNodeManager
 	static void MarkAllReferencedNodesInUseRecurse(EvaluableNode *tree);
 
 #ifdef MULTITHREAD_SUPPORT
-	static void MarkAllReferencedNodesInUseRecurseConcurrent(EvaluableNode* tree);
+	static void MarkAllReferencedNodesInUseRecurseConcurrent(EvaluableNode *tree);
 #endif
 
 	//helper method for ValidateEvaluableNodeTreeMemoryIntegrity

src/Amalgam/evaluablenode/EvaluableNodeTreeDifference.cpp

@@ -140,7 +140,7 @@ EvaluableNode *EvaluableNodeTreeDifference::DifferenceTrees(EvaluableNodeManager
 		}
 		else //need to create a list and transform it into (set_type ... type)
 		{
-			replacement->SetType(ENT_LIST, enm);
+			replacement->SetType(ENT_LIST, enm, false);
 			EvaluableNode *set_type = enm->AllocNode(ENT_SET_TYPE);
 			set_type->AppendOrderedChildNode(replacement);
 			set_type->AppendOrderedChildNode(enm->AllocNode(ENT_STRING, GetStringFromEvaluableNodeType(replacement_type)));

src/Amalgam/evaluablenode/EvaluableNodeTreeFunctions.cpp

@@ -478,10 +478,17 @@ EvaluableNodeReference AccumulateEvaluableNodeIntoEvaluableNode(EvaluableNodeRef
 		{
 			auto [cur_value_valid, cur_value] = EvaluableNode::ToString(value_destination_node);
 			auto [inc_value_valid, inc_value] = EvaluableNode::ToString(variable_value_node);
-			value_destination_node->SetType(ENT_STRING, enm);
+
 			//string will default to invalid value -- only set if both strings are valid
 			if(cur_value_valid && inc_value_valid)
+			{
+				value_destination_node->SetType(ENT_STRING, nullptr, false);
 				value_destination_node->SetStringValue(cur_value.append(inc_value));
+			}
+			else
+			{
+				value_destination_node->SetType(ENT_NULL, nullptr, false);
+			}
 
 			value_destination_node.unique = true;
 		}
@@ -558,12 +565,12 @@ EvaluableNodeReference AccumulateEvaluableNodeIntoEvaluableNode(EvaluableNodeRef
 	{
 		auto [cur_value_valid, cur_value] = EvaluableNode::ToString(value_destination_node);
 		auto [inc_value_valid, inc_value] = EvaluableNode::ToString(variable_value_node);
-		value_destination_node->SetType(ENT_STRING, enm);
+
 		//string will default to invalid value -- only set if both strings are valid
 		if(cur_value_valid && inc_value_valid)
 			value_destination_node.SetReference(enm->AllocNode(ENT_STRING, cur_value.append(inc_value)), true);
 		else
-			value_destination_node.SetReference(enm->AllocNode(ENT_STRING), true);
+			value_destination_node.SetReference(enm->AllocNode(ENT_NULL), true);
 	}
 	else //add ordered child node
 	{

src/Amalgam/evaluablenode/EvaluableNodeTreeFunctions.h

@@ -442,7 +442,7 @@ inline EvaluableNodeReference CreateListOfStringsIdsFromIteratorAndFunction(Stri
 
 	size_t index = 0;
 	for(auto string_element : string_container)
-		ocn[index++]->SetStringIDWithReferenceHandoff(get_string_id(string_element));
+		ocn[index++]->SetTypeViaStringIdValueWithReferenceHandoff(get_string_id(string_element));
 
 	return EvaluableNodeReference(list, true);
 }

src/Amalgam/evaluablenode/EvaluableNodeTreeManipulation.cpp

@@ -1527,7 +1527,7 @@ EvaluableNode *EvaluableNodeTreeManipulation::MutateNode(EvaluableNode *n, Mutat
 	switch(mutation_type)
 	{
 		case ENBISI_change_type:
-			n->SetType(mp.randEvaluableNodeType->WeightedDiscreteRand(mp.interpreter->randomStream), mp.enm);
+			n->SetType(mp.randEvaluableNodeType->WeightedDiscreteRand(mp.interpreter->randomStream), mp.enm, true);
 			if(IsEvaluableNodeTypeImmediate(n->GetType()))
 				MutateImmediateNode(n, mp.interpreter->randomStream, *mp.strings);
 			break;
@@ -1556,7 +1556,7 @@ EvaluableNode *EvaluableNodeTreeManipulation::MutateNode(EvaluableNode *n, Mutat
 
 			}
 			else
-				n->SetType(ENT_NULL, mp.enm);
+				n->SetType(ENT_NULL, nullptr, false);
 			break;
 
 		case ENBISI_insert:

src/Amalgam/interpreter/Interpreter.cpp

@@ -598,13 +598,21 @@ StringInternPool::StringID Interpreter::InterpretNodeIntoStringIDValueWithRefere
 	}
 }
 
-EvaluableNodeReference Interpreter::InterpretNodeIntoUniqueStringIDValueEvaluableNode(EvaluableNode *n)
+EvaluableNodeReference Interpreter::InterpretNodeIntoUniqueStringIDValueEvaluableNode(
+	EvaluableNode *n, bool immediate_result)
 {
 	//if can skip InterpretNode, then just allocate the string
 	if(n == nullptr || n->GetIsIdempotent()
-			|| n->GetType() == ENT_STRING || n->GetType() == ENT_NUMBER)
-		return EvaluableNodeReference(evaluableNodeManager->AllocNodeWithReferenceHandoff(ENT_STRING,
-												EvaluableNode::ToStringIDWithReference(n)), true);
+		|| n->GetType() == ENT_STRING || n->GetType() == ENT_NUMBER)
+	{
+		auto sid = EvaluableNode::ToStringIDWithReference(n);
+
+		if(immediate_result)
+			return EvaluableNodeReference(sid, true);
+		else
+			return EvaluableNodeReference(evaluableNodeManager->AllocNodeWithReferenceHandoff(ENT_STRING,
+				sid), true);
+	}
 
 	auto result = InterpretNode(n);
 
@@ -614,8 +622,9 @@ EvaluableNodeReference Interpreter::InterpretNodeIntoUniqueStringIDValueEvaluabl
 
 	result->ClearMetadata();
 
-	if(result->GetType() != ENT_STRING)
-		result->SetType(ENT_STRING, evaluableNodeManager);
+	auto type = result->GetType();
+	if(type != ENT_STRING && type != ENT_NULL)
+		result->SetType(ENT_STRING, evaluableNodeManager, true);
 
 	return result;
 }
@@ -654,7 +663,7 @@ EvaluableNodeReference Interpreter::InterpretNodeIntoUniqueNumberValueOrNullEval
 
 	auto type = result->GetType();
 	if(type != ENT_NUMBER && type != ENT_NULL)
-		result->SetType(ENT_NUMBER, evaluableNodeManager);
+		result->SetType(ENT_NUMBER, evaluableNodeManager, true);
 
 	return result;
 }

src/Amalgam/interpreter/Interpreter.h

@@ -537,7 +537,8 @@ class Interpreter
 
 	//Calls InterpretNode on n, convers to a string, and makes sure that the node returned is
 	// new and unique so that it can be modified
-	EvaluableNodeReference InterpretNodeIntoUniqueStringIDValueEvaluableNode(EvaluableNode *n);
+	EvaluableNodeReference InterpretNodeIntoUniqueStringIDValueEvaluableNode(EvaluableNode *n,
+		bool immediate_result = false);
 
 	//Calls InterpretNode on n, converts to double and returns, then cleans up any resources used
 	double InterpretNodeIntoNumberValue(EvaluableNode *n);

src/Amalgam/interpreter/InterpreterOpcodesBase.cpp

@@ -212,9 +212,9 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_SYSTEM(EvaluableNode *en,
 	{
 		EvaluableNode *debugger_info = evaluableNodeManager->AllocListNodeWithOrderedChildNodes(ENT_FALSE, 2);
 		if(Interpreter::GetDebuggingState())
-			debugger_info->GetOrderedChildNodesReference()[0]->SetType(ENT_TRUE, evaluableNodeManager);
+			debugger_info->GetOrderedChildNodesReference()[0]->SetType(ENT_TRUE, evaluableNodeManager, false);
 		if(asset_manager.debugSources)
-			debugger_info->GetOrderedChildNodesReference()[1]->SetType(ENT_TRUE, evaluableNodeManager);
+			debugger_info->GetOrderedChildNodesReference()[1]->SetType(ENT_TRUE, evaluableNodeManager, false);
 
 		return EvaluableNodeReference(debugger_info, true);
 	}

src/Amalgam/interpreter/InterpreterOpcodesDataTypes.cpp

@@ -279,7 +279,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_SET_TYPE(EvaluableNode *en
 	if(new_type == ENT_NOT_A_BUILT_IN_TYPE)
 		new_type = ENT_NULL;
 
-	source->SetType(new_type, evaluableNodeManager);
+	source->SetType(new_type, evaluableNodeManager, true);
 
 	return source;
 }
@@ -1227,16 +1227,14 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_SPLIT(EvaluableNode *en, b
 	auto [valid_string_to_split, string_to_split] = InterpretNodeIntoStringValue(ocn[0]);
 	if(!valid_string_to_split)
 	{
-		retval->SetType(ENT_STRING, evaluableNodeManager);
-		retval->SetStringID(string_intern_pool.NOT_A_STRING_ID);
+		retval->SetType(ENT_NULL, nullptr, false);
 		return retval;
 	}
 
 	auto [valid_split_value, split_value] = InterpretNodeIntoStringValue(ocn[1]);
 	if(!valid_split_value)
 	{
-		retval->SetType(ENT_STRING, evaluableNodeManager);
-		retval->SetStringID(string_intern_pool.NOT_A_STRING_ID);
+		retval->SetType(ENT_NULL, nullptr, false);
 		return retval;
 	}
 
@@ -1350,7 +1348,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_SUBSTR(EvaluableNode *en,
 
 	//if only string as the parameter, just return a new copy of the string
 	if(ocn.size() == 1)
-		return InterpretNodeIntoUniqueStringIDValueEvaluableNode(ocn[0]);
+		return InterpretNodeIntoUniqueStringIDValueEvaluableNode(ocn[0], immediate_result);
 
 	//have at least 2 params
 	auto [valid_string_to_substr, string_to_substr] = InterpretNodeIntoStringValue(ocn[0]);
@@ -1675,7 +1673,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CONCAT(EvaluableNode *en,
 
 	//if only one parameter is specified, do a fast shortcut
 	if(ocn.size() == 1)
-		return InterpretNodeIntoUniqueStringIDValueEvaluableNode(ocn[0]);
+		return InterpretNodeIntoUniqueStringIDValueEvaluableNode(ocn[0], immediate_result);
 
 	std::string s;
 	for(auto &cn : ocn)

src/Amalgam/interpreter/InterpreterOpcodesEntityAccess.cpp

@@ -108,7 +108,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CONTAINED_ENTITIES_and_COM
 		//create the string references all at once and hand off
 		string_intern_pool.CreateStringReferences(contained_entities, [](Entity *e) { return e->GetIdStringId(); });
 		for(size_t i = 0; i < contained_entities.size(); i++)
-			result_ocn[i]->SetStringIDWithReferenceHandoff(contained_entities[i]->GetIdStringId());
+			result_ocn[i]->SetTypeViaStringIdValueWithReferenceHandoff(contained_entities[i]->GetIdStringId());
 
 		//if not using SBFDS, make sure always return in the same order for consistency, regardless of cashing, hashing, etc.
 		//if using SBFDS, then the order is assumed to not matter for other queries, so don't pay the cost of sorting here
@@ -371,7 +371,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_RETRIEVE_FROM_ENTITY_and_D
 	else if(to_lookup->IsAssociativeArray())
 	{
 		//reference to keep track of to_lookup nodes to free
-		EvaluableNodeReference cnr(nullptr, to_lookup.unique);
+		EvaluableNodeReference cnr(static_cast<EvaluableNode *>(nullptr), to_lookup.unique);
 
 		//need to return an assoc, so see if need to make copy; will overwrite all values
 		if(!to_lookup.unique)
@@ -398,7 +398,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_RETRIEVE_FROM_ENTITY_and_D
 	else //ordered params
 	{
 		//reference to keep track of to_lookup nodes to free
-		EvaluableNodeReference cnr(nullptr, to_lookup.unique);
+		EvaluableNodeReference cnr(static_cast<EvaluableNode *>(nullptr), to_lookup.unique);
 
 		//need to return an assoc, so see if need to make copy; will overwrite all values
 		if(!to_lookup.unique)