-
Notifications
You must be signed in to change notification settings - Fork 659
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
False positives testing for array emptiness #10825
Comments
I found these snippets: https://psalm.dev/r/4609d92f6f<?php
class Foo {
/** @var string[]|null */
protected ?array $arr;
function __construct()
{
$this->arr = [];
// ...do stuff here that *may* populate $this->arr, or set it to NULL...
if (empty($this->arr)) {
echo "empty\n";
}
}
}
|
Could you provide a full example? |
I found these snippets: https://psalm.dev/r/d12fcff59a<?php
class Foo {
/** @var string[]|null */
protected ?array $arr;
function __construct()
{
$this->arr = [];
// ...do stuff here that *may* populate $this->arr, or set it to NULL...
$this->foo();
if ($this->arr === [] || $this->arr === null) {
echo "empty\n";
}
}
function foo() : void {
if (rand(0, 1) > 0) {
$this->arr = ['world'];
}
}
}
|
Hi @kkmuffme - I'm not sure I follow. The snippet I posted is a minimal example that reproduces the issue. It is from a big app codebase that started to emit warnings after upgrading from 5.19.0 to latest Psalm. I don't see anything wrong with that code. IMHO your changed code not issuing any warnings just confirm the problem, since your version do the same thing, i.e. |
I found these snippets: https://psalm.dev/r/d12fcff59a<?php
class Foo {
/** @var string[]|null */
protected ?array $arr;
function __construct()
{
$this->arr = [];
// ...do stuff here that *may* populate $this->arr, or set it to NULL...
$this->foo();
if ($this->arr === [] || $this->arr === null) {
echo "empty\n";
}
}
function foo() : void {
if (rand(0, 1) > 0) {
$this->arr = ['world'];
}
}
}
|
Here's another minimal example that reproduces the issue, extracted from the same app that started to emit warnings after upgrade: |
I found these snippets: https://psalm.dev/r/44845ca739<?php
$arr = explode('.', phpversion());
if (empty($arr)) {
echo "Empty\n";
}
|
Maybe a somewhat more minimal case: /**
* @param true[] $array
*/
function check_key_in_array(array $array, int $key): void
{
if (!empty($array[$key])) echo "key $key not empty";
} gives: the element is ALWAYS truthy when present, but it can be missing -> empty -> falsy, since the keys are not specified. |
I found these snippets: https://psalm.dev/r/557faa3f96<?php
/**
* @param true[] $array
*/
function check_key_in_array(array $array, int $key): void
{
if (!empty($array[$key])) echo "key $key not empty";
}
|
This regression seems to be related to:
It's still reproducible in 5.23.1 |
@flaviovs your first example is not a minimal example - the place where you put
is the issue - since this code that you claim may modify it, is analyzed by psalm. This is why I provided an example where this code is actually there (and not just a comment) to show it works if there's actual code there, and not just a comment. But feel free to improve upon my example, since again - this is just one example to showcase it works with actual code that is not a comment.
Yeah, almost but not entirely. Anyway you can just use empty: https://psalm.dev/r/206b194230 (the error you get now is bc it's almost the same but not entirely as the verbose check - but this is unrelted to this question. I just wanted to clarify this to prevent further questions in this regard, since that's not the issue we agree.)
This error is correct too. explode never returns a falsy (empty) value. @ArtemGoutsoul can you please open a separate issue, since this is an entirely different issue caused by the true array (afaik there's an issue for that alraedy, but I couldn't find it right now). As a temp workaround you can just use isset() there |
I found these snippets: https://psalm.dev/r/206b194230<?php
class Foo {
/** @var string[]|null */
protected ?array $arr;
function __construct()
{
$this->arr = [];
// ...do stuff here that *may* populate $this->arr, or set it to NULL...
$this->foo();
if (empty($this->arr)) {
echo "empty\n";
}
}
function foo() : void {
if (rand(0, 1) > 0) {
$this->arr = ['world'];
}
}
}
https://psalm.dev/r/44845ca739<?php
$arr = explode('.', phpversion());
if (empty($arr)) {
echo "Empty\n";
}
|
My team are debating this assertion
Does anyone have a value of Tried a set here: https://3v4l.org/VlLK6#v8.3.8 |
Can you please create an example on psalm.dev bc I have no idea what psalm error you're referring too, since the OP issue was a non-issue, then it got hijacked by someone with a completely different issue and I have no idea what you're referring to :-) |
For a defined $arr that is |
OK great; I was wondering about the "almost the same" comment because I really couldn't see why they were not identical. Someone pointed out that empty is safe for checking undefined values, but that's a different condition. |
OP here. I've done some more tests an here's what I came up: Leaving Using So the issue may not be even related to |
I found these snippets: https://psalm.dev/r/ba93e8414a<?php
class Foo
{
public array $data = [];
public function resetData(): void
{
$this->data = [];
$this->changeData();
if (empty($this->data)) {
echo "Is empty\n";
}
}
public function changeData(): void
{
if (rand(0, 10) > 5) {
$this->data[] = 1;
}
}
}
https://psalm.dev/r/a93bf069be<?php
class Foo
{
public array $data = [];
public function resetData(): void
{
$this->data = [];
$this->changeData();
if (count($this->data) === 0) {
echo "Is empty\n";
}
}
public function changeData(): void
{
if (rand(0, 10) > 5) {
$this->data[] = 1;
}
}
}
https://psalm.dev/r/ee7c8a7f07<?php
class Foo
{
public array $data = [];
public function resetData(): void
{
$this->data = [];
$this->changeData();
if ($this->data === []) {
echo "Is empty\n";
}
}
public function changeData(): void
{
if (rand(0, 10) > 5) {
$this->data[] = 1;
}
}
}
|
https://psalm.dev/r/4609d92f6f
Note: using
empty()
because it's a common pattern in PHP apps to assert nullable arrays. However, removingempty()
(i.e. doingif ($this->arr)...
) still generates warnings, albeit different ones:The text was updated successfully, but these errors were encountered: