core: set zend_pass_function as default constructor

[Girgias]

Currently, it's basically impossible to determine reliably if an internal class is instantiable.

The current way is to overload the get_constructor object handler to throw an exception and return NULL.
This is problematic for two reasons:

• It's easy to forget to do, compared to simply checking ce->constructor
• get_constructor can return NULL if no explicit constructor was set

This complexity in semantics was one of the motivating factors for introducing the object_init_with_constructor() API. However, this doesn't cleanly apply to many cases due to the "unorthodox" behaviour of DB drivers prepopulating object properties and being able to modify them in the constructor.

These issues are mainly problematic as it means it's possible to create broken instances of internal objects (e.g. CurlHandler).

One other issue with the current design, is that to be able to call the get_constructor object handler one must first create the object just to immediately discard it, this is wasteful but moreover in those cases the destructor of the object shouldn't run and one must remember to call the zend_object_store_ctor_failed(obj); API instead of the usual OBJ_RELEASE().

Therefore, we propose to use the "magic" internal zend_pass_function, which is already used as a dummy constructor call, as the default value for ce->constructor. And rather than overriding the get_constructor object handler to mark a class as instantiable we add a \#[NonInstantiableClass("reason")] attribute that replaces the ce->constructor with NULL.

This design has a few advantages:

• A class is instantiable if ce->constructor != NULL
• It is extensible to userland
• It is possible to remove the get_constructor object handler (as it is only overridden to throw a custom exception)

This would also fix certain Reflection bugs such as #17796, and should make it finally possible to grab a closure using the FCC syntax of new MyClass(...).

I'm quite happy to not offer this to userland yet while we iron out the kinks (and I don't have the energy to submit an RFC for this at the moment) by making the attribute internal use only.

Remaining planned items:

• Remove the get_constructor object handler.
• Ensure opcache works as intended
• Potentially use a class flag instead of needing to always fetch the attribute?

[arnaud-lb]

+1 for eventually removing get_constructor (or at least making it class-scoped instead of object-scoped by moving it to zend_class_entry).

I'm not a fan of using ce->constructor == NULL as a proxy for "object is not instantiable" and of setting it to a no-op function by default because this forces the new operator for check for it in the happy path.

An alternative would be to add a private constructor that throws on these classes (as suggested in #17796 (comment)). This can be done by gen_stubs when a class is annotated with \#[NonInstantiableClass("reason")]. The same constructor implementation can be re-used (the reason message can be fetched at runtime from attributes).

This way the new operator can simply call the constructor if there is one, and visibility checks will do the rest.

Reflection ::isInstantiable() can lookup the attribute, but checking that a class has a private constructor may be enough?

[Girgias]

+1 for eventually removing get_constructor (or at least making it class-scoped instead of object-scoped by moving it to zend_class_entry).

I'm not a fan of using ce->constructor == NULL as a proxy for "object is not instantiable" and of setting it to a no-op function by default because this forces the new operator for check for it in the happy path.

An alternative would be to add a private constructor that throws on these classes (as suggested in #17796 (comment)). This can be done by gen_stubs when a class is annotated with \#[NonInstantiableClass("reason")]. The same constructor implementation can be re-used (the reason message can be fetched at runtime from attributes).

This way the new operator can simply call the constructor if there is one, and visibility checks will do the rest.

I'm happy to make the attribute add a private constructor, but my initial understanding as to why nobody wanted to pursue this is because it wouldn't allow a custom message like we currently have. But I guess this is just changing how we handle errors.

And just to clarify, when you say gen_stub can generate the implementation do you want it to generate individually constructor methods, or that it points to a single implementation that is aliased?

Reflection ::isInstantiable() can lookup the attribute, but checking that a class has a private constructor may be enough?

This probably could work as most newInstance reflection methods check if the constructor is public, but not newInstanceWithoutConstructor() so we might need to do a small BC break to normalise its behaviour with the rest of the extension and language.

Might make sense to add a helper function zend_is_object_instantiable that does all the checks if it's a concrete class with a public constructor, as the logic is repeated quite often.

[arnaud-lb]

Possibly this can be generated with a @generate-function-entries stub (like ext/zend_test/tmp_methods.stub.php).

[arnaud-lb]

Should you error if there is already a constructor? Otherwise we can do this:

#[NonInstantiableClass("test")]
class C {
    public function __construct() {}
}

new C(); // works

[arnaud-lb]

Suggested change

	. pg_fetch_object() now surfaces non-instantiable class errorsv before
	. pg_fetch_object() now surfaces non-instantiable class errors before

[arnaud-lb]

From the context of ZEND_NEW there is some redundancy as some of these checks are also performed by object_init_ex. We could rename this method to "check_constructor_visibility_or_throw", remove the ZEND_ACC_UNINSTANTIABLE check, and rely on the non_instantiable_constructor to throw when it's called.

[arnaud-lb]

What is the reason for this? This provides no extra safety to internal classes as sub-classing does not inherit NonInstantiableClass.