Narrow mixed types across the stubs, fix two value-constructor crashes, ship IDE stubs - #152
Merged
Merged
Conversation
php_scylladb_duration_init and php_scylladb_timeuuid_init are reached on two paths: as the class constructor, and through Type\Scalar::create(). Each helper only handled the constructor path. duration_init called PHP_SCYLLADB_GET_DURATION(getThis()). Through create(), $this is the Type\Scalar object, so the macro applied the Duration offset to a Scalar and wrote past the object. Type::duration()->create(1, 2, 3) returned null and corrupted memory. timeuuid_init allocated the object only when return_value was IS_UNDEF. The VM sets return_value to IS_NULL before it calls an internal handler, so the allocation was skipped and Z_OBJ_P read a null zval. Type::timeuuid()->create(1) crashed with SIGSEGV. Both helpers now test whether $this already holds the target class, the same way php_scylladb_uuid_init and php_scylladb_bigint_init do, and allocate into return_value when it does not. All 21 scalar types now build through create().
Every stub already declared a type, but 71 declarations were mixed. mixed gives
opcache and the JIT nothing to work with, so the call sites keep a type guard
the engine could otherwise drop. This narrows 53 of them.
The value union is Value|string|int|float|bool|null. It comes from
ResultDecoder.c: a decoded Cassandra value is a string, an int, a bool, a float,
null, or one of 19 objects, and all 19 classes implement Value.
Returns now name what the C code produces. FutureRows::get() returns ?Rows,
FutureSession::get() returns ?Session, FuturePreparedStatement::get() returns
?PreparedStatement, and FutureClose::get() returns null. Rows::current(),
Rows::offsetGet() and Rows::first() return ?array, and Rows::key() returns ?int.
The container accessors and the schema option accessors return the value union.
Input parameters carry the union too. This is safe because
php_scylladb_validate_object throws on every value outside the union, and it
accepts a strict subset of it. A value the C code accepts can never fall outside
the declared type, so the engine's arginfo check in debug builds cannot fire.
An array or a foreign object still raises InvalidArgumentException, not
TypeError, so callers see no change.
18 mixed declarations stay, and each one is forced:
- 10 ArrayAccess and Iterator parameters. Parameter types are contravariant,
so a narrower type is a fatal error at class registration.
- 7 getResource() and Reactor::resource() returns. PHP has no resource type
declaration.
- Future::get() on the interface. Its implementations return different types,
so mixed is the correct supertype.
Two related corrections:
Type\Scalar::create() declared one parameter, but the branch for duration parses
up to three and the branch for timestamp parses up to two. A debug build turned
Type::duration()->create(1, 2, 3) into a fatal "Arginfo / zpp mismatch" because
the handler accepted arguments arginfo forbade. The parameter is now variadic,
which leaves each branch to enforce its own arity.
Table::populateIOCacheOnFlush() and Table::replicateOnWrite() declared
bool|false on three interfaces. PHP rejects that union as redundant, so the
generated stub file did not parse. The type is bool, which already covers the
value and the false-when-absent sentinel.
Editors and static analysers cannot read a compiled extension, so users get no completion for Cassandra\ classes unless they install the extension's headers by hand. This attaches a generated stub package to every release. tools/gen_ide_stubs.php builds the package from the .stub.php sources. Those files are the source of truth for the PHP-visible API, but they cannot ship as they are: - @cvalue constants are written as `= UNKNOWN`. The real values live in cassandra.h, so the tool reads them back from a loaded extension. A plain copy would show users CONSISTENCY_ANY = UNKNOWN. - @generate-class-entries, @strict-properties and @scylladb-struct are build-time directives with no meaning for an editor, so the tool strips them. Documentation comments stay. After it writes the files, the tool compares the classes it declared against ReflectionExtension. A class the extension registers but no stub declares fails the run. That check found Cassandra\Custom, which src/Custom.c registers by hand with no stub, so this adds src/Custom.stub.php. The file is declaration-only and is not passed to php_scylladb_generate_arginfo, because the C file still does the registration. All 101 classes now match. The release workflow gains an ide-stubs job. It needs a real build to resolve the constants, so it compiles the extension once and uploads ide-stubs-v<version>.tar.gz next to the binaries. One limit is permanent. PHP reserves float as a class name in every namespace, so Cassandra\Float cannot be declared in valid PHP and only C registration can create it. Numbers/Float.php therefore fails php -l. PhpStorm, Intelephense, PHPStan and Psalm all parse it, and the package declares no autoloader, so the file never loads at runtime.
|
Tick the box to add this pull request to the merge queue (same as
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Narrows
mixedacross the stub files, fixes two crashes found while verifying the narrowing, and ships IDE stubs with each release.Type narrowing
The stubs were already fully typed — nothing was missing. But 71 declarations were
mixed, which gives opcache and the JIT nothing to work with. 53 of them are now precise.The value union is
Value|string|int|float|bool|null, derived fromResultDecoder.c: a decoded Cassandra value is a string, int, bool, float, null, or one of 19 objects, and all 19 implementValue.Highlights:
FutureRows::get()mixed?RowsFutureSession::get()mixed?SessionFuturePreparedStatement::get()mixed?PreparedStatementFutureClose::get()mixednullRows::current()/first()/offsetGet()mixed?arrayRows::key()mixed?intMap/Set/Collection/TupleaccessorsmixedKeyspace::function()/aggregate()$typesmixedstring|TypeWhy narrowing input parameters is safe
The engine computes
should_throwfrom arginfo before the handler runs, then raises a fatalArginfo / zpp mismatchif the handler returns without throwing (zend_execute.c,zend_internal_call_should_throw). The comment there is explicit: "In release builds, we trust that arginfo matches what is enforced by zend_parse_parameters."This is safe here because
php_scylladb_validate_objectthrows on every value outside the union and accepts a strict subset of it — so no accepted value can fall outside the declared type, and the check can never fire.No BC break. An
arrayor foreign object still raisesCassandra\Exception\InvalidArgumentException, notTypeError, because the C handler throws before the engine's check. Verified directly.What stays
mixed, and why18 declarations remain, each one forced:
ArrayAccess/Iteratorparameters — parameter types are contravariant, so narrowing is a fatal error at class registration. Confirmed:Declaration of T::offsetExists(int $o) must be compatible with ArrayAccess::offsetExists(mixed $offset).getResource()/Reactor::resource()returns — PHP has noresourcetype declaration.Future::get()on the interface — its implementations return genuinely different types, somixedis the correct supertype.Bug fixes
Both found while verifying the narrowing, and both confirmed pre-existing on
trunkby stashing these changes and reproducing on a clean build.php_scylladb_duration_init— type-confused write. It calledPHP_SCYLLADB_GET_DURATION(getThis())unconditionally, but throughType\Scalar::create()$thisis aType\Scalar, so the macro applied the Duration offset to a Scalar and wrote past the object.Type::duration()->create(1,2,3)returnednull.php_scylladb_timeuuid_init— SIGSEGV. It allocated only whenreturn_valuewasIS_UNDEF, but the VM pre-initializes it toIS_NULL, soZ_OBJ_Pdereferenced a null zval.Type::timeuuid()->create(1)crashed with exit 139.Both now use the same guard
php_scylladb_uuid_initandphp_scylladb_bigint_initalready use. All 21 scalar types build correctly throughcreate().Two related corrections:
Type\Scalar::create()arity. Arginfo declared 1 parameter while thedurationbranch parses up to 3 andtimestampup to 2 — a guaranteedArginfo / zpp mismatchfatal on debug builds. Now variadic, so each branch enforces its own arity.bool|falseon threeTableinterfaces. PHP rejects it as a redundant union. The type isbool.IDE stubs
tools/gen_ide_stubs.phpgenerates a stub package from the.stub.phpsources. It resolves@cvalueUNKNOWNconstants against a loaded extension (a plain copy would shipCONSISTENCY_ANY = UNKNOWN), strips build-time annotations, and keeps documentation comments.It verifies its output against
ReflectionExtensionand fails if the extension registers a class no stub declares. That check immediately foundCassandra\Custom, registered by hand insrc/Custom.cwith no stub — hence the new declaration-onlysrc/Custom.stub.php. All classes now match.The release workflow gains an
ide-stubsjob that builds the extension once (needed to resolve constants) and attacheside-stubs-v<version>.tar.gz.Reviewer notes
Cassandra\Floatcannot be declared in valid PHP.floatis reserved as a class name in every namespace, so only C registration can create it, andNumbers/Float.phpfailsphp -l. PhpStorm, Intelephense, PHPStan and Psalm all parse it, and the package declares no autoloader so it never loads at runtime. This is a permanent wart, not something this PR introduces.*_arginfo.his build-generated and gitignored (.gitignore:65-66), so nothing regenerated is committed here.CLAUDE.mdstill says to commit those files — that instruction is stale and worth correcting separately.Testing
980 passed, 0 failed(13360 assertions) on PHP 8.5 NTS against a live ScyllaDB container. Also verified by reflection that every narrowed signature is what the engine actually registers, and swept all 21 scalarcreate()paths.Note the local build is a release build, so the debug-only
Arginfo / zpp mismatchcheck does not fire there — the mismatches were identified by readingzend_execute.cand confirmed by the arity analysis. CI debug builds will exercise it.