Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

explicit registration mode #329

Open
faassen opened this issue Oct 21, 2024 · 3 comments
Open

explicit registration mode #329

faassen opened this issue Oct 21, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@faassen
Copy link
Contributor

faassen commented Oct 21, 2024

Since in #327 (and maybe #328) I run into issues with registration order, I think that Rust compilation order, which ext-php-rs relied has been broken at least under some circumstances, which screws up at the very least rust analyzer.

I started investigating the cause of this error. ext-php-rs has a static STATE that things get registered in during macro expansion. This has lines like:

        if state.built_module {
            bail!("The `#[php_module]` macro must be called last to ensure functions and classes are registered.");
        }

        if state.startup_function.is_some() {
            bail!("The `#[php_startup]` macro must be called after all the classes have been defined.");
        }

which are causing the problem.

Not checking

As a relatively simple hack we could just disable these lines. That would fix the macro errors I think, but then you'd get absolutely no warning that your functions and classes aren't actually being registered. That's risky as we are already getting errors about the order of registration, and the order of macro expansion is not guaranteed.

Explicit mode

I started investigating what it would be required to have explicit registration. I envision a mode where you write

#[php_class(explicit)

and you're then required to register it explicitly in the module builder.

What does explicit do?

With an explicit flag, we could instead generate static annotations for the various macros (such as a class description, or function description) that only later is explicitly referred to during registration.

Then you'd register things explicitly:

#[php_class(explicit)]
struct Foo {
}

#[php_impl(explicit)]
impl Foo {
}

#[php_module(explicit)]
pub fn get_module(module: ModuleBuilder) -> ModuleBuilder {
     php_class_explicit!(module, Foo)
}

I'll note that php_class_explicit is a macro so that it looks for associated registrations that are generated by the php_class. These registrations are stored by the macro in a module that's generated associated with Foo. Unfortunately we can't call this module Foo as we need access to the original Foo in non-macro code, so some magic will need to remain (you can't import Foo and register it in another module, you're better off implementing a new get_module in that module and then invoke it from the original one.

Implementation challenges

Unfortunately the current system relies heavily on the ability to register things during compile time. It builds extensive structs during compile time (such as Class) which are then registered. Ideally in the explicit system we don't want such a global object at all, as in that case we'd still be dependent on import order which can break - php_module needs to be called at the very last for it to work, and we can't guarantee that.

To be really explicit, each registration has to be independent, and that means generating quite a bit of stuff in the macro. But luckily it looks like this is already happening anyway in order to support build_classes. So for each class for instance we'd want to generate a separate function that builds the class. The current logic also calls builder.build() which is NOT what we want in explicit mode, as it registers the class with PHP too. We only want to invoke build once the explicit get_module has collected all required information.

So this is going to require quite a bit of refactoring to make it work...

PS

Some of this may be difficult to follow as I'm thinking as I'm writing it. If you're interested in this problem, let me know!

@Xenira
Copy link
Collaborator

Xenira commented Oct 21, 2024

Will definitely have a look at this. First priority will be to get CI working again, but after that this should be fixed.

@faassen
Copy link
Contributor Author

faassen commented Oct 21, 2024

@Xenira I'm planning to do some preparatory refactoring first. I didn't know there was a CI issue, good you're looking into that.

@Xenira
Copy link
Collaborator

Xenira commented Oct 21, 2024

I would like to have a more in depth look at how macros can be improved first, before tackling this one, as that might have implications on how to implement this (or even make this obsolete).

From what I have seen a bigger refactoring of how the macros work might be in order. Will tag you once I've written a proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants