Fix divergences between the native extension and the PHP implementations

Found by a line-by-line review of every shadowed pair against its PHP twin:

- The parser dropped T_BAD_CHARACTER only below the grammar's symbol-map size,
  but its id (411 on PHP 8.5) sits exactly at that bound — a stray control
  byte threw RangeException instead of parsing with a collected error. The
  drop-token table has its own size; bound both checks by it.
- String_::parseEscapeSequences() throwing (oversized \u{...} codepoints in
  interpolated strings and backticks) left a raw pending PhpParser\Error where
  doParse()'s catch block hands it to the error handler. The translation
  createNode already did is extracted into abortForPendingException() and the
  escape-sequence helper routes through it.
- The node-class registry latched the first caller's useCtor flag, so
  isInstanceOf('Node\Arg') — reached by parsing exit(...)/die(...) — poisoned
  Node\Arg into the constructor path, whose attributes-last convention Arg's
  signature breaks: every later file with call arguments failed with a
  TypeError. The property-write plan is now derived lazily and the path chosen
  per call.
- TrinaryLogic::lazyMaxMin([]) threw ShouldNotHappenException; the PHP twin
  returns Yes ( starts at YES). The class is @api, so third parties can
  hit the empty case.
- ScopeOps::finishMerge()'s array_merge() renumbered integer-coerced
  expression keys (an expression printing as '5' is stored under int key 5),
  corrupting them — the native side preserves keys. The PHP twin now uses +
  (the key sets are disjoint by construction).
- CombinationsHelper's eagerly materialized product could overflow zend_ulong
  and silently truncate; it now fails loudly at PRODUCT_LIMIT. Inner elements
  are dereferenced like the twin's by-value foreach.
- NodeTraverser owns references to the current subnode across visitor hooks
  and to the visitors in its plan — userland writes to the parent property or
  the visitor list mid-traversal could otherwise free objects still in use.
- ScopeOps::getTypeFromCache() writes the by-ref $key on hit and miss (the
  twin assigns it unconditionally), and treats a stored null as a miss exactly
  like the twin's ?? null.
- The array_map-args suffix of an expression key appended full node keys
  (re-appending /*pos:...*/ and /*keepVoid*/ suffixes of the arguments) where
  the twin appends plain printExpr() output.
- Pass 2 of matchConditionalExpressions() validates holder types like Pass 1
  instead of reading property slots type-confused.
- zv.h's Ref::assign()/ObjRef::propAtWrite() install the new value before
  destroying the old one (the engine's assignment idiom), so a __destruct
  re-reading the slot cannot observe a freed zval.

Corpus fixtures cover the three parser bugs (malformed input and the
first-class-callable exit ordering); smoke covers lazyMaxMin([]).

Interleaved re-benchmark after the fixes: user CPU and Used memory unchanged
(-17% vs PHPSTAN_TURBO=0, +5-6% memory), analysis output byte-identical.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LVQhbBxDjDVAVeJVvLpWYE

Author: ondrejmirtes

src/Analyser/ScopeOps.php

@@ -25,7 +25,6 @@
 use function array_filter;
 use function array_key_exists;
 use function array_key_first;
-use function array_merge;
 use function count;
 use function get_class;
 use function in_array;
@@ -307,7 +306,11 @@ public static function finishMerge(
 
 		return [
 			$mergedExpressionTypes,
-			array_merge($mergedNativeExpressionTypes, array_filter(self::mergeVariableHolders($ourNativeExpressionTypes, $theirNativeExpressionTypes), $filter)),
+			// + instead of array_merge: the key sets are disjoint (matching entries were
+			// unset from both native maps above), and array_merge would renumber
+			// integer-coerced expression keys (an expression printing as '5' is stored
+			// under int key 5), corrupting them. The native implementation preserves keys.
+			$mergedNativeExpressionTypes + array_filter(self::mergeVariableHolders($ourNativeExpressionTypes, $theirNativeExpressionTypes), $filter),
 		];
 	}
 

turbo-ext/src/CombinationsHelper.cpp

@@ -19,6 +19,10 @@ class CombinationsHelper
 	/* caps the result array's pre-allocation hint, not the product itself */
 	static constexpr zend_ulong SIZE_HINT_LIMIT = 1048576;
 
+	/* combinations() refuses products beyond this instead of letting the
+	 * multiply overflow and silently truncate the result */
+	static constexpr zend_ulong PRODUCT_LIMIT = 1ULL << 32;
+
 	/* UNDEF = pending exception */
 	static zv::Val combinations(zv::ArrRef arrays)
 	{
@@ -57,11 +61,29 @@ class CombinationsHelper
 				vecs[i] = NULL;
 				continue;
 			}
+			if (UNEXPECTED(total > PRODUCT_LIMIT / sizes[i])) {
+				/* The PHP twin is a lazy generator and never materializes the
+				 * product, but every consumer iterates it fully, so a product
+				 * this size is unreachable in practice. Failing loudly beats
+				 * the silent truncation an overflowed multiply would cause. */
+				for (uint32_t k = 0; k < i; k++) {
+					if (vecs[k] != NULL) {
+						efree(vecs[k]);
+					}
+				}
+				efree(vecs);
+				efree(sizes);
+				efree(inner);
+				pt_throw_should_not_happen();
+				return zv::Val();
+			}
 			total *= sizes[i];
 			vecs[i] = (zval **) emalloc(sizes[i] * sizeof(zval *));
 			uint32_t j = 0;
 			for (auto entry : innerArr) {
-				vecs[i][j++] = entry.value().raw();
+				/* deref like the twin's by-value foreach: a reference slot
+				 * must not propagate a shared reference into every combination */
+				vecs[i][j++] = entry.value().deref().raw();
 			}
 		}
 

turbo-ext/src/NodeTraverser.cpp

@@ -203,6 +203,12 @@ class NodeTraverser
 	~NodeTraverser()
 	{
 		if (plan != NULL) {
+			/* the plan owns its visitor references: removeVisitor() (or any
+			 * userland write to $this->visitors) during traversal must not be
+			 * able to free a visitor the plan still dispatches to */
+			for (uint32_t i = 0; i < nplanned; i++) {
+				OBJ_RELEASE(plan[i].visitor);
+			}
 			efree(plan);
 		}
 	}
@@ -370,6 +376,11 @@ class NodeTraverser
 			if (!value.instanceOf(nodeIface)) {
 				continue;
 			}
+			/* Own a reference for the whole block: a visitor writing to the
+			 * parent's property from a hook can otherwise drop the node's
+			 * last reference while later hooks still run on it. The PHP twin
+			 * survives that by construction ($subNode owns a reference). */
+			zv::Val subNodeOwned = zv::Val::copyOf(zv::Ref(value.raw()));
 			zend_object *subNode = value.asObject();
 
 			bool traverseChildren = true;
@@ -401,6 +412,7 @@ class NodeTraverser
 					if (UNEXPECTED(!writeSubnode(node, info->names[i], retRef))) {
 						return;
 					}
+					subNodeOwned = zv::Val::copyOf(retRef);
 					subNode = retRef.asObject();
 					continue;
 				}
@@ -465,6 +477,7 @@ class NodeTraverser
 					if (UNEXPECTED(!writeSubnode(node, info->names[i], retRef))) {
 						return;
 					}
+					subNodeOwned = zv::Val::copyOf(retRef);
 					subNode = retRef.asObject();
 					continue;
 				}
@@ -748,6 +761,8 @@ class NodeTraverser
 			}
 			pt_visitor_plan *p = &plan[i];
 			p->visitor = visitor.asObject();
+			GC_ADDREF(p->visitor);
+			nplanned = i + 1;
 			p->ce = p->visitor->ce;
 			p->enter_fn = findHook(p->ce, "enternode", sizeof("enternode") - 1);
 			p->leave_fn = findHook(p->ce, "leavenode", sizeof("leavenode") - 1);
@@ -821,6 +836,8 @@ class NodeTraverser
 	zend_object *self;
 	pt_visitor_plan *plan = NULL;
 	uint32_t nvisitors = 0;
+	/* how many plan entries own a visitor reference (build can fail mid-way) */
+	uint32_t nplanned = 0;
 	bool stop = false;
 	bool failed = false;
 };

turbo-ext/src/ScopeOps.cpp

@@ -108,10 +108,11 @@ class ScopeOps
 	}
 
 	/*
-	 * Mirrors ScopeOps::getTypeFromCache(). On a hit *keyOut stays NULL and
-	 * the resolved Type is returned; on a miss null is returned and *keyOut
-	 * receives the computed key (owned by the caller) so the PHP slow path
-	 * can proceed without recomputing it.
+	 * Mirrors ScopeOps::getTypeFromCache(). *keyOut receives the computed key
+	 * (owned by the caller) on both hit and miss, like the twin's
+	 * unconditional `$key = ...` assignment; it stays NULL only when an
+	 * exception is pending. A stored null counts as a miss — the twin's
+	 * `?? null` cannot distinguish it from an absent entry either.
 	 */
 	static zv::Val getTypeFromCache(zval *scope, zend_object *node, zend_string **keyOut)
 	{
@@ -137,8 +138,10 @@ class ScopeOps
 		}
 		if (EXPECTED(Z_TYPE_P(table) == IS_ARRAY)) {
 			zval *found = zend_symtable_find(Z_ARRVAL_P(table), key.get());
-			if (found != NULL) {
-				return zv::Val::copyOf(zv::Ref(found));
+			if (found != NULL && Z_TYPE_P(found) != IS_NULL) {
+				zv::Val result = zv::Val::copyOf(zv::Ref(found));
+				*keyOut = key.take();
+				return result;
 			}
 		}
 
@@ -1121,6 +1124,12 @@ class ScopeOps
 						}
 						zv::Ref conditionHolder = conditionEntry.value().deref();
 						zv::Ref specifiedHolder = zv::Ref(specifiedSlot).deref();
+						/* Pass 1 validates only the entries it reaches before
+						 * its first mismatch, so these can be unchecked here;
+						 * the twin raises a catchable Error on wrong types */
+						if (UNEXPECTED(!pt_check_holder(conditionHolder.raw()) || !pt_check_holder(specifiedHolder.raw()))) {
+							return zv::Val();
+						}
 						if (pt_holder_certainty_value(conditionHolder.asObject()) != pt_holder_certainty_value(specifiedHolder.asObject())) {
 							all = false;
 							break;
@@ -2088,7 +2097,7 @@ void pt_register_scope_ops()
 			RETURN_THROWS();
 		}
 		if (key != NULL) {
-			/* cache miss: hand the computed key to the by-ref parameter */
+			/* hand the computed key to the by-ref parameter (hit and miss) */
 			if (Z_ISREF_P(key_out)) {
 				ZEND_TRY_ASSIGN_REF_STR(key_out, key);
 			} else {

turbo-ext/src/TrinaryLogic.cpp

@@ -293,10 +293,9 @@ static void pt_trinary_lazy(INTERNAL_FUNCTION_PARAMETERS, LazyEvaluation::Mode m
 		Z_PARAM_FUNC(fci, fcc)
 	ZEND_PARSE_PARAMETERS_END();
 
-	if (mode == LazyEvaluation::MAX_MIN && UNEXPECTED(zend_hash_num_elements(objects) == 0)) {
-		pt_throw_should_not_happen();
-		RETURN_THROWS();
-	}
+	/* no empty-array check for MAX_MIN: unlike extremeIdentity()/maxMin(), the
+	 * PHP twin's lazyMaxMin([]) returns Yes ($min starts at YES), and run()'s
+	 * accumulator reproduces that */
 
 	zval objectsZv;
 	ZVAL_ARR(&objectsZv, objects);

turbo-ext/src/parser/ParserEngine.h

@@ -77,7 +77,8 @@ struct Tables
 	int *ruleToLength;
 	zend_string **symbolToName; /* borrowed persistent copies (interned dup) */
 	int symbolToNameSize;
-	bool *dropTokens;           /* indexed by php token id, size phpTokenToSymbolSize */
+	bool *dropTokens;           /* indexed by php token id */
+	int dropTokensSize;         /* ≥ phpTokenToSymbolSize: T_BAD_CHARACTER sits above the grammar's symbol map */
 };
 
 /* Per-node-class construction plan, cached per process in the class registry. */
@@ -89,7 +90,13 @@ struct NodeClassInfo
 	int propSlots[16];
 	uint32_t attrsSlot; /* slot of NodeAbstract::$attributes */
 	int numProps;
-	bool useCtor; /* subNodes-style or otherwise non-trivial ctor */
+	/* The slot-write plan is derived lazily, on the first resolve that wants
+	 * it — the registry entry must not depend on which caller saw the class
+	 * first (isInstanceOf() resolves with useCtor=true; letting that poison
+	 * the cache once made every later Node\Arg construction take the ctor
+	 * path, whose attributes-last convention Arg's signature breaks). */
+	enum PlanState : uint8_t { PLAN_NONE = 0, PLAN_OK, PLAN_FAILED };
+	PlanState planState;
 };
 
 /*
@@ -229,6 +236,9 @@ class ParserEngine
 	void emitError(zend_string *msg, zv::Val attributes); /* msg borrowed */
 	/* `throw new Error(...)`: records the abort; the caller must return */
 	void fatalError(const char *msg, zv::Val attributes);
+	/* doParse()'s catch (Error $e) for a pending exception thrown by PHP
+	 * code the engine called (node ctors, String_::parseEscapeSequences) */
+	void abortForPendingException();
 
 	/* ===== class resolution + node creation (scoped-phar safe) ===== */