coding-standard icon indicating copy to clipboard operation
coding-standard copied to clipboard

DisallowArrayTypeHintSyntax issue with collection objects

Open dereuromark opened this issue 4 years ago • 2 comments

    /**
     * @param \Propel\Runtime\Collection\ObjectCollection|\Orm\Zed\ExampleStateMachine\Persistence\PyzExampleStateMachineItem[] $exampleStateMachineItems
     *
     * @return array<\Generated\Shared\Transfer\StateMachineItemTransfer>
     */
    protected function hydrateTransferFromPersistence($exampleStateMachineItems)

results in

 54 | ERROR | [x] Usage of array type hint syntax in
    |       |     "\Orm\Zed\ExampleStateMachine\Persistence\PyzExampleStateMachineItem[]"
    |       |     is disallowed, use generic type hint syntax
    |       |     instead.
    |       |     (SlevomatCodingStandard.TypeHints.DisallowArrayTypeHintSyntax.DisallowedArrayTypeHintSyntax)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

We would like to use the sniff to fix all trivial cases of array vs generics, but we do need to keep object ones for IDE and PHPStan compatibility. In other words: The sniff fixes it to proposed \Propel\Runtime\Collection\ObjectCollection|array<\Orm\Zed\ExampleStateMachine\Persistence\PyzExampleStateMachineItem> which is invalid as per PHPStan. Rightfully so, as this is now a collection OR some iterable array instead of the "old legacy syntax of saying "iterable object of this type".

PHPDoc tag @param for parameter $entities with type array<DateTimeImmutable>|Test\Collection is not subtype of native type Test\Collection. and has parameter $entities with no value type specified in iterable type Test\Collection It does however not complain with legacy syntax \Object|type[].

If anything it would need to be \Propel\Runtime\Collection\ObjectCollection<\Orm\Zed\ExampleStateMachine\Persistence\PyzExampleStateMachineItem> to be valid, but this is not yet understood by most IDEs.

So we would like to only let it fix the trivial cases of generics (specifically array/iterable), maybe some of the trivial collections IDEs already understand like ArrayObject/ArrayAccess etc, but that does not need to be.

So far it doesnt seem to have an option to toggle this behavior. It would be awesome if you could consider providing a config value for this - and also if the above would fix it to the correct generic type then instead of making this PHPStan/Psalm invalid.

dereuromark avatar Oct 30 '21 11:10 dereuromark

I solved it locally by adding

    /**
     * The following classes are supported for object generics by IDEs like PHPStorm already.
     * E.g. `\ArrayObject<type>` instead of legacy syntax `\ArrayObject|type[]`.
     *
     * @var array<string>
     */
    protected static $genericCollectionClasses = [
        '\\Traversable',
        '\\ArrayAccess',
        '\\ArrayObject',
        '\\Generator',
        '\\Iterator',
    ];

and adding

    /**
     * These generic object collections are not yet understood by IDEs like PHPStorm.
     *
     * @param array<\PHPStan\PhpDocParser\Ast\Type\TypeNode> $types
     *
     * @return bool
     */
    protected function isComplexGenericObjectCollection(array $types): bool
    {
        foreach ($types as $type) {
            if (!$type instanceof IdentifierTypeNode) {
                continue;
            }

            $className = (string)$type;
            if (strpos((string)$type, '\\') === 0 && !in_array($className, static::$genericCollectionClasses, true)) {
                return true;
            }
        }

        return false;
    }

    /**
     * @param array<\PHPStan\PhpDocParser\Ast\Type\TypeNode> $types
     *
     * @return bool
     */
    protected function containsArrayTypeNode(array $types): bool
    {
        foreach ($types as $type) {
            if (!$type instanceof ArrayTypeNode) {
                continue;
            }

            if ($type->type instanceof IdentifierTypeNode) {
                return true;
            }
        }

        return false;
    }

and doing a check inside main method:

                if ($annotation->isInvalid()) {
                    continue;
                }

                // here
                if ($this->isGenericObject($annotation)) {
                    continue;
                }

dereuromark avatar Oct 30 '21 13:10 dereuromark

Well, this just skipped things

It would still be nice to have this feature to auto convert those "collections" to the new "generic" syntax: Also, e.g.

     /**
-     * @return \Iterator|int[]
+     * @return \Iterator|array<int>
      */

should be fixed to

     /**
-     * @return \Iterator|int[]
+     * @return \Iterator<int>
      */

As this is what is meant with the legacy syntax.

dereuromark avatar Jan 21 '22 09:01 dereuromark