[RFC] Amendments to Attributes View as Markdown

Hey Benjamin,
http://ocramius.github.com/

On Wed, May 20, 2020 at 7:08 PM Benjamin Eberlei kontakt@beberlei.de
wrote:

Hi everyone,

the Attributes RFC was rather large already, so a few things were left
open
or discussions during the vote have made us rethink a things.

https://wiki.php.net/rfc/attribute_amendments

These points are handled by the Amendments RFC to Attributes:

1. Proposing to add a grouped syntax <<Attr1, Attr2>
2. Rename PhpAttribute to Attribute in global namespace (independent of
   the
   namespace RFC)
3. Add validation of attribute class targets, which internal attributes
   can
   do, but userland can't
4. Specification if an attribute is repeatable or not on the same
   declaration and fail otherwise.

Each of them is a rather small issue, so I hope its ok to aggregate all
four of them in a single RFC. Please let me know if it's not.

Do you hope to get nested attributes to 8.0, or is it something you'd
prefer to happen at a later time?

Martin evaluated the technical requirements in more detail and we discussed
with others that the timeframe is probably to short to get all the details
fleshed out :-( So we are going to table that for 8.1

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

Hey,

I personally think that UserlandAttribute is too verbose and a quite
useless change since just using Attribute will make little to no BC
compatibility breaks (we know that thanks to Rowan, please check out
Renaming PhpAttribute thread) and would also sound more natural. This would
also mean that we would rename PhpCompilerAttribute to simply
CompilerAttribute.

Best regards,
Benas Seliuginas

2. Rename PhpAttribute to Attribute in global namespace (independent of
   the
   namespace RFC)

I suggested in a previous thread that we consider renaming PhpAttribute
to UserlandAttribute, since this is the intent of the attribute, and it
helps distinguish it from CompilerAttribute.

I noticed that the compiler attribute is actually named
PhpCompilerAttribute, so unless we change its name to
CompilerAttribute, I don’t think it makes sense to change PhpAttribute
to Attribute. Perhaps we change it to PhpUserlandAttribute, for clarity?

Ah that's a good point that still needs to be clarified. We realized that
PhpAttribute and PhpCompilerAttribute should be merged, because the
difference doesn't make a difference to userland code and it complicates
things. For example enforcing that PhpCompilerAttribute is just on internal
classes would not work for generated stub code in Phan/Psalm and so on that
"describe" internal code by imitating it in userland. This would not be
allowed by the current implementation and lead to a compiler error, lets
say if we imitated Deprecated for documentation purposes:

<?php

<<PhpCompilerAttribute>>
class Deprecated {}

This file could not be compiled by current implementation as an error would
prevent Userland Deprecated class from using PhpCompilerAttribute.

Cheers,
Ben

While I'm happy with "Attribute" living in the global
namespace, I don't really think we'd want to introduce "Jit" as a class in
the global namespace. The name is simply to generic and does not indicate
that this is part of the attribute system. We'd be forced to go with things
like DeprecatedAttribute or JitAttribute, which seems rather non-optimal to
me, as we're just reinventing namespaces to avoid using them...

As such, I would suggest to introduce a common namespace for all attributes
provided by PHP. This means we'd have Attributes\Attribute,
Attributes\Deprecated, Attributes\Jit, Attributes\NoJit etc. (I'm also okay
with the PHP\Attributes\Deprecated variant, but that's a separate
question).

This is the best real-world argument in favour of PHP namespace in core
that I've heard.
https://wiki.php.net/rfc/php-namespace-in-core

If we don't want to introduce classes in the global namespace, it makes
sense to have a reserved PHP namespace we use.

Peter

On Wed, May 20, 2020 at 7:08 PM Benjamin Eberlei kontakt@beberlei.de
wrote:

Hi everyone,

the Attributes RFC was rather large already, so a few things were left
open
or discussions during the vote have made us rethink a things.

https://wiki.php.net/rfc/attribute_amendments

These points are handled by the Amendments RFC to Attributes:

1. Proposing to add a grouped syntax <<Attr1, Attr2>

2. Rename PhpAttribute to Attribute in global namespace (independent of the

namespace RFC)

As was mentioned in one of the previous discussions, we expect that PHP is
going to ship more language-provided attributes in the future. With this
proposal we have the "Attribute" attribute, but I expect we'll at least
have "Deprecated" as well, and probably also something along the lines of
"Jit" or "NoJit". While I'm happy with "Attribute" living in the global
namespace, I don't really think we'd want to introduce "Jit" as a class in
the global namespace. The name is simply to generic and does not indicate
that this is part of the attribute system. We'd be forced to go with things
like DeprecatedAttribute or JitAttribute, which seems rather non-optimal to
me, as we're just reinventing namespaces to avoid using them...

As such, I would suggest to introduce a common namespace for all
attributes provided by PHP. This means we'd have Attributes\Attribute,
Attributes\Deprecated, Attributes\Jit, Attributes\NoJit etc. (I'm also okay
with the PHP\Attributes\Deprecated variant, but that's a separate question).

Deprecated would be an "engine level" feature, but Opcache is an extension,
so it can have its own namespace "Opcache\Jit". The namespace RFC goes that
far mentioning only " core symbols which cannot be unbundled" should go
into a PHP namespace, which would exclude Opcache (and its "sub-extension"
JIT). https://wiki.php.net/rfc/php-namespace-in-core

So for me that is not necessarily an argument against Attribute in global
NS, because Jit would live in Opcache.

Nikita

3. Add validation of attribute class targets, which internal attributes can

do, but userland can't

4. Specification if an attribute is repeatable or not on the same
   declaration and fail otherwise.

Each of them is a rather small issue, so I hope its ok to aggregate all
four of them in a single RFC. Please let me know if it's not.

greetings
Benjamin

Am 22.05.2020 um 13:08 schrieb Nikita Popov nikita.ppv@gmail.com:

On Wed, May 20, 2020 at 7:08 PM Benjamin Eberlei kontakt@beberlei.de
wrote:

2. Rename PhpAttribute to Attribute in global namespace (independent of the
   namespace RFC)

As was mentioned in one of the previous discussions, we expect that PHP is
going to ship more language-provided attributes in the future. With this
proposal we have the "Attribute" attribute, but I expect we'll at least
have "Deprecated" as well, and probably also something along the lines of
"Jit" or "NoJit". While I'm happy with "Attribute" living in the global
namespace, I don't really think we'd want to introduce "Jit" as a class in
the global namespace. The name is simply to generic and does not indicate
that this is part of the attribute system. We'd be forced to go with things
like DeprecatedAttribute or JitAttribute, which seems rather non-optimal to
me, as we're just reinventing namespaces to avoid using them...

As such, I would suggest to introduce a common namespace for all attributes
provided by PHP. This means we'd have Attributes\Attribute,
Attributes\Deprecated, Attributes\Jit, Attributes\NoJit etc. (I'm also okay
with the PHP\Attributes\Deprecated variant, but that's a separate question).

Nikita

At that point, don't we just want to be able to generically mark all attribute classes visible as being an Attribute?

Also the classical examples like <<ORM\Entity("...")>> do not tell you anyhow that they're Attributes, if the class is accessed outside of attribute context. And I guess, with the logic you proposed, it will likely be named "ORM\Attribute\Entity" making it even longer.

I think it would be good to be able to later on just write <<Deprecated>>, <<JIT>> etc. without further imports, as they seem to be quite basic functionality. I think it should remain possible to write simple code without much namespace awareness.

Perhaps we should actually name attribute classes including their << and >>. Thus "class <<JIT>> {}" (attribute decl), "new <<JIT>>()" (custom instantiation), "$attribute instanceof <<JIT>>". (and, for a namespaced attribute "$attribute instanceof MyNamespace<<MyAttrr>>")
The engine could quite easily support that, it's just a little parser work.

That way the whole discussion as to where to put attributes in the namespace hierarchy would be quite moot, for PHP itself as well as userland.

Bob

P.s.: As a caveat, on parameter types, if we expect a value being of an attribute class, we'd need to require a qualified name containing at least one backslash or import-alias there. But that's acceptable I think, it will just happen within some internal attribute processing code.

As such, I would suggest to introduce a common namespace for all attributes
provided by PHP. This means we'd have Attributes\Attribute,
Attributes\Deprecated, Attributes\Jit, Attributes\NoJit etc. (I'm also okay
with the PHP\Attributes\Deprecated variant, but that's a separate question).

One possible policy which came up in chat last night was that universal
base classes (which tend to be few in number, and act almost as
keywords) should be in the root namespace; but built-in implementations
of those (at the same level as ones users could create) are prefixed
with "PHP\Something".

For instance, if we had a time machine and were adding them all now:

• "\Throwable" and "\Exception", but "\PHP\Exceptions\RuntimeException"
• "\Iterator" but "\PHP\Iterators\FilterIterator"
• "\Attribute", but "\PHP\Attributes\Deprecated"

Regards,

--
Rowan Tommins (né Collins)
[IMSoP]

On Thu, May 28, 2020 at 12:05 AM Benas IML benas.molis.iml@gmail.com
wrote:

It seems that the RFC was updated to use the Attributes namespace. I
think this is a bad idea since we're reserving a generic namespace that we
haven't even "soft" reserved. Also, the loss of fallback to global
namespace is a turning point for me.

Generally, I think we should instead do something like Rowan said: use
namespaced classes only for implementations (e.g. \Attribute but
\PHP\Deprecated).

I agree on sentiment, but the PHP namespace vote is failing right now, so
that makes this plan just a rehashing of the existing, failing vote.

The likelihood of a class Attributes\Attribute or Attributes\Deprecated
existing in any code out there is much much lower, than the classes
Attribute and Deprecated existing in the global namespace. The "PHP"
namespace would imply some sort of claim of the project, which the failing
namespace vote shows does not exist.

The way attributes map to classes, suffixes/prefixes make them strange to
look at, so for future non-breaks with userland code it will be safer to
put them off to a new namespace and this potentially increases the user
perception.

If this vote fails, this implicitly means that all future internal
attributes will probably go into the global namespace.

Best regards,
Benas

On Wed, May 20, 2020, 8:08 PM Benjamin Eberlei kontakt@beberlei.de
wrote:

Hi everyone,

the Attributes RFC was rather large already, so a few things were left
open
or discussions during the vote have made us rethink a things.

https://wiki.php.net/rfc/attribute_amendments

These points are handled by the Amendments RFC to Attributes:

1. Proposing to add a grouped syntax <<Attr1, Attr2>
2. Rename PhpAttribute to Attribute in global namespace (independent of
   the
   namespace RFC)
3. Add validation of attribute class targets, which internal attributes
   can
   do, but userland can't
4. Specification if an attribute is repeatable or not on the same
   declaration and fail otherwise.

Each of them is a rather small issue, so I hope its ok to aggregate all
four of them in a single RFC. Please let me know if it's not.

greetings
Benjamin

Having a distinct token for attributes would entirely avoid the issues
of having to modify multiple lines when adding/removing attributes, as
well as confusion with shift operators and comma-separated attribute
arguments. E.g. the RFC example would look like this instead:

@@Attr2("foo")
@@Attr2("bar")
public function test()
{
}

I'm not sure what you mean by "having a distinct token"; the cases where
there is any chance of confusion with bit-shifts are going to be very rare,
and aren't related to any of the other issues you're discussing. You make
some good points about not having comma-separated attributes, but they
don't require us to change the syntax, just not add that feature to it.

On a personal note, I find @@ a lot uglier than << and think a bracket-link
syntax looks clearer when writing multiple attributes on one line to avoid
long thin columns of code:

<<Attr2("foo")>> <<Attr2("bar")>>
public function test()
{
}

vs

@@Attr2("foo") @@Attr2("bar")
public function test()
{
}

Regards,

Rowan Tommins
[IMSoP]

On Mon, Jun 1, 2020 at 1:48 AM Theodore Brown theodorejb@outlook.com
wrote:

On Wed, May 20, 2020 at 12:07 PM Benjamin Eberlei kontakt@beberlei.de
wrote:

the Attributes RFC was rather large already, so a few things were left
open or discussions during the vote have made us rethink a things.

https://wiki.php.net/rfc/attribute_amendments

These points are handled by the Amendments RFC to Attributes:

1. Proposing to add a grouped syntax <<Attr1, Attr2>

Hi Benjamin,

I find the grouped attribute proposal somewhat troubling. The RFC
contains the following example:

<<Attr2("foo"),
  Attr2("bar")>>
public function test()
{
}

The problem with this syntax is that adding a new attribute at the
start or end of the list, or removing one of the attributes, will
require modifying multiple lines. For some time the language has been
moving away from this (see the various RFCs to allow trailing commas
in more places), so this feels like a step backwards.

If trailing commas are allowed in grouped attributes, you could
write it this way instead:

<<
  Attr2("foo"),
  Attr2("bar"),
>>
public function test()
{
}

But to me this still feels rather clunky. It requires two extra lines,
and when moving from two attributes to one attribute (or vice versa),
you'd still probably end up modifying multiple lines.

I have added trailing commas to the attribute grouping. Now that its
possible everywhere, we should go with it here directly from the beginning.

About clunkyness, I am not sure this makes sense as an argument, bceause
the extra lines are present for all other cases of trailing commas in
popular coding styles:

[
"arg1",
"arg2",
]

or

foo(
"arg1",
"arg2",
);

Another issue with the grouped syntax is that comma separated
attributes can be easy to confuse with comma separated attribute
arguments. For example:

<<FooAttr(2 * 3 + 3), Bar(4 + 5 * 2)>>
<<BarAttr(2 * (3 + 3), Baz, (4 + 5) * 2)>>
function bar() {}

It can be hard to tell which line contains multiple attributes vs.
multiple attribute arguments.

The same is true about commas in nested short arrays.

Ultimately it seems like the grouped attribute proposal is attempting
to work around the poor usability of the current verbose syntax. Maybe
it would be better to instead propose a simpler syntax that avoids
these issues. I know that some internals members expressed interest
in an @@ token, but this was never voted on.

If you feel strongly about it, please create your own RFC for this. You
would need to put it under discussion within this week though, as the
feature freeze deadlines are looming.

Personally I have no interest in opening up the syntax question again,
because there are really not many objective arguments to be made, its
mostly subjective and feeling based.

Having a distinct token for attributes would entirely avoid the issues
of having to modify multiple lines when adding/removing attributes, as
well as confusion with shift operators and comma-separated attribute
arguments. E.g. the RFC example would look like this instead:

@@ is not a distinct token though, because it is already valid syntax to
have the error silence operator multiple times after each other.

@@Attr2("foo")
@@Attr2("bar")
public function test()
{
}

To me this would be a lot cleaner and fit in better with the rest of
the language.

Best regards,
Theodore

Thank you for the update! Given that there is still an open issue, is the
RFC proposing flags or a separate <<Repeatable>> attribute?

Best regards,
Benas

I have changed back the rename from namespacing to Attributes\Attribute to
using just Attribute after a few discussions off list. The reasoning is
that it becomes more clear that a majority of core contributors strongly
prefers using the global namespace as the PHP namespace and opening up this
point again makes no sense. So the state of the RFC is again what it was
when I originally posted it with renaming PhpAttribute to Attribute.

Unless there is some new significant feedback I am going to open up this
RFC for voting on Monday next week.

On Wed, May 20, 2020 at 7:07 PM Benjamin Eberlei kontakt@beberlei.de
wrote:

Hi everyone,

the Attributes RFC was rather large already, so a few things were left
open or discussions during the vote have made us rethink a things.

https://wiki.php.net/rfc/attribute_amendments

These points are handled by the Amendments RFC to Attributes:

1. Proposing to add a grouped syntax <<Attr1, Attr2>
2. Rename PhpAttribute to Attribute in global namespace (independent of
   the namespace RFC)
3. Add validation of attribute class targets, which internal attributes
   can do, but userland can't
4. Specification if an attribute is repeatable or not on the same
   declaration and fail otherwise.

Each of them is a rather small issue, so I hope its ok to aggregate all
four of them in a single RFC. Please let me know if it's not.

greetings
Benjamin