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

Switch to Outer Macro pattern #335

Open
Xenira opened this issue Oct 23, 2024 · 15 comments · May be fixed by #342
Open

Switch to Outer Macro pattern #335

Xenira opened this issue Oct 23, 2024 · 15 comments · May be fixed by #342
Assignees
Labels
enhancement New feature or request question Further information is requested

Comments

@Xenira
Copy link
Collaborator

Xenira commented Oct 23, 2024

Motivation

Currently the macros depend on the line order of the code. Since recently the way the current macros work break rust analyzer. This was noted in #327.
A solution to this problem is to switch to the Outer Macro pattern, which allows to track the state of the macro without depending on the line order of the code.

Design Detail

The main idea is to use a #[php_module] macro on a module that contains all the functions, classes, etc. that should be exported to PHP.
As everything in that module is the input to the macro, the macro can track the state of the module and generate the necessary code.

While parsing the 'pseudo' inner macros are evaluated similar to the current macros.

This would look something like this:

#[php_module]
pub mod module {
    #[php_function]
    fn func() { ... }

    #[php_class]
    pub struct Example {
        [...]
    }

    #[php_impl]
    impl Example {
        [...]
    }
}

Drawbacks

The main drawback is that all functions, classes, etc. must be defined in the same module. This can make the code harder to read and maintain.
A workaround for this would be to include a require! macro that allows to include the code from another module. Functionality for this can be added using macro_magic.
It should be noted, that macro_magic has this warning about that approach:

Any types you may have brought into scope with use statements in the foreign module may or may not be available in their new context without additional use statements.

Another drawback is that this will break existing code that uses the old macro pattern.

Rationale and Alternatives

The main alternative is to drop the #[php_module] macro and require the user to register all functions, classes, etc. manually (#329).
This gives the author more control over how the module is wired in the end. I guess this would be similar to how crates like actix_web do things.
While this would work, it would also be a breaking change and not that easy to migrate to as this approach requires some manual work by the author.
Using the Outer Macro pattern would in theory allow to support both patterns, but would make most cases easier.

Another alternative is to keep the current macro pattern and fix the issues with it. This would still depend on the line order of the code and has no guarantee that it will not break again in the future.

Prior Art

This youtube video explains the concept of Outer Macros.

@Xenira Xenira added enhancement New feature or request question Further information is requested labels Oct 23, 2024
@Xenira
Copy link
Collaborator Author

Xenira commented Oct 23, 2024

@faassen

@faassen
Copy link
Contributor

faassen commented Oct 24, 2024

Okay, I wasn't aware of this pattern, so I'm going to have to read up on it.

I disagree an explicit strategy "makes it harder to read and maintain". It's more typing, but it's also more straightforward - less magic that can go wrong. We've already seen that implicit can lead to problems. So it depends on whether outer macros avoid those and whether the magic has other possible drawbacks.

@faassen
Copy link
Contributor

faassen commented Oct 24, 2024

I just watched a bit of the video on the outer macro pattern. That's a neat trick! The outer macro pattern really needs to be documented in the form of text somewhere. Since the outer macro has access to the complete picture it can generate module information freely in any order that makes PHP happy.

I do have the use case of building a relatively large extension where I'd like to break stuff up into separate Rust modules that need to be able to refer to each other. I don't understand yet how to make that work, unfortunately. (reading about macro_magic now).

A question, does this have to be defined on an explicit mod statement that has actual Rust content? That is, would this work at all?

#[php_module]
mod my_module; 

As to breaking backwards compatibility - that's a big one and I'm not sure how to handle it. I'm fine with breaking it as I'm writing something new, but existing PHP extension authors may have different ideas. If it'd just involve moving #[php_module] on an existing mod statement, it's easy enough, but is it?

@faassen
Copy link
Contributor

faassen commented Oct 24, 2024

As to a require! style statement, the example in macro_magic mentions this:

Any types you may have brought into scope with use statements in the foreign module may or may not be available in their new context without additional use statements.

which sounds scary. I don't understand this very well yet though. Presumably any use statements in the included module B are also in module A that does the including, and does this mean types are not available in A or in B?

@Xenira
Copy link
Collaborator Author

Xenira commented Oct 24, 2024

I disagree an explicit strategy "makes it harder to read and maintain". It's more typing, but it's also more straightforward - less magic that can go wrong. We've already seen that implicit can lead to problems. So it depends on whether outer macros avoid those and whether the magic has other possible drawbacks.

True, I have updated the Issue. Hope this better reflects that option.
I would like to offer both (but do one first). As the macros inside the outer macro are not 'real' it would be possible to still provide a real version of those for ppl who like to do the wiring manually.

A question, does this have to be defined on an explicit mod statement that has actual Rust content? That is, would this work at all?

#[php_module]
mod my_module; 

No, this won't work. You need to have an actual body.

As to breaking backwards compatibility - that's a big one and I'm not sure how to handle it. I'm fine with breaking it as I'm writing something new, but existing PHP extension authors may have different ideas. If it'd just involve moving #[php_module] on an existing mod statement, it's easy enough, but is it?

Should be easy enough to migrate. Ideally its just moving everything into that module. But an upgrade guide would definitely be required.
As this crate hasn't reached 1.0.0 yet I personally have no problem breaking things from time to time. But I am relatively new here, so I will refer to the others if there are differing opinions.

which sounds scary. I don't understand this very well yet though. Presumably any use statements in the included module B are also in module A that does the including, and does this mean types are not available in A or in B?

Wasn't entirely sure myself when I read that, but tested it and if I import mod B into A it will also copy all the use statements. If both have a use statement with the same name this will cause conflicts. This can be resolved with something like this use crate::ext2::HELLO as HELLO_EXT;
Basically its acting like a copy paste from the other module.

@faassen
Copy link
Contributor

faassen commented Oct 24, 2024

I'm happy to break stuff to make progress on this crate, but I'm even more new! :)

As to migration, it would require people to introduce an outer module. I also think I managed to make multi-module registrations work with ext-php-rs, so you'd use the #[php_class] macro in a second module, and import it into the one defining #[php_module]. The magic of a global STATE will cause that stuff to get registered, but that won't work with the outer macro pattern.

@faassen
Copy link
Contributor

faassen commented Oct 24, 2024

I wanted to understand the outer macro pattern better, and produced this repository to explore it:

https://github.com/faassen/outer_macro_pattern

I can confirm you can't mark non-inline mod foo with an attribute macro; stable Rust doesn't even support it I found out.

I intend to play around a bit more with modular patterns. I am wondering whether with macro magic we can cause other modules to be registered without importing them into the same namespace. Maybe we can cause something like this to work:

#[php_module]
mod my_module {
    #[php_register]
    use other;
}

Then the special #[php_register] attribute expands the use statement to this:

mod inner_other {
     // content of that other module included here
}

Then the #[php_module] macro can then treat these inner modules the same way as the module directly being registered, scanning them for structs, functions and impls. By wrapping them in a (hidden) module statement we won't get import conflicts. I'm handwaving a lot here, but I'm hopeful we can make something like that work.

(I just thought of something; inclusion of code with use statements into a submodule or something would break them, wouldn't it? hopefully we can come up with some restrictions, only direct sibling modules can be registered)

@Xenira
Copy link
Collaborator Author

Xenira commented Oct 24, 2024

Then the #[php_module] macro can then treat these inner modules the same way as the module directly being registered, scanning them for structs, functions and impls. By wrapping them in a (hidden) module statement we won't get import conflicts. I'm handwaving a lot here, but I'm hopeful we can make something like that work.

Sounds interesting. So you are thinking about something that expands into something like this?

mod my_mod {
    use crate::ext2::HELLO;
    use proc::require;
    pub fn hello2() {
        {
            ::std::io::_print(format_args!("{0}\n", HELLO));
        };
    }
    
    mod inner_other {
        use crate::ext2::HELLO;
        pub fn hello() {
            {
                ::std::io::_print(format_args!("{0}\n", HELLO_EXT));
            };
        }
    }
}

Will try to create a poc on the weekend.

@Xenira
Copy link
Collaborator Author

Xenira commented Oct 27, 2024

Did not have that much time, but built a rly simle poc only supporting functions for now.

#[php_module]
mod module {
    #[php_function()]
    /// Test Function
    pub fn hello_world() {
        println!("Hello, world!");
    }
}

expands to

#![feature(prelude_import)]
#[prelude_import]
use std::prelude::rust_2021::*;
#[macro_use]
extern crate std;
use ext_php_rs::prelude::*;
mod module {
    fn hello_world() {
        {
            ::std::io::_print(format_args!("Hello, world!\n"));
        };
    }
    #[doc(hidden)]
    pub extern "C" fn _internal_php_hello_world(
        ex: &mut ::ext_php_rs::zend::ExecuteData,
        retval: &mut ::ext_php_rs::types::Zval,
    ) {
        use ::ext_php_rs::convert::IntoZval;
        let parser = ex.parser();
        let parser = parser.parse();
        if parser.is_err() {
            return;
        }
        let result = hello_world();
        if let Err(e) = result.set_zval(retval, false) {
            let e: ::ext_php_rs::exception::PhpException = e.into();
            e.throw().expect("Failed to throw exception");
        }
    }
}
#[doc(hidden)]
#[no_mangle]
pub extern "C" fn get_module() -> *mut ::ext_php_rs::zend::ModuleEntry {
    let mut builder = ::ext_php_rs::builders::ModuleBuilder::new(
            "ext_outer_macro",
            "0.1.0",
        )
        .function(
            ::ext_php_rs::builders::FunctionBuilder::new(
                    "hello_world",
                    module::_internal_php_hello_world,
                )
                .build()
                .unwrap(),
        );
    match builder.build() {
        Ok(module) => module.into_raw(),
        Err(e) => {
            ::std::rt::panic_fmt(format_args!("Failed to build PHP module: {0:?}", e));
        }
    }
}
#[cfg(debug_assertions)]
#[no_mangle]
pub extern "C" fn ext_php_rs_describe_module() -> ::ext_php_rs::describe::Description {
    use ::ext_php_rs::describe::*;
    Description::new(Module {
        name: "ext_outer_macro".into(),
        functions: <[_]>::into_vec(
                #[rustc_box]
                ::alloc::boxed::Box::new([
                    Function {
                        name: "hello_world".into(),
                        docs: DocBlock(
                            <[_]>::into_vec(
                                    #[rustc_box]
                                    ::alloc::boxed::Box::new([" Test Function".into()]),
                                )
                                .into(),
                        ),
                        ret: abi::Option::None,
                        params: ::alloc::vec::Vec::new().into(),
                    },
                ]),
            )
            .into(),
        classes: ::alloc::vec::Vec::new().into(),
        constants: ::alloc::vec::Vec::new().into(),
    })
}

Was not 100% straight forward to implement, but not so bad.

  • You need to reproduce the content of the module while striping out the inner macros
  • Parsing arguments works a little different. parse_args panics if there are no args (empty is fine though, notice the brackets at #[php_function()]). This should be easily solvable though.
  • Multiple attributes on an item need to be handled.
  • Proper error messages are more difficult. By default the entire macro gets marked.

Other than that I was able to just reuse most of the existing macro code. This is great, as this would allow both the manual registration using the existing macros and auto registration using the #[php_module] macro.

Will post the src once I have cleaned it up a little.

@faassen
Copy link
Contributor

faassen commented Nov 18, 2024

Sorry to be MIA, I focused on other parts of my project for a while. It's great you made progress! Is there a way we can collaborate on it?

@Xenira
Copy link
Collaborator Author

Xenira commented Nov 18, 2024

Didn't have much time to work on it either. I hope I can find some time this or next week.

@Xenira
Copy link
Collaborator Author

Xenira commented Nov 25, 2024

Created a branch with early wip. Still a lot of experimentation and stuff is not where its supposed to be.
feat/outer-macro

@Xenira Xenira linked a pull request Nov 26, 2024 that will close this issue
12 tasks
@Xenira Xenira linked a pull request Nov 26, 2024 that will close this issue
12 tasks
@Xenira
Copy link
Collaborator Author

Xenira commented Nov 27, 2024

@faassen think I have a first version of everything implemented. Needs some cleanup, but all macros should work with outer macro pattern.

@Xenira Xenira self-assigned this Nov 28, 2024
Xenira added a commit that referenced this issue Dec 4, 2024
Xenira added a commit that referenced this issue Dec 4, 2024
@Xenira
Copy link
Collaborator Author

Xenira commented Jan 7, 2025

Dependencies in macro generation. Gonna have a more detailed look at some stuff, as the tokio stuff would not work as is. Mostly for me as reference

---
  config:
    class:
      hideEmptyMembersBox: true
---
classDiagram
    STARTUP-FUNCTION *-- CLASS
    STARTUP-FUNCTION *-- CONSTANT
    IMPLEMENTATION *-- CLASS
    STUBS *-- CLASS
    STUBS *-- FUNCTION
    STUBS *-- CONSTANT
    BUILDER *-- STARTUP-FUNCTION
    BUILDER *-- FUNCTION
Loading

@Xenira
Copy link
Collaborator Author

Xenira commented Jan 7, 2025

@danog could you give your opinion.

The trouble with php-tokio is, that it generates the function too late:

  1. #[php-module] outer macro is evaluated leaving #[php_async_impl] untouched
  2. #[php_async_impl] is evaluated generating #[php_impl]
  3. Module is already build and the impl from (2.) is not included

I am not entirely sure, if the trade offs regarding 'syntactic sugar' vs extensibility are worth it anymore.

Extending something like this would require to also use outer macro pattern to generate the 'inner outer #[php_module]' if that makes sense.

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

Successfully merging a pull request may close this issue.

2 participants