feat: make key-of/value-of usable with non-const arrays

[Patrick-Remy]

Currently key-of<T> and value-of<T> helper-types are only usable with class constants. But they can be even more powerful! This PR tries to add the following capabilites:

1. Usage with array-like types (includes keyed array, and lists), even with literals

/**
 * @return key-of<list<string>>
 */
/**
 * @return key-of<array{a: string, b: string)>
 */

2. Support for unions as T-param

/**
 * @return key-of<array<int, string>|array<float, string>>
 */

3. Improve template support of key-of, so that correctly not only array-key is checked, but instead only methods like array_keys can be used to ensure template support.

/**
 * @template T of array
 * @param T $array
 * @return key-of<T>[]
 */
function getKeys($array) {
    return array_keys($array);
}

4. Add template support to value-of which accepts array_values if used correctly.

/**
 * @template T of array
 * @param T $array
 * @return value-of<T>[]
 */
function getValues($array) {
    return array_values($array);
}

Unfortunately @psalm-assert-if-true seems to not support template types, and even if I fix a bug inside FunctionLikeDocblockScanner where template types aren't passed I have no success. So I welcome any suggestions to this! I saw that there is made use of assertions for array-key-exists but I didn't find how I can change the type of $key so that this test won't fail:

/**
 * @template T of array
 * @param T $array
 * @return key-of<T>|null
 */
function getKey(string $key, $array) {
    if (array_key_exists($key, $array)) {
        return $key;
     }
     return null;
}

[orklah]

Converting to draft (just for easier maintenance)

[Patrick-Remy]

Do you have any idea how I can achieve the described case for array_key_exists?

[orklah]

I'm not sure I understood the issue exactly, but did you look at this:

psalm/src/Psalm/Internal/Type/AssertionReconciler.php

Line 278 in b633619

if ($assertion === 'array-key-exists'

using array_key_exists will generate an assertion array-key-exists. It may be possible to adapt this to be able to change $key.
However, you'll probably start asking questions about this kind of cases: https://psalm.dev/r/e28860fd8a

The array here has int|string keys. You know $key is already a string. What will $key be after you array_key_exists? If you say key-of<$array>, you lost information about $key being a string. If you say string, you lost the information about $key being a valid offset. You'd need something like string-key-of<$array>, I think. But this is only a valid solution for keys, values will be different. And even for keys, if $key was 'a'|'b' instead of string, you're back to square one.

The type system is very complicated and I can only advise that you finish your first PR and all that will be related to it until you start on more

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/e28860fd8a

<?php

/**
 * @template T of array<string|int, mixed>
 * @param T $array
 * @return key-of<T>|null
 */
function getKey(string $key, $array) {
    if (array_key_exists($key, $array)) {
        return $key;
     }
     return null;
}

Psalm output (using commit 1f0d3de):

No issues!

[Patrick-Remy]

The array here has int|string keys. You know $key is already a string. What will $key be after you array_key_exists? If you say key-of<$array>, you lost information about $key being a string. If you say string, you lost the information about $key being a valid offset.
You're completely right, replacing the type with key-of<$array> was a bad idea.

You'd need something like string-key-of<$array>, I think. But this is only a valid solution for keys, values will be different. And even for keys, if $key was 'a'|'b' instead of string, you're back to square one.
It just would be great, if the snippet would pass with my implementation, as it's completely valid. My current approach was to extend UnionTypeComparator::isContainedBy to check if $container_type is TTemplateKeyOf and there is an assertion array-key-exists for the $input_type with a relation to the type of $container_type, it would be ok. Unfortunately in this context all assertions are gone. So I'm not sure if this is the right context.

Probably this case is something for later. I fixed all other outstanding issues, the original parsing for TTemplateKeyOf didn't initialized with the original TTemplateParam type, which led to not being replaced and was always empty.
Could you please review again?

[orklah]

This will need to target the master branch, removing existing types is a big BC break.

Also, can you fix the errors in shepherd before we review? It's possible you'll see issues that could have a big impact on your existing code. Reviewing is kinda hard, so I'd prefer doing that once everything is fixed!

[Patrick-Remy]

Okay, I will rebase to master, do you have some time schedule for the 5.x release?

I'm not quite sure, if the latest shepherd issues are related to my changes. I'll have a look tomorrow.

[orklah]

We don't have a release date yet for Psalm 5, we'll make sure to communicate when we will

About the errors on shepherd, I'm pretty sure they're related to your changes. Upstream branch is clean and the errors seem consistent with what you changed

[Patrick-Remy]

I think I'm finally done rebasing to master and fixing the outstanding shepherd bugs, which were caused by not using TemplateStandinTypeReplacer::getMostSpecificTypeFromBounds() to replace the template param.

All failing tests/checks are also currently failing on the master branch, if I didn't oversee something.

Ready for review now – hopefully 😅

FYI, I now no longer will force-push the branch, so that changes are transparent.

[orklah]

there are errors in shepherd, they are related to a change in test format in master.

We know require tests to be all associative arrays. Can you change the format of the tests you added?

[Patrick-Remy]

I already changed the format of my added tests, but there are tests in the master branch which aren't correct, see:

my shepherd run: https://github.com/vimeo/psalm/runs/4859814032?check_suite_focus=true
latest master shepherd run: https://github.com/vimeo/psalm/runs/4857096400?check_suite_focus=true

Same for the failing unit tests.

[orklah]

sorry, this is actually my fault. I'm fixing that

[orklah]

fixed :) Thanks for reporting, I forgot to check after the last merge

[Patrick-Remy]

Thanks! I just rebased 👍

[Patrick-Remy]

@orklah any idea how to pass this test here?

<?php
/**
  * @template TKey of int
  * @template TArray of array<TKey, bool>
  * @param TArray $array
  * @return list<TKey>
  */
function toListOfKeys(array $array): array {
    return array_keys($array);
}

I have no idea why, but while debugging TArray has key-type array-key instead of expected int (or TTemplateParam TKey).

[Patrick-Remy]

@Patrick-Remy Don't know if you care to, but you might be able to add support for stuff like this without too much trouble.

@AndrolGenhald sorry, this PR has already been 'exploded' a bit and took a lot of time, should be handled in a separate PR after this has been merged.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/3531bf3469

<?php

class Foo
{
    /** @var non-empty-list<int> */
    public const BAZ = [1];

    /** @return value-of<static::BAZ> */
    public static function foobar(int $int): int
    {
        return static::BAZ[0];
    }
}

Psalm output (using commit 9168cef):

ERROR: UnresolvableConstant - 8:17 - Could not resolve constant static::BAZ

ERROR: MismatchingDocblockReturnType - 8:17 - Docblock has incorrect return type 'value-of<static::BAZ>', should be 'int'

INFO: MixedInferredReturnType - 8:17 - Could not verify return type 'value-of<static::BAZ>' for Foo::foobar

[orklah]

@orklah any idea how to pass this test here?

Not sure what you mean, the snippet https://psalm.dev/r/d16778139e seems to work and return list<1> on your branch as I'd expect

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/d16778139e

<?php
/**
  * @template TKey of int
  * @template TArray of array<TKey, bool>
  * @param TArray $array
  * @return list<TKey>
  */
function toListOfKeys(array $array): array {
    return array_keys($array);
}

$a = toListOfKeys([1 => 'a']);

/** @psalm-trace $a */;

Psalm output (using commit 8914793):

ERROR: InvalidArgument - 12:19 - Argument 1 of toListOfKeys expects array<int, bool>, array{1: 'a'} provided

INFO: Trace - 14:23 - $a: list<1>

INFO: UnusedVariable - 12:1 - $a is never referenced or the value is not used

[Patrick-Remy]

@orklah any idea how to pass this test here?

Not sure what you mean, the snippet https://psalm.dev/r/d16778139e seems to work and return list<1> on your branch as I'd expect

Are you sure you ran it on my branch (commit 51cab704c4626b8ee55472d82542ea4b031b8960)? I get:

1) Psalm\Tests\KeyOfTemplateTest::testValidCode with data set "keyOfNestedTemplates" ('<?php\n                /**\n ...$a */;')
Psalm\Exception\CodeException: MixedReturnTypeCoercion - src/somefile.php:9:27 - The type 'list<key-of<TArray:fn-array_keys as TArray:fn-array_keys as array<array-key, mixed>>>' is more general than the declared return type 'list<TKey:fn-tolistofkeys as int>' for toListOfKeys

It's also already added as a test in tests/Template/KeyOfTemplateTest.php as SKIPPED-.

[Patrick-Remy]

I got it, I didn't implement replaceWithTemplateArgs and I didn't add a comparison for TTemplateKeyOf also in AtomicTypeComparator before TTemplateParam. Need to refactor a bit tomorrow and then it will be ready for review!

[Patrick-Remy]

@orklah It's finally done – could you review again please?

[orklah]

I'm ready to merge, no last change? :)

[Patrick-Remy]

Needs anything to be added to the UPGRADING.md file?

[orklah]

Yeah, maybe document the change of inheritance

[Patrick-Remy]

I also added the renames as BCs.

[orklah]

Thanks!

[Patrick-Remy]

Many thanks for your intensive and always fast review! 😊