#[implements] doesn't generate the right stubs

[faassen]

The docs here:

https://davidcole1340.github.io/ext-php-rs/macros/classes.html#implementing-an-interface

tell me I can use #[implements()] to implement an interface.

And indeed using this test code:

use ext_php_rs::prelude::*;
use ext_php_rs::{exception::PhpResult, types::Zval, zend::ce};

#[php_class]
#[implements(ce::arrayaccess())]
#[implements(ce::countable())]
#[derive(Default)]
pub struct EvenNumbersArray;

/// Returns `true` if the array offset is an even number.
/// Usage:
/// ```php
/// $arr = new EvenNumbersArray();
/// var_dump($arr[0]); // true
/// var_dump($arr[1]); // false
/// var_dump($arr[2]); // true
/// var_dump($arr[3]); // false
/// var_dump($arr[4]); // true
/// var_dump($arr[5] = true); // Fatal error:  Uncaught Exception: Setting values is not supported
/// ```
#[php_impl]
impl EvenNumbersArray {
    pub fn __construct() -> EvenNumbersArray {
        EvenNumbersArray {}
    }
    pub fn count(&self) -> i64 {
        10
    }

    // We need to use `Zval` because ArrayAccess needs $offset to be a `mixed`
    pub fn offset_exists(&self, offset: &'_ Zval) -> bool {
        offset.is_long()
    }
    pub fn offset_get(&self, offset: &'_ Zval) -> PhpResult<bool> {
        let integer_offset = offset.long().ok_or("Expected integer offset")?;
        Ok(integer_offset % 2 == 0)
    }
    pub fn offset_set(&mut self, _offset: &'_ Zval, _value: &'_ Zval) -> PhpResult {
        Err("Setting values is not supported".into())
    }
    pub fn offset_unset(&mut self, _offset: &'_ Zval) -> PhpResult {
        Err("Setting values is not supported".into())
    }
}

#[php_module]
pub fn get_module(module: ModuleBuilder) -> ModuleBuilder {
    module
}

array access and use of the count function both work as I expect when I execute things:

<?php

$a = new EvenNumbersArray();

$even = $a[0];
$uneven = $a[1];
$c = count($a);

var_dump($even);
var_dump($uneven);
var_dump($c);

But my editor tooling (PHP Intelephense in vs code) claims I can't use the count function as "Expected type 'Countable|array'. Found 'EvenNumbersArray'.".

Looking at the generated PHP stubs, something seems to be wrong with the interface declaration - it's as if the Rust strings are directly inserted:

  class EvenNumbersArray implements ce :: arrayaccess(), ce :: countable() {

and at least Intelephense says that :: is a syntax error.

I think it should be:

   class EvenNumberArray implements ArrayAccess, Countable {

Strangely enough I don't get a complaint about array accesses here, even though the proper array access isn't declared.

PS. In non-example code I also am having trouble convincing not only stubs but also the PHP runtime that something is an array or countable, but that does work in this minimal example so I'm still looking at what could have caused this.

[faassen]

I've delved into the code somewhat but I still don't have a fix.

In crates/macros/src/module.rs there's an impl Describe which generates the implements information that is then, I think, later used by cargo php stubs to generate the stubs.

The interface information takes the form of strings like "ce::accesscontrol()" which are then literally quoted into the output, as strings. That's not going to work - we want the string "AccessControl" instead.

I tried adding an import to zend::ce in the generated code, then the following:

      let interfaces = self.interfaces.iter().map(|iface| {
            let iface: TokenStream = iface.parse().unwrap();
            quote! { #iface.name().unwrap().into() }
        });

First I parse code like ce::accesscontrol() to a TokenStream. Then I include that call literally. From it, I should get a ClassEntry which has a name method which returns an Option<&str>.

Unfortunately when I try this the classes seem to be nameless and cargo php stubs fails the Option unwrap.

I'm confused as if I go into the PHP C code the name does seem to be set for ClassEntry. I'll do more digging but I thought I'd leave a report here first so others interested may be able to fllow allow.

[faassen]

Hm, I was wrong about the unwrap. It somehow takes place in

pub fn arrayaccess() -> &'static ClassEntry {
    unsafe { zend_ce_arrayaccess.as_ref() }.unwrap()
}

So perhaps this code is running before these C values are fully initialized somehow.

[faassen]

There seems to some module startup code that initializes these classes, so apparently that's not being run during stub generation.

I looked at introducing the right PHP class names like ArrayAccess and Countable earlier on, but unfortunately that literally takes the tokenstream from the #[implements macro so that's going to be ce::arrayaccess() and such.

I think there are two possible ways forward:

• the nicest way would be to somehow initialize the names of these PHP things during stub generation. I think the modifications I made would then work. Unfortunately I don't know how to do that yet.

• a hacky way would be to create a hardcoded match to recognize strings like ce:arrayaccess() and replace it with "ArrayAccess". But that wouldn't be very maintainable.

[Xenira]

I might have found the reason. Will need to do some testing. Will report back.

[faassen]

@Xenira Hey! Glad to see someone else is looking into this! If you think it would help to chat on discord or something to debug this together, let me know!

[Xenira]

Small update. Root cause is that the string was not converted back into an syn::Expr before writing it. That caused it to be interpreted as a string literal.

Changing this revealed some more underlying issues.

[Xenira]

Well, guess now that I think about it I did the same thing as you. Also failing at the unwrap now...

[Xenira]

So as far as I understand it the problem is, that the extension is not loaded inside a PHP context when generating stubs. That causes the ClassEntry to not beeing available.

In addition to @faassen s suggestions maybe do the following:

• Use the interface name directly instead of a method that returns a class entry.
• Change the implements in startup to ClassEntry::try_find
• Generate the stubs with the string literal

This would shift some validation to the startup logic, instead when generating stubs.
But as this seems to be the case already with string literals beeing used it should be fine for now.

Edit: Well, after testing it this does not seem to work, as the class entry is not found on startup...
Edit2: ClassEntry::try_find/zend_lookup_class_ex seems to be the wrong approach. Will try something different tomorrow

[faassen]

I guess it's not possible to make the PHP context exist using cargo php stubs?

By saying "use the interface name" you mean this?

#[implements(ArrayAccess)]

That would be a better UI! And if there's a way to find a ClassEntry based on name, then that would solve it.

Is there a way currently to implement an interface defined in PHP code rather than built-in?

If we made this change we'd need to worry about backwards compatibility though, if that's important.

[Xenira]

I guess it's not possible to make the PHP context exist using cargo php stubs?

Not sure, but prob. not that easy. But I might look into this.

By saying "use the interface name" you mean this?

#[implements(ArrayAccess)]

Exactly

Is there a way currently to implement an interface defined in PHP code rather than built-in?

If I understand it correctly ClassEntry::try_find would be what you would use? I'll try to dig through the zend code tomorrow to understand how to fetch the interfaces.

If we made this change we'd need to worry about backwards compatibility though, if that's important.

Ye, would be a breaking change. But as crate is 0.x.x it should be possible. But that is something a maintainer would need to answer @ptondereau @danog

[faassen]

So if ClassEntry::try_find indeed also finds PHP-defined interfaces, this would actually support a new use case too.

An argument for using "ce::foo()" is discoverability: you can find that the PHP interface exists in the API documentation. But you'd only do that to actually find how to support a PHP interface you already know the name of, so that doesn't really work. Plus since the "call" is inside an #[implements()] macro, IDE support isn't going to pop up anyway. So I think using the PHP names would actually be cleaner overall.

[Xenira]

Take everything I say here with a grain of salt. Not that well versed in php zend api. Just some thoughts after reading some of the src.

So if ClassEntry::try_find indeed also finds PHP-defined interfaces, this would actually support a new use case too.

This won't work as it is, because PHP interfaces are not available when the extension loads (or might never be available depending on what code you are running). Also not much of a benefit, as those are not 'Magic' interfaces.

Regarding how to find the class entry id need to do some more digging.

Maybe as a temp workaround just adding a second optional parameter to #[implements()] for stub generation would be easiest.

[faassen]

Yeah, I realized that the PHP defined interfaces wouldn't be available too. I still think it would be useful: PHP ecosystems likely exist that define new interfaces outside of the core ones and it makes sense to me someone may want to implement them using Rust. But it's not a priority.

I thought of a second parameter as well. It would certainly be the easiest to implement. I wonder though that this may make the upgrade path harder should we ever want to go name-only.

An alternative would be to implement the full new system and use a new directive like #[interface()] to declare them.

Or to have a second parameter to trigger the new interpretation of the first, then issue warnings if it is false, qnd eventually make it superfluous, should we care about a migration path.