Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for assertArrayHasKey and assertCount #114

Open
VincentLanglet opened this issue Aug 21, 2021 · 13 comments
Open

Add support for assertArrayHasKey and assertCount #114

VincentLanglet opened this issue Aug 21, 2021 · 13 comments

Comments

@VincentLanglet
Copy link
Contributor

This code is valid https://psalm.dev/r/e1d61bda01

But when I use assertArrayHasKey and assertCount, the offset are reported as possibly undefined.
It would be great to tell psalm that

assertArrayHasKey work as `array_key_exists` so the offset is not undefined for arrays
assertCount work as `count` so the offset is not undefined for list

I'm not familiar with, but I'm not sure it can be done with psalm assertion https://psalm.dev/docs/annotating_code/assertion_syntax/

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/e1d61bda01
<?php

/**
 * @return array<string, list<string>>
 */
function foo() {
    return [];
}

function foo2(): string
{
    $a = foo();
    assert(array_key_exists('foo', $a));
    
    $foo = $a['foo'];
    assert(count($foo) === 1);
    
    return $foo[0];
}
Psalm output (using commit 9222b24):

No issues!

@zerkms
Copy link

zerkms commented Oct 8, 2021

I'm 99% sure it's impossible to implement with just @psalm-assertion (via stubbing) and it would require writing a plugin.

@VincentLanglet
Copy link
Contributor Author

@orklah maybe it's a good opportunity for some new features about @psalm-assertion wdyt ?

@orklah
Copy link
Contributor

orklah commented Feb 1, 2022

I think you should already be able to use non-empty-countable or the inverted !non-empty-countable as an assertion to reconcile a non empty array or an empty array (but technically, empty or !empty would probably be enough in most contexts, unless you also need to assert arrayness)

count() with greater or less than comparison would be harder without a plugin because the assertion is technically has-at-least-X (or !has-at-least-X) X being the number you compared against

Same with equal comparison, the assertion is has-exact-count-X (or !has-exact-count-X)

The harder will probably be key existence. Psalm has specific code for that because it implies to recursively change the type of different array dimensions. (Think assert(array_key_exists('a', $a['foo']['bar']));. Psalm must change the type of $a['foo']['bar'], $a['foo'] and $a.

You'd need to recheck but there are actually two set of assertions depending on what you want to change has-array-key-X and array-key-exists (the last one doesn't have X because it is applied directly onto the targeted variable id, like $a['a'])

If you want to progress on that, you'd have to find a syntax to use a param value to complete the assertion. (Something like @psalm-assert has-exact-count-$b $a for a function that takes $a as an array and $b as a count). You'd enter the realm of what you do when $b is not a single literal :)

This might be easier with a plugin, but I'm not actually sure on which hook you should code for adding assertions into Psalm though...

@orklah
Copy link
Contributor

orklah commented Feb 1, 2022

What I said above is partially true. I just learned that internal assertions are not available automatically to docblock assertions.

Given the changes to be made, it's only a small hiccup more, but it's important to note.

@VincentLanglet
Copy link
Contributor Author

I again encounter issue with assertArrayHasKey.

Phpstan has somehthing like a TypeSpecifier https://github.com/phpstan/phpstan-phpunit/blob/15dc3e0d81b711c38d02fb33fe4482dfa570951c/src/Type/PHPUnit/Assert/AssertTypeSpecifyingExtensionHelper.php

Is there something equivalent in psalm ?

@orklah
Copy link
Contributor

orklah commented Mar 4, 2022

https://github.com/vimeo/psalm/blob/11e60fa261c682627e6b85300dd6f424b87520b1/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php#L926

This is the method that handles assert calls in the code.

You could use some variation of this in a plugin hooked to assertArrayHasKey and assertCount to have the same effect as if they were simple asserts I guess.

I hoped to find another entry point to our internal assertion module but I couldn't find one. It could be cool to be able to add an arbitrary assertion to a variable from plugins, especially with Psalm 5 that's coming with the brand new system :)

@muglug, maybe I missed something? How would you add an arbitrary assertion from a plugin?

@VincentLanglet
Copy link
Contributor Author

https://github.com/vimeo/psalm/blob/11e60fa261c682627e6b85300dd6f424b87520b1/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php#L926

This is the method that handles assert calls in the code.

You could use some variation of this in a plugin hooked to assertArrayHasKey and assertCount to have the same effect as if they were simple asserts I guess.

I hoped to find another entry point to our internal assertion module but I couldn't find one. It could be cool to be able to add an arbitrary assertion to a variable from plugins, especially with Psalm 5 that's coming with the brand new system :)

@muglug, maybe I missed something? How would you add an arbitrary assertion from a plugin?

I would really like to add the assertArrayHasKey feature to this library, but I understand nothing to this code 🥲

@orklah
Copy link
Contributor

orklah commented Mar 6, 2022

I understand nothing to this code

I'm not sure it's even required that you do. I think you could just add this function somewhere in this plugin and call it.

Which means, all it would take is to send the correct value here: https://github.com/vimeo/psalm/blob/11e60fa261c682627e6b85300dd6f424b87520b1/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php#L929

Those are nodes from PHParser that need to be simulated (i.e. replacing the assertArrayHasKey($array, $key) by array_key_exists($array, $key)) and just roll with it

@VincentLanglet
Copy link
Contributor Author

Those are nodes from PHParser that need to be simulated (i.e. replacing the assertArrayHasKey($array, $key) by array_key_exists($array, $key)) and just roll with it

Best would be to be able to support the syntax

/** 
 * @psalm-assert-if-true isset($array[$key])
 */

It would also be useful for webmozart/assert,
https://github.com/webmozarts/assert/blob/master/src/Assert.php#L1678
because there is no psalm/assert-plugin and this could just be solved with phpdoc.

@zerkms
Copy link

zerkms commented Nov 6, 2022

would be to be able to support the syntax

it would be horrible: it addresses one specific use case by creating a special unique syntax that you (as a user) now need to learn and maintain (as psalm developer). And that knowledge would not scale, eg: @psalm-assert-if-true myfunction($key) <- what would this mean?

@VincentLanglet
Copy link
Contributor Author

would be to be able to support the syntax

it would be horrible: it addresses one specific use case by creating a special unique syntax that you (as a user) now need to learn and maintain (as psalm developer). And that knowledge would not scale, eg: @psalm-assert-if-true myfunction($key) <- what would this mean?

psalm supports

if (isset($array[$key])) {
     // Understand that the key exists
}

the syntax

/** 
 * @psalm-assert-if-true expression
 */

could be translated to

if (expression) {
}

so it would be a generic feature, and not a specific one.

In your example, @psalm-assert-if-true myfunction($key), will be resolved the same way it would be resolved by psalm if you wrote

if (myfunction($key)) {
}

@zerkms
Copy link

zerkms commented Nov 6, 2022

will be resolved the same way

what would it mean type-wise? What assertion would it establish?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants