Attributes that target functions are not valid for anonymous functions defined within a method

[ollieread]

Description

If an anonymous function is defined inside a method, attempting to create a new instance of any attribute attached to it, through reflection, that only targets functions (#[Attribute(Attribute::TARGET_FUNCTION)]) will cause an error with the message.

Fatal error: Uncaught Error: Attribute "AttributeName" cannot target method (allowed targets: function)

This happens in both object and static contexts. Changing the attribute to work with methods instead will cause an inverted error if the closure is defined outside of a method within the global context.

Examples

This code works fine and gives no output, as expected. (https://3v4l.org/iDuIL#v8.1.4)

<?php

#[Attribute(Attribute::TARGET_FUNCTION)]
class FunctionAttribute
{
}

$closure = #[FunctionAttribute]
fn() => true;

class Something
{
    public function get(Closure $closure)
    {
        return (new ReflectionFunction($closure))
                   ->getAttributes(FunctionAttribute::class)[0]
            ->newInstance();
    }
}

(new Something)->get($closure);

This code gives an error. (https://3v4l.org/Clenb#v8.1.4)

<?php

#[Attribute(Attribute::TARGET_FUNCTION)]
class FunctionAttribute
{
}

class Something
{
    public function get()
    {
        $closure = #[FunctionAttribute]
        fn() => true;
        return (new ReflectionFunction($closure))
                   ->getAttributes(FunctionAttribute::class)[0]
            ->newInstance();
    }
}

(new Something)->get();

Its output is:

Fatal error: Uncaught Error: Attribute "FunctionAttribute" cannot target method (allowed targets: function) in /in/Clenb:16
Stack trace:
#0 /in/Clenb(16): ReflectionAttribute->newInstance()
#1 /in/Clenb(20): Something->get()
#2 {main}
  thrown in /in/Clenb on line 16

Process exited with code 255.

As does this same code, but using a static method instead. (https://3v4l.org/s4Gk0#v8.1.4)

<?php

#[Attribute(Attribute::TARGET_FUNCTION)]
class FunctionAttribute
{
}

class Something
{
    public static function get()
    {
        $closure = #[FunctionAttribute]
        fn() => true;
        return (new ReflectionFunction($closure))
                   ->getAttributes(FunctionAttribute::class)[0]
            ->newInstance();
    }
}

Something::get();

Which also outputs:

Fatal error: Uncaught Error: Attribute "FunctionAttribute" cannot target method (allowed targets: function) in /in/s4Gk0:16
Stack trace:
#0 /in/s4Gk0(16): ReflectionAttribute->newInstance()
#1 /in/s4Gk0(20): Something::get()
#2 {main}
  thrown in /in/s4Gk0 on line 16

Process exited with code 255.

PHP Version

8.1.4

Operating System

No response

[ollieread]

I don't actually know C, and the last 2 hours I spent going through the PHP source is quite possibly the first time I've ever had to navigate a C codebase, but I think I have found the issue. (journey thread, if it helps)

It's ReflectionFunctionAbstract::getAttributes (https://github.com/php/php-src/blob/master/ext/reflection/php_reflection.c#L1875)

ZEND_METHOD(ReflectionFunctionAbstract, getAttributes)
{
	reflection_object *intern;
	zend_function *fptr;
	uint32_t target;

	GET_REFLECTION_OBJECT_PTR(fptr);

	if (fptr->common.scope) {
		target = ZEND_ATTRIBUTE_TARGET_METHOD;
	} else {
		target = ZEND_ATTRIBUTE_TARGET_FUNCTION;
	}

	reflect_attributes(INTERNAL_FUNCTION_PARAM_PASSTHRU,
		fptr->common.attributes, 0, fptr->common.scope, target,
		fptr->type == ZEND_USER_FUNCTION ? fptr->op_array.filename : NULL);
}

If the function reference has a scope, it assumes it's a method. I can't find anything that would point to an easy fix, as it seems that within these chunks of code there's nothing to identify a closure over any other kind of function. Not in reflection_type_t or zend_function.type. There is zend_function.common.type which according to a comment is never used.

Again, my C knowledge is limited (to the last 2 hours), but thought this may be able to help someone.

[kallesommernielsen]

A closure is not a procedural function, a closure is actually a object of the \Closure class with the method body being the code executed inside the __invoke() magic method for which the closure syntax is sugar for really.

I doubt closures will be supported by attributes anytime soon as it will require copying attributes to the object and you would then only be able to fetch it via ReflectionObject as there isn't really another reliable way to accomplish this.

[ollieread]

A closure is not a procedural function, a closure is actually a object of the \Closure class with the method body being the code executed inside the __invoke() magic method for which the closure syntax is sugar for really.

I doubt closures will be supported by attributes anytime soon as it will require copying attributes to the object and you would then only be able to fetch it via ReflectionObject as there isn't really another reliable way to accomplish this.

But closures do support attributes. The type of target they require depends entirely on whether or not they have a scope, which feels like a definite bug.

[iluuu1994]

Hm, the documentation doesn't really specify this very well. Technically $this is bound in the closure which makes it more than a normal function, but it's also not a method, but TARGET_METHOD currently passes. @beberlei Do you have any opinions on this? Note that a static closure also only passes the TARGET_METHOD check.

[ollieread]

Hm, the documentation doesn't really specify this very well. Technically $this is bound in the closure which makes it more than a normal function, but it's also not a method, but TARGET_METHOD currently passes. @beberlei Do you have any opinions on this? Note that a static closure also only passes the TARGET_METHOD check.

That was what I thought initially, but attributes are relevant to their target structure/element, not their context.

[nikic]

Hm, the documentation doesn't really specify this very well. Technically $this is bound in the closure which makes it more than a normal function, but it's also not a method, but TARGET_METHOD currently passes. @beberlei Do you have any opinions on this? Note that a static closure also only passes the TARGET_METHOD check.

That was what I thought initially, but attributes are relevant to their target structure/element, not their context.

Agree, I think closures should use TARGET_FUNCTION. It should be possible to handle this with an additional check for ZEND_ACC_CLOSURE.

[beberlei]

Yes indeed the check for closure is missing

[ollieread]

Agree, I think closures should use TARGET_FUNCTION. It should be possible to handle this with an additional check for ZEND_ACC_CLOSURE.

So if my understanding is correct, a fix for this would be;

ZEND_METHOD(ReflectionFunctionAbstract, getAttributes)
{
	reflection_object *intern;
	zend_function *fptr;
	uint32_t target;

	GET_REFLECTION_OBJECT_PTR(fptr);

	if (fptr->common.scope && !(fptr->common.fn_flags & ZEND_ACC_CLOSURE)) {
		target = ZEND_ATTRIBUTE_TARGET_METHOD;
	} else {
		target = ZEND_ATTRIBUTE_TARGET_FUNCTION;
	}

	reflect_attributes(INTERNAL_FUNCTION_PARAM_PASSTHRU,
		fptr->common.attributes, 0, fptr->common.scope, target,
		fptr->type == ZEND_USER_FUNCTION ? fptr->op_array.filename : NULL);
}

If this is the case, I'm happy to make a PR with the changes and a test, unless somebody else would rather pick it up.

[iluuu1994]

@ollieread That looks sensible. Yes please, feel free to create a PR with a corresponding test 🙂 Remember to add a test for static closures.

[ollieread]

Will do @iluuu1994, though it appears there's a problem with the PHP 8.1 branch. I can run and build from master, but not PHP 8.1. A lot of issues with ZEND_MM_ALIGNMENT.