Psalm: Incorrect RedundantCast and lack of MixedArgumentTypeCoercion with array-key type

[theodorejb]

I have an array which contains both string and integer keys. After upgrading from Psalm 3.14.2 to 4.3.1, I ran into a couple different issues with this code:

1. When looping over the array in the constructor method, Psalm fails to show a MixedArgumentTypeCoercion error when passing the array key to a function that requires a string. However, Psalm correctly shows this error when the same code is placed in another function called by the constructor.
2. In the function called by the constructor, even though Psalm correctly shows an error when directly passing the array key to a function expecting an int, if I cast the array key to a string to avoid a runtime TypeError, Psalm incorrectly shows a RedundantCast error.

<?php

declare(strict_types=1);

class Thing
{
    public function __construct(
        public string $name,
        public string $value,
    ) {}
}

new Foo([
    new Thing('foo', 'bar'),
    // Foo::__construct will cause the string '123' to end up as an int array key
    new Thing('123', 'baz'),
]);

class Foo
{
    /**
     * @var array<mixed, string>
     */
    public array $map;

    /**
     * @param Thing[] $things
     */
    public function __construct(array $things)
    {
        $this->map = [];

        foreach ($things as $thing) {
            $this->map[$thing->name] = $thing->value;
        }

        // map may now contain some string keys and some int keys

        foreach ($this->map as $key => $value) {
            // $key may be an int, but Psalm fails to produce the error it does inside useMap()
            echo strlen($key);
        }

        $this->useMap();
    }

    public function useMap(): void
    {
        foreach ($this->map as $key => $value) {
            // this line correctly causes an error
            echo strlen($key); // MixedArgumentTypeCoercion: Argument 1 of strlen expects string, parent type of array-key provided

            // this line incorrectly says the cast is redundant
            echo strlen((string) $key); // RedundantCast: Redundant cast to string
        }
    }
}

[psalm-github-bot[bot]]

Hey @theodorejb, can you reproduce the issue on https://psalm.dev ?

[theodorejb]

https://psalm.dev/r/246cc48833

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/246cc48833

<?php

declare(strict_types=1);

class Thing
{
    public function __construct(
        public string $name,
        public string $value,
    ) {}
}

new Foo([
    new Thing('foo', 'bar'),
    // Foo::__construct will cause the string '123' to end up as an int array key
    new Thing('123', 'baz'),
]);

class Foo
{
    /**
     * @var array<mixed, string>
     */
    public array $map;

    /**
     * @param Thing[] $things
     */
    public function __construct(array $things)
    {
        $this->map = [];

        foreach ($things as $thing) {
            $this->map[$thing->name] = $thing->value;
        }

        // map may now contain some string keys and some int keys

        foreach ($this->map as $key => $value) {
            // $key may be an int, but Psalm fails to produce the error it does inside useMap()
            echo strlen($key);
        }

        $this->useMap();
    }

    public function useMap(): void
    {
        foreach ($this->map as $key => $value) {
            // this line correctly causes an error
            echo strlen($key); // MixedArgumentTypeCoercion: Argument 1 of strlen expects string, parent type of array-key provided

            // this line incorrectly says the cast is redundant
            echo strlen((string) $key); // RedundantCast: Redundant cast to string
        }
    }
}

Psalm output (using commit 4a0e2f5):

INFO: UnusedVariable - 39:40 - $value is never referenced or the value is not used

INFO: MixedArgumentTypeCoercion - 51:25 - Argument 1 of strlen expects string, parent type array-key provided

INFO: UnusedVariable - 49:40 - $value is never referenced or the value is not used

ERROR: RedundantCast - 54:25 - Redundant cast to string

[orklah]

On __construct method, Psalm considers $this->map as an array<string, string> so there's no error displayed. PHP quirks makes working on string key really difficult, especially in strict_types and Psalm doesn't really handle this for now. @muglug not sure how I could suppress the issue in this case, maybe something like from_array_key??

On useMap, your array is indeed an array<array-key, string> and $key is array-key, this is a full blown bug. I'll look into it

[orklah]

Simple reproducer the redundantCast:
https://psalm.dev/r/57aa4a7a0d

The $key is an array-key but complains on the cast. If we remove the __construct or even the call to useMap in the __construct, the issue disappear, which means the code flow is important here.
I'd guess this has something to do with memoization but the settings on psalm.dev don't change anything.

In the other hand, that means this is just the continuation of the first issue.
The whole issue disappear if you document the map array on initialization:
https://psalm.dev/r/46ef8349ce
or even if you initialize it in property:
https://psalm.dev/r/beffa4073a

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/57aa4a7a0d

<?php
class Foo
{
    /**
     * @var array<mixed, string>
     */
    public array $map;

    public function __construct()
    {
        $this->map = [];
        $this->map['test'] = 'test';

        $this->useMap();
    }

    public function useMap(): void
    {
        foreach ($this->map as $key => $value) {
            /** @psalm-trace $key */;
            echo (string) $key;
        }
    }
}

Psalm output (using commit 4a0e2f5):

INFO: Trace - 20:37 - $key: array-key

INFO: UnusedVariable - 19:40 - $value is never referenced or the value is not used

ERROR: RedundantCast - 21:18 - Redundant cast to string

https://psalm.dev/r/46ef8349ce

<?php

declare(strict_types=1);

class Thing
{
    public function __construct(
        public string $name,
        public string $value,
    ) {}
}

new Foo([
    new Thing('foo', 'bar'),
    // Foo::__construct will cause the string '123' to end up as an int array key
    new Thing('123', 'baz'),
]);

class Foo
{
    /**
     * @var array<mixed, string>
     */
    public array $map;

    /**
     * @param Thing[] $things
     */
    public function __construct(array $things)
    {
        /**
         * @var array<mixed, string>
         */
        $this->map = [];

        foreach ($things as $thing) {
            $this->map[$thing->name] = $thing->value;
        }

        // map may now contain some string keys and some int keys

        foreach ($this->map as $key => $value) {
            // $key may be an int, but Psalm fails to produce the error it does inside useMap()
            echo strlen($key);
        }

        $this->useMap();
    }

    public function useMap(): void
    {
        foreach ($this->map as $key => $value) {
            // this line correctly causes an error
            echo strlen($key); // MixedArgumentTypeCoercion: Argument 1 of strlen expects string, parent type of array-key provided

            // this line incorrectly says the cast is redundant
            echo strlen((string) $key); // RedundantCast: Redundant cast to string
        }
    }
}

Psalm output (using commit 4a0e2f5):

INFO: MixedArgumentTypeCoercion - 44:25 - Argument 1 of strlen expects string, parent type array-key provided

INFO: UnusedVariable - 42:40 - $value is never referenced or the value is not used

INFO: MixedArgumentTypeCoercion - 54:25 - Argument 1 of strlen expects string, parent type array-key provided

INFO: UnusedVariable - 52:40 - $value is never referenced or the value is not used

https://psalm.dev/r/beffa4073a

<?php

declare(strict_types=1);

class Thing
{
    public function __construct(
        public string $name,
        public string $value,
    ) {}
}

new Foo([
    new Thing('foo', 'bar'),
    // Foo::__construct will cause the string '123' to end up as an int array key
    new Thing('123', 'baz'),
]);

class Foo
{
    /**
     * @var array<mixed, string>
     */
    public array $map = [];

    /**
     * @param Thing[] $things
     */
    public function __construct(array $things)
    {
        foreach ($things as $thing) {
            $this->map[$thing->name] = $thing->value;
        }

        // map may now contain some string keys and some int keys

        foreach ($this->map as $key => $value) {
            // $key may be an int, but Psalm fails to produce the error it does inside useMap()
            echo strlen($key);
        }

        $this->useMap();
    }

    public function useMap(): void
    {
        foreach ($this->map as $key => $value) {
            // this line correctly causes an error
            echo strlen($key); // MixedArgumentTypeCoercion: Argument 1 of strlen expects string, parent type of array-key provided

            // this line incorrectly says the cast is redundant
            echo strlen((string) $key); // RedundantCast: Redundant cast to string
        }
    }
}

Psalm output (using commit 4a0e2f5):

INFO: MixedArgumentTypeCoercion - 39:25 - Argument 1 of strlen expects string, parent type array-key provided

INFO: UnusedVariable - 37:40 - $value is never referenced or the value is not used

INFO: MixedArgumentTypeCoercion - 49:25 - Argument 1 of strlen expects string, parent type array-key provided

INFO: UnusedVariable - 47:40 - $value is never referenced or the value is not used

[muglug]

Everything Psalm is saying here is correct.

[muglug]

When you have declare(strict_types=1) on, PHP will exit with a type error if the type is incorrect. Therefore any statements _after_ that one must assume the type is correct.

[muglug]

oh but the array-key redundant cast is an actual bug, sorry

[orklah]

This redundantCast is also problematic because it's needed to avoid a TypeError in strict_types
https://psalm.dev/r/71609264b3

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/71609264b3

<?php

declare(strict_types=1);

class Thing
{
    public function __construct(
        public string $name,
        public string $value,
    ) {}
}

new Foo([
    new Thing('foo', 'bar'),
    // Foo::__construct will cause the string '123' to end up as an int array key
    new Thing('123', 'baz'),
]);

class Foo
{
    /**
     * @var array<mixed, string>
     */
    public array $map;

    /**
     * @param Thing[] $things
     */
    public function __construct(array $things)
    {
        $this->map = [];

        foreach ($things as $thing) {
            $this->map[$thing->name] = $thing->value;
        }

        // map may now contain some string keys and some int keys

        foreach ($this->map as $key => $value) {
            // $key may be an int, but Psalm fails to produce the error it does inside useMap()
            echo strlen((string)$key);
        }
    }
}

Psalm output (using commit 3f15579):

ERROR: RedundantCast - 41:25 - Redundant cast to string

INFO: UnusedVariable - 39:40 - $value is never referenced or the value is not used

[theodorejb]

@muglug Thank you for all your work on this project and for fixing the redundant cast bug.

On __construct method, Psalm considers $this->map as an array<string, string> so there's no error displayed.

Shouldn't this be considered a bug? $this->map was declared as array<mixed, string>, so I would expect the constructor to respect this and not require me to duplicate the type when assigning to $this->map.

It seems like what's happening is that Psalm sees that the array keys are only populated with values of type string, and it incorrectly assumes that this means the array only contains string keys (which isn't the case since values like '123' become integer keys). The outcome is that there's a hidden bug which is unlikely to be caught during development, and only manifests later as a TypeError in production once a Thing happens to be constructed with a numeric name.

https://psalm.dev/r/3adce1cc53

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/3adce1cc53

<?php

declare(strict_types=1);

class Thing
{
    public function __construct(
        public string $name,
        public string $value,
    ) {}
}

new Foo([
    new Thing('foo', 'bar'),
    // Foo::__construct will cause the string '123' to end up as an int array key
    new Thing('123', 'baz'),
]);

class Foo
{
    /**
     * @var array<mixed, string>
     */
    public array $map;

    /**
     * @param Thing[] $things
     */
    public function __construct(array $things)
    {
        $this->map = [];

        foreach ($things as $thing) {
            $this->map[$thing->name] = $thing->value;
        }

        // map may now contain some string keys and some int keys

        foreach ($this->map as $key => $value) {
            // $key may be an int, but Psalm fails to detect this, resulting in a TypeError at runtime
            echo strlen($key);
        }
    }
}

Psalm output (using commit 373c0bb):

INFO: UnusedVariable - 39:40 - $value is never referenced or the value is not used

[orklah]

I proposed something to handle this issue a few month ago: https://github.com/vimeo/psalm/pull/4237.

However, this is something that will probably have a lot of false positives (almost every string array-key is potentially problematic) for only a few actual bug caught

[theodorejb]

@orklah Maybe as you proposed in that PR a check for this would only be activated at level 1 with strict_types enabled. I think there are a lot of legitimate bugs this could catch, which people just haven't noticed since most of the time their data happens to not contain integer strings.