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

Attributes v2 #2

Closed
wants to merge 43 commits into from
Closed

Attributes v2 #2

wants to merge 43 commits into from

Conversation

beberlei
Copy link
Owner

@beberlei beberlei commented Feb 20, 2020

RFC: https://wiki.php.net/rfc/attributes_v2

Required work

  • Full resolve class names, currently hardcoded NOFQ
  • ReflectionAttribute::getAsObject implementation.
  • Fix name inconsistencies, attribute_value should become attribute_argument, attribute_list should be the top definition of << attributes >> in parser.
  • Memory leaks
  • Multiple arguments with a constant expression seem to be ignored by parser
  • Add support for attributes on ReflectionParameter
  • Implement $name filter for attribute or subclass of
  • Add PhpAttribute and PhpCompilerAttribute classes
  • Validate correctness of compiler attributes during compile step.

Examples

@brzuchal
Copy link

brzuchal commented Mar 9, 2020

@beberlei why forbidding the use of two attributes with the same name? While I cannot find a good example from Doctrine and Symfony Routing component, I can imagine <<ParamConverter()>> being used several times describing the conversion of params individually.

@tsantos84
Copy link

tsantos84 commented Mar 9, 2020

@beberlei why forbidding the use of two attributes with the same name? While I cannot find a good example from Doctrine and Symfony Routing component, I can imagine <<ParamConverter()>> being used several times describing the conversion of params individually.

For class validation, we could use the same annotation twice on one attribute:

class Foo {
    <<Length(groups=['bar'], min=10)>>
    <<Length(groups=['baz'], min=20)>>
    private string $foo;
}

@tsantos84
Copy link

tsantos84 commented Mar 9, 2020

Any special reason to not allow annotations for function arguments? It could be useful to annotate, for example, an argument to hold request content body like this:

class MyController {
    function fooAction(<<RequestBody>> string $body) {}
    function barAction(<<RequestBody(required=false)>> string ?$body) {}
}

Dependency Injection tools can use argument annotations to improve its autowiring mechanism.

@beberlei
Copy link
Owner Author

beberlei commented Mar 9, 2020

@tsantos84 Yes and no, arguments don't have docblocks either, so the internal structure isn't as prepared for this. I will see what should be done about this.

@zmitic
Copy link

zmitic commented Mar 9, 2020

Question; I understand we can't have @ for annotations now but if this is for PHP8, couldn't we get it?
BC can be broken in major versions and STFU operator can be replaced with some other.

But whatever happens, I really hope we could get them for arguments as well. It opens a whole new world of autowiring possibilities 🍺

@mehranhadidi
Copy link

I agree with @zmitic. I really hope we can have this feature with @. It can be a lot nicer & convenient to read & write. Thank anyway

@stollr
Copy link

stollr commented Mar 10, 2020

Just an idea: Maybe it would be better to use the namespace Php\Lang\ for this kind of symbols?

@brzuchal
Copy link

brzuchal commented Mar 10, 2020

Please stop talking about namespaces, this has nothing to do with that. Most of the internals opinion is about the right to use top-level namespace (unprefixed) for all kind of things related to PHP core.

The naming of the interface to implement is also a minor thing. It doesn't make a difference if that's gonna be Attributes or Annotations, the biggest advantage here is getting it to be widely accepted as a feature itself - which is about a way to provide additional metadata in a structured way using resolvable class names and arguments for all places which are reflectable currently.

I believe either @beberlei nor anyone wanna discuss adding Attributes/Annotations for method/function parameters for now. I know that part is already reflectable but that might come over next time in a separate RFC. Just keep the focus on getting it done in the way it could be accepted.

@garygreen
Copy link

garygreen commented Mar 10, 2020

the biggest advantage here is getting it to be widely accepted as a feature itself - which is about a way to provide additional metadata in a structured way using resolvable class names and arguments for all places which are reflectable currently.

We already have that, it's called docblocks.

Rather than introducing new syntax, would it make more sense to create a "docblock reflection API" for functions? Docblocks is a well documented and understood format for developers and flexible so it will allow consumers to do whatever they like with them.

@garygreen
Copy link

garygreen commented Mar 10, 2020

Another consideration - would the syntax of this proposal possibly affect generics? As someone mentioned on reddit:

please make sure you arent making generic syntax impossible with this. My hopes are low for generics, but if we do get them and the syntax isnt <T> I'm gonna have a mental breakdown.

😂

@Saphyel
Copy link

Saphyel commented Mar 10, 2020

What about the option of depricating in PHP 7.5 the @ and use it for this in PHP 8?
Python, TypeScript, etc... use it in this way so I think is a great opportunity to remove a bad practice and use a standard from other languages.

@beberlei
Copy link
Owner Author

@Saphyel There will be no version 7.5 and @ is not necessarily a bad practice, functions like fopen and file_get_contents sometimes have to set them.

@zmitic
Copy link

zmitic commented Mar 10, 2020

I don't mind <<>> syntax, I am sure PHPStorm will nicely display it. Minor concern about readability would be this:

public function __construct(<<Inject(MyInterface::class)>> $tagged)

vs this:

public function __construct(@Inject(MyInterface::class) $tagged)

but really, it is not a big deal; other languages have other characters and no one cares.

But I think @garygreen gave excellent idea. Even if we could get @ character, doc blocks are still more useful. Example: if Reflection could read this:

/**
 * This class does something with array of tagged services
 *
 * @param ServiceLocator<MyInterface> $tagged
 */
public function __construct($tagged)

we could not only autowire tagged services but also get static analysis in just one line of code. This would shut up all those people annoying about generics, including me 😄

@tzsk
Copy link

tzsk commented Mar 10, 2020

This is a great addition to PHP. Can we also look at other options instead of <<>>. More like,

  1. Double at: @@Required
  2. At scope resolution @::Required
  3. At tilde @~Required

And many more...

It would be great if we just use the opening <<

@m50
Copy link

m50 commented Mar 10, 2020

I really love the idea of this, and I'm perfectly happy with the << >> notation as I personally think it would look better with full namespaces (<<\App\Annotations\Something>> rather than @\App\Annotations\Something, the @\ looks weirder).

But my one thing I think is missing, and maybe it fits better in a separate RFC as an addition to this one, but I think it's kind of lacking in the "Decorators" department.

I think there should probably be a couple extra interfaces (with actual functions to call):

  1. AssignableAttribute => public function assign(mixed $value): mixed;
  2. InstantiableAttribute => public function instantiate(object $object): object;
  3. CallableAttribute => public function call(callable $object, array $parameterList): callable;

These would then have the function definitions required as above.

Then, anything with the AssignableAttribute has the assign function called when the object gets assigned to. Usually, this is a property of a class.

A class would usually be what has the InstantiableAttribute, and that gets called whenever new is called for that class.

And CallableAttribute is used whenever a callable is executed (I.e. an invokable class or a function or method).

Example of an AssignableAttribute (using your validation example):

<?php

namespace App\Attributes\Validator;

use Php\Attribute;
use Php\AssignableAttribute;
use App\Attributes\Validator\Exceptions\ValidationException;

class Min implements Attribute, AssignableAttribute
{
    private int $minVal;
    public function __construct(int $minVal)
    {
        $this->minVal = $minVal;
    }
    public function assign($object)
    {
        if (is_numeric($object) && $object < $this->minVal) {
            throw new ValidationException();
        } elseif (is_string($object) && strlen($object) < $this->minVal) {
            throw new ValidationException();
        }

        return $object
    }
}

And how it's used:

namespace App;

class Object
{
    <<Validator\Min(5)>>
    <<Validator\Max(250)>>
    public string $description;
}

Now, if we have:

$object = new Object;

$object->description = 'hi!'; // Throws a ValidationException.

$object->description = 'helllllooooooo'; // Acceptable, gets assigned.

Additionally, this could modify it, for example, have a password property always be Bcrypted when assigned to.

I also have an example for an InstantiableAttribute:

<?php

namespace App\Attributes;

use Php\Attribute;
use Php\InstantiableAttribute;

class Singleton implements Attribute, InstantiableAttribute
{
    private static array $instantiated = [];

    public function instantiate(object $object): object
    {
        $class = get_class($object);
        if (! isset(static::$instantiated[$class])) {
            static::$instantiated[$class] = $object;
        }

        return static::$instantiated[$class];
    }
}

And then it's used like so:

<?php

namespace App;

use App\Attribute\Singleton;

<<Singleton>>
class MySuperObject
{
    // Do stuff...
}

$o = new MySuperObject;

$o2 = new MySuperObject; // Same object as $o because of the singleton attribute

Lastly, here is an example of a CallableAttribute (Though, this is a bit of a contrived example):

<?php

namespace App\Attributes;

use Php\Attribute;
use Php\CallableAttribute;

class MutateTo implements Attribute, CallableAttribute
{
    private string $class;

    public function __construct(string $class)
    {
        $this->class = $class;
    }

    public function call(callable $object, array $parameterList): callable
    {
        return function () use ($object, $parameterList) {
            $returnVal = $object(...$parameterList);
            $class = $this->class;

            return new $class($returnVal);
        };
    }
}

And then it can be used as such:

namespace App;

<<Attributes\MutateTo(AnotherObject::class)>>
class InvokableObject
{
    public function __invoke(string $param1)
    {
        // Do something

        return 2; // Whatever needs to be passed into the constructor of AnotherObject.
    }
}

As you can see, it's very clear how having Attributes be able to affect specific actions can be useful. And of course, one attribute could implement all of these, if it made sense to do so, and these can all be accessed using Reflection as well, but the key thing is, the language automatically runs these decorators at the appropriate time without having to use Reflection 100% of the time. This falls in line with decorators from other languages as well, which do not all require reflection for their implementation.

With the current implementation, it isn't possible to do a lot of things these decorators would offer, without a tertiary class (or the class using the decorators) implementing some reflection to handle, which has a bit of unnecessary separation (and potentially duplication of code). Additionally, currently, there isn't much purpose to the Attribute being a class, assigning from containing some data, as it doesn't actually do anything itself, so granting it the ability to implement functionality I feel would be very important.

@javiereguiluz
Copy link

Note that we can use the @... syntax for annotations if we decide to not support them for stand-alone functions defined outside of classes.

In my experience, this is a reasonable constraint because annotations are used only when using classes (and traits, interfaces, etc.) and not when using stand-alone functions (e.g. in stand-alone PHP scripts). However, others might have a very different experience than me.

The fact that most programming languages use @ ... and that one which was using << ... >> is changing it to @ could be a strong signal for us to reconsider this. Sorry for being so insisting and thanks for your patience 🙏

@beberlei
Copy link
Owner Author

beberlei commented Apr 8, 2020

@javiereguiluz 1. not supporting on functions is inconsistent, considering that these are the same things in the engine. 2. its not true that we can support it this way without making the parser context sensitive, which is the primary reason to avoid here, because that usually gets the Nay votes.

@beberlei
Copy link
Owner Author

beberlei commented Apr 8, 2020

@javiereguiluz also if you'd see the RFC, most programming languages actually use [] as syntax, ONLY java uses @ if you consider that javascript and python have a decorator feature, not attributes/metadata per se.

@yaotzin1
Copy link

I am fine with <> syntax... also bracket syntax [Annotation] or maybe [@annotation ]. Nevermind... please introduce it in PHP...

@moliata
Copy link

moliata commented Apr 14, 2020

Edit: seems this is no longer a problem given that Opcache\Jit and Opcache\NoJit aren't part of this RFC.

@moliata
Copy link

moliata commented Apr 14, 2020

@beberlei Is there any ETA when this RFC will move into a voting phase?

@beberlei
Copy link
Owner Author

@moliata depends on how much feedback the last changes get, probably in a few days to 2 weeks max.

@nunoperalta
Copy link

nunoperalta commented Apr 14, 2020

Not sure if this was already suggested,
but would it be possible that one of the questions in the poll is about which syntax is preferred by each voter?

<<Attr>>
@:Attr

EDIT: I wonder why the downvotes... the syntax is what is causing the most controversy in this project, so I believe it would make sense to go for votes, not so?

EDIT 2: And the downvotes continued to arrive, even though beberlei has agreed with this suggestion, and those who downvoted me, upvoted beberlei when he agreed...

@azjezz
Copy link

azjezz commented Apr 14, 2020

@nunoperalta

I wonder why the downvotes... the syntax is what is causing the most controversy in this project, so I believe it would make sense to go for votes, not so?

No, its up to the PR author whether to vote on it or not. and @beberlei has already stated that the syntax won't be changed.

So the vote is clear: include this feature or not.


I really hope everyone stops suggestion alternative syntaxes, because I'm pretty sure the author already thought and tried other solutions, and came to the conclusion that this is the only possible syntax with no side effects/BC breaks.

If someone has a better solution, i'm pretty sure a new RFC can be opened after this one is approved to change the syntax.

Let's move on and get this amazing feature in PHP!

@nunoperalta
Copy link

@azjezz Thank you for the explanation. No problem with that, but I was just trying to be helpful and hopeful. This is the first time I was really involved on a PR for PHP, so I am not fully aware of the rules. The downvotes felt like I couldn't make a suggestion here.

If someone has a better solution, i'm pretty sure a new RFC can be opened after this one is approved to change the syntax.

Well, the idea would be to avoid that, and then having two syntaxes appearing in repos/examples overtime, and then causing developers to be confused... but I understand.

Thanks again.

@beberlei
Copy link
Owner Author

At this point i plan to add a secondary vote to decide between <<>> and @: as symbols, since @kooldev helped me out on the code a lot and he wanted to try it.

Personally I am going to keep advocsting for <<>> and voting this way, as well getting feedback from some voters they are leaning that way.

@beberlei
Copy link
Owner Author

beberlei commented Apr 16, 2020

Squashed commits and moved PR to master repo here: php#5394

@beberlei beberlei closed this Apr 16, 2020
beberlei pushed a commit that referenced this pull request Mar 14, 2021
explode(): Passing null to parameter #2 ($string) of type string is
deprecated

Closes phpGH-6698.
beberlei pushed a commit that referenced this pull request Jun 19, 2021
1. For statement "$a->change($a = array("a" => range(1, 5)));", the
following opcodes will be generated:

  0002 ASSIGN CV0($a) V1
  0003 INIT_METHOD_CALL 1 CV0($a) string("change")
  0004 INIT_NS_FCALL_BY_NAME 2 string("A\range")
  0005 SEND_VAL_EX int(1) 1
  0006 SEND_VAL_EX int(5) 2
  0007 V1 = DO_FCALL_BY_NAME

The updates in function zend_jit_init_fcall(), zend_jit_send_val() and
zend_jit_do_fcall() are made to support INIT_NS_FCALL_BY_NAME,
SEND_VAL_EX and DO_FCALL_BY_NAME respectively.

2. For method $change(), opcode RECV is used to obtain the argument:

  0000 #1.CV0($config) [rc1, rcn, array of [any, ref]] = RECV 1

Accordingly the updates in functions zend_jit_recv() and
zend_jit_verify_arg_type() are made.

3. For statement "array_keys($config["a"])", the following opcodes will
be generated:

  0001 INIT_NS_FCALL_BY_NAME 1 string("A\array_keys")
  0002 CHECK_FUNC_ARG 1
  0003 #3.V1 [ref, rc1, rcn, any] = FETCH_DIM_FUNC_ARG #1.CV0($config)
     ... -> #2.CV0($config) [rc1, rcn, ...
  0004 SEND_FUNC_ARG #3.V1 [ref, rc1, rcn, any] 1
  0005 #4.V1 [ref, rc1, rcn, any] = DO_FCALL_BY_NAME

CHECK_FUNC_ARG and SEND_FUNC_ARG are not supported before. See the
updates in functions zend_jit_check_func_arg() and zend_jit_send_var().

Besides, a new path is covered in macro OBJ_RELEASE when leaving.
beberlei pushed a commit that referenced this pull request Jun 19, 2021
The opcodes for function $foo are:

  0001 INIT_FCALL 1 96 string("var_dump")
  0002 #2.T1 [null, long] = FETCH_DIM_R array(...) #1.CV0($n) [...]
  0003 SEND_VAL #2.T1 [null, long] 1
  0004 DO_ICALL
  0005 RETURN null

Opcode FETCH_DIM_R is not touched before, and the updates in function
zend_jit_fetch_dim_read() are made to support it.
As different types of arguments are used for $foo, several cases in
function zend_jit_fetch_dimension_address_inner() are covered as well.

Besides, opcode DO_ICALL can reach one site of cold code in function
zend_jit_do_fcall().
beberlei pushed a commit that referenced this pull request Jun 19, 2021
The following opcodes would be generated for $foo:

  0000 #2.CV0($test) [bool] RANGE[0..1] = RECV 1
  0001 #3.CV1($x) [long] RANGE[MIN..MAX] = RECV 2
  0002 JMPZ #2.CV0($test) [bool] RANGE[0..1] BB4
  0003 #4.T2 [bool] ... = IS_SMALLER_OR_EQUAL int(1) #3.CV1($x) ...
  0004 JMP BB5
  ...

The updates in function zend_jit_verify_arg_type() are made to support
RECV opcode.

The updates in function zend_jit_bool_jmpznz() are made to support JMPZ
opcode.

New path is covered in functions zend_jit_cmp() and
zend_jit_cmp_long_long() for IS_SMALLER_OR_EQUAL opcode.
beberlei pushed a commit that referenced this pull request Jun 19, 2021
Opcodes for $Test::method are:

  BB0:
  0000 #0.T0 [rcn, any] = FETCH_OBJ_R THIS string("prop")
  0001 #1.T0 [bool] RANGE[0..1] = JMPZ_EX #0.T0 [rcn, any] BB3

  BB1:
  0002 #2.T1 [rcn, any] = FETCH_OBJ_R THIS string("prop")
  0003 INIT_METHOD_CALL 0 #2.T1 [rcn, any] string("method2")
  0004 #3.V1 [ref, rc1, rcn, any] = DO_FCALL
  ...

New path is covered in functions zend_jit_fetch_obj() and
zend_jit_zval_copy_deref() for FETCH_OBJ_R THIS opcode.

New path is covered in function zend_jit_init_method_call() for opcode
INIT_METHOD_CALL.

Major chagnes lie in function zend_jit_bool_jmpznz() to support opcode
JMPZ_EX.

Note that macro ZVAL_DTOR_FUNC is updated to remove the hard-coded use
of REG0.
beberlei pushed a commit that referenced this pull request Jun 19, 2021
Opcodes for $test are:

  BB0:
  0000 #1.CV0($char_code) [rc1, rcn, any] = RECV 1

  BB1:
  0001 #2.T1 [rc1, ...] = BW_AND #1.CV0($char_code) ...
  0002 #3.T2 [bool] RANGE[0..1] = BOOL_NOT #2.T1 [rc1, ...]
  0003 #4.T1 [bool] RANGE[0..1] = IS_EQUAL #1.CV0($char_code) ...
  0004 JMPZ #4.T1 [bool] RANGE[0..1] BB3
  ...

New path is covered in function zend_jit_long_math_helper() for opcode
BW_AND.

New path is covered in function zend_jit_bool_jmpznz() for opcode
BOOL_NOT.

Major changes lie in functions zend_jit_cmp(), zend_jit_cmp_slow() and
zend_jit_check_exception_undef_result() to support opocdes IS_EQUAL and
JMPZ.
beberlei pushed a commit that referenced this pull request Jun 19, 2021
Excerpt from the release news:

Version 10.37 26-May-2021
-------------------------

A few more bug fixes and tidies. The only change of real note is the removal of
the actual POSIX names regcomp etc. from the POSIX wrapper library because
these have caused issues for some applications (see 10.33 #2 below).

Version 10.36 04-December-2020
------------------------------

Again, mainly bug fixes and tidies. The only enhancements are the addition of
GNU grep's -m (aka --max-count) option to pcre2grep, and also unifying the
handling of substitution strings for both -O and callouts in pcre2grep, with
the addition of $x{...} and $o{...} to allow for characters whose code points
are greater than 255 in Unicode mode.

NOTE: there is an outstanding issue with JIT support for MacOS on arm64
hardware. For details, please see Bugzilla issue php#2618.

Signed-off-by: Anatol Belski <ab@php.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet