chore(macro): Deprecate global state in macro system #174

davidcole1340 · 2022-11-10T02:36:49Z

closes #131

azjezz · 2022-11-26T08:33:05Z

@davidcole1340 what is the status here? anything i can do to help? :)

ptondereau · 2022-11-26T12:37:30Z

@davidcole1340 what is the status here? anything i can do to help? :)

Testing testing and testing haha. From the author, this is mainly finished

azjezz · 2022-11-26T13:09:34Z

I have actually tested it a bit today :D seems good to me

ju1ius · 2022-11-28T14:32:36Z

guide/src/macros/impl.md

-annotated with the `#[this]` attribute. This can also be used to return a
-reference to `$this`.
+`ZendClassObject<T>` in place of the self parameter, where the parameter must
+be named `self_`. This can also be used to return a reference to `$this`.


Why not requiring this as a name?
Makes it both idiomatic and obvious to me.

My future self is still sad about using self_ instead of the idiomatic and less typo-prone this... 😢

ju1ius · 2022-11-28T15:15:26Z

NEW_MACROS.md

+## Startup Function
+
+The `#[php_startup]` macro has been deprecated. Instead, define a function with
+the signature `fn(ty: i32, mod_num: i32) -> i32` and provide the function name


Maybe explain what those numbers mean?

I've done a small explanation here

Off the top of my head the module type can only be one of MODULE_PERSISTENT or MODULE_TEMPORARY, so it would be nicer to have a :

pub enum ModuleType { Persistent, Temporary, } impl From<i32> for ModuleType { fn from(value: i32) -> Self { match value => { ffi::MODULE_PERSISTENT => Persistent, ffi::MODULE_TEMPORARY => Temporary, _ => panic!("onoes!") } } } // TODO: impl From<ModuleType> for i32

and then:

fn startup(ty: ModuleType, id: i32) -> Result<(), SomeErrorType> { Ok(()) }

Temporary remains a rare case, and then the default case is totally fine for now.
Let's open another PR, after merging it

ju1ius · 2022-11-28T15:16:04Z

guide/src/macros/classes.md

@@ -16,11 +16,10 @@ There are also additional macros that modify the class. These macros **must** be
 placed underneath the `#[php_class]` attribute.

 - `#[extends(ce)]` - Sets the parent class of the class. Can only be used once.
-  `ce` must be a valid Rust expression when it is called inside the
-  `#[php_module]` function.
+  `ce` must be a function with the signature `fn() -> &'static ClassEntry`.


Good move! 👍🏻

I like this change but it's not clear how you'd retrieve the ClassEntry of one of your registered classes.
There should be an example of how to do that in the documentation, i.e:

#[php_class] pub struct Parent; #[php_class] #[extends(get_parent_class)] pub struct Child; fn get_parent_class() -> &'static ClassEntry { // ???????? }

Answering to my past self:

#[php_class] pub struct Parent; fn get_parent_class() -> &'static ClassEntry { <Parent as RegisteredClass>::get_metadata().ce() }

ju1ius · 2022-11-28T17:14:25Z

src/convert.rs

@@ -119,6 +119,9 @@ pub trait IntoZval: Sized {
    /// The corresponding type of the implemented value in PHP.
    const TYPE: DataType;

+    /// Whether converting into a [`Zval`] may result in null.
+    const NULLABLE: bool;


What's the rationale behind these NULLABLEs in conversion traits?

ju1ius · 2022-11-28T19:00:03Z

I did a bit of basic tests and so far your PR solves one of the big pain points I have with the current macro system:
When you have a modestly large project with some submodules, you can not register functions/classes without having to star-import your subodule into the main lib file.

With this PR, now it is possible to do this:

// lib.rs
mod sub;

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

// sub/mod.rs

#[php_function]
fn hello_sub(name: &str) -> String {
    format!("Hello sub, {}!", name)
}

pub fn register(module: ModuleBuilder) -> ModuleBuilder {
    module.function(wrap_function!(hello_sub))
}

without having to import all the things into the main module, so big 👍🏻 on this !

ju1ius · 2022-11-28T19:25:19Z

crates/macros/src/syn_ext.rs

@@ -136,6 +136,7 @@ impl DropLifetimes for syn::TypePath {
        if let Some(qself) = &mut self.qself {
            qself.ty.drop_lifetimes();
        }
+        self.path.segments.drop_lifetimes();


Tell me if I'm wrong but if this is the part that removes lifetimes from items, wouldn't it be better to throw an error here ?
Because currently if you try to add a lifetime to a registered class, this is the compiler message:

error[E0726]: implicit elided lifetime not allowed here --> src/lib.rs:4:8 | 4 | struct Foo<'a> { | ^^^ expected lifetime parameter | = note: assuming a `'static` lifetime... help: indicate the anonymous lifetime | 4 | struct Foo<'_><'a> { | ++++

Which is rather misleading.
Given that the restrictions on lifetime and generic parameters are easily overlooked by newcomers, I think that an error stating explicitely that this is not allowed by the library would be better.

Answering my past self: self.path.segments.drop_lifetimes() drops lifetimes of function and method arguments.

Since I'm not fond of type conversion magic at function boundaries, I have no further opinion on whether dropping lifetime for arguments is a good thing or not.

But I still think that #[php_class] should fail to compile with an error message like:

Lifetimes and generic parameters are not allowed on classes and class members

instead of the message seen in the previous comment (but this can be done in a later PR).

src/builders/module.rs

ju1ius · 2022-11-28T19:32:13Z

The build fails when the closure feature is enabled.

error[E0046]: not all trait items implemented, missing: `BUILDER_MODIFIER`, `EXTENDS`, `IMPLEMENTS`, `method_builders`, `constructor`, `constants`
   --> /ext-php-rs/src/closure.rs:152:1
    |
152 | impl RegisteredClass for Closure {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `BUILDER_MODIFIER`, `EXTENDS`, `IMPLEMENTS`, `method_builders`, `constructor`, `constants` in implementation
    |
   ::: /ext-php-rs/src/class.rs:28:5
    |
28  |     const BUILDER_MODIFIER: Option<fn(ClassBuilder) -> ClassBuilder>;
    |     ---------------------------------------------------------------- `BUILDER_MODIFIER` from trait
...
31  |     const EXTENDS: Option<fn() -> &'static ClassEntry>;
    |     -------------------------------------------------- `EXTENDS` from trait
...
34  |     const IMPLEMENTS: &'static [fn() -> &'static ClassEntry];
    |     -------------------------------------------------------- `IMPLEMENTS` from trait
...
60  |     fn method_builders() -> Vec<(FunctionBuilder<'static>, MethodFlags)>;
    |     --------------------------------------------------------------------- `method_builders` from trait
...
63  |     fn constructor() -> Option<ConstructorMeta<Self>>;
    |     -------------------------------------------------- `constructor` from trait
...
66  |     fn constants() -> &'static [(&'static str, &'static dyn IntoZvalDyn)];
    |     ---------------------------------------------------------------------- `constants` from trait

ptondereau · 2023-10-26T10:45:18Z

Green CI here! Maybe have a second round of review @ju1ius @joehoyle @joelwurtz @danog

ju1ius · 2023-10-26T13:50:42Z

@joelwurtz I'm not sure I understand...

I have the feeling that you're confusing modules with extensions.

Modules are loaded from ini entries (extension=libfoo.so) and must export a get_module symbol which must be a function returning a zend_module_entry pointer.

Extensions are loaded from ini entries (zend_extension=libbar.so) and must export a zend_extension_entry symbol which must be a statically allocated zend_extension struct.

So to me it sounds like what you'd like to implement is the hybrid extension pattern described in the book.

Or maybe I'm just missing something, in which case would you mind clarifying? 😉

joelwurtz · 2023-10-26T14:14:47Z

I was correctly talking about a PHP extension (zend_module_entry) but i misunderstood how sapi (and so embed) loaded themself as a PHP extension (so you could build a php binary with extra functions)

But in fact this is done when using the php_module_startup function (https://github.com/php/php-src/blob/master/main/main.c#L2027) which takes an sapi module and add optional zend_module_entry pointer (so it can register a new extension when starting up)

As an example this is done by php-fpm to register it's own functions (like it's shutdown one) https://github.com/php/php-src/blob/master/sapi/fpm/fpm/fpm_main.c#L774

So disregard my comment it's off topic for this PR, sorry for the noise

I was confused on how PHP builtin extension (like date or other ones) were loaded (when they are not a shared file), and in fact it's just a template header file that is computed during make which list all extensions that will be compiled directly inside binaries (php cli / fpm / embed / etc ...) see https://github.com/php/php-src/blob/master/main/internal_functions.c.in

joelwurtz

About this move i'm 👍 even if it's add boilerplate i find this just as verbose as it should to be self understandable with code without too much work

joelwurtz · 2023-10-26T14:32:14Z

NEW_MACROS.md

+
+fn startup(ty: i32, mod_num: i32) -> i32 {
+    // register extra classes, constants etc
+    5.register_constant("SOME_CONST", mod_num);


typo ? i believe it should be in the get_module function

I think not. It's the global constant declaration here, and it's mostly used as a flag for the PHP end user
see:

ext-php-rs/src/constant.rs

Lines 15 to 23 in 417f495

/// Registers a global module constant in PHP, with the value as the content

/// of self. This function _must_ be called in the module startup

/// function, which is called after the module is initialized. The

/// second parameter of the startup function will be the module number.

/// By default, the case-insensitive and persistent flags are set when

/// registering the constant.

///

/// Returns a result containing nothing if the constant was successfully

/// registered.

typo ? i believe it should be in the get_module function

No this code is correct.

The get_module function is used for registering functions, ini settings and module metadata (name, version, dependencies). Everything else must happen in startup (AKA MINIT), unless it need to be allocated per-request, in which case one must use request_startup (AKA RINIT).

EDIT: I was thinking in terms of the zend engine and forgot that this library actually lets you register everything inside get_module (by internally delaying the registration until startup). The code is correct nevertheless.

ju1ius · 2023-10-26T15:59:15Z

NEW_MACROS.md

+to the `#[php_module]` attribute:
+`mod_num` is the module number assigned by PHP for its internal management. The `ModuleBuilder` always defaults this parameter to `0`.
+`ty` is the type of module (flag) which can be set to `MODULE_PERSISTENT = 0`.
+The return number is for internal purposes and should always be `0` in this case. It's an internal mechanism to tell to the ModuleBuilder weither to use the defautlt startup function or the overriden one. (see: `crates/macros/src/module.rs:L35`)


Are you shure of this ?

The module lifecycle callbacks are supposed to return a zend_result_code (either SUCCESS or FAILURE), so returning something like Result<(), ()> or Result<(), Box<dyn Error>> would be a much better choice IMO.

It could be done in seperate PR, it will require some parsing here to enforce this signature

ju1ius · 2023-10-26T16:05:42Z

NEW_MACROS.md

+## Startup Function
+
+The `#[php_startup]` macro has been deprecated. Instead, define a function with
+the signature `fn(ty: i32, mod_num: i32) -> i32` and provide the function name


Off the top of my head the module type can only be one of MODULE_PERSISTENT or MODULE_TEMPORARY, so it would be nicer to have a :

pub enum ModuleType { Persistent, Temporary, } impl From<i32> for ModuleType { fn from(value: i32) -> Self { match value => { ffi::MODULE_PERSISTENT => Persistent, ffi::MODULE_TEMPORARY => Temporary, _ => panic!("onoes!") } } } // TODO: impl From<ModuleType> for i32

and then:

fn startup(ty: ModuleType, id: i32) -> Result<(), SomeErrorType> { Ok(()) }

ju1ius · 2023-10-26T17:30:29Z

I was correctly talking about a PHP extension

Yes the fact that these terms are mixed and matched in the source PHP source code and documentation is rather unfortunate (mostly due to legacy I guess) and doesn't ease communication. I personnaly think that «modules vs extensions» is clearer than «PHP extensions vs Zend extensions», but to each his own I guess.

WRT the actual PR

Sorry, I don't have the bandwidth to make an exhaustive review but from what I can remember from last year it was a clear step forward to me, so I'm all in favour of merging it while it's hot !

…tion

davidcole1340 force-pushed the macro-state-deprecation branch 4 times, most recently from 3ebc440 to 5898aaf Compare November 10, 2022 23:14

ju1ius reviewed Nov 28, 2022

View reviewed changes

src/builders/module.rs Outdated Show resolved Hide resolved

davidcole1340 added 9 commits October 25, 2023 17:27

Start moving away from global macro state

783b1ab

Rewrite #[php_function] attribute

3388f5f

many months ago

babbaff

tidy up

2e2f361

update docs

706803f

add alternative startup func

47c6e4f

intozval etc.

8070733

start updating docs

ee45551

documentation

ab0bbe9

ptondereau force-pushed the macro-state-deprecation branch from 0b8996c to ab0bbe9 Compare October 25, 2023 15:30

ptondereau changed the title ~~deprecate global state in macro system~~ chore(macro): Deprecate global state in macro system Oct 25, 2023

ptondereau added 4 commits October 25, 2023 17:39

correct clippy and stuff

a98129d

fix closure impl

f605c54

fix doc+skeptic tests

b55e6c2

fix and add some doc for startup function

0823e5e

add attribute for windows feature

417f495

This comment was marked as off-topic.

Sign in to view

joelwurtz reviewed Oct 26, 2023

View reviewed changes

ju1ius reviewed Oct 26, 2023

View reviewed changes

ptondereau marked this pull request as ready for review November 8, 2023 20:56

danog and others added 4 commits November 24, 2023 18:49

Merge remote-tracking branch 'origin/master' into macro-state-depreca…

ff0a298

…tion

Cleanup

397e805

Merge branch 'master' into macro-state-deprecation

6e7b5f9

defaulting property attributes when #[prop] fails to parse.

ebe4170

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(macro): Deprecate global state in macro system #174

chore(macro): Deprecate global state in macro system #174

davidcole1340 commented Nov 10, 2022

azjezz commented Nov 26, 2022

ptondereau commented Nov 26, 2022

azjezz commented Nov 26, 2022

ju1ius Nov 28, 2022

ju1ius Oct 26, 2023 •

edited

Loading

ju1ius Nov 28, 2022

ptondereau Oct 26, 2023

ju1ius Oct 26, 2023 •

edited

Loading

ptondereau Oct 26, 2023

ju1ius Nov 28, 2022

ju1ius Nov 29, 2022

ju1ius Oct 26, 2023 •

edited

Loading

ju1ius Nov 28, 2022

ju1ius commented Nov 28, 2022

ju1ius Nov 28, 2022

ju1ius Oct 26, 2023 •

edited

Loading

ju1ius commented Nov 28, 2022 •

edited

Loading

ptondereau commented Oct 26, 2023

This comment was marked as off-topic.

ju1ius commented Oct 26, 2023 •

edited

Loading

joelwurtz commented Oct 26, 2023

joelwurtz left a comment

joelwurtz Oct 26, 2023

ptondereau Oct 26, 2023

ju1ius Oct 26, 2023 •

edited

Loading

ju1ius Oct 26, 2023 •

edited

Loading

ptondereau Nov 8, 2023

ju1ius Oct 26, 2023 •

edited

Loading

ju1ius commented Oct 26, 2023

	/// Registers a global module constant in PHP, with the value as the content
	/// of self. This function _must_ be called in the module startup
	/// function, which is called after the module is initialized. The
	/// second parameter of the startup function will be the module number.
	/// By default, the case-insensitive and persistent flags are set when
	/// registering the constant.
	///
	/// Returns a result containing nothing if the constant was successfully
	/// registered.

chore(macro): Deprecate global state in macro system #174

Are you sure you want to change the base?

chore(macro): Deprecate global state in macro system #174

Conversation

davidcole1340 commented Nov 10, 2022

azjezz commented Nov 26, 2022

ptondereau commented Nov 26, 2022

azjezz commented Nov 26, 2022

Choose a reason for hiding this comment

ju1ius Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ju1ius Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ju1ius Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ju1ius commented Nov 28, 2022

Choose a reason for hiding this comment

ju1ius Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

ju1ius commented Nov 28, 2022 • edited Loading

ptondereau commented Oct 26, 2023

This comment was marked as off-topic.

ju1ius commented Oct 26, 2023 • edited Loading

joelwurtz commented Oct 26, 2023

joelwurtz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ju1ius Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

ju1ius Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ju1ius Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

ju1ius commented Oct 26, 2023

WRT the actual PR

ju1ius Oct 26, 2023 •

edited

Loading

ju1ius Oct 26, 2023 •

edited

Loading

ju1ius Oct 26, 2023 •

edited

Loading

ju1ius Oct 26, 2023 •

edited

Loading

ju1ius commented Nov 28, 2022 •

edited

Loading

ju1ius commented Oct 26, 2023 •

edited

Loading

ju1ius Oct 26, 2023 •

edited

Loading

ju1ius Oct 26, 2023 •

edited

Loading

ju1ius Oct 26, 2023 •

edited

Loading