Null is not allowed actually. The only place where we have null === $this->groups could be replaced by an isset(), and leads to an error.

This change looks wrong to me. Passing groups=null in your annotations is not a supported usage, and I don't see any other way it could be null (and if you account for unsupported usages, all phpdoc types would be mixed)

This use case is broken, not calling the parent constructor is not supported.

@zerkms when you create a custom constraint, you must call the parent constructor. Otherwise, the initialization of the class is not complete, and things are in a broken state.

How would one write statically analysable code then:

final class IsRegex extends Constraint
{
    /** @var string */
    public $message = 'must be a valid regular expression';
}

This as expected fails with

PropertyNotSetInConstructor: Property IsRegex::$groups is not defined in constructor of IsRegex or in any methods called in the constructor

At the moment annotation says: string[]. It means the field always exists and always is an array of strings.

How about

@property string[] $groups

for the Symfony\Component\Validator\Constraint class instead? That's how you annotate properties managed by magic methods. Right?

@zerkms failing where ? If this is about psalm or phpstan, this is a false positive from those tools, that don't understand the lazy initialization done in __get thanks to unsetting the property in the constructor (which is perfectly valid code).

this is a false positive from those tools

it's not false-positive: class properties managed by magic methods should be declared as @property string[] $groups.

The property is already declared on L66.

Please open a bug report on your favorite SA instead.

@nicolas-grekas

The property is already declared on L66.

declaration on line 66 is a lie. Take get_object_vars() and see yourself.

<?php

include_once __DIR__ . '/vendor/autoload.php';

class MyConstraint extends \Symfony\Component\Validator\Constraint
{
}

var_dump(get_object_vars(new MyConstraint()));

it outputs

array(1) {
  ["payload"]=>
  NULL
}

So, how $groups is an array in this output?

I don't disagree that code works, but typing there is a lie and it is has nothing to do with SA.

At this stage, it is in "uninitialized" state.
Check https://wiki.php.net/rfc/deprecate_dynamic_properties for more background.

@zerkms get_object_vars returns null for uninitialized properties, but this does not mean that those properties actually contain null

At this stage, it is in "uninitialized" state.

A constructed object must be fully initialised, that's what @stof said above.

@stof

but this does not mean that those properties actually contain null

right, but there is no $groups field whatsoever. Yet the property declaration states it's a string[].

I understand you now would protect the current solution no matter what, but current implementation is a dirty hack, that wouldn't be possible to implement in a statically strictly typed language like Rust or even Java.

This is the relevant part of the RFC in case you missed it.
Uninitialized properties are lazy properties that can be initialized by magic methods.

@nicolas-grekas it's absolutely impossible to type Constraint then so that it was not a lie.

get_object_vars must return a field for a field that is declared as string[].

Well, get_object_vars() doesn't care about OOP API semantics. Using it is "the hack", since it allows getting around the OOP API. As such it should be used with care (eg like reflection).

I understand this is complex, but there is a reason for the current design (laziness), and the current way is the only way.

Here, I really think this is something that should be improved on the SA's side. The code in Constraint is unusual, but perfectly legit.

Why not accept suggesed

@property string[] $groups

?

phpdoc designed it for exactly this use case.

Here, I really think this is something that should be improved on the SA's side.

it's really impossible: I can override __set and $this->groups would never be initialised.

Types should be statically verifiable.

Oh, File constraint already implements it the way I suggested:

/**
 * @Annotation
 * @Target({"PROPERTY", "METHOD", "ANNOTATION"})
 *
 * @property int $maxSize
 *
 * @author Bernhard Schussek <bschussek@gmail.com>
 */

@zerkms when you use the object API, you will never get null when reading the groups property.

The @property phpdoc is about telling SA tools about properties that don't exist on the object, but are accessible thanks to __get. This is not the case of this groups property. We do have a property that exists and is

it's really impossible: I can override __set and $this->groups would never be initialised.

Types should be statically verifiable.

if you override the __get of Symfony without calling the parent method, then your code is broken, and then SA tools might indeed report it (just like they might want to report that properties initialized in the constructor cannot be trusted either to be initialized if you override the constructor). But seeing that the method is overridden is also something that SA tools can detect.

The issue with SA tools is that today, the way they deal with the uninitialized state that exists in PHP is to enforce initialization in the constructor, instead of enforcing initializing it before usage. That's a shortcut they took to simplify their analysis, but it means that those tools are rejecting perfectly valid code. and that's an issue of the SA tool, not of Symfony.

Oh, File constraint already implements it the way I suggested:

that's totally different. In this class, we indeed rely on a magic public property. In Constraint, the public property is already the declared one being lazy-initialized.

that's totally different. In this class, we indeed rely on a magic public property. In Constraint, the public property is already the declared one being lazy-initialized.

oh, come on: it's defined as a field there too: protected $maxSize;

If the culprit is having it public vs protected: can we please have $groups protected then too, as long as it improves typing?

if you override the __get of Symfony without calling the parent method, then your code is broken

you just made this rule up to prove your point. Nothing in php requires overridden magic methods to always call parents.

That's a shortcut they took to simplify their analysis

It's not a shortcut: static analysis is called static because it's done statically, while current Constraint typing relies on runtime behaviour + a set of self made up rules.

I fail to see how a PHPdoc improves typing. For PHP, it's just a comment so your code is just as typesafe. For non-code, the property is defined as public, so it can be argued that it's rather a limitation from the SA side to not detect this use-case properly (given it was also defined in the RFC).

you just made this rule up to prove your point. Nothing in php requires overridden magic methods to always call parents.

Please try to not be offended and maybe close this tab for an hour and take a breath of fresh air. We're just discussing code here, not persons or people (we all don't know eachother, so we can't judge anything about anyone personally).

Overriding a method is always changing behavior of a parent method, if you don't change behavior you wouldn't need to override it. In the case of this class, overriding __get() or the constructor creates an invalid class. The same is true for e.g. the Command class of the Console command, we even created a user error to warn you about this mistake. In other cases, overriding the method can produce invalid states (e.g. if you override Router::match() to do nothing, no route will be matched anymore).
This is why inheritance is often less favored over composition in object oriented programming.

Overriding a method is always changing behavior of a parent method, if you don't change behavior you wouldn't need to override it.

that's correct, but it should not break API: if you declare a field as string[] - it should be so.

Overriding a method should not break typing of an irrelevant field.

Code is either statically analysable or not: at the moment Constraint lies about types statically.

Try to design the same class in Java or Rust - how would it look?

It's unlikely anybody will post here now, so I will summarise myself:

The current implementation is done that way to be a performance optimisation and it makes it really impossible to statically analyse it. Because typing relies on runtime and php semantics don't guarantee the field to be of specified type.

I understand you are tied to code you designed and implemented (me too - to mine), but just accepting that "code is not ideal and it's not statically analysable" would be a much better answer than blaming SA tools imperfection.

Update: after a long and useful chat with @nicolas-grekas in the symfony slack they could provide a valid path for an extremely advanced (and not existing) SA to be able to verify the declarations statically (it's unfortunate that it will never exist, but at least it was convincing that it's really possible to verify at least theoretically).

Linking to #44854 for reference.