Hi Rowan and Nicolas (and internals), On Wed, Oct 28, 2020 at 12:57 PM Rowan Tommins <rowan.coll...@gmail.com> wrote:
> On 28/10/2020 16:58, Nicolas Grekas wrote: > > about why we'd need nested attributes, here is a discussion about > > nested validation constraints: > > https://github.com/symfony/symfony/issues/38503 > > Thanks, it's useful to see some real-world examples. As I suspected, > nearly all of these explicitly take a *list* of nested attributes, > not a single attribute. > > > While most of the constraints receive the nested constraints as > > simple array, |Collection| however requires a mapping (field to > > constraint) and is usually combined with other composite > > constraints, which gives us a second nesting level. > > There is much discussion of Collection as an edge case, but although > the nested attributes are indeed inside a mapping, the *value* of > that mapping is again a list of attributes. Single items are allowed, > but this seems to be a special case for convenience. ... This > reinforces my earlier suggestion (https://externals.io/message/111936#112109) > that #[Foo] in a nested context can simply imply an array of one > attribute, While passing all nested attributes as an array would at least enable consistent semantics, it has the notable disadvantage of preventing some use cases from being expressed in PHP's type system. Specifically, how would you express that an attribute parameter must be passed a single attribute of a particular type? For example, suppose a developer wants to create/use an attribute like the following: #[Limit( min: 10, max: 100, error: #[CustomError(message: "...", notify: true, withTrace: false)] )] I would expect to be able to write the `Limit` attribute class like this: #[Attribute] class Limit { public function __construct( public int $min, public int $max, public CustomError $error, ) {} } But if nested attributes are always passed as an array, the `$error` parameter would have to have an `array` type, which provides essentially no context about what is really required. The type system would be perfectly happy to allow an empty array, or an array with multiple values instead of the expected single `CustomError` attribute. It may be possible to enable static analysis type checks and IDE autocompletion by adding extra docblock annotations or attributes specifying that the array has to contain exactly one `CustomError` value, but this is a workaround requiring a lot more effort than should be necessary. Overall the dev experience becomes a lot worse for these use cases. On Wed, Oct 28, 2020 at 11:58 AM Nicolas Grekas <nicolas.grekas+...@gmail.com> wrote: > > The problem with this is that it results in inconsistent semantics, > > where the number of items in an attribute group results in a > > different type of value being passed. I.e. if you remove two of the > > three attributes from the list, suddenly the code will break since > > the `Assert\All` attribute is no longer being passed an array. > > Yes. > This would be solved by NOT accepting #[] inside attribute declarations. > Since we borrowed from Rust, let's borrow this again: > http://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/reference/attributes.html#attributes > > The Rust syntax accepts the #[] only on the outside of the declaration. > The example above should be written as: > > #[Assert\All([ > Assert\Email(), > Assert\NotBlank(), > Assert\Length(max: 100) > ])] > > The closing brackets () would be required to remove any ambiguity > with constants. Since constant expressions won't ever allow functions > in them, this isn't ambiguous with function calls. This does seem more readable and also avoids the grouped syntax inconsistency. On the other hand, presumably it would prevent ever supporting function calls in constant expressions. At present I'm not convinced constant expressions *should* support this, but nevertheless I have reservations about a syntax choice that would rule it out completely. Furthermore, the requirement of parentheses seems inconsistent with normal attribute declarations and `new ClassName` instantiations, where parentheses are optional. As I see it, we have the following other options: 1. Simply ban the grouped syntax in nested context. This has the downside of extra verbosity and that it may be unexpected/surprising for developers. 2. Drop support for attribute grouping before PHP 8 is finalized. Rust (which we borrowed the `#[]` syntax from) itself does not support this anyway. The primary downside of no attribute grouping is additional verbosity in some circumstances. Note: a number of people who voted for `#[]` expressed disfavor of attribute grouping, which was originally accepted only for the `<<>>` syntax with the qualification that it would be superseded by any other RFC that changes the syntax. [1] 3. Vote to switch to a less verbose syntax that avoids these issues. I.e. revert to `@@`, or alternatively to `##` (the latter would preserve forward compatibility). This would allow the same syntax to be used for both parent and nested attributes with identical semantics. The downside is that some tooling (e.g. PHP-Parser and PhpStorm) has already done work to support the `#[]` syntax and this would need to be changed (though removing attribute grouping will simplify the implementation for such tooling). Examples of option 3: @@Limit( min: 10, max: 100, error: @@CustomError(message: "...", notify: true, withTrace: false) ) // or ##Limit( min: 10, max: 100, error: ##CustomError(message: "...", notify: true, withTrace: false) ) Best regards, Theodore [1]: https://wiki.php.net/rfc/attribute_amendments#group_statement_for_attributes -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
