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

non-empty-list type assertions are incorrectly considered redundant when list is encountered #2450

Closed
Ocramius opened this issue Dec 9, 2019 · 4 comments

Comments

@Ocramius
Copy link
Contributor

Ocramius commented Dec 9, 2019

While refining type declarations for webmozart/assert, I found that psalm considers non-empty-list to be a redundant condition on top of list or array: https://psalm.dev/r/e8ec0750f5

<?php

/** 
 * @psalm-assert non-empty-list $v
 * @psalm-param mixed $v
 */
function assertNonEmptyList($v):void {}

/** @psalm-param array<int> $v */
function firstElement($v) : int
{
    assertNonEmptyList($v);
    return $v[0];
}
Psalm output (using commit 74de32f):

INFO: UnusedParam - 7:29 - Param $v is never referenced in this method

ERROR: RedundantConditionGivenDocblockType - 12:5 - Found a redundant condition when evaluating docblock-defined type $v and trying to reconcile type 'array<array-key, int>' to non-empty-list<mixed>

INFO: PossiblyUndefinedIntArrayOffset - 13:12 - Possibly undefined array offset 'int(0)' is risky given expected type 'array-key'. Consider using isset beforehand.

Also seems that non-empty-list doesn't implicitly have key 0.

@muglug
Copy link
Collaborator

muglug commented Dec 9, 2019

As with #2449, the best option here is for separate @psalm-assert calls:

https://psalm.dev/r/4228415ada

@muglug muglug closed this as completed Dec 9, 2019
@Ocramius
Copy link
Contributor Author

Ocramius commented Dec 9, 2019

Hmm, I think I'm hitting a different bug with webmozart/assert then. If I add @psalm-assert !empty to Assert::stringNotEmpty(), then type inference fails to restrict the type to string.

I can't seem to isolate through psalm.dev though.

This happens if I use the suggested approach from #2449 in following code location:

https://github.com/Ocramius/assert-1/blob/ea5e87e60c1a4e643d063c29a570daa7f5f8a022/src/Assert.php#L221-L233

This static analysis verification file will no longer pass:

https://github.com/Ocramius/assert-1/blob/ea5e87e60c1a4e643d063c29a570daa7f5f8a022/tests/static-analysis/assert-stringNotEmpty.php#L7-L15

@Ocramius
Copy link
Contributor Author

Ocramius commented Dec 9, 2019

Note: comment above may be due to #2449 (comment)

@muglug
Copy link
Collaborator

muglug commented Dec 10, 2019

Yeah part of the fix was to allow multiple @psalm-assert for a given variable.

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

2 participants