Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:94302
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain mindplay.dk from 209.85.213.47 cause and error)
MIME-Version: 1.0
In-Reply-To: <CAJGzsHojxcw-KBDw+c8RkF9c2qCRkfKkm-VXQCSa4PaRmmT5KA@mail.gmail.com>
References: <CADqTB_i5WL+rzCuFHkRmXsPj16WDc4eWmpAoFmyeuYXvuOP_NQ@mail.gmail.com>
 <211db59e-9c22-6df4-1f72-66ebbc5095bd@fleshgrinder.com> <CADqTB_gyjkVZ50c1x=c4mUSs3q0UvzzZdcLm6iJZGD=E+2n1VA@mail.gmail.com>
 <CADyq6s+cNM0RMpvGF_0qmhU1Wpx=_zK6O3kKLxUr-Vu-7jJXNg@mail.gmail.com> <CAJGzsHojxcw-KBDw+c8RkF9c2qCRkfKkm-VXQCSa4PaRmmT5KA@mail.gmail.com>
Date: Tue, 28 Jun 2016 18:31:01 +0200
Message-ID: <CADqTB_gRbArM4A9MWTkO8_pGwn+9+toXWsFbD1avNL+STEwidA@mail.gmail.com>
To: Pedro Cordeiro <pedronaroga@gmail.com>
Cc: Marco Pivetta <ocramius@gmail.com>, PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] [RFC] Simple Annotations
From: rasmus@mindplay.dk (Rasmus Schultz)

Note that I only make comparisons with doc-blocks to point out the
fact that, functionally, they would not be very different from an
annotation feature that solely permits you to annotate with values. A
doc-block is a string, which is a value - an
array/int/float/string/bool is a value. It's only very slightly better
than a bare string, in the sense that neither doc-blocks nor simple
values permit any kind of definition of which annotations are actually
available, hence offer no means of validation of that data (except at
run-time by a userland facility), provides no support for static
analysis, no IDE support, no clear indication of real dependencies,
and so on.

Since the discussion isn't completely dead yet (thank you Pedro and
Marco), I will just briefly explain what I had in mind for another
RFC.

This would be largely based on my previous proposal:

https://wiki.php.net/rfc/simple-annotations

But with the following differences.

The syntax would no longer allow arbitrary expressions - instead, it
would allow object annotations only, declared in one of three ways:

<< RequiresLogin >> // equivalent to new RequiresLogin()

<< Length(20) >> // equivalent to new Length(20)

<< HttpMethod::post() >> // equivalent to static method-call HttpMethod::post()

The latter permits you to declare and call static factory methods,
e.g. "named constructors" for some types of annotations.

Annotations are now consistently objects, which simplifies filtering
annotations, etc.

This declaration syntax enables better in-language analysis - because
the declarations always include the annotation class-name, this means
it can now support deferred as well as conditional creation, by having
a slightly different reflection API:

    $annotations = $class_reflection->getAnnotations();

At this point, we have a list of ReflectionAnnotation objects - the
actual annotation objects have *not* been created at this point, which
would allow for conditional evaluation, such as:

    foreach ($annotations as $annotation) {
        echo $annotation->className; // => "Length" etc.

        if ($annotation->classExists()) {
            var_dump($annotation->getInstance()); // => Length object, etc.
        } else {
            var_dump($annotation->getInstance()); // => UnknownAnnotation object
       }
    }

The UnknownAnnotation object has three properties: class-name,
method-name ("__construct" if constructor was used) and the list of
arguments.

In other words, this would provide support for "optional
dependencies", in much the same way that unserialize() does it - the
getInstance() method will trigger autoloading, and if the class is
unavailable, it takes this "typeless" information and puts it in a
general object, so that the information itself isn't lost.

It may *seem* like this solves the optional dependency problem, but
let me be clear, it does *not* fully address that issue, because
something like << Method(Http::GET) >> could still fail, if the Method
class is defined, but the Http class is not. That's a much more
marginal case, but it's not completely unlikely that you would use
constants from one dependency and pass them to annotation classes from
another dependency, e.g. if the HTTP method constants were part of an
HTTP package, and the annotation itself were part of a different
package. Arguably it's much less likely to occur though - if you have
the annotation package installed, most likely you have the dependent
package of that package installed as well.

So this is much safer, but just to be clear, the only way you can make
it any safer than that, is by completely disallowing anything but
values as arguments to annotation constructors - which, in my opinion,
simply isn't useful.

Alternatively to the above, you could imagine a simpler reflection
facility that does eagerly construct annotations, but constructs
UnknownAnnotation instances for missing classes - this would perhaps
balance more towards immediate usefulness and less towards performance
perfectionism, as it increases the chance of loading an unused
annotation.

This would be my preference though - as previously argued, the odds of
having a large number of unrelated annotations on the same member are
extremely low in practice; an annotated entity tends to be a persisted
property, a form input element, or an action method, etc... rarely do
you mix annotations from different domains onto the same member;
annotations that are applicable to a given domain are usually relevant
to a consumer belonging to the same (or a closely related) domain, so
this performance concern is mostly a theoretical micro optimization.

For another, eager construction means we can filter annotations with
instanceof rather than by class-name, which is much powerful, enabling
you to leverage inheritance - for example, you'd be able to ask
directly for all instances of ValidationAnnotation and get those with
a single call, which is incredibly useful.

Anyways, let me know your feelings about that idea? If there's any
interest, I'd be happy to write yet another RFC based on that.


On Tue, Jun 28, 2016 at 4:30 PM, Pedro Cordeiro <pedronaroga@gmail.com> wrote:
> I completely agree with Marco. Throwing class-loading errors for
> value-object classes is alright. For logic-parsing inside annotations, I
> don't feel any need for it. Metadata should be static, IMO, but it could
> very well be (and should be) value-objects.
>
>>At the end of the day, what we end up with will have all of the same
>>issues we have with php-doc blocks and existing libraries.
>>
>>I don't understand how such a feature would be useful to anybody?
>
> As an addition, I don't think this feature should ADD new functionality to
> those already present on DocBlocks. I just feel like docblocks are an ugly
> hack for the lack of native annotations. Pasting from a previous discussion
> in this same list, Rowan and I were discussing whether to add a native
> docblock-annotation-retrieval feature or a native annotation syntax:
>
> 1) It might not be objectively bad to add this feature in docblocks, but it
> will be objectively wrong to keep calling them "DocBlocks" if they are no
> longer documentation-only blocks.
>
> 2) Even though PHP already treats docblocks differently from comments,
> that's not the common view on userland, nor is explained on the manuals.
> There are no separate entries to explain docblocks, and no mention to them
> on the "Comments" page. The Reflection method to retrieve DocBlocks is
> "getDocCOMMENT", which suggests they are comment that do not affect runtime
> behaviour.
>
> We'd have to update the comments page to say "'/*' starts a comment, unless
> if they're immediately followed by another asterisk ('/**'), in which case
> it may or may not be a comment, depends on the following token". It's very
> confusing.
>
> 3) To make this work within docblocks, we'd have to get rid of at least one
> configuration setting (opcode.save_comments).
>
> 4) Dockblocks are stripped from obfuscators and transpilers, which are a
> part of the ecosystem and do affect users, even if they are not first-class
> citizens here.
>
> Annotations in dockblocks are hard to understand because they they may or
> may not be runtime-affecting annotations and there is no way to tell.
>
> I hate this comparison with docblock annotations, because they are an UGLY
> HACK. It works wonderfully, yes, but so do global variables, singletons and
> what-not, and we are not using them anymore, are we? Adding this very same
> feature that already exists in userland as a dedicated syntax would fix a
> lot of issues, cited above. There is no need to evaluate logic inside
> annotations, let them be value-objects, let it throw errors autoloading
> said objects, and let's get this rolling.
>
> Please, don't embargo this ONCE again. It's a very much needed feature, and
> people want it! There have been innumerous discussions on reddit, and
> almost everyone agrees annotations are a missing feature and
> comment-annotations are an ugly hack for the lack of native syntax.
>
> Please, make this happen.
>
> 2016-06-28 11:12 GMT-03:00 Marco Pivetta <ocramius@gmail.com>:
>
>> Hi Rasmus,
>>
>> On 27 June 2016 at 20:37, Rasmus Schultz <rasmus@mindplay.dk> wrote:
>>
>> > Tonight I started trying to write a proposal for even more simplified
>> > annotations, e.g. something simple enough that it cannot result in
>> > fatal errors. I eventually gave up, and here's why.
>> >
>> > Essentially, if you have to restrict yourself to things that cannot
>> > under any circumstances error out, the only things you can use are
>> > constant scalar values - even constant scalar expressions could not be
>> > allowed, because even those could error when CONST_NAME or
>> > Class::CONST_NAME aren't defined.
>> >
>> > So, in a nutshell, what you're saying (Richard) is that annotations
>> > can only consist of scalar values, and possibly arrays of values? (and
>> > I don't even see your gist doing that - it's just invoking closures,
>> > as far as I can tell, and any of those could error, or not?)
>> >
>> > So, regardless of syntax, literally the only annotation data allowed
>> > would be blatantly simple things like:
>> >
>> >     ["max_length" => 20]
>> >
>> >     ["data_type" => "int"]
>> >
>> > Or in other words, constant arrays, int, string, float and bool
>> > values. Nothing else.
>> >
>> > Nothing can depend on any kind of external symbol (class names,
>> > constants, etc.) since any such reference can cause an error.
>> >
>> > No static analysis or type-checking of any kind can ever be performed
>> > - no assertions or declarations can be made about the shape or types
>> > of metadata, at all.
>> >
>> > The problem is, that's simply not useful.
>> >
>>
>> That's actually what doctrine/annotations has been for a while  tho, and a
>> lot of people rely just on that.
>> The data-structure is still enforced by a VO-style class (the annotation
>> itself) in the library, though.
>>
>> It would still be very useful, in my opinion.
>>
>> Note that the existing annotation libs out there do indeed trigger
>> autoloading for referenced class constants.
>>
>> Marco Pivetta
>>
>> http://twitter.com/Ocramius
>>
>> http://ocramius.github.com/
>>