PHP RFC: Bound-Erased Generic Types

[azjezz]

RFC: https://wiki.php.net/rfc/bound_erased_generic_types

Note: This PR has been squashed, full history can be found in here

[DanielEScherzer]

not even going to pretend like I understand this, or even read it all, but some initial notes, I hope you don't mind

I very much hope that this RFC gets to a point where it is voted on and hopefully passes, generics are great!

You may want to consider adding some kind of overview for how the implementation logic works (e.g. a new file in docs/source) to aid reviewers in understanding how all of the parts go together

You may want to also split this up, it currently adds support for

Classes, interfaces, traits, functions, methods, closures, and arrow functions

and an initial version might just support classes, interfaces, and methods. Just a thought, please don't take this as criticism - I really hope that something like this is added to PHP

[azjezz]

Hi @DanielEScherzer

Thanks for the review!

On the internal docs: i'd lean toward holding off for now. If the RFC doesn't pass, we'd just throw the prose away; if it does, the API/internals will likely shift between now and acceptance, and I'd rather not rewrite the doc multiple times along the way. Once the RFC lands (hoping it does), I'm happy to write a proper INTERNALS section in one go against the final shape. or do you think there's value in having something now? (for review?)

[DanielEScherzer]

Hi @DanielEScherzer

Thanks for the review!

On the internal docs: i'd lean toward holding off for now. If the RFC doesn't pass, we'd just throw the prose away; if it does, the API/internals will likely shift between now and acceptance, and I'd rather not rewrite the doc multiple times along the way. Once the RFC lands (hoping it does), I'm happy to write a proper INTERNALS section in one go against the final shape. or do you think there's value in having something now? (for review?)

I was thinking that this would be for reviewers to understand the rest of the patch - you know it best, since you wrote it, but others could use a roadmap/overview. I don't even known where to start for giving an example of "something like ..." for this, so I'll give one for #16952 (which I know best because I wrote it)

For supporting attributes on constants

• the grammar for the language is adjust to allow attributes on constants, moving constant declarations into attributed_statement which can have (but doesn't need) attributes
• the AST is stored in the same list of children as the constants in the declaration, as the last child
• zend_compile_const_decl() will compile the attributes into a normal hashtable with zend_compile_attributes, and then emit a ZEND_DECLARE_ATTRIBUTED_CONST with a data znode where the data is a pointer to that table
• at runtime, the VM handler will call zend_constant_add_attributes() to add the attributes

For supporting the deprecated attribute specifically

• zend_constant_add_attributes() will flag the constant with CONST_DEPRECATED if the deprecated attribute is present
• the existing logic to emit deprecation warnings for deprecated internal constants is instead replaced with calls to the new zend_deprecated_constant() function which handles reading for attributes if present

Just to have an understanding of how all of the pieces fit together

[azjezz]

Makes sense. Will add it tomorrow morning 👍🏼

[prateekbhujel]

I checked the failing CircleCI ARM job.

Current failure is only:

Zend/tests/generics/inheritance/diamond/trait_diamond_variadic.phpt

It prints the reflected type correctly:

param: string|int
is union: yes

and then segfaults when the merged trait method is called: (new C)->sink(1, "x", 2, "y").

I built the branch locally on macOS arm64 with debug + ZTS and the same test passes for me, both plain and with opcache enabled. So this looks more like Linux ARM / JIT / merged arg_info lifetime than a test expectation issue.

The useful split is probably to rerun that one test on the ARM worker with JIT disabled. If it only crashes with JIT, the bad path is likely the merged trait method clone being executable but not looking quite like a normal trait clone to the runtime/JIT.

[azjezz]

@prateekbhujel reproduced it locally in aarch64 Ubuntu 24.04 Docker container. pushing a fix now :)

[morrisonlevi]

It's a big RFC so sorry if this is working-as-intended, but I seem to have found a bug.

Reproducer code

src/sequence.php:

<?php declare(strict_types=1);

interface Sequence<+T>
{
}

src/arraysequence.php:

<?php declare(strict_types=1);

final class ArraySequence<+T>
    implements Sequence<T>
{
}

fails.php:

<?php declare(strict_types=1);

set_include_path(__DIR__ . '/src' . PATH_SEPARATOR . get_include_path());
spl_autoload_register();

// class_exists(Sequence::class);
class_exists(ArraySequence::class);

echo "unreachable\n";

Run it:

php -d display_errors=1 fails.php

Results in:

Fatal error: Type parameter T declared covariant (+T) cannot appear in invariant position in <redacted>/src/arraysequence.php on line 3
Stack trace:
#0 [internal function]: spl_autoload('ArraySequence')
#1 <redacted>/fails.php(6): class_exists('ArraySequence')
#2 {main}

If you un-comment the line:

// class_exists(Sequence::class);

Then it works.

It's a bug because T in class ArraySequence<+T> implements Sequence<T> is covariant and so is the T in interface Sequence<+T>, so they should be compatible.

[azjezz]

@morrisonlevi looking into it!

[azjezz]

@morrisonlevi fixed!

[morrisonlevi]

Found another issue. if we use namespace/ syntax to fully qualify something, it loses generics in reflection.

reflection.php

<?php declare(strict_types=1);

namespace X;

interface Iface<+A>
{
}

trait Mixin<+A>
{
}

class FqnImplements
    implements \X\Iface<int>
{
}

class NamespaceImplements
    implements namespace\Iface<int>
{
}

class FqnUsesTrait
{
    use \X\Mixin<int>;
}

class NamespaceUsesTrait
{
    use namespace\Mixin<int>;
}

/**
 * @param list<\ReflectionNamedType> $args
 */
function render_args(array $args): string
{
    if ($args === []) {
        return '[]';
    }

    return '[' . \implode(', ', \array_map(
        static fn(\ReflectionNamedType $arg): string => $arg->getName(),
        $args,
    )) . ']';
}

function show(string $label, array $args): void
{
    \printf("%s: count=%d args=%s\n", $label, \count($args), render_args($args));
}

$fqn_implements_args = (new \ReflectionClass(FqnImplements::class))
    ->getGenericArgumentsForParentInterface(Iface::class);
$namespace_implements_args = (new \ReflectionClass(NamespaceImplements::class))
    ->getGenericArgumentsForParentInterface(Iface::class);

$fqn_trait_args = (new \ReflectionClass(FqnUsesTrait::class))
    ->getGenericArgumentsForUsedTrait(Mixin::class);
$namespace_trait_args = (new \ReflectionClass(NamespaceUsesTrait::class))
    ->getGenericArgumentsForUsedTrait(Mixin::class);

show('implements FQN', $fqn_implements_args);
show('implements namespace-relative', $namespace_implements_args);
show('trait use FQN', $fqn_trait_args);
show('trait use namespace-relative', $namespace_trait_args);

if (\count($namespace_implements_args) !== 1 || \count($namespace_trait_args) !== 1) {
    \fwrite(\STDERR, "BUG: namespace-relative inheritance clauses compile but lose generic arguments in reflection.\n");
    exit(1);
}

[azjezz]

@morrisonlevi fixed! :D I have added both to test suite, please report any other edge cases you might encounter.

[prateekbhujel]

Small follow-up on the optimizer note above: I pushed the patch to my fork so it is not just a pasted diff.

Commit: prateekbhujel@ccb50d3

It is based on the current RFC head (a38781a) and only skips the backward scan when the current op_array has no turbofish args table. If you prefer a PR against the RFC branch, I can open that instead.

[azjezz]

@prateekbhujel feel free to open a PR against carthage-software:rfc/bound-erased-generic-types-history branch ( the working branch ), this branch ( PR ) is squashed from that one.

[prateekbhujel]

Opened it against the working branch here: carthage-software#2

[prateekbhujel]

Opened one more small PR against the working branch for a Reflection edge case I hit while testing transitive interfaces:

carthage-software#3

It fixes ReflectionClass::getGenericArgumentsForParentInterface() returning an empty list when the generic ancestor is reached through a non-generic intermediate interface/class.

[prateekbhujel]

Opened a small PR for the current CI failure here: carthage-software#4

The failing test was creating an inherited-method arg_info clone even though the method-level Box<O> return did not actually substitute any class-scope slot.

[prateekbhujel]

Opened another Reflection follow-up against the working branch: carthage-software#5

It fixes nested transitive interface args, e.g. Mid<X> extends Root<Box<X>> plus Concrete implements Mid<int> reflecting Box<X> instead of Box<int>. This is mainly for consumers relying on Reflection metadata rather than reparsing source.

[prateekbhujel]

Opened the concrete Reflection piece here: carthage-software#6

It keeps the current singular method alone, but adds an all-bindings view for tools that need to see cases like Foo, Foo from Reflection without reparsing source. It is stacked on #5 for now, so the diff should shrink once that one lands.

[Youssef-Amjad]

Is there anything new?

[pop1989bb]

How is the current state?