-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add very basic draft of some PHP 8 attributes #1218
Conversation
Very interesting idea! Pinging @ondrejmirtes as a PHPStan developer. |
The point of PHP annotations is to speed up runtime reflection for annotation data (e.g. for Doctrine annotations). Readonly/immutability are not things that are checked at runtime, so (in my mind) there's no point shifting them from docblocks into PHP annotations. |
@muglug I don't think Doctrine annotations are parsed in runtime as well. Furthermore, how text annotations that required to be parsed by PHP tools is any faster than native annotations (aka attributes)? Native annotations provide autocompletion by default (they are just classes), arguments (including named ones since PHP 8) and it's really easier to work with them as they part of reflection API now. I see no reason to do not use this feature as in my practice 70% of docblocks are simple /**
* @psalm-immutable
*/ Also it is framework-specific tag, isn't would be better to have framework-agnostic one? Also, docblock is a mess of different concepts: regular text, markdown-docs, original phpdoc-annotations, Doctrine-style annotations. And all this mess is a text. IDE doesn't work well with it, because it is required to parse text as another sub-language. So I really doubt that runtime/compile time is a subject of discussion: it's matter of comfort. We had annotations for a long time and now PHP dev team made them really usable, I don't see why we should ignore it. P.S. Honestly, I think PHPdoc-blocks will be obsolete at some time or, at least, they won't contain annotations. Text and code should be represented differently: separation of concerns. E.g. @@Author(name: 'Eliezer S. Yudkowsky', email: 'yudkowsky@gmail.com', site: 'https://www.lesswrong.com')
final class HarryPotterAndMethodsOfRationality
{
} Due to another level of abstraction, such tags would allow IDE be more interactive with end-user. |
All the examples in the rfc are designed to be read at runtime – that's mostly the point of the whole thing. Also, and please correct me if I'm wrong, the |
This is impossible, as all userland annotations will have to be part of some sort of library available to PHP at runtime. So the |
What do you mean? You "parse" them exactly in the same way: via Reflection API. But you don't need to parse DocBlock, you directly get instances of annotations.
Right, I just skipped it in my post, fixed example is like <?php
use Psr\Annotations\Author;
@@Author(name: 'Eliezer S. Yudkowsky', email: 'yudkowsky@gmail.com', site: 'https://www.lesswrong.com')
final class HarryPotterAndMethodsOfRationality
{
// ...
}
This is declarative annotations, there are no no guarantees that class is really immutable. But in dev team, you can always setup git receive hook with forced checked of such classes and recommended to use PhpStorm. By saying "framework-agnostic" I mean all tools which support this PSR will be able to check classes. PhpStorm in the first place. @psalm-immutable is not agnostic, because it requires you to have installed exactly psalm. PHP-FIG is that group which may solve it.
Does it matter if nobody in sane mind does import manually? PhpStorm does it since ages and block with imports is hidden by default. I just type |
Ah, I got you. You mean annotation is a class and it belongs to some library. But that's why we are here, in PHP-FIG repository. PHP-FIG provides contracts like PSR-3 logger and it doesn't belong to any framework. It's sort of "standard".
Nope. There is even example with familiar Doctrine annotations: /** @ORM\Id @ORM\Column(type="integer"*) @ORM\GeneratedValue */
<<ORM\Id>><<ORM\Column("integer")>><<ORM\GeneratedValue>>
private $id;
/**
* @ORM\Column(type="string", unique=true)
* @Assert\Email(message="The email '{{ value }}' is not a valid email.")
*/
<<ORM\Column("string", ORM\Column::UNIQUE)>>
<<Assert\Email(array("message" => "The email '{{ value }}' is not a valid email."))>>
private $email; It is just the same, but now it's a language construct and not just text. They are not being instantiated when you call method or use class: they even can't hold non-static arguments, look at them: scalars, arrays, that's all. They can't work with arguments of methods, etc. And you can get instance only via Reflection API. |
@muglug to be more explicit: I suggest to create package in PSR-namespace with classes like namespace Psr\Attributes;
@@Attribute(Attribute::TARGET_CLASS | Attribute::TARGET_PROPERTY)
final class Immutable
{
} Then the tools read them (by parsing AST or via Reflection API). In order to use them, end-user of course need to Just imagine phan and PhpStan will introduce their own |
Also another thing bothers me: it's a tons of identical names of attributes from different vendors, it will make importing harder and it's easier to make an error. That's why so common names is better to be seized by most neutral vendor. And not just seized, but precisely declared to reduce number of different interpretations, e.g. is it allowed for Another example is We can't get such necessary features from PHP dev team so far, but we have good chances to solve it with tools like psalm. So I think you should be more positive: your tool is one of the best so far, but we need to decouple things. It's more than enough inconsistencies in PHP. |
Procedural note: If FIG were to weigh in here and standardize some attributes, the way to do so would be via a Working Group and PSR. That should be discussed on the mailing list before any code or specific tags are dug into. To that end, please start a thread on the mailing list to discuss it, and if possible bring the PHPStan and Psalm people into the discussion early, possibly someone from PHPStorm as well. In practice, this would work best as "Stan, Psalm, and Storm want to standardize their metadata between themselves and leverage FIG to do so." Without their early buy in it would be mostly a wasted effort. Whether or not that is a good idea I mostly defer to the PHPstan, Psalm, and PHPStorm developers. Also, IIRC the Attributes functionality does not require that an attribute have a matching class... it's optional, but frankly a really good idea. Whether any PSR on the topic would insist on a defined class would be up to the Working Group to figure out. |
Sent mail messages to @ondrejmirtes (PhpStan), @muglug (psalm), @TysonAndre (phan) and to JetBrains. Personally, for me it would be comfortable to start discussion in the evening (18:00 UTC) on Friday or in any time on Saturday. Let's see if it would work. |
Hi, I was already participating in the PSR-5 draft discussion and I found out that discussions about tech specs over mailing lists are not my cup of tea and I have other things with higher priority in my life :) But I'm happy to implement anything that reaches consensus in PHPStan. |
I can't agree more: the PHP-FIG mailing list is a terrible place. |
I'm sort of uncomfortable with encouraging the use of a third-party package (even one supported by With the docblock annotation I can just say "add With a PHP annotation the process is:
I can't ever see myself recommending the latter. |
|
If they don't have a matching class, you get this mess:
Since In any event, |
I'd declined working in the PSR-5 working group for similar reasons to those mentioned in this comment chain. phan/phan#1635 I'm also happy to implement anything that reaches consensus, but am discouraged by PSR-5 having been in draft for many years. (Both doc comments and attributes annotate methods) On an unrelated note, I'd potentially expect to see some proposals in 8.1 that would affect the behavior of the PHP engine (e.g. Deprecated, Immutable (not sure how well-received that'd be to actually change behaviors, related rfcs had objections if I remember correctly), etc.). Nobody's started working on them or gathering feedback. I only remember the Jit Attribute being discussed
|
I don't think so. RFC doesn't say about it anything and part of API makes no sense without class, e.g. |
I just checked your library and it has in total 604 dependencies + dependency on
Psalm requires to be installed as well, what do you mean?
It's hard to believe such dialog between developers. But even in this case: your library will depend on And you still will be able to suggest write |
It's 75 for psalm.
|
Other programs besides psalm are capable of getting the doc comment from a class and treating E.g. |
I find this solution ugly. No offense, but native annotations are much prettier for me. You need to give a choice to people. |
I think it's important to understand that Psalm is not a library – it's mainly a tool, and it doesn't require that the code it runs on have any explicit dependencies (it doesn't even require Composer). If PHP (the language) adds engine support for specific annotations that have a bearing on Psalm's type system, Psalm will support those. I could also imagine a Psalm plugin for Doctrine interpreting some doctrine annotations. |
Let's assume for a minute that we managed to describe in PSR some attributes in PHP-8-style. Do I understand right , that you will be OK with enhancing your tool in a such way, so it would support several formats including the native PHP 8 one? Anyway you will analyze PHP-8 annotations: it's already part of PhpParser and why not? Let me... to leave this stream of thoughts here. Regarding
However, as I noticed above, we also need paired annotation like Also similar annotation is already use in Symfony: https://github.com/symfony/symfony/pull/21452/files#diff-a10c9afd8feaccd1ff66ffd54e2835bcR295 And few words about Let's try to discuss it. @ondrejmirtes @muglug @TysonAndre |
<?php
@@Final()
class A {} results in a syntax error. |
Right. I'd call it I also noticed PhpStorm's |
PHPMD should have its own annotation classes, if the maintainers decide to use annotations. PHPCS should have yet another set of annotation classes, if the maintainers decide to use annotations. Otherwise, you end up in situation where you can't identify which suppressions are no longer needed, because they belong to a mix of projects |
Clarifying the definition of Internal is something I would find useful. I don't remember seeing any official specification of what it's meant to mean, but everything in a namespace and its subnamespaces would be a reasonable default that I haven't gotten around to switching (while waiting for a standard to be finalize).
|
We can require that each vendor must use prefix with warning id, e.g.
Given |
Another one is Offtopic: I faced one framework (WebOnyx) which requires to have exactly same instances for corresponding scalar values, otherwise it cannot map it from user input scalars on my value objects and vice versa. I think it's consequence of "blind" porting from JavaScript original framework, but but anyway it's interesting and some serializer tools probably may gain some advantage on large amount of data. Even annotation by itself will never save memory and runtime reflection check would just eliminate any time/memory difference. So it was just offtopic, but it was odd for me.
namespace Acme\SnakeGame;
use function ExplicitContent\Assertions\assertThat;
use Psr\Annotations\Enum;
use Psr\Annotations\EnumVal;
@@Enum([Direction::N, Direction::E, Direction::S, Direction::W])
final class Direction
{
public const N = 'north';
public const E = 'east';
public const S = 'south';
public const W = 'west';
public function __construct(
@@EnumVal
public string $value
) {
assertThat(\in_array($value, [self::N, self::E, self::S, self::W]);
}
public function reverse(): self
{
$value = match ($this->value) {
self::N => self::S,
self::E => self::W,
self::S => self::N,
self::W => self::E,
};
return new ($value);
}
public function equals(self $direction): bool
{
return $this->value === $direction->value;
}
} (the idea here is to have top-level (class-level) annotation Maybe too verbose here, but concept by itself is useful:
The tools, on other hand, must ensure immutable status of object, match types of list values and |
Templates can be look like this: use Psr\Annotation\Templates\TypeExt as TE;
@@Template('T of {}', Animal::class)
// or
@@Template('T', of: Animal::class)
final class Container
{
@@TE('array<int, T>')
private array $values;
public function add(@@TE('T') $animal): self
{
// ...
}
}
new @@TE(Dog::class) Container() // new Container<Dog>
|
By the way, FFI (PHP 7.4) allows to make an object immutable too: https://github.com/lisachenko/immutable-object @lisachenko what do you think if PHP 8 would have Otherwise we have many different markers: different phpdoc (text), interfaces like We need interoperable marker for sure, so, IDE, analyzers, FFI-based libraries can work consistently. |
For me it is perfectly ok to have well-defined class names for attributes. My Aspect-oriented library will benefit a lot from this, also Z-engine can use immutable attribute to make runtime changes according to provided attributes. Also, good potential for static analysis tools like PhpStan/Psalm to use this attributes during statical analysis. |
@Crell adding new attributes to existing PSR is allowed thing or not? On one hand, there could be many new attributes in the future, hence many PSRs. Sometimes it may just one single attribute per PSR. On other hand, it would be impossible to say "we support PSR-20" (or whatever next number is), because it may contain new unsupported attributes. Is it possible to make an exception in versioning for this PSR like PSR-20.1, PSR-20.2? |
I asked GitHub support whether it's possible to enable discussions sections like this one: https://github.com/vercel/next.js/discussions There are a lot of things to discuss.
|
Ah, GitHub already provides discussions for teams: https://docs.github.com/en/github/building-a-strong-community/about-team-discussions (but https://github.com/vercel/next.js/discussions is better) So, is it possible to join us in the single team like "AttributesWorkingGroup" and let us discuss it there? It looks like you just need to enable discussions here: https://github.com/organizations/php-fig/settings/teams Also, I see it's possible to create private team discussions, it's perfect for internal PHP-FIG discussions, isn't it? @Crell what do you say? We can try it. |
@unkind No, PSRs right now are effectively immutable once passed. This is a limitation we've discussed needing a new mechanism for (such as for coding standards updates, or phpDoc updates), some kind of living standard. We don't have a mechanism for that at present. Also, I am not a GitHub admin. I can't enable anything. The Secretaries are the ones that would do that. You really don't need to ping me repeatedly in a short span of time. The way to get something like this really moving is to stop discussing details of what standard attributes you want and put together a working group with Editor and Sponsor who can make the case to the Core Committee "we want to develop some standard attributes." The CC would then approve a charter for the working group (or not; I cannot predict what its overall opinion would be; I'm just one member), and the WG can figure out how it wants to coordinate. |
[JetBrains] I also was participating in the PSR-5 draft discussion; I also find PHP-FIG mailing lists absolutely unusable; |
PHPStorm introduced attributes available out-of-the-box: https://blog.jetbrains.com/phpstorm/2020/10/phpstorm-2020-3-eap-4/ I think it would be great to have a PSR convention to not have the same issue as Java (https://stackoverflow.com/questions/4963300/which-notnull-java-annotation-should-i-use), having many implementations. |
I pushed a similar idea in this Twitter thread: https://twitter.com/AlessandroLai/status/1319939704738009089 |
I don't think it makes sense to make interfaces here. There is no sense to make "different implementations" of attributes: they are simple value objects like, let's say, |
Interfaces would be a common denominator though, that with an |
Why all packages can't just use the same attributes? |
Because that would be a de-facto standard. Who decides what's the common package to be used? |
At least in concept, IF FIG were to define some standard attributes, making them interfaces wouldn't make any sense. The important part of the attribute is its constructor, which can't be part of an interface anyway. This would be a case where a "living standard" repo that contains well-known standardized attribute classes would make sense. That of course still requires buy in from the major static analysis players, who have said they want to wait a few months and see what shakes out first. So this is probably on hold until summer 2021, at least. |
Hey,
I'd like to discuss attributes in PHP 8. Most of them will be provided by specific frameworks as soon as they will start to prepare code for PHP 8, and my goal is to have more framework-agnostic ones with explicit declared behavior.
PhpStorm already released future integration of psalm and PhpStan: https://blog.jetbrains.com/phpstorm/2020/07/phpstan-and-psalm-support-coming-to-phpstorm/
So there is no doubt such features by itself will be supported.
However, it would be cool to get away from phpdoc-annotations like
@psalm-immutable
to the new PHP 8 attributes, framework-agnostic ones.In short, this PSR is about:
@@Final('Do not inherit me unless you are ProxyManager or MockBuilder')
:Symfony started to use such annotation (soft
final
) about 3 years ago for classes and methods.Added: probably, it requires also to add new annotation like
@@Proxy
to avoid issues in generated code of proxies and mocks by CI validators.@@Immutable
(https://psalm.dev/docs/annotating_code/supported_annotations/#psalm-immutable, https://wiki.php.net/rfc/immutability);This is not just best practice for value objects and similar things, but it also makes code way more readable and compact. Value objects, commands, events, DTOs, etc. No more tons of getters.
@@ReadOnly(strict: false)
(like https://psalm.dev/docs/annotating_code/supported_annotations/#psalm-readonly-and-readonly,strict
is optional param that implies whether object itself can modify this value after it was set once)@@Internal('Acme\Foo')
(@internal
with explicit acceptable namespace; by default is just current namespace including subnamespaces):Class
Acme\Foo\Bar
is allowed to useExperimentalServiceFoo
, but classes outside of this namespace (e.g.Acme\
) should not do it. Tools can help to detect incorrect usage here too.